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

[Andrew Bartlett]

On Thu, 2013-12-05 at 11:43 +1300, Andrew Bartlett wrote:
> 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. 

The WINBINDD_GETUSERSIDS call would seem to be ideal for this. 

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