[PATCH] using gensec_gse and gensec_spnego in s3 sesssion setup

[Andrew Bartlett]

On Wed, 2012-01-25 at 06:02 +0100, Stefan (metze) Metzmacher wrote:
> Hi Andrew,
> 
> > In looking over your changes, I note that in:
> > s3-gse: add GENSEC_FEATURE_NEW_SPNEGO detection in
> > gensec_gse_have_feature()
> > https://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=986b8487d69c85e2118d58b23642aacf4964d02e
> > 
> > I can't see where the lucid context is free'ed in the success case.  In
> > gensec_gssapi this is done in the talloc destructor. 
> 
> Thanks for finding this, I've added it to the destructor.
> 
> > Finally, I should point out a remaining difference between the PAC
> > handling code in auth_generic and the current code inline in
> > sesssetup.c.
> > 
> > The sesssetup.c:reply_spnego_kerberos() code has:
> > 	/* setup the string used by %U */
> > 	sub_set_smb_name(real_username);
> > 
> > 	/* reload services so that the new %U is taken into account */
> > 	reload_services(sconn, conn_snum_used, true);
> 
> This has only impact on the make_session_info_krb5()
> and register_initial_vuid() code, so I guess there's no impact at all.
> Do you see any impact?

On further inspection, I agree.  

> > This matches (roughly) the code in
> > auth_ntlmssp.c:auth_ntlmssp_check_password():
> > 	/* setup the string used by %U */
> > 	/* sub_set_smb_name checks for weird internally */
> > 	sub_set_smb_name(gensec_ntlmssp->ntlmssp_state->user);
> > 
> > 	lp_load(get_dyn_CONFIGFILE(), false, false, true, true);
> > 
> > This block isn't in auth_generic.c:auth3_generate_session_info_pac() as
> > the gse RPC server code on which it was based.  The previous behaviour
> > was inconsistent - the NTLMSSP code always did this, as it has been
> > common for longer. 
> 
> register_existing_vuid() calls set_current_user_info(), which calls
> sub_set_smb_name(), so I think the smb1 session setup is consistent now.
> 
> For the kerberos case (above) we call register_existing_vuid() and
> reload_services(sconn, conn_snum_used, true); again.
> 
> I'm about to also add this to the smb2_sesssetup.c code...
> (The patch is in autobuild)
> 
> > I don't like the idea of reloading the smb.conf and changing %U in
> > general, but our users have come to expect it.  The challenge is doing
> > this right in generic gensec code (rather than only on session setups).
> > If we make it conditional we may wish to inherit in a GENSEC_FEATURE_
> > for this. 
> 
> Maybe, but could we copy the auth_ntlmssp.c chunk to auth_generic
> as a first step?

We could, or we could make a deliberate behaviour change, and make the
NTLMSSP code consistent with the krb5 case, and remove the
sub_set_smb_name().  This would mean we do not act on the user's claimed
name before we authenticate the user. 

Thank you for all your hard work here,

Andrew Bartlett

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