[Gluster-devel] What would be ideal option for 'auth.allow' to support subdir mount?

Wed Jul 26 12:17:54 UTC 2017

On Thu, Jul 20, 2017 at 10:58 PM, Amar Tumballi <atumball at redhat.com> wrote:

>
>
> On Thu, Jul 20, 2017 at 9:21 PM, Niels de Vos <ndevos at redhat.com> wrote:
>
>> On Thu, Jul 20, 2017 at 08:25:23PM +0530, Amar Tumballi wrote:
>> > On Thu, Jul 20, 2017 at 7:36 PM, Niels de Vos <ndevos at redhat.com>
>> wrote:
>> >
>> > > On Thu, Jul 20, 2017 at 07:11:29PM +0530, Amar Tumballi wrote:
>> > > > Hi,
>> > > >
>> > > > I was working on subdir mount for fuse clients [1], and was able to
>> > > handle
>> > > > pieces just fine in filesystem part of gluster. [2]
>> > > >
>> > > > What is pending is, how will we handle the authentication options
>> for
>> > > this
>> > > > at each subdir level?
>> > > >
>> > > > I propose to keep the current option and extending it to handle new
>> > > feature
>> > > > with proper backward compatibility.
>> > > >
>> > > > Currently, the option auth.allow (and auth.reject) are of the type
>> > > > GF_OPTION_TYPE_INTERNET_ADDRESS_LIST. Which expects valid internet
>> > > > addresses with comma separation.
>> > > >
>> > > > For example the current option looks likes this:
>> > > >
>> > > >  'option auth.addr.brick-name.allow *' OR 'option
>> > > > auth.addr.brick-name.allow "192.168.*.* ,10.10.*.*"'.
>> > > >
>> > > > In future, it may look like:
>> > > >
>> > > > `option auth.addr.brick-name.allow "10.0.1.13;192.168.1.*
>> > > > =/subdir1;192.168.10.* ,192.168.11.104 =/subdir2"`
>> > > >
>> > > > so each entries will be separated by ';'. And in each entry, first
>> part
>> > > ("
>> > > > =") is address list and second part is directory. If directory is
>> empty,
>> > > > its assumed as '/'. (Handles the backward compatibility). And if
>> there is
>> > > > no entry for a $subdir here, that $subdir won't be mountable.
>> > >
>> > > IIRC Gluster/NFS allows you to set permissions for subdir mounting
>> with
>> > > a format like this:
>> > >
>> > >   /subdir/next/dir(IP,IP-range,...) /subdir2(IP)
>> > >
>> > > This is good, but would currently break the compatibility with
>> existing
>> > auth.allow of gluster.
>> >
>> > Backward compatibility was the main reason for me to consider the above
>> > approach.
>> >
>> > It would be best to use the existing format if we can to prevent
>> > > confusion among our users.
>> > >
>> > > Currently existing gluster's option is not same as NFS in my opinion.
>> How
>> > do you want to handle it?
>>
>> I'm wondering if the current format that us used for NFS is not
>> sufficient? Some defaults and quircks that would apply:
>>
>>
> Should be sufficient. Earlier I was not sure of which option you were
> talking.
>
> For everyone's clarity, I assume Niels is talking about
> 'nfs3.*.export-dir' option in xlators/nfs/server/src/nfs.c.
>
> It is of the form:  /foo(192.168.1.0/24|host1|10.1.1.8
> <http://192.168.1.0/24%7Chost1%7C10.1.1.8>),/host2.
>
> <dir>[(hostspec[|hostspec|...])][,...]
>
> But point to note here, it is of form GF_OPTION_STR, which means there
> won't be any validation for this key, unline current gluster's
> server-protocol auth.allow, which check for valid_hostname during gluster
> volume set itself.
>
> I am fine to support this format for auth-allow too, by handling current
> format as special case for backward compatibility. I will give others time
> till Monday before confirming this and going ahead with implementation.
> Suggest other valid options and reason if this is not enough.
>
>
'auth.allow' is implemented as per the suggestions from Niels now in the
patch [1]. Also added a test case to make sure we have the validation done
too.

Please review the patch and see if it can be merged, so we can say this is
an experimental feature in 3.12 (I will send another patch with release
notes on same feature in some days).

For people who would be concerned the changes may cause issues in protocol
xlator, don't worry, a user can't use the feature without setting
auth.allow/reject on particular subdirectory before using it (on all the
other nodes than trusted pool).

So, should be fine to make it to 3.12 branch IMO.

[1] - https://review.gluster.org/17141

Regards,
> Amar
>
>
>>  - if an entry does not start with "/", assume it is an IP/host/... and
>>    apply the restriction to the whole volume
>>  - separator between entries can be either " " or "," or a combination
>>
>> It would be good not to break any of the current accepted formats, and
>> make them equal if we can.
>>
>> Do you see a problem with this that I might have missed?
>> Niels
>>
>>
>> >
>> > -Amar
>> >
>> >
>> > > Thanks,
>> > > Niels
>> > >
>> > >
>> > > >
>> > > > (The above format is handled properly already at [2] in addr.c, the
>> > > pending
>> > > > thing is to handle the option properly in options.c's validate).
>> > > >
>> > > > [1] - https://github.com/gluster/glusterfs/issues/175
>> > > > [2] - https://review.gluster.org/17141
>> > > >
>> > > > If everyone agrees to this, I guess we can pull it off before
>> absolute
>> > > > feature freeze date for 3.12 branch.
>> > > >
>> > > > Let me know the feedback. (I am updating the same content in
>> github, so
>> > > > feel free to comment there too).
>> > > >
>> > > > NOTE: I thought of using ':' (colon) as field separator between
>> addr_list
>> > > > and subdir entry, but with IPv6 ':' is valid character in string.
>> Hence
>> > > > using ' ='.
>> > > > --
>> > > > Amar Tumballi (amarts)
>> > >
>> > > > _______________________________________________
>> > > > Gluster-devel mailing list
>> > > > Gluster-devel at gluster.org
>> > > > http://lists.gluster.org/mailman/listinfo/gluster-devel
>> > >
>> > >
>> >
>> >
>> > --
>> > Amar Tumballi (amarts)
>>
>
>
>
> --
> Amar Tumballi (amarts)
>

-- 
Amar Tumballi (amarts)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20170726/d1e7f4af/attachment.html>