[PATCH 1/2] s3-auth: fix force user for AD user
Andrew Bartlett
abartlet at samba.org
Mon Dec 2 21:44:29 MST 2013
On Thu, 2013-11-28 at 10:25 +0100, Andreas Schneider wrote:
> On Tuesday 15 November 2011 09:18:26 Andrew Bartlett wrote:
> > On Mon, 2011-11-14 at 17:31 +0100, David Disseldorp wrote:
> > > Do not add a unix_users_domain_name() username prefix prior to the
> > > lookup. This ensures winbind is consulted before a unix user SID is
> > > manually composed.
> > >
> > > Use get_primary_group_sid() only if gid_to_sid() fails lookup.
> >
> > https://bugzilla.samba.org/show_bug.cgi?id=8598
> >
> > > ---
> > >
> > > source3/auth/auth_util.c | 18 ++++++++----------
> > > source3/passdb/lookup_sid.c | 3 +--
> > > 2 files changed, 9 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/source3/auth/auth_util.c b/source3/auth/auth_util.c
> > > index fcfed83..f21cbe1 100644
> > > --- a/source3/auth/auth_util.c
> > > +++ b/source3/auth/auth_util.c
> > > @@ -677,9 +677,9 @@ NTSTATUS make_server_info_pw(struct
> >
> > auth_serversupplied_info **server_info,
> >
> > > {
> > >
> > > NTSTATUS status;
> > > struct samu *sampass = NULL;
> > >
> > > - char *qualified_name = NULL;
> > >
> > > TALLOC_CTX *mem_ctx = NULL;
> > > struct dom_sid u_sid;
> > >
> > > + struct dom_sid g_sid;
> > >
> > > enum lsa_SidType type;
> > > struct auth_serversupplied_info *result;
> > >
> > > @@ -701,15 +701,7 @@ NTSTATUS make_server_info_pw(struct
> >
> > auth_serversupplied_info **server_info,
> >
> > > return NT_STATUS_NO_MEMORY;
> > >
> > > }
> > >
> > > - qualified_name = talloc_asprintf(mem_ctx, "%s\\%s",
> > > - unix_users_domain_name(),
> > > - unix_username );
> > > - if (!qualified_name) {
> > > - TALLOC_FREE(mem_ctx);
> > > - return NT_STATUS_NO_MEMORY;
> > > - }
> > > -
> > > - if (!lookup_name(mem_ctx, qualified_name, LOOKUP_NAME_ALL,
> > > + if (!lookup_name(mem_ctx, unix_username, LOOKUP_NAME_ALL,
> > >
> > > NULL, NULL,
> > > &u_sid, &type)) {
> > >
> > > TALLOC_FREE(mem_ctx);
> > >
> > > @@ -739,6 +731,12 @@ NTSTATUS make_server_info_pw(struct
> >
> > auth_serversupplied_info **server_info,
> >
> > > /* set the user sid to be the calculated u_sid */
> > > pdb_set_user_sid(sampass, &u_sid, PDB_SET);
> >
> > As regards the first part of the patch, I will simply note 'there be
> > dragons', and to take care not to break forcing a unix user when
> > allowing an AD user to be forced, and the valid/invalid user stuff. In
> > particular, check when winbindd is not in use for nsswitch.
> >
> > Because of historical smb.conf requirements, we have not kept a very
> > good separation between 'windows style names' and simple unix usernames
> > here.
> >
> > This code needs a continuing refactor - with the introduction of the
> > 'unix domain', there needs to be a clearer separation or layering,
> > between the codepath as used the keep smbpasswd files working (in
> > make_samu_pw()), the codepath for krb5 logins when not part of AD and
> > security=server, and the codepath used for things like force user.
> >
> > Are you able to work out where this part of the regression occurred?
>
> I've tried to find the regression but no change, it must be in Samba 3.2 or
> something like that. However Günther and I looked very long at the code and we
> are confident that the part above is fixed with the attached patch.
>
> See also create_token_from_username() introduced by Andrew with
> 1c3c5e2156d9096f60bd53a96b88c2f1001d898a
>
> > > + /* 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.
> 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.
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.
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.
I'm sorry I can't be much more hopeful.
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
More information about the samba-technical
mailing list