Comments on the auth_serversupplied_info rework in Samba3

[simo]

On Wed, 2010-06-02 at 09:31 +1000, Andrew Bartlett wrote:
> 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;
> }

Maybe, probably then just
struct extra_auth_info {
	struct dom_sid user_sid;
	struct dom_sid pgid_sid;
};

Initialized to the null sid. This will avoid extra memory allocation.

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

No, I think I really want a -1 here to make it very clear this was
explicitly set as an invalid value and not that we forgot to store the
actual rid after a talloc_zero.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>