Comments on the auth_serversupplied_info rework in Samba3

[simo]

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

Simo.

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