[RFC]fix for multiple domains support without NTLMSSP

Uri Simchoni uri at samba.org
Thu Jan 28 13:06:47 UTC 2016


Hi,

Attached is some initial work to fix 
https://bugzilla.samba.org/show_bug.cgi?id=11691. I think I need some 
advice.

The issue is that users from a foreign domain cannot access member 
server shares if the foreign domain has a policy restricting NTLM. The 
setup of one "resource" domain for servers and other domains for users 
is quite common in enterprises (not necessarily justifiable on technical 
merits but that's another story).

The patch appears to fix the issue, but I think this area of the code 
needs some cleanup/improvement. What I'm struggling with is forming a 
clear mental picture of winbindd parent-child initialization and 
offline/online logic, and hence I'm sending this patch to see if I'm in 
the right direction and as a basis to ask some questions.

The root cause for the bug is that winbindd is trying to authenticate to 
the foreign DC using NTLMSSP and that's not allowed in this setup.

Why? because when the domain object is created at the parent winbindd, 
before forking the child, the "active_directory" member is set to false 
by default, and later, in the child, all attempts to find out if it's an 
active directory domain either fail or are skipped.

I think it's conceptually incorrect to figure out, in the child process, 
whether we're active directory or not - we obtained the properties of 
the trust when we initially discovered the domain and that should be 
enough. The patch takes this direction, and I hope that eventually we'll 
be able to remove all checks as to the type of domain from the foreign 
domain child process.

Beside being what I call "conceptually incorrect", the check that we do, 
in set_dc_type_and_flags_trustinfo(), doesn't cover the case where 
there's no direct trust between our domain and the domain in question, 
and the check in set_dc_type_and_flags_connect() can't work if we can't 
connect to the domain because we try NTLMSSP.

But even if there is a direct trust, it still doesn't work because what 
one might call a chicken-and-egg problem:
a. To find the direct trust we need a connection to our DC from within 
the child process
b. On initial attempt, we have no such connection so this fails. We also 
try using connection to the foreign domain itself, but this fails too 
because we try NTLM.
c. Having failed to determine the type of domain, we still try to 
contact it, but fail due to NTLM. This failure marks the domain as 
offline and "initialized".
d. Later, a connection to our domain is formed (yes, from each domain 
child we have a connection to a DC of our domain)
e. However, when trying to go online, since the foreign domain object is 
already "initialized", it doesn't try to figure out the domain type 
again, so it remains non-active-directory, re-tries NTLM, and fails.

I wonder whether I should tackle this chicken-and-egg problem, or just 
stipulate that it's not a responsibility of the child process to 
determine its type, and remove all trace of 
set_dc_type_and_flags_trustinfo().

Another thing I'm wondering about is the domain object initialization 
process at the parent and at the child. It seems that if the parent 
sends a request to a child using wb_domain_request_send(), it makes sure 
to first send an WINBINDD_INIT_CONNECTION request, but not all requests 
from parent to child take that route - NDR requests for example don't. 
I'm wondering what's the significance of this - is it a bug that the 
child gets initialized on some requests and doesn't on others.

Any comments would be welcome.

Thanks,
Uri.
-------------- next part --------------
From e8de02990eac09789aeb795383a352880aadf6f0 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 28 Jan 2016 14:04:47 +0200
Subject: [PATCH 1/2] winbindd: return trust parameters when internally listing
 trust

When asking a child domain process to list trusts on that domain,
return (along with trust domain names and SID) the trust properties -
flags, type, and attributes.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=11691

Signed-off-by: Uri Simchoni <uri at samba.org>
---
 source3/winbindd/winbindd_misc.c | 11 ++++----
 source3/winbindd/winbindd_util.c | 56 +++++++++++++++++++++++++++++++++-------
 2 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/source3/winbindd/winbindd_misc.c b/source3/winbindd/winbindd_misc.c
index 29831aa..d32a71e 100644
--- a/source3/winbindd/winbindd_misc.c
+++ b/source3/winbindd/winbindd_misc.c
@@ -181,11 +181,12 @@ enum winbindd_result winbindd_dual_list_trusted_domains(struct winbindd_domain *
 		}
 
 		extra_data = talloc_asprintf_append_buffer(
-			extra_data, "%s\\%s\\%s\n",
-			trusts.array[i].netbios_name,
-			trusts.array[i].dns_name,
-			sid_string_talloc(state->mem_ctx,
-					  trusts.array[i].sid));
+		    extra_data, "%s\\%s\\%s\\%u\\%u\\%u\n",
+		    trusts.array[i].netbios_name, trusts.array[i].dns_name,
+		    sid_string_talloc(state->mem_ctx, trusts.array[i].sid),
+		    trusts.array[i].trust_flags,
+		    (uint32_t)trusts.array[i].trust_type,
+		    trusts.array[i].trust_attributes);
 	}
 
 	/* add our primary domain */
diff --git a/source3/winbindd/winbindd_util.c b/source3/winbindd/winbindd_util.c
index 57ee40c..a597689 100644
--- a/source3/winbindd/winbindd_util.c
+++ b/source3/winbindd/winbindd_util.c
@@ -312,24 +312,36 @@ static void trustdom_list_done(struct tevent_req *req)
 	struct winbindd_response *response;
 	int res, err;
 	char *p;
+	struct winbindd_tdc_domain trust_params = {0};
+	ptrdiff_t extra_len;
 
 	res = wb_domain_request_recv(req, state, &response, &err);
 	if ((res == -1) || (response->result != WINBINDD_OK)) {
-		DEBUG(1, ("Could not receive trustdoms\n"));
+		DBG_WARNING("Could not receive trustdoms\n");
 		TALLOC_FREE(state);
 		return;
 	}
 
+	if (response->length < sizeof(struct winbindd_response)) {
+		DBG_ERR("ill-formed trustdom response - short length\n");
+		TALLOC_FREE(state);
+		return;
+	}
+
+	extra_len = response->length - sizeof(struct winbindd_response);
+
 	p = (char *)response->extra_data.data;
 
-	while ((p != NULL) && (*p != '\0')) {
+	while ((p - (char *)response->extra_data.data) < extra_len) {
 		char *q, *sidstr, *alt_name;
 		struct dom_sid sid;
 		char *alternate_name = NULL;
 
+		DBG_DEBUG("parsing response line '%s'\n", p);
+
 		alt_name = strchr(p, '\\');
 		if (alt_name == NULL) {
-			DEBUG(0, ("Got invalid trustdom response\n"));
+			DBG_ERR("Got invalid trustdom response\n");
 			break;
 		}
 
@@ -338,22 +350,48 @@ static void trustdom_list_done(struct tevent_req *req)
 
 		sidstr = strchr(alt_name, '\\');
 		if (sidstr == NULL) {
-			DEBUG(0, ("Got invalid trustdom response\n"));
+			DBG_ERR("Got invalid trustdom response\n");
 			break;
 		}
 
 		*sidstr = '\0';
 		sidstr += 1;
 
-		q = strchr(sidstr, '\n');
-		if (q != NULL)
-			*q = '\0';
+		q = strtok(sidstr, "\\");
+		if (q == NULL) {
+			DBG_ERR("Got invalid trustdom response\n");
+			break;
+		}
 
 		if (!string_to_sid(&sid, sidstr)) {
 			DEBUG(0, ("Got invalid trustdom response\n"));
 			break;
 		}
 
+		q = strtok(NULL, "\\");
+		if (q == NULL) {
+			DBG_ERR("Got invalid trustdom response\n");
+			break;
+		}
+
+		trust_params.trust_flags = (uint32_t)strtoul(q, NULL, 10);
+
+		q = strtok(NULL, "\\");
+		if (q == NULL) {
+			DBG_ERR("Got invalid trustdom response\n");
+			break;
+		}
+
+		trust_params.trust_type = (uint32_t)strtoul(q, NULL, 10);
+
+		q = strtok(NULL, "\n");
+		if (q == NULL) {
+			DBG_ERR("Got invalid trustdom response\n");
+			break;
+		}
+
+		trust_params.trust_attribs = (uint32_t)strtoul(q, NULL, 10);
+
 		/* use the real alt_name if we have one, else pass in NULL */
 
 		if ( !strequal( alt_name, "(null)" ) )
@@ -369,9 +407,7 @@ static void trustdom_list_done(struct tevent_req *req)
 					    &cache_methods,
 					    &sid);
 
-		p=q;
-		if (p != NULL)
-			p += 1;
+		p = q + strlen(q) + 1;
 	}
 
 	/*
-- 
2.4.3


From 7f06366fb6cad6bf26d101b2d95183a9e508683e Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 28 Jan 2016 14:05:18 +0200
Subject: [PATCH 2/2] winbindd: initialize trusted domain parameters based on
 trust

Initialize trusted domain parameters based on data obtained from
the trust object. This will make it unnecessary to re-fetch the
trust object or determine the parameters based on contacting the
domain.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=11691

Signed-off-by: Uri Simchoni <uri at samba.org>
---
 source3/winbindd/winbindd_util.c | 56 ++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/source3/winbindd/winbindd_util.c b/source3/winbindd/winbindd_util.c
index a597689..3ab9b56 100644
--- a/source3/winbindd/winbindd_util.c
+++ b/source3/winbindd/winbindd_util.c
@@ -119,9 +119,10 @@ static bool is_in_internal_domain(const struct dom_sid *sid)
    If the domain already exists in the list,
    return it and don't re-initialize.  */
 
-static struct winbindd_domain *add_trusted_domain(const char *domain_name, const char *alt_name,
-						  struct winbindd_methods *methods,
-						  const struct dom_sid *sid)
+static struct winbindd_domain *
+add_trusted_domain(const char *domain_name, const char *alt_name,
+		   struct winbindd_methods *methods, const struct dom_sid *sid,
+		   struct winbindd_tdc_domain *trust_params)
 {
 	struct winbindd_domain *domain;
 	const char *alternative_name = NULL;
@@ -243,6 +244,14 @@ static struct winbindd_domain *add_trusted_domain(const char *domain_name, const
 		if (lp_security() == SEC_ADS) {
 			domain->active_directory = true;
 		}
+	} else if (!domain->internal && trust_params != NULL) {
+		domain->domain_flags = trust_params->trust_flags;
+		domain->domain_type = trust_params->trust_type;
+		domain->domain_trust_attribs = trust_params->trust_attribs;
+
+		if (domain->domain_type == LSA_TRUST_TYPE_UPLEVEL) {
+			domain->active_directory = True;
+		}
 	}
 
 	/* Link to domain list */
@@ -403,9 +412,8 @@ static void trustdom_list_done(struct tevent_req *req)
 		 * This is important because we need the SID for sibling
 		 * domains.
 		 */
-		(void)add_trusted_domain(p, alternate_name,
-					    &cache_methods,
-					    &sid);
+		(void)add_trusted_domain(p, alternate_name, &cache_methods,
+					 &sid, &trust_params);
 
 		p = q + strlen(q) + 1;
 	}
@@ -474,10 +482,9 @@ static void rescan_forest_root_trusts( void )
 		d = find_domain_from_name_noinit( dom_list[i].domain_name );
 
 		if ( !d ) {
-			d = add_trusted_domain( dom_list[i].domain_name,
-						dom_list[i].dns_name,
-						&cache_methods,
-						&dom_list[i].sid );
+			d = add_trusted_domain(
+			    dom_list[i].domain_name, dom_list[i].dns_name,
+			    &cache_methods, &dom_list[i].sid, NULL);
 		}
 
 		if (d == NULL) {
@@ -543,10 +550,10 @@ static void rescan_forest_trusts( void )
 			   about it */
 
 			if ( !d ) {
-				d = add_trusted_domain( dom_list[i].domain_name,
-							dom_list[i].dns_name,
-							&cache_methods,
-							&dom_list[i].sid );
+				d = add_trusted_domain(dom_list[i].domain_name,
+						       dom_list[i].dns_name,
+						       &cache_methods,
+						       &dom_list[i].sid, NULL);
 			}
 
 			if (d == NULL) {
@@ -672,9 +679,8 @@ static void wb_imsg_new_trusted_domain(struct imessaging_context *msg,
 	}
 
 	d = add_trusted_domain(info.netbios_name.string,
-			       info.domain_name.string,
-			       &cache_methods,
-			       info.sid);
+			       info.domain_name.string, &cache_methods,
+			       info.sid, NULL);
 	if (d == NULL) {
 		TALLOC_FREE(frame);
 		return;
@@ -760,7 +766,7 @@ bool init_domain_list(void)
 	/* BUILTIN domain */
 
 	(void)add_trusted_domain("BUILTIN", NULL, &cache_methods,
-				    &global_sid_Builtin);
+				 &global_sid_Builtin, NULL);
 
 	/* Local SAM */
 
@@ -778,10 +784,9 @@ bool init_domain_list(void)
 				"domain info from sam.ldb\n"));
 			return false;
 		}
-		domain = add_trusted_domain(pdb_domain_info->name,
-					pdb_domain_info->dns_domain,
-					&cache_methods,
-					&pdb_domain_info->sid);
+		domain = add_trusted_domain(
+		    pdb_domain_info->name, pdb_domain_info->dns_domain,
+		    &cache_methods, &pdb_domain_info->sid, NULL);
 		TALLOC_FREE(pdb_domain_info);
 		if (domain == NULL) {
 			DEBUG(0, ("Failed to add our own, local AD "
@@ -830,7 +835,8 @@ bool init_domain_list(void)
 
 	} else {
 		(void)add_trusted_domain(get_global_sam_name(), NULL,
-					 &cache_methods, get_global_sam_sid());
+					 &cache_methods, get_global_sam_sid(),
+					 NULL);
 	}
 	/* Add ourselves as the first entry. */
 
@@ -843,8 +849,8 @@ bool init_domain_list(void)
 			return False;
 		}
 
-		domain = add_trusted_domain( lp_workgroup(), lp_realm(),
-					     &cache_methods, &our_sid);
+		domain = add_trusted_domain(lp_workgroup(), lp_realm(),
+					    &cache_methods, &our_sid, NULL);
 		if (domain) {
 			/* Even in the parent winbindd we'll need to
 			   talk to the DC, so try and see if we can
-- 
2.4.3



More information about the samba-technical mailing list