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

[Andrew Bartlett]

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?

> +	/* 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. 

> diff --git a/source3/passdb/lookup_sid.c b/source3/passdb/lookup_sid.c
> index cfc78ad..3939fee 100644
> --- a/source3/passdb/lookup_sid.c
> +++ b/source3/passdb/lookup_sid.c
> @@ -55,8 +55,7 @@ bool lookup_name(TALLOC_CTX *mem_ctx,
>  		return false;
>  	}
>  
> -	p = strchr_m(full_name, '\\');
> -
> +	p = strchr_m(full_name, *lp_winbind_separator());
>  	if (p != NULL) {
>  		domain = talloc_strndup(tmp_ctx, full_name,
>  					PTR_DIFF(p, full_name));

It seems that with lookup_lsa_rids() as a caller, this would be
hard-coded to use '\\' due to client lookups over LSA, so should not be
changed. 

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org