[PATCH] Use conn->session_info->security_token in posix_acls.c to make sysvolreset faster (was: Re: [PATCH] improve performance for samba-tool ntacl sysvolreset)

[Andrew Bartlett]

On Tue, 2018-07-10 at 07:49 +0300, Uri Simchoni wrote:
> Hi,
> 
> The idea of using whatever information we already have instead of
> re-calculating it is correct IMHO.
> 
> What I like less is the idea of an incomplete session_info that we have
> to second-guess, as in 3/16 (checking for null unix_info). We may miss
> checks now or on in the future, and null-case-handling may be
> non-uniform. AFAICT we don't have checks like this in the code, which is
> (hopefully) a testament for this field always being populated when the
> session_info is built.

Correct, in the VFS case in smbd this is always filled in if the
session_info was filled in.  The previous case where the session_info
wasn't supplied at all was the python code and presumably vfstest
(where this was all cribbed from). 

> So I'm just asking - what would it take to construct a session info with
> unix_info on the Python side.

It could be done, but for the purposes of that patch the username isn't
used for anything important (passed to VFS modules at connect time for
historical reasons).  

However lots of other code assumes ->unix_info is non-NULL so perhaps
we just got lucky.  Bindings just need to be added for
auth_session_info_fill_unix(), and I'll do that tomorrow. 

The bit to watch more is the new check in uid_entry_in_group() which
takes a new route if conn->session_info it set (which it now much more
often is) but keeps the old UID based check also. 

Currently however what the python code doesn't do is change the UID of
the process via the whole become_user() stack, so I was hoping to avoid
setting that up.  

Perhaps we should have just SID based checks.

> Beside that I'm curious - it seems like the function we're optimizing
> (uid_entry_in_group()) gets called in one of the following case:
> 1. If the SD somehow doesn't translate into a POSIX ACL with a USER_OBJ
> 2. To emulate deny ACE

Something like that.  I understand it is to fold any group permissions
into the user permission because of the mismatch between NT and POSIX
semantics. 

> Which one of the two gets called in the sysvolreset? (and if it's 1.,
> why do we get an ACL without a USER_OBJ when we do a "reset" operation
> which should bring things to the detault state)

It gets called a lot, I find this code very difficult to follow but
seems to be needed for every group even if it isn't a DENY or such.

Thanks for the thoughtful feedback.  I was a little worried this might
not attract enough attention so I added a scarier subject :-)

Andrew Bartlett

-- 
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT   
https://catalyst.net.nz/services/samba