vfs_fruit bug in ACL get/set - [PATCH] attached.

[Ralph Böhme]

On Fri, Mar 02, 2018 at 02:14:04PM -0800, Jeremy Allison wrote:
> On Fri, Mar 02, 2018 at 10:05:37PM +0100, Ralph Böhme wrote:
> > Hi Jeremy,
> > 
> > On Fri, Mar 02, 2018 at 09:14:59AM -0800, Jeremy Allison wrote:
> > > I just created:
> > > 
> > > https://bugzilla.samba.org/show_bug.cgi?id=13319
> > > 
> > > ------------------------------------------------
> > > In fruit_fget_nt_acl() the 3 extra ACE entries
> > > corresponding to mode/uid/gid are always added
> > > to the returned ACL as virtual ACE entries that
> > > are not expected to be stored in the file ACL on disk.
> > > 
> > > In fruit_fset_nt_acl() the client-sent ACL is applied
> > > to the file, then the mode entry (if sent) is used to
> > > do the CHMOD - but the client-sent ACL is applied
> > > without removing any virtual ACE entries.
> > > 
> > > If a naive client just round-trips get/set/get/set,
> > > on every set the 'virtual' ACE entries will be stored
> > > in the xattr.
> > > 
> > > I think the correct action on fruit_fset_nt_acl()
> > > is to check for - then *remove* any virtual ACE
> > > entries sent by the client before passing down
> > > to the underlying SET_NT_ACL call.
> > 
> > hm, I guess the idea was that the NFS ACE *could* be of interest to lower layers
> > as well, so I chose not to filter them out.
> 
> No, that's the wrong thing to do (IMHO). You're
> always generating them yourself in the fruit layer on
> get, so you can't then pass them down in set.

I was thinking about moving setting *and* getting to a lower layer. Whichever
layer ends up doing it, should be fixed to correctly filter of course. I'm just
questioning whether keeping this in fruit is the right thing to do when we're
starting to use this more broadly.

> There's nothing the lower layer can do with these,
> and if it really wants them it can generate them
> itself.
> 
> > If we're starting to use NFS ACEs more broadly (by coincidence just today metze
> > mentioned that he's thinkin about using them in s3/auth iirc), maybe we should
> > move the functionality to a lower layer and out of fruit.
> 
> Let's but that is a patch for another day :-).
> 
> In the meantime, here's a patch that fixes fruit
> and the bug:
> 
> https://bugzilla.samba.org/show_bug.cgi?id=13319
> 
> Please review and let me know if you're happy with it !

I'll take a look. We can then reshuffle things later as needed.

-slow

-- 
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
GPG Key Fingerprint:           FAE2 C608 8A24 2520 51C5
                               59E4 AA1E 9B71 2639 9E46