Comments on the auth_serversupplied_info rework in Samba3

[Andrew Bartlett]

On Tue, 2010-06-01 at 10:06 -0400, simo wrote:
> On Tue, 2010-06-01 at 12:03 +1000, Andrew Bartlett wrote:
> > On Mon, 2010-05-31 at 21:49 -0400, simo wrote:
> > > On Tue, 2010-06-01 at 10:37 +1000, Andrew Bartlett wrote:
> > > > My biggest concern is the way that this patch uses 'magic values' that
> > > > are only defined in Samba.  
> > > > 
> > > > s3:auth handle unix domain sids in samu
> > > > http://git.samba.org/?p=idra/samba.git;a=commitdiff;h=5648836a26f39966ff3f1fa9bc5a287ea368afb6
> > > > 
> > > > +/* special bits used to identify non domain users
> > > > + * the may be generated in a samba server
> > > > + * These must not conflict with assigned values
> > > > + * in samr_GroupAttrs
> > > > + */
> > > > +#define SE_SAMBA_USER_SID ( 0x00100000 )
> > > > +#define SE_SAMBA_PGID_SID ( 0x00200000 )
> > > > 
> > > > 
> > > > s3:auth create nt token from info3 directly
> > > > http://git.samba.org/?p=idra/samba.git;a=commitdiff;h=56c1b9648324f3bd8f0b386298beaa24b859dc3c
> > > > 
> > > > +       /* USER SID */
> > > > +       if (info3->base.rid == (uint32_t)(-1)) {
> > > > +               /* this is a signal the user was fake and generated,
> > > > +                * the actual SID we want to use is stored in the
> > > > extra
> > > > +                * sids */
> > > > 
> > > > I would strongly prefer that we pass in a 'wrapper' or associated
> > > > structure that contains the info3, that has a flag of 'user rid fake'
> > > > etc.  We do not control these structures, and while we can fill them
> > > > in
> > > > with values that are semantically correct for their original use, I'm
> > > > happy, but I don't like the use of magic values.
> > > > 
> > > > This isn't just a preference - it took quite some time to understand
> > > > why
> > > > a new windows client failed against Samba4 with this same trick
> > > > (overloading a bitfield with Samba-only internal values) was used to
> > > > pass down special meanings in the Samba4 file server. 
> > > 
> > > Yes, this was the most controversial for me too.
> > > I will see if we can move this stuff out w/o making it a pain.
> > > As you can see samu_to_info3 is quite generic and does not take an
> > > auth_serversupplied_info() and that's the main reason the trick is done
> > > within info3.
> > > I don't think there really is a chance of spilling this out, but I
> > > totally understand the concern.
> > 
> > I think we should have it return two structures - one info3, and another
> > that I can also fill out in auth_samba4 (for s3compat) that describes
> > the exceptions.  We can put that additional structure in the common
> > auth.h when I get that merged, without needing to have this function
> > deal with much more intricate details of the auth subsystem.  
> 
> Ok I pushed changes to put this info in an "extra" structure and pushed
> to my wip tree.
> 
> This patch shows the different approach:
> http://git.samba.org/?p=idra/samba.git;a=commitdiff;h=50ae434d6e88e39bd885389844fe1e0af03d8ed6

What about

struct extra {
	struct dom_sid *unix_user_sid;
	sturct dom_sid *unix_group_sid;
}
 or
struct extra {
	struct dom_sid *replacement_user_sid;
	sturct dom_sid *replacement_group_sid;
}

Rather than the sid position stuff, which is much harder to follow.  

Then init those to NULL for the normal case, and set them when we need a
replacement.  

Also, rather than -1 as a magic RID value, 0 is more appropriate - we
never use 0 anyway.  (But we should change behaviour based on the
presence of replacement_user_sid, not the fill-in value).  

What do you think?

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/20100602/67be8f01/attachment.pgp>