[PATCH] Add LoadParm optional parameter for Py_Credentials class constructor

[Jelmer Vernooij]

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

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.

Cheers,

Jelmer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20110207/968f4252/attachment.pgp>