[PATCH] passdb: handle UPN in lookup_name correctly

Ralph Wuerthner ralphw at de.ibm.com
Tue Feb 12 12:38:09 UTC 2019


Hi Andreas!

On 12.02.19 10:39, Andreas Schneider wrote:
> On Monday, February 11, 2019 5:39:33 PM CET Ralph Wuerthner wrote:
>> Hi Andreas!
>>
>> On 11.02.19 16:07, Andreas Schneider wrote:
>>> Hi Ralph,
>>>
>>>> Please see attached patchset:
>>>> The fix for Samba bugzilla 13312 (commit 1775ac8aa4) caused a regression
>>>> when looking up names in UPN notation: Because winbind_lookup_name is
>>>> called with lp_workgroup as domain name the lookup is now failing and
>>>> the SID for an unmapped Unix user is returned by lookup_name. Fixed by
>>>> calling winbind_lookup_name with an empty domain name in case the name
>>>> is in UPN notation.
>>>>
>>>> The patchset already passed a CI run:
>>>> https://gitlab.com/samba-team/devel/samba/pipelines/46689980
>>>
>>> Thanks for your contribution!
>>>
>>> Please use DGB_DEBUG() instead of DEBUG(10, ...)
>>>
>>> In lookup_upn() use a helper variable 'bool ok'. And check talloc_strdup()
>>> for NULL.
>>
>> Thanks for your feedback! I prepared a new version of the patchset with
>> the following changes:
>> - using a helper variable in lookup_upn()
>> - use DBG_DEBUG() instead of DEBUG(10, ...)
>>
>> I didn't add a NULL check for talloc_strdup() because there is already a
>> NULL check right after the ok: label. This check is used by other
>> sequence steps in lookup_name() too.
> 
> But then the function would need documentation that the caller is responsible
> for the NULL check.
> 
> I think this is strange code and we should do the check in place and not defer
> it to later.

Ahh, I got your point. I thought you were referring to the 
talloc_strdup() call from patch #2 in lookup_name(), instead you are 
referring to the talloc_strdup() call in lookup_upn(). You are right, 
there should be a check in lookup_upn(). I didn't pay much attention to 
this because patch #3 is elimination the talloc_strdup() call in 
lookup_upn(). Please see the attached patchset with the missing check 
(though it will be gone after applying patch #3).

-- 
Regards

    Ralph Wuerthner
-------------- next part --------------
From f7e909f98a2b0a5320519601c83660ec6e892d79 Mon Sep 17 00:00:00 2001
From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Date: Wed, 12 Dec 2018 11:26:10 +0100
Subject: [PATCH 1/3] passdb: Add debug code to print result of lookup_name
 query

Signed-off-by: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
---
 source3/passdb/lookup_sid.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/source3/passdb/lookup_sid.c b/source3/passdb/lookup_sid.c
index 6bda783..ce2c20b 100644
--- a/source3/passdb/lookup_sid.c
+++ b/source3/passdb/lookup_sid.c
@@ -86,6 +86,7 @@ bool lookup_name(TALLOC_CTX *mem_ctx,
 	struct dom_sid sid;
 	enum lsa_SidType type;
 	TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx);
+	struct dom_sid_buf buf;
 
 	if (tmp_ctx == NULL) {
 		DEBUG(0, ("talloc_new failed\n"));
@@ -386,6 +387,11 @@ bool lookup_name(TALLOC_CTX *mem_ctx,
 		return false;
 	}
 
+	DBG_DEBUG("lookup_name results: sid=%s domain=%s (%s)\n",
+		  dom_sid_str_buf(&sid, &buf),
+		  domain,
+		  sid_type_lookup(type));
+
 	/*
 	 * Hand over the results to the talloc context we've been given.
 	 */
-- 
2.7.4


From 81f93ddfb36e51cfc487c9afc4fc677be0ce836f Mon Sep 17 00:00:00 2001
From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Date: Fri, 30 Nov 2018 17:32:51 +0100
Subject: [PATCH 2/3] passdb: handle UPN in lookup_name correctly

The fix for Samba bugzilla 13312 (commit 1775ac8aa4) caused a regression when
looking up names in UPN notation: Because winbind_lookup_name is called with
lp_workgroup as domain name the lookup is now failing and the SID for an
unmapped Unix user is returned by lookup_name.
Fixed by calling winbind_lookup_name with an empty domain name in case the
name is in UPN notation.

Signed-off-by: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
---
 source3/passdb/lookup_sid.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/source3/passdb/lookup_sid.c b/source3/passdb/lookup_sid.c
index ce2c20b..c008dd4 100644
--- a/source3/passdb/lookup_sid.c
+++ b/source3/passdb/lookup_sid.c
@@ -65,6 +65,24 @@ static bool lookup_unix_group_name(const char *name, struct dom_sid *sid)
 	return sid_compose(sid, &global_sid_Unix_Groups, grp->gr_gid);
 }
 
+static bool lookup_upn(TALLOC_CTX *mem_ctx,
+		       const char *name,
+		       struct dom_sid *sid,
+		       const char **domain,
+		       enum lsa_SidType *type)
+{
+	if (!winbind_lookup_name("", name, sid, type)) {
+		return false;
+	}
+	*domain = talloc_strdup(mem_ctx, lp_workgroup());
+
+	if (*domain == NULL) {
+		return false;
+	} else {
+		return true;
+	}
+}
+
 /*****************************************************************
  Dissect a user-provided name into domain, name, sid and type.
 
@@ -315,10 +333,25 @@ bool lookup_name(TALLOC_CTX *mem_ctx,
 	/* If we are not a DC, we have to ask in our primary domain. Let
 	 * winbind do that. */
 
-	if (!IS_DC &&
-	    (winbind_lookup_name(lp_workgroup(), name, &sid, &type))) {
-		domain = talloc_strdup(tmp_ctx, lp_workgroup());
-		goto ok;
+	if (!IS_DC) {
+		if (strchr_m(name, '@')) {
+			/* UPN */
+			if (lookup_upn(tmp_ctx,
+				       name,
+				       &sid,
+				       &domain,
+				       &type)) {
+				goto ok;
+			}
+		} else {
+			if (winbind_lookup_name(lp_workgroup(),
+						name,
+						&sid,
+						&type)) {
+			domain = talloc_strdup(tmp_ctx, lp_workgroup());
+			goto ok;
+			}
+		}
 	}
 
 	/* 9. Trusted domains */
-- 
2.7.4


From 53d23e44f7ab9ebb5c606527d64bc8f9a934ec3d Mon Sep 17 00:00:00 2001
From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Date: Thu, 7 Feb 2019 13:06:17 +0100
Subject: [PATCH 3/3] passdb: query domain name when looking up UPN names

The user could be member of a different domain, so query the domain name.

Signed-off-by: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
---
 source3/passdb/lookup_sid.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/source3/passdb/lookup_sid.c b/source3/passdb/lookup_sid.c
index c008dd4..1a020e8 100644
--- a/source3/passdb/lookup_sid.c
+++ b/source3/passdb/lookup_sid.c
@@ -71,16 +71,22 @@ static bool lookup_upn(TALLOC_CTX *mem_ctx,
 		       const char **domain,
 		       enum lsa_SidType *type)
 {
+	struct dom_sid *domain_sid = NULL;
+	enum lsa_SidType tmp;
+	NTSTATUS status;
+	bool ok;
+
 	if (!winbind_lookup_name("", name, sid, type)) {
 		return false;
 	}
-	*domain = talloc_strdup(mem_ctx, lp_workgroup());
-
-	if (*domain == NULL) {
+	status = dom_sid_split_rid(mem_ctx, sid, &domain_sid, NULL);
+	if (!NT_STATUS_IS_OK(status)) {
 		return false;
-	} else {
-		return true;
 	}
+
+	ok = winbind_lookup_sid(mem_ctx, domain_sid, domain, NULL, &tmp);
+
+	return ok;
 }
 
 /*****************************************************************
-- 
2.7.4



More information about the samba-technical mailing list