[PATCH SET] Refactoring of auth_ntlmssp

[simo]

On Mon, 2010-07-19 at 16:56 +1000, Andrew Bartlett wrote:
> 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.  

At them moment the aim is to unify cli_pipe_auth_data and pipe_auth_data
to be the same structure (ntlmssp_state vs auth_ntlmssp-state is the
only relevant difference) so that I can use the same common functions to
deal with the auth footer both in the client and in the server rpc code.

> > > > 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 I understand gensec abstract all auth/encryption mechanism, not just
ntlmssp, so I am not sure why do you care about keeping the
auth_ntlmssp_state wrapper.

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

Why not use directly the gensec security API as absraction ?
It doesn't have to match immediately, but will avoid useless and
confusing wrappings. Even in your tree any auth_ntlmsssp_* call is
nothing more than just a wrap around a gensec call.

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

Gensec is the abstraction, auth_ntlmssp_state is not, why should we go
through this abstration to get to another, it seem the longest route to
me.

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

We can certainly do even more surgery later. But here I am trying to
streamline the rpc code to make it decent after many years of neglet.
This work is more important for me right now.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>