[PATCH] Two attempts in fixing default ACL in vfs_acl_xattr

[Ralph Böhme]

On Fri, Jul 15, 2016 at 01:10:45PM -0700, Jeremy Allison wrote:
> Really interesting problem and good solution. Short answer,
> for the proposed patchset - Reviewed-by: Jeremy Allison <jra at samba.org>.

phew, I wasn't totally sure I got this right. :)

> The second (hack) patch is too ugly to live :-).

Ok, die patch, die. :)

> Longer answer, the ACL return here comes from Samba's original
> role as a method of exposing the underlying POSIX semantics
> of the filesystem to a Windows client.
> 
> If that's the intention, the current behavior is correct as
> it shows the user exactly what the 'real' underlying POSIX
> permissions are on the filesystem (even the addition of the
> SYSTEM:FULL_ACCESS ace representing root :-).
> 
> For previous versions of Samba that didn't check Windows ACLs
> in userspace the existing behavior is correct, as it shows
> the client what will happen if they try and do an operation.
> 
> However over the years our role has become different, and we're
> designing our behavior much more around complete seamless emulation
> of a Windows fileserver - checking the stored Windows ACLs
> in userspace which can provide additional restrictions on
> top of the underlying file system permissions.
> 
> Given that premise, your change is completely correct and
> fixes unexpected behavior.
> 
> There's only once subtle case we need to think about - and
> that is the original behavior of a fileserver that wants
> to represent and expose underlying filesystem permissions
> to the client, and expects all permission changes to be
> expressed as ACLs that will map into POSIX-style ACLs.
> 
> However for that case I think we're covered in that we
> just tell users not to load the acl_xattr module and
> they should get the desired behavior.

Yes. It's also covered for the case where the user wants the best of
both worlds, uses vfs_acl_xattr with the default mode of
"acl_xattr:ignore system acls = no" and expects Windows ACLs be
applied as POSIX ACLs as well.

In that case the modifed function isn't called so we have unchanged
behaviour for this case.

> Long winded way of saying LGTM. Please log the bug
> (with your excellent explaination) and push it with
> the bug link in the commit message !

Will do. Thanks!

Now I have another fix for the "acl_xattr:ignore system acls = yes"
case, or probably kind of more of a general enhancement: when adding
ACEs to ACLs we don't check whether the permissions are empty, which
results in ACLs like this:

$ ./bin/smbcacls -Uslow%pass //localhost/normal "dir1" -d 0
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\slow
ACL:SLOW\slow:ALLOWED/0x0/FULL
ACL:Unix Group\slow:ALLOWED/0x0/
ACL:Everyone:ALLOWED/0x0/
ACL:Creator Owner:ALLOWED/OI|CI|IO/FULL
ACL:Creator Group:ALLOWED/OI|CI|IO/
ACL:Everyone:ALLOWED/OI|CI|IO/

Notice the four ACEs without permissions? But that's something for a
new thread.

Cheerio!
-slow