[PATCH] Add LoadParm optional parameter for Py_Credentials class constructor

[Jelmer Vernooij]

On Tue, 2011-02-08 at 09:36 +1100, Andrew Bartlett wrote:
> On Mon, 2011-02-07 at 22:44 +0100, Jelmer Vernooij wrote:
> > 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 ? 

> 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. 
I think the proper fix in that case is either to have the NTLMSSP code
return an error if invalid data is passed in, and/or have the python
bindings raise an exception when NTLMSSP asks for data that is not
available.

> 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. 
The C code doesn't require loadparm at the moment either, but it can
optionally use it. I realize you can shoot yourself in the foot badly if
you don't pass in loadparm at the moment as you might not be setting all
the right parameters. But the (fixed) auth method you're going to use
might not need all parameters, or perhaps you're getting full set of
settings elsewhere (e.g. gconf). 

It would be nice if in the future we could get to a point where gensec
was usable without a smb.conf file, so non-Samba users could use it too.

Can't we just make the current code return a decent status rather than
crashing?

Cheers,

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