[PATCH REVIEW] s3-auth

[Andrew Bartlett]

On Mon, 2011-06-27 at 12:50 +0200, Andreas Schneider wrote:
> On Wednesday 22 June 2011 10:11:34 Andreas Schneider wrote:
> > On Tuesday 21 June 2011 16:04:53 Andreas Schneider wrote:
> > > I've discussed this with metze how it should be done before I
> > > implemented
> > > it. I think we can cache it in smbd_server_connection. I will implement
> > > it and come back to you.
> > 
> > I've done the changes. Could you please look at it again.
> > 
> > http://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/s3-auth
> 
> Jeremy,
> 
> could you please review it?

Andreas,

I'm sorry, but I still object to the design and use of
get_remote_hostname(), particularly as it involves
+       store_nc(&nc);
+       lookup_nc(&nc);

I object to adding a reference to memcache in the dependencies of the
auth subsystem.  You still have not addressed my query why this could
not be held in the client_id, changing only the address (as distinct
from name) element to be in the new tsocket format. 

Perhaps I need to be clearer as to why I object:

Each little cache like this seems trivial, but they present challenges
as we work to make Samba a set of libraries that have dependency chains
that can be expressed on with fully resolved symbols.  A reference to
memcache forever binds the function to whatever library holds the global
variables for memcache.

In particular as regards a cache attached to the auth subsystem, adding
such caches makes it harder for these functions to be merged into the
common code.  This is important to me because I still plan to merge
Samba's authentication code, and I must then attend to each additional
dependency.  I therefore take a strong interest in changes to our
authentication code, because I do intend to remain involved in it's
future. 

I therefore strongly request that you rework the design of this patch
series to retain the client_id structure, and have that structure handle
the cache of the name lookup, possibly working to copy it less if that
is the cause of the major concern around lookups. 

I do think we can sort this out:  The hostname is looked up in
smbd/process.c for every connection, and we should be able to pass that
down to the various functions that use it.  

Additionally, the function users in the SAMR server don't need to be
fast, they are only called because we want to feed PAM the correct
remote hostname for a PAM based password reset.  The primary difficulty
lies with the spoolss server and the 'pam account check' code.

I hope this more clearly expresses my concerns, 

Andrew Bartlett

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