[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)

[Uri Simchoni]

On 07/11/2018 08:33 AM, Andrew Bartlett wrote:
> 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.
>>
>> So I'm just asking - what would it take to construct a session info with
>> unix_info on the Python side.
> 
> I was very hopeful, but sadly the security_token_to_unix_token() that I
> wrapped via auth_session_info_fill_unix() fails if winbindd isn't
> running.  
> 
> Winbindd can't start until after provision, but provision and the
> domain restore is an important user of this API.  (Internally in smbd
> the SID->ID handling falls back to passdb if winbindd isn't up, but
> this is not available to the token manipulation APIs right now. 
> 
> I could try for this, catch the exception and cope without the token in
> that case, but for now I would prefer to proceed with the patch as-was.
> 
> I've added the bindings and tests that operate no token and with and
> without the unix part of the token.
> 
> Let me know if you are OK with the above, or would prefer no token (and
> slower if winbind is not up) to the partial token.
> 
> CI: https://gitlab.com/catalyst-samba/samba/pipelines/25524636
> 
> Thanks,
> 
> Andrew Bartlett
> 

I'd prefer the original patch, because unless I'm missing something, the
new functionality (session_info_fill_unix) isn't being used, and isn't
tested (seems like considerable effort has been made but 19/20 seems to
be some infrastructure for tests and I don't see the tests themselves).

As I said before - I don't like adding null possibility, but I don't
have the time to implement what I've proposed, I don't want to be in the
way of progress, and the proposed patch doesn't break anything, it just
IMO "adds entropy" to the code where not strictly needed.

To recap:
Setting the owner of of the default ACL to be BA instead of LA, a
one-line-two-character diff, is likely to fix the performance problem,
and if not, then smbd can be modified to support this as we strive to
support files being owned by BA.

Another thing I proposed was some special rule to support LA using BA ACE.

Thanks,
Uri.