s3 auth changes

[Andrew Bartlett]

On Mon, 2010-06-07 at 16:34 +0200, Volker Lendecke wrote:
> On Mon, Jun 07, 2010 at 03:44:47PM +0200, Volker Lendecke wrote:
> > On Mon, Jun 07, 2010 at 07:51:01AM -0500, Andrew Bartlett wrote:
> > > The branch, master has been updated
> > >        via  00089fd... s3:auth make sure the primary group sid is usable
> > >        via  048575d... s3:auth return the full passwd struct from check_account
> > >        via  0a7ff14... s3:passdb Export function to calculate the proper primary group sid
> > >        via  aaf45cd... s3:auth remove unused structure member
> > >        via  aa1a3cb... s3:auth create nt token from info3 directly
> > >        via  e6456df... s3:auth handle unix domain sids in samu
> > >        via  61823fb... s3:auth set the resolved user sid in the fake sam account
> > >        via  ef94217... s3:auth check the user is valid first
> > >        via  1bb0afa... s3:auth make sure we set the right username
> > >       from  aa32725... s4:ldap.py - add some "objectclass" behaviour tests
> > 
> > Please revert these changes until they are explicitly
> > acknowledged by a S3 developer.
> 
> Gna, replied to the wrong commit message, thanks to Simo for
> pointing that out to me. This applies to
> 
> 0dc88d2..9a747d5
> 
> Please revert those. While I don't like the struct renaming,
> I would be okay with them. In particular 8f1cec5 and 4a7f45b
> definitely need to be reverted now, and I don't see the
> point for edba46c. There is only one error condition here,
> so why do we have to bother with NTSTATUS?
> 
> So please revert those three.

Volker,

Can you please explain a little more what is wrong with those patches?

You may have missed that I've been working with Simo and Günther on
improving the source3/ auth subsystem.  I'll be doing similar work in
the source4/ auth subsystem soon, in the hope that we can change to more
rational structures and share more code.  Of those that I've pushed,
Günther specifically asked me to merge the 'rename and similar'
patches. 

Regarding the patches that you explicitly call out:
8f1cec5 s3:smbd Fix segfault if register_existing_vuid() fails

This I spent quite a bit of time looking into with valgrind.  The
problem that showed it up was was caused by my s3compat auth module not
returning a server_info->ptok:
	if (!vuser->server_info->ptok) {
		DEBUG(1, ("register_existing_vuid: server_info does not "
			"contain a user_token - cannot continue\n"));
		goto fail;
	}
However, there are other failure paths in register_existing_vuid(),
including:
	if (!session_claim(vuser)) {
		DEBUG(1, ("register_existing_vuid: Failed to claim session "
			"for vuid=%d\n",
			vuser->vuid));
		goto fail;
	}
If the goto fail is executed in this situation, it will case a
use-after-free if the caller tries to call auth_ntlmssp_end.  It hasn't
shown up in testing before, because for session_claim() to fail is very,
very rare. 

4a7f45b s3:smbd Give the kerberos session key a parent
I followed the code paths here quite carefully after the error showed up
under valgrind and s3compat.  By stealing the key here, we ensure we are
very clear as to where we want it to be in the long term - in the
source3/ kerberos code it is allocated on the NULL context, and that's
not a good place to keep it.  I discussed this patch with Simo and gd on
IRC, who asked me to check that the smb2 code was not similarly
afflicted.  There it avoids the issue by making an explicit copy/free.  

I hope to create a common 'tail function' for this handling soon, based
on the auth_ntlmssp API (there is too much duplication between SMB and
SMB2 here).

edba46c s3:auth Change auth_ntlmssp_server_info API to return NTSTATUS
As I say in the commit message, I'm using this API as part of s3compat,
and the reimplementation there can return a richer set of error codes.
It was in debugging those error codes (or more particularly, debugging a
difficult to pin down NT_STATUS_NO_MEMORY) that I chose to improve the
API here. 

I hope this clarifies things,

Andrew Bartlett

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

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