[PATCH] support for kerberos in plugin DC code

[Andrew Bartlett]

On Tue, 2011-08-02 at 14:21 +0200, Stefan (metze) Metzmacher wrote:
> Am 02.08.2011 01:01, schrieb Andrew Bartlett:
> > On Fri, 2011-07-29 at 17:09 +0200, Stefan (metze) Metzmacher wrote:
> >>>> I'd like you to give quite some time to review and decide if it is ok.
> >>>> I have been opposed on introducing gensec in s3 for a few reasons. One
> >>>> is dependencies, the other is that IIRC gensec does not create new event
> >>>> loops bu allows nesting of loops. That is something too dangerous for
> >>>> the file server imho.
> >>>
> >>> Yes, this needs a lot of review, I hope to get some time in the next days.
> >>
> >> Here're my first result, but I'll do more review on monday:
> > 
> > I've addressed all these issues (including the
> > auth_ntlmssp_steal_session_info(), but not removing the auth4_context
> > for the reasons I've explained).  Can you please let me know soon if you
> > have any further comments that cannot be addressed in-tree, as I would
> > like to move forward with this work.
> 
> Thanks, here some more comments:
> 
> - Please avoid } else { after return statements,
>   neesting can be avoided in that case and it makes the
>   code easier to read.
> 
>   +                       return nt_status;
>   +               } else {

I presume you refer to auth_ntlmssp_prepare(), which I've now fixed. 

> - Please use (at least in future)
> 
>   /*
>    * some multi line
>    * text...
>    */
> 
>   instead of
>   /* some multi line
>    * text ... */
> 
>   It makes the code much more readable.
> 
> - In source4/utils/ntlm_auth.c
>   nt_status = gensec_session_info(state->gensec_state, NULL, &session_info);
>   NULL => mem_ctx to avoid memory leaks.

Thanks.  I didn't see that a mem_ctx was available when I first did the
patch (there was no leak, the only success path free'ed the pointer).

> - In s3-ntlmssp Add hooks to optionally call into GENSEC in auth_ntlmssp
>   In auth_ntlmssp_want_feature this comment seems to be wrong:
>   /* You need to negotiate signing to get a windows server to calculate
> a session
>    * key */

I wish this were wrong, but I do remember when we first learnt about
this.  At least at the time, for SMB connections (where NTLMSSP level
signing makes no sense), you really do need to ask for signing otherwise
the session key wasn't available on the server side for decrypting SAMR
password sets. 

> - In s3-ntlmssp Add mem_ctx argument to auth_ntlmssp_update
>   auth_ntlmssp_update doesn't handle MORE_PROCESSING_REQUIRED.

Thanks.

> - It would be nice to have
>   (blob1.length > 7 && strncmp((char *)(blob1.data), "NTLMSSP", 7) == 0)
>   in a helper function.

We actually have gensec_ntlmssp_magic() for exactly this purpose, but
it's not in a place or form that is easily exposed to the session setup
spnego code.  I'll see what can be done. 

> The rest looks good to me, did you run your wintest?

Sadly my latest Fedora upgrade broke wintest for me - I can no longer
take snapshots!  But I'll either prevail on tridge's wintest, or run the
tests manually. 

For the curious, when I try to create a new snapshot I get:
sudo virsh snapshot-create Win2008R2-5
error: internal error unable to execute QEMU command 'savevm': The
command savevm has not been found

I'll get that testing done tomorrow, and hope to have this into the tree
soon after.

Thanks!

Andrew Bartlett

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