[PATCH] Add LoadParm optional parameter for Py_Credentials class constructor

[Andrew Bartlett]

On Mon, 2011-02-07 at 22:44 +0100, Jelmer Vernooij wrote:
> Hi Andrew,
> 
> On Tue, Feb 08, 2011 at 07:32:38AM +1100, Andrew Bartlett wrote:
> > On Mon, 2011-02-07 at 18:31 +0100, Jelmer Vernooij wrote:
> > > On Wed, 2011-01-26 at 22:04 +0200, Kamen Mazdrashki wrote:
> > > > Could you please take a look at following branch:
> > > > http://git.samba.org/?p=kamenim/samba.git;a=shortlog;h=refs/heads/py-review
> 
> > > > It is test to exploit a segfault when Py_Credentials object is used without
> > > > initializing 'domain' and 'workstation' members.
> 
> > > > After a short discussion with abartlet, he prefers the fix to be
> > > > implemented in python layer via adding LoadParm parameter
> > > > for Credentials constructor.
> > > > This way Py_Credentials instances will get the chance to
> > > > call cli_credentials_guess() during construction to try to populate
> > > > cli_credentials structure fields.
> > > > Basically, most common pattern was:
> > > >   creds = Credentials()
> > > >   creds.guess(lp)
> > > > which now turns into just:
> > > >   creds = Credentials(lp)
> > > In particular since we have a global loadparm again, I don't like to
> > > make more things dependent on loadparm. It makes it hard to do proper
> > > testing with alternative settings, and it makes it hard to write
> > > isolated code that's not accidentally influenced by the outside
> > > environment.
> 
> > > Is there any reason we can't simply initialize the domain and
> > > workstation members ?
> > It's not just those members - the underlying C credentials code is tied
> > to loadparm, and attempts to make it independent of loadparm have not
> > succeeded.  
> 
> > The elements are actually initialised, but to NULL, which isn't a
> > sensible value we can put into NTLMSSP.  As all callers need to provide
> > a sensible loadparm context, it makes most sense to allow them to do so
> > in the constructor. 
> 
> > We could of course make the cli_credentials layer do the loadparam based
> > guess internally, and avoid all the callers dealing with lp objects in C
> > or python, but I would like to keep those objects in case we can use
> > those provide specialisation of the global loadparm context for things
> > like %u subs etc. 
> 
> > That's why I asked that the patch be written this way. 
> I don't see why the loadparm object is mandatory in the first place.
> Why do we need it as more than /a/ way to obtain the credentials.
> 
> Are you sure it's the underlying C credentials code, and not gensec
> that relies on loadparm ? 

Jelmer,

I'm quite sure about this.  The settings this sets are used in the
credentials code, in handling various settings: 
 - default netbios name
 - default domain
 - default realm

These are then passed in to routines that prompt for the password if
these details are not specified.   It is these values that are missing
and cause the segfault in the NTLMSSP code. 

Similarly, if the local machine account is requested, then a loadparm
context is required to work out the secrets.ldb location.  I would
actually like to roll this back into the main 'guess' based lp_ctx. 

I originally hoped to avoid needing to pass in a loadparm context in all
instances, and allow callers to specify the 3 things mentioned above,
but no caller does so.  It remains a trap simply waiting to swallow up
another developer. 

> I'd really rather slowly get rid of loadparm
> in this code (so other people can use it as well, without having to
> write a smb.conf) than to make more code dependant on loadparm.

This doesn't change that, just makes the python interface recognise the
limitations of the C code on which it is based. 

> I'm worried about making the cli_credentials code into a kitchen sink.
> It's just a callback interface for obtaining credentials, it should not the
> place to get the systems NTLMSSP settings - that's what "struct
> gensec_settings" is for.

These are not settings that are specific to NTLMSSP or a particular
GENSEC session, so no, I think your fears about cli_credentials being
the kitchen sink are unfounded. 

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