[PATCH] Add LoadParm optional parameter for Py_Credentials class constructor

[Andrew Bartlett]

On Tue, 2011-02-08 at 00:10 +0100, Jelmer Vernooij wrote: 
> 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. 

That is a bug.  It is a long-standing design flaw, but it is also
nothing more than a bug.  Would this whole discussion be easier if I
just changed the C interface first?

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

We should not be able to create a credentials context without these
values.  They need to be obtained from somewhere, and the loadparm
conetxt is as good a source as any.  It can be overridden at any time,
if a better value is known (that is the whole point of the ratchet
mechanism in this code), and does not require that an smb.conf exist,
just that the loadparm is run. 

I'll change the C interface to just require loadparm context for
cli_credentials_init().  Then we can fix the python interface to match,
and it can all be self-consistent. 

OK?

Andrew Bartlett

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