[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