Attachment 17973 Details for Bug 15413 – Patches for v4-17-test

Patches for v4-17-test

bfixes-tmp417.txt (text/plain), 10.97 KB, created by Stefan Metzmacher on 2023-07-05 18:45:33 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Stefan Metzmacher

Created: 2023-07-05 18:45:33 UTC

Size: 10.97 KB

patch

obsolete

>From 4f910510a1d0b9312593c593e4b28d2c6b92b1e8 Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Tue, 4 Jul 2023 12:32:34 +0200
>Subject: [PATCH 1/5] s3:winbindd: call reset_cm_connection_on_error() in
> wb_cache_query_user_list()
>
>This is mostly for consistency, every remote call should call
>reset_cm_connection_on_error(). Note this is more than
>a simple invalidate_cm_connection() as it may set
>domain->conn.netlogon_force_reauth = true.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15413
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>Reviewed-by: Volker Lendecke <vl@samba.org>
>(cherry picked from commit cb59fd43bbf758e4bad774cfc19ef87b157052c2)
>---
> source3/winbindd/winbindd_cache.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/source3/winbindd/winbindd_cache.c b/source3/winbindd/winbindd_cache.c
>index 1835d0b9d63e..de8cab04707c 100644
>--- a/source3/winbindd/winbindd_cache.c
>+++ b/source3/winbindd/winbindd_cache.c
>@@ -1508,6 +1508,7 @@ do_query:
> 			DEBUG(3, ("query_user_list: returned 0x%08x, "
> 				  "retrying\n", NT_STATUS_V(status)));
> 		}
>+		reset_cm_connection_on_error(domain, NULL, status);
> 		if (NT_STATUS_EQUAL(status, NT_STATUS_UNSUCCESSFUL)) {
> 			DEBUG(3, ("query_user_list: flushing "
> 				  "connection cache\n"));
>-- 
>2.34.1
>
>
>From 97da035603559bd4b0ca42f67745dae0b2ee762d Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Tue, 4 Jul 2023 12:32:34 +0200
>Subject: [PATCH 2/5] s3:winbindd: make use of reset_cm_connection_on_error()
> for winbindd_lookup_{names,sids}()
>
>Note this is more than a simple invalidate_cm_connection() as it may set
>domain->conn.netlogon_force_reauth = true.
>
>This is not strictly needed as the callers call
>reset_cm_connection_on_error() via reconnect_need_retry().
>But it might avoid one roundtrip.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15413
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>Reviewed-by: Volker Lendecke <vl@samba.org>
>(cherry picked from commit 4ad5a35a3f67860aa7a1345efcfc92fe40578e31)
>---
> source3/winbindd/winbindd_msrpc.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
>diff --git a/source3/winbindd/winbindd_msrpc.c b/source3/winbindd/winbindd_msrpc.c
>index 3ac13b0e3d11..2926bd65e227 100644
>--- a/source3/winbindd/winbindd_msrpc.c
>+++ b/source3/winbindd/winbindd_msrpc.c
>@@ -954,16 +954,13 @@ NTSTATUS winbindd_lookup_sids(TALLOC_CTX *mem_ctx,
> 	/* And restore our original timeout. */
> 	dcerpc_binding_handle_set_timeout(b, orig_timeout);
> 
>-	if (NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED) ||
>-	    NT_STATUS_EQUAL(status, NT_STATUS_RPC_SEC_PKG_ERROR) ||
>-	    NT_STATUS_EQUAL(status, NT_STATUS_NETWORK_ACCESS_DENIED)) {
>+	if (reset_cm_connection_on_error(domain, b, status)) {
> 		/*
> 		 * This can happen if the schannel key is not
> 		 * valid anymore, we need to invalidate the
> 		 * all connections to the dc and reestablish
> 		 * a netlogon connection first.
> 		 */
>-		invalidate_cm_connection(domain);
> 		domain->can_do_ncacn_ip_tcp = domain->active_directory;
> 		if (!retried) {
> 			retried = true;
>@@ -1033,16 +1030,13 @@ static NTSTATUS winbindd_lookup_names(TALLOC_CTX *mem_ctx,
> 	/* And restore our original timeout. */
> 	dcerpc_binding_handle_set_timeout(b, orig_timeout);
> 
>-	if (NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED) ||
>-	    NT_STATUS_EQUAL(status, NT_STATUS_RPC_SEC_PKG_ERROR) ||
>-	    NT_STATUS_EQUAL(status, NT_STATUS_NETWORK_ACCESS_DENIED)) {
>+	if (reset_cm_connection_on_error(domain, b, status)) {
> 		/*
> 		 * This can happen if the schannel key is not
> 		 * valid anymore, we need to invalidate the
> 		 * all connections to the dc and reestablish
> 		 * a netlogon connection first.
> 		 */
>-		invalidate_cm_connection(domain);
> 		if (!retried) {
> 			retried = true;
> 			goto connect;
>-- 
>2.34.1
>
>
>From 04bcb7fa7df01c9168f8a5c31607954eb00bf71f Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Wed, 16 Feb 2022 14:19:16 +0100
>Subject: [PATCH 3/5] s3:winbindd: let winbind_samlogon_retry_loop() always
> start with authoritative = 1
>
>Otherwise we could treat a local problem as non-authoritative.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15413
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>Reviewed-by: Volker Lendecke <vl@samba.org>
>(cherry picked from commit 0cb6de4b1d5410f3699172952be81c6eb75c2c86)
>---
> source3/winbindd/winbindd_pam.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c
>index 9805d90fef03..d870188767e7 100644
>--- a/source3/winbindd/winbindd_pam.c
>+++ b/source3/winbindd/winbindd_pam.c
>@@ -1649,6 +1649,15 @@ static NTSTATUS winbind_samlogon_retry_loop(struct winbindd_domain *domain,
> 		struct rpc_pipe_client *netlogon_pipe;
> 		struct netlogon_creds_cli_context *netlogon_creds_ctx = NULL;
> 
>+		/*
>+		 * We should always reset authoritative to 1
>+		 * before calling a server again.
>+		 *
>+		 * Otherwise we could treat a local problem as
>+		 * non-authoritative.
>+		 */
>+		*authoritative = 1;
>+
> 		retry = false;
> 
> 		result = cm_connect_netlogon_secure(domain, &netlogon_pipe,
>-- 
>2.34.1
>
>
>From b8d3c7126d530facccf12976e262024b89271ab0 Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Tue, 4 Jul 2023 13:01:24 +0200
>Subject: [PATCH 4/5] s3:winbindd: make use of reset_cm_connection_on_error()
> in winbind_samlogon_retry_loop()
>
>Note this is more than a simple invalidate_cm_connection() as it may set
>domain->conn.netlogon_force_reauth = true, which is important in order
>to recover from NT_STATUS_RPC_SEC_PKG_ERROR errors.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15413
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>Reviewed-by: Volker Lendecke <vl@samba.org>
>(cherry picked from commit b317b10dffd99d1add3ff0b85b958edd9639abc8)
>---
> source3/winbindd/winbindd_pam.c | 42 ++++++++++++---------------------
> 1 file changed, 15 insertions(+), 27 deletions(-)
>
>diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c
>index d870188767e7..9953ffeb94b1 100644
>--- a/source3/winbindd/winbindd_pam.c
>+++ b/source3/winbindd/winbindd_pam.c
>@@ -1678,6 +1678,8 @@ static NTSTATUS winbind_samlogon_retry_loop(struct winbindd_domain *domain,
> 				 "(error: %s, attempts: %d)\n",
> 				  nt_errstr(result), netr_attempts));
> 
>+			reset_cm_connection_on_error(domain, NULL, result);
>+
> 			/* After the first retry always close the connection */
> 			if (netr_attempts > 0) {
> 				DEBUG(3, ("This is again a problem for this "
>@@ -1800,27 +1802,21 @@ static NTSTATUS winbind_samlogon_retry_loop(struct winbindd_domain *domain,
> 		   might not yet have noticed that the DC has killed
> 		   our connection. */
> 
>-		if (!rpccli_is_connected(netlogon_pipe)) {
>-			retry = true;
>+		retry = reset_cm_connection_on_error(domain,
>+						     netlogon_pipe->binding_handle,
>+						     result);
>+		if (retry) {
>+			DBG_PREFIX(attempts > 1 ? DBGLVL_NOTICE : DBGLVL_INFO, (
>+				   "This is problem %d for this "
>+				   "particular call,"
>+				   "DOMAIN[%s] DC[%s] - %s\n",
>+				   attempts,
>+				   domain->name,
>+				   domain->dcname,
>+				   nt_errstr(result)));
> 			continue;
> 		}
> 
>-		/* if we get access denied, a possible cause was that we had
>-		   an open connection to the DC, but someone changed our
>-		   machine account password out from underneath us using 'net
>-		   rpc changetrustpw' */
>-
>-		if ( NT_STATUS_EQUAL(result, NT_STATUS_ACCESS_DENIED) ) {
>-			DEBUG(1,("winbind_samlogon_retry_loop: sam_logon returned "
>-				 "ACCESS_DENIED.  Maybe the DC has Restrict "
>-				 "NTLM set or the trust account "
>-				"password was changed and we didn't know it. "
>-				 "Killing connections to domain %s\n",
>-				domainname));
>-			invalidate_cm_connection(domain);
>-			retry = true;
>-		}
>-
> 		if (NT_STATUS_EQUAL(result, NT_STATUS_RPC_PROCNUM_OUT_OF_RANGE)) {
> 			/*
> 			 * Got DCERPC_FAULT_OP_RNG_ERROR for SamLogon
>@@ -1845,15 +1841,7 @@ static NTSTATUS winbind_samlogon_retry_loop(struct winbindd_domain *domain,
> 			break;
> 		}
> 
>-	} while ( (attempts < 2) && retry );
>-
>-	if (NT_STATUS_EQUAL(result, NT_STATUS_IO_TIMEOUT)) {
>-		DEBUG(3,("winbind_samlogon_retry_loop: sam_network_logon(ex) "
>-				"returned NT_STATUS_IO_TIMEOUT after the retry. "
>-				"Killing connections to domain %s\n",
>-			domainname));
>-		invalidate_cm_connection(domain);
>-	}
>+	} while ( (attempts < 3) && retry );
> 
> 	if (!NT_STATUS_IS_OK(result)) {
> 		return result;
>-- 
>2.34.1
>
>
>From 5e307c37258fa57f2172a38ca47c2aa62d732f51 Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Tue, 4 Jul 2023 14:12:03 +0200
>Subject: [PATCH 5/5] s3:winbindd: let winbind_samlogon_retry_loop() fallback
> to NT_STATUS_NO_LOGON_SERVERS
>
>When we were not able to get a valid response from any DC we should
>report NT_STATUS_NO_LOGON_SERVERS with authoritative = 1.
>
>This matches what windows does. In a chain of transitive
>trusts the ACCESS_DENIED/authoritative=0 is not propagated,
>instead NT_STATUS_NO_LOGON_SERVERS/authoritative=1 is
>passed along the chain if there's no other DC is available.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15413
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>Reviewed-by: Volker Lendecke <vl@samba.org>
>(cherry picked from commit 50e771c12f84f9268c2e9ddeef0965f79f85de3d)
>---
> source3/winbindd/winbindd_pam.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
>diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c
>index 9953ffeb94b1..b1acc7efabca 100644
>--- a/source3/winbindd/winbindd_pam.c
>+++ b/source3/winbindd/winbindd_pam.c
>@@ -1637,6 +1637,7 @@ static NTSTATUS winbind_samlogon_retry_loop(struct winbindd_domain *domain,
> 	int attempts = 0;
> 	int netr_attempts = 0;
> 	bool retry = false;
>+	bool valid_result = false;
> 	NTSTATUS result;
> 	enum netr_LogonInfoClass logon_type_i;
> 	enum netr_LogonInfoClass logon_type_n;
>@@ -1817,6 +1818,8 @@ static NTSTATUS winbind_samlogon_retry_loop(struct winbindd_domain *domain,
> 			continue;
> 		}
> 
>+		valid_result = true;
>+
> 		if (NT_STATUS_EQUAL(result, NT_STATUS_RPC_PROCNUM_OUT_OF_RANGE)) {
> 			/*
> 			 * Got DCERPC_FAULT_OP_RNG_ERROR for SamLogon
>@@ -1843,6 +1846,25 @@ static NTSTATUS winbind_samlogon_retry_loop(struct winbindd_domain *domain,
> 
> 	} while ( (attempts < 3) && retry );
> 
>+	if (!valid_result) {
>+		/*
>+		 * This matches what windows does. In a chain of transitive
>+		 * trusts the ACCESS_DENIED/authoritative=0 is not propagated
>+		 * instead of NT_STATUS_NO_LOGON_SERVERS/authoritative=1 is
>+		 * passed along the chain if there's no other DC is available.
>+		 */
>+		DBG_WARNING("Mapping %s/authoritative=%u to "
>+			    "NT_STATUS_NO_LOGON_SERVERS/authoritative=1 for"
>+			    "USERNAME[%s] USERDOMAIN[%s] REMOTE-DOMAIN[%s] \n",
>+			    nt_errstr(result),
>+			    *authoritative,
>+			    username,
>+			    domainname,
>+			    domain->name);
>+		*authoritative = 1;
>+		return NT_STATUS_NO_LOGON_SERVERS;
>+	}
>+
> 	if (!NT_STATUS_IS_OK(result)) {
> 		return result;
> 	}
>-- 
>2.34.1
>

Flags:

vl: review+
asn: review+

Actions: View

Attachments on bug 15413: 17972 | 17973