[PATCH] support for kerberos in plugin DC code

Stefan (metze) Metzmacher metze at samba.org
Tue Aug 2 08:42:24 MDT 2011


Am 02.08.2011 15:46, schrieb 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. 

You're using this in a lot of places.

>> - 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. 

I mean it's wrong to have this comment in auth_ntlmssp_want_feature()
ntlmssp_want_feature() is the place that handles the mapping from
session key
to NTLMSSP_NEGOTIATE_SIGN.

>> - 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. 

Just create a simple bool ntlmssp_blob_is_magic(const DATA_BLOB *blob)
next to ntlmssp_want_feature() and let gensec_ntlmssp_magic() also use it.

>> 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.

Ok.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20110802/a8f07b48/attachment.pgp>


More information about the samba-technical mailing list