[PATCH 1/2] s3-auth: fix force user for AD user
Andreas Schneider
asn at samba.org
Fri Dec 13 11:27:11 MST 2013
On Tuesday 10 December 2013 14:51:41 Andrew Bartlett wrote:
> 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
Günther and I are working on it. Here is our WIP branch:
https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/force_user
-- andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team asn at samba.org
www.samba.org
More information about the samba-technical
mailing list