[PATCH 1/2] s3-auth: fix force user for AD user

[Andreas Schneider]

On Tuesday 03 December 2013 17:44:29 Andrew Bartlett wrote:
> > > > +	/* samu_to_SamInfo3() calls get_primary_group_sid() if mapping fails
> > > > */
> > > > +	gid_to_sid(&g_sid, pwd->pw_gid);
> > > > +	if (!is_null_sid(&g_sid)) {
> > > > +		pdb_set_group_sid(sampass, &g_sid, PDB_SET);
> > > > +	}
> > > > +
> > > > 
> > > >  	result = make_server_info(NULL);
> > > >  	if (result == NULL) {
> > > >  	
> > > >  		TALLOC_FREE(sampass);
> > > 
> > > This second part seems reasonable - gid_to_sid logic was once inside the
> > > passdb pdb_get_primary_group_sid code, and would explain the regression
> > > (this area was changed for 3.6), but again it would be good if you could
> > > work out when this regressed, so we can better understand this delicate
> > > part of the system.
> > 
> > This part is the 'here be dragons' part. It is needed cause of the
> > following codepath:
> > 
> > samu_to_SamInfo3() -> pdb_get_group_sid() -> get_primary_group_sid()
> 
> I wish to put firmly on the record that I strongly dislike the 'magic'
> behaviour of calling get_primary_group_sid() here.  'get' routines like
> pdb_get_group_sid() should not do things like that.

Yes, I also found it strange that the function does the magic here. Maybe I 
find time to look into this too and we can remove it and put it at the place 
were it is actually needed.

> 
> >         gid_to_sid(group_sid, pwd->pw_gid);
> >         if (!is_null_sid(group_sid)) {
> >         
> >                 struct dom_sid domain_sid;
> >                 uint32_t rid;
> >                 
> >                 /* We need a sid within our domain */
> >                 sid_copy(&domain_sid, group_sid);
> >                 sid_split_rid(&domain_sid, &rid);
> >                 if (dom_sid_equal(&domain_sid, get_global_sam_sid())) {
> >                 
> >                         /*
> >                         
> >                          * As shortcut for the expensive lookup_sid call
> >                          * compare the domain sid part
> >                          */
> >                         
> >                         switch (rid) {
> >                         case DOMAIN_RID_ADMINS:
> >                         
> >                         case DOMAIN_RID_USERS:
> >                                 goto done;
> >                         
> >                         default:
> >                                 need_lookup_sid = true;
> >                                 break;
> >                         
> >                         }
> >                 
> >                 } else {
> >                 
> >                         /* Try group mapping */
> >                         ZERO_STRUCTP(group_sid);
> >                         if (pdb_gid_to_sid(pwd->pw_gid, group_sid)) {
> >                         
> >                                 need_lookup_sid = true;
> >                         
> >                         }
> >                 
> >                 }
> >         
> >         }
> > 
> > if (dom_sid_equal(&domain_sid, get_global_sam_sid()))
> > 
> > The domain sid of a AD user is not the global sam sid, so we hit the else
> > branch here and pdb_gid_to_sid() is the wrong call here for an AD user. So
> > either the hunk above is correct or we need to fix
> > get_primary_group_sid().
> 
> I would like to find a better way to do get_primary_group_sid(), but in
> the meantime, I think your code is correct enough.  It would perhaps be
> nice to only do it for the winbind case, but this would seem hard to
> distinguish.

So I have your Reviewed-by or should someone else look into it too?


I think it should be called outside of pdb_get_group_sid(). We need to 
identify the places. I wonder how the test coverage is here. Can I just remove 
the get_primary_group_sid() code and then try to fix it.
 
> I would caution however that the code in create_token_from_sid() for
> winbind users seems dubious to say the least, and that I think a new
> approach is wanted.

I don't know what you mean exactly here. This is a different codepath.

> We really need to query winbind for the token (the whole, correct info3)
> and so avoid converting it from the struct passwd, and therefore all
> this horrid, horrid code.

You mean the complete samu -> info3 conversion. Hmm. But if we don't have 
winbind running?

> 
> I'm sorry I can't be much more hopeful.
> 
> Andrew Bartlett



-- 
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             asn at samba.org
www.samba.org