NTLMSSP and GENSEC

[Andrew Bartlett]

On Mon, 2011-10-17 at 10:11 +0200, Stefan (metze) Metzmacher wrote:
> Hi Andrew,
> 
> >>>>> For now, the module does not use any event context, so I've made no
> >>>>> change here yet.  
> >>>>
> >>>> If only the gensec_update() routines use any event context
> >>>> stuff at all, wouldn't it be better to pass the event
> >>>> context explicitly there instead of putting an event context
> >>>> into the gensec_security struct? This way the risk of using
> >>>> gensec wrongly leading to nested event loops is greatly
> >>>> reduced.
> >>>>
> >>>> If we absolutely have to accept the whole gensec thing into
> >>>> the Samba3 code (something which needs much broader
> >>>> discussion I think), then we should only do it if we can
> >>>> agree on handling the event stuff. Looking at 'struct
> >>>> gensec_security' right now I don't think we are there yet.
> >>>> The risk of accidentially getting nested event loops into
> >>>> the main Samba3 code, leading to very hard to debug
> >>>> situations is much too high for my taste with the tevent
> >>>> context being part of a central structure.
> >>>
> >>> I've updated my branch at and addressed the suggestions that you and
> >>> metze made, using the approach indicated in the commits you referenced
> >>> earlier.  gensec_update() now takes a tevent context, and is the only
> >>> gensec function that needs one. 
> >>>
> >>> http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/s3-auth-gensec-module-2
> >>
> >> Thanks! I'll have a closer look later today.
> >> But
> >> http://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=ffc9033d0a7ed07
> >> looks good, except for py_gensec_update() which seems to lack a
> >> TALLOC_FREE(ev); after gensec_update().
> > 
> > The mem_ctx here is only valid for the life of the call, so there is no
> > need. 
> 
> Ah, ok.
> 
> >> In the long run we should change the callers and backends to
> >> gensec_update_send/recv(),
> >> but for now having the event context just on gensec_update() is fine for me.
> > 
> > Great, thanks!
> 
> I've fixed some formatting and fixed the autoconf build
> (auth/ntlmssp/gensec_ntlmssp.c was missing)
> http://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-auth
> 
> Can you take a look at the TODO's?
> http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=130432987826662c2187e7a88ad9a2d9dda0cd1e
> Why do we downgrade to the next spnego mech on LOGON_FAILURE?
> 
> I think the backend has to decide that and convert LOGON_FAILURE into
> NT_STATUS_INVALID_PARAMETER if it's started as a subcontext.

Before the start and update methods were combined, when a mech failed to
start for any reason, it would be skipped.  This just attempts to catch
the common cases - it may be that the server appears to support
kerberos, but that we are trying to log in with a local user.

We can't change the error code, because if kerberos was forced, then we
really want to return LOGON_FAILURE back to the user. 

I'm quite open to alternate patterns or suggestions - perhaps we should
always fall back to the next mechanism on update() failure, like we did
for start()?

> Not related to your code, but can you have a look at
> http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=ff36902f17caff35e29dc9ab71df80910f1f34df
> I think we should add the unix id of the user to the group array if
> it's mapped with WBC_ID_TYPE_BOTH.

I guess that's reasonable.  We should ensure winbindd answers for id ->
name lookups for that, and that when an ACL is set, we set permissions
for the group id, not the user id where possible.

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org