Behaviour mismatch between "store dos attributes" and "map archive" from man smb.conf(5)

[Anoop C S]

On Thu, 2020-05-14 at 17:47 -0700, Jeremy Allison via samba-technical
wrote:
> On Fri, May 15, 2020 at 01:26:14AM +0200, Michael Adam wrote:
> > If I disable acl_xattr but set "inherit acls = yes", this still
> > fails the same way. Only acl_xattr sets the execute perms.
> > So at least it is not "inherit acls" alone.
> 
> You'll probably have to add extra DEBUG statements
> to see what is happening step by step.

Seems like we have two different mapping methods.

When vfs_acl_xattr is used along with 'ignore system acls' we have
special kind of treatment as follows inside make_default_acl_posix():

if (mode & S_IRUSR) {
        if (mode & S_IWUSR) {
                access_mask |= SEC_RIGHTS_FILE_ALL;
        } else {
                access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
        }
}
if (mode & S_IWUSR) {
        access_mask |= SEC_RIGHTS_FILE_WRITE | SEC_STD_DELETE;
}

This might be the reason why smb2.read.position is always passing with
vfs_acl_xattr + "ignore system acls = yes".  

> > At this point I'm mostly trying to understand.
> > I still have the impression that the behavior is somewhat
> > inconsistent and random.
> 
> Yes. It isn't to a spec and there's no comprehensive
> test. Adding such things would be good.
> 
> > And it seems to me that the test was not written to pass against
> > exactly the configuration of the tmp share in the test env, with
> > the awareness that it fails against other standard configurations.
> > I can't help the impression that this is somewhat accidential.
> 
> Yep.
> 
> > That is apparently only true for creating new files.  For opening
> > existing files however, the desired access mode seems to have a
> > lot to with the ACL on the file. So this is strangely asymmetric:
> 
> No, that's only to do with what you're allowed after
> the existing ACL is read and compared with what you're
> asking or in access_mask.

Here comes the default mapping method, invoked all the way from
smbd_check_access_rights() we have the following in
map_canon_ace_perms():

if (directory_ace) {
        nt_mask |= ((perms & S_IRUSR) ? UNIX_DIRECTORY_ACCESS_R : 0 );
        nt_mask |= ((perms & S_IWUSR) ? UNIX_DIRECTORY_ACCESS_W : 0 );
        nt_mask |= ((perms & S_IXUSR) ? UNIX_DIRECTORY_ACCESS_X : 0 );
} else {
        nt_mask |= ((perms & S_IRUSR) ? UNIX_ACCESS_R : 0 );
        nt_mask |= ((perms & S_IWUSR) ? UNIX_ACCESS_W : 0 );
        nt_mask |= ((perms & S_IXUSR) ? UNIX_ACCESS_X : 0 );
}

Thus an existing file created without 'x' bit will end up with a
access_mask without SEC_FILE_EXECUTE.

> > (1) We create a file requesting SEC_FILE_EXECUTE.
> >     The file gets created without any execut perms.
> >     (without acl_xattr and with map archive = no)
> >     The create call succees.
> > 
> > (2) Later open of the existing file also requesting
> >     SEC_FILE_EXECUTE fails.

This is because we never go through smbd_check_access_rights() if file
is being created i.e, not existing. So first time it passes and fails
from next time onwards. At least we have fixed[1] smb2.read.position to
start with fresh file.

[1] 
https://git.samba.org/?p=samba.git;a=commit;h=dbfc197f65f28c7f4e889045d7b04c46c4f6680d

> > If I do acl_xattr or map archive = yes though, then #1 also
> > creates execute bits on the file. And #2 succeeds.
> 
> Yes, but that's nothing to do with what you're asking for.
> That's to do with the config settings - not "requesting
> SEC_FILE_EXECUTE". I think :-).

Difference in behaviour is because of different mapping methods
resulting in different access_mask when used with and without
vfs_acl_xattr. Why is it so? Why shouldn't vfs_acl_xattr just deal with
setting "security.NTACL" with a value obtained out of a common mapping
method?