netlogon_creds_cli_validate() in master4-schannel

[Andrew Bartlett]

On Thu, 2013-11-28 at 16:18 +0100, Stefan (metze) Metzmacher wrote:
> Hi Andrew,
> 
> > I was looking at this in particular, because it seems to be blocking a
> > lot of other work.
> > https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=b96ee68714389abee2f0cc0743246a335818376a
> > 
> > What I don't understand is why we have this complex
> > netlogon_creds_cli_validate() routine, rather than something at the NDR
> > layer using memcmp()?  If we ever add something to this structure, we
> > are going to have to keep this in sync, and that seems unfortunate. 
> 
> I've changed that.
> 
> > Other than that, it looks good.  I was supprised by the downgrade
> > handling in netlogon_creds_cli_check_caps but as long as we are careful
> > as to when we choose not to propose AES, I guess it's OK. 
> > 
> > How can I help you get the rest of this set merged?
> 
> Please review/test/push my
> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-schannel-base
> branch. This contains the netlogon secure channel changes which should
> be ready for master.
> 
> The rest needs more work. I need to do some more research regarding the
> password change code and
> fallback to the old password.

I've reviewed them:
http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/master4-schannel-base

Some non-critical comments:
 In 's3:libnet_join: make use of rpccli_{create,setup}_netlogon_creds()'
you have 
+ /*
+ * We now have an anonymous connection to IPC$ on the domain password
server.
+ */

However the connection appears to be authenticated.  

I was worried about netlogon_creds_cli_context_copy() but I see you
remove it in the last patch.  Nice work!

Finally, I'm a little worries that the rpcclient changes don't have a
testsuite. 

We just need to extend the rpcclient blackbox test script.  I'll hack at
this later today.  

It would also be a good idea to run some of our make test with the
different options etc, and to have a test showing the defence against a
downgrade attack, given how important that is. 

Andrew Bartlett



-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba