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

[Andrew Bartlett]

On Wed, 2013-12-04 at 11:49 +0100, Andreas Schneider wrote:
> 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 would like you to fix it to get the user's supplementary groups
correct first, so we don't just come back here. 

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

It calls into this code, as I read it, once it had done this conversion.
The issue is that this will appear to work, but only the user's primary
and primary Group SIDs will be in the token. 

make_session_info_from_username -> create_local_token ->
create_token_from_username -> create_token_from_sid

> > 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?

If we don't have winbind running, then all users are local users anyway.

For this to work properly, we need this not the be an nss_token, but to
be started with the user's real info3, or at least a much better
approximation. 

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba