More s3compat-for-review patches

[Stefan (metze) Metzmacher]

Hi Andrew,

>>> On Wed, 2010-05-26 at 11:45 +1000, Andrew Bartlett wrote:
>>>> On Wed, 2010-05-26 at 09:54 +1000, Andrew Bartlett wrote:
>>>>> I've updated my s3compat-for-review tree, which now includes the use of
>>>>> the source3/ ntlmssp_sign.s and ntlmssp.h in common code.
>>>>>
>>>>> I would appreciate any useful comments that folks have, as I would like
>>>>> to merge this code.  Most of the patches should be able to be
>>>>> cherry-picked on their own, if only some are acceptable.
>>>>>
>>>>> http://gitweb.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/s3compat-for-review
>>>>
>>>> I've updated this branch following feedback on IRC from simo.  (A number
>>>> of the patches to move functions into different files have been split
>>>> into multiple patches). 
>>>>
>>>> It passes 'make selftest' with the same errors as master.
>>>
>>> If I don't get any further comments (and I believe I've addressed simo's
>>> concerns about the file split-up being taken any further), I intend to
>>> push this as-is at the end of this week (tomorrow). 
>>
>> Please don't push it as is.
>>
>> I reviewed it yesterday via gitweb on my cellphone (in readonly mode:-)
>>
>> I'd like to review it more detailed today, as I found some minor details
>> I'd like to be fixed before it's pushed to master.
>>
>> The first things I noticed are:
>> - we don't need a ntlmssp_private.h (at least in this patch stream).
> 
> Yes and no.  There is no reason ntlmssp_debug_flags needs to be used
> outside the NTLMSSP code, but must be shared between the sign and main
> code.  

yes, I kept it with debug_ntlmssp_flags() only in my branch.

> I agree the other changes are not strictly required, but as you may have
> guessed or seen in my s3compat branch, I need those for the way GENSEC
> uses the common code.  I don't see how they do any harm. 

we only need it for struct ntlmssp_crypt_direction save_direction;
correct? I think we should try to solve that in a different way
and keep the structures private.

maybe adding a flag to ntlmssp_sign_init() that it should allocate
a struct ntlmssp_crypt_direction *crypt_save;

the caller needs to do a ntlmssp_save_crypt_state() and
ntlmssp_restore_crypt_state().

>> - for "s3:auth Make AUTH_NTLMSSP_STATE a private structure"
>>   the change to source3/auth/auth_util.c is unrelated and changes
>>   the behavior. It should be its own commit.
> 
> Indeed!  Sorry about that. 
> 
>>   Would it make sense to handle the case where
>>   auth_ntlmssp_server_info() is called twice without segfaulting?
> 
> I don't think so - we would have to resort to talloc references and hope
> that the code that it calls handles that well.  In any case, I wanted to
> be very careful to preserve the existing behaviour, because of all the
> warnings I've been given not to change behaviour. 

I think we can leave it as is then.

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/20100527/090d6847/attachment.pgp>