[PATCH SET] Refactoring of auth_ntlmssp

Andrew Bartlett abartlet at samba.org
Mon Jul 19 00:56:52 MDT 2010


On Sun, 2010-07-18 at 09:52 -0400, simo wrote:
> On Sun, 2010-07-18 at 17:10 +1000, Andrew Bartlett wrote:
> > On Sat, 2010-07-17 at 22:34 -0400, simo wrote:
> > > On Sun, 2010-07-18 at 08:23 +1000, Andrew Bartlett wrote:
> > > > On Sat, 2010-07-17 at 15:08 -0400, simo wrote:
> > > > > Hello,
> > > > > I have refactored a bit the way we deal with auth_ntlmssp in this tree:
> > > > > http://git.samba.org/?p=idra/samba.git;a=shortlog;h=refs/heads/ntlmssp
> > > > > 
> > > > > The aim was to be able to always use ntlmssp_state instead of a mix of
> > > > > ntlmss_state/auth_ntlmssp_state so that I can proceed with cleaning up
> > > > > cli_pipe.c/srv_pipe.c and make more code common between them. The main
> > > > > obstacle was the use of auth_ntlmssp_state vs ntlmssp_state.
> > > > > 
> > > > > If there are no objections I'd like to push the top most 15 patches to
> > > > > master. It passes make test and make selftest.
> > > > 
> > > > Thanks for looking into this.  I'll look at it early next week.  
> > > > 
> > > > However, my main concern (and perhaps it isn't warranted, which is why
> > > > I'll look at it properly next week) is that currently I can replace
> > > > auth_ntlmssp_state with a GENSEC wrapper in s3compat.  Without the
> > > > intermediate structure of a different name, it may become much more
> > > > difficult to make this change.   (We had a similar problem when trying
> > > > to merge the NTLMSSP code late last year, and had to re-introduce a
> > > > wrapper structure in GENSEC). 
> > > 
> > > Sorry,
> > > but I need this change to go on with my other work in the msrpc branch.
> > 
> > Simo,
> > 
> > You asked me for my views on this, and I've asked for some time to look
> > over the problem.  I would ask that you hold off on putting this code
> > in, until I can look over the changes fully, and we come to an
> > acceptable resolution. 
> 
> I am certainly not going to push the code until I get an ack from one of
> the people in CC. I am just saying that I can't stop working and just
> sit and wait.

Certainly!  To that end I've been very happy to sign off and push some
of your changes today, and I hope we can work on the rest. 

If you could describe your goals in a little more detail, I would be
happy to try and craft an alternative patch, if that would help.  

> > > So unless the code is factually wrong I would still go on with this
> > > change. Once I will be done with the changes I am making it will be a
> > > simple matter replacing the rpc wrappers, as for the smbd code there
> > > very few places where ntlmssp_state is used I am sure it can be dealt
> > > with.
> > > 
> > > Note that auth_ntlmssp_state as a structure still exist, it is simply
> > > concealed in ntlmssp_state.
> > 
> > I did consider this a little while out today, and wondered if we could
> > do the reverse:  Always use a wrapping structure (called
> > auth_ntlmssp_state or otherwise), that would not conflict in name with
> > ntlmssp_state.
> 
> To be honest I am not convinced that wrappers here is the right
> solution. If you want to use gensec in the S3 code you should really
> make it possible to share it first, and then just use it directly.

The reason I hoped to take this direction is that it is the way (from
memory) that metze suggested I merge GENSEC into Samba3.  That is, first
create a matching layer of abstraction, then match the APIs, then match
the implementation.  

As we already have an abstraction that so closely matches GENSEC that
I've been able to replace it directly in s3compat, I would ask that we
keep that abstraction, and then extend from there. 

> > Please do not make this change until we can work out a good way to both
> > address your requirements, and make the merge for Samba 4.0 as easy as
> > possible.  Quite possibly your change is perfectly sensible, I'll let
> > you know in the next day or so, if you would give me that opportunity.
> 
> The merge should be done by making code public and then sharing it,
> compile tricks with wrappers should be used only as stop-gap measures,
> they are not long term solutions.

Indeed!  However, we should ensure we keep the shortest route to the
solution, and don't make changes that we have to essentially reverse
later on.   My hope is that if we keep this abstraction, we don't have
to reintroduce it when I bring GENSEC in common.

I'm thinking we should probably do the more major surgery after the 3.6
tree has branched.  What do you think? 

Andrew Bartlett

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100719/6db1eda3/attachment.pgp>


More information about the samba-technical mailing list