[Samba] Issue with acl_xattr:ignore system acls in 4.5rc2

[Uri Simchoni]

On 08/18/2016 08:28 AM, Ralph Böhme wrote:
> Hi Eric,
> 
> On Wed, Aug 17, 2016 at 05:56:51PM -0600, Eric Eastman via samba wrote:
>> I was testing Samba 4.5rc2 with an existing smb.conf file that I have been
>> using since Samba 4.1.x and found that I cannot access data in the share on
>> Windows 2012 (my AD server), a Windows 2008 client or on a Ubuntu 16.04
>> client.  I built a new version of Samba 4.4.5 using the same procedure as I
>> did for building 4.5rc2 and the 4.4.5 Samba version worked with my smb.conf
>> file.  I then created a very basic smb.conf file and slowly built it up,
>> and by doing testing between 4.4.5 and 4.5rc2, I finally found the line
>> that caused the problem. Here is my cut down smb.conf file:
>>
>> [global]
>> security = ads
>> realm = ERIC.LOCAL
>> workgroup = ERIC
>> netbios name = C1-GW03-T5
>> idmap uid = 500-10000000
>> idmap gid = 500-10000000
>> winbind use default domain = Yes
>> winbind nested groups = Yes
>> map acl inherit = yes
>> vfs objects = acl_xattr streams_xattr
>> acl_xattr:ignore system acls = yes
>> load printers = no
>> printcap name = /dev/null
>>
>> [zzz]
>> path = /zzz
>> writeable = yes
>> browseable = yes
>>
>>
>> The line causing the problem with 4.5rc2 is:
>>   acl_xattr:ignore system acls = yes
> 
> this change was introduced in
> <https://bugzilla.samba.org/show_bug.cgi?id=12028>
> 
> Before explaining the gory details, one question: why are you setting
> this option?
> 
> With the patches from the bugreport the behaviour of vfs_acl_xattr
> changed when "acl_xattr:ignore system acls" is set to yes for the case
> a file or directory lacks the NT ACL xattr (eg when creating files and
> directories on the server).
> 
> Before the change, we would query the system POSIX permissions and map
> them to a NT ACL.
> 
> After the change we sythesize a default NT ACL with just
> 
>   ACL:$uid:ALLOWED/0x0/$mapped_mode
>   ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL
> 
> As this severly impacts existing setups, we have three options to
> address this:
> 
> 1. Revert it,
> 2. Document it, or
> 3. Do it differently
> 
> 1. Revert it
> 
> Brings back the original problem: not behaving as a Windows server and
> in certain situations unexpectedly exposing system POSIX permissions
> as described in the above bug.
> 
> 2. Document it
> 
> One could argue that this works as designed, so just add a big note to
> the release notes so people are aware of this change. As everybody
> reads release notes, there'll be no surprise. :)
> 
> When acl_xattr:ignore system acls is set to yes, we now completely
> ignore the system POSIX permissions. Since bug 11806 we already ignore
> the the system POSIX permissions when setting an NT ACL, we now ignore
> them as well when querying them. So this can be seen as improvement
> because previously our behaviour was inconsistent.
> 
> In some sense "acl_xattr:ignore system acls = yes" works like a
> fictional option "acl_xattr:ignore system permissions = yes".
> 
> Any filesytem object without NT ACL xattr will get default permissions
> matching Windows:
> 
>   ACL:$uid:ALLOWED/0x0/$mapped_mode
>   ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL
> 
> This affects directories created on the server, like in Eric'c case,
> or when an SMB client creates a file or directory where the parent
> directory has no inheritable ACEs, as described in bug 12028.
> 
> 3. Do it differently
> 
> Implement the fictional option from 2 "acl_xattr:ignore system
> permissions" and only ignore system POSIX permissions when querying
> permissions if this is set. Sing: one more option before you go, to
> the valley below.
> 
> *scratching head* Thoughts?
> 
> Cheerio!
> -slow
> 
Ralph,

I think the current behavior is correct (also tested it against Windows
for comparison).

However, if it breaks things for users, we need a way out for them, and
the only way out that's guaranteed to work in all cases is a
compatibility switch - generate the "old" DACL as default, wrong or
surprising as it may be, to maintain backward compatibility. So that's
an additional "acl_xattr:legacy default acl = yes" param IMHO.

Thanks,
Uri.