[PATCH] addon patches for domain join failure (Re: [SCM] Samba Shared Repository - branch master updated)

Michael Adam obnox at samba.org
Thu Jun 16 09:30:32 UTC 2016


On 2016-06-15 at 20:29 +0200, Günther Deschner wrote:
> The branch, master has been updated
>        via  234a470 s3:libnet: accept empty realm for AD domains when only security=domain is set.
>        via  632faa8 s3-libnet: Print error string even on successfuly completion of libnetjoin.
>        via  169e8ae s3-libnetapi: Correctly check for lp_realm.
>       from  4c408f6 libnet_join: use sitename if it was set by pre-join detection
> 
> https://git.samba.org/?p=samba.git;a=shortlog;h=master
> 
> 
> - Log -----------------------------------------------------------------
> commit 234a470f198f8f09f46aaeaf58f966faccedef18
> Author: Günther Deschner <gd at samba.org>
> Date:   Tue May 31 18:47:34 2016 +0200
> 
>     s3:libnet: accept empty realm for AD domains when only security=domain is set.
>     
>     Initial patch from Matt Rogers @ RedHat.
>     
>     BUG: https://bugzilla.samba.org/show_bug.cgi?id=11977
>     
>     Guenther
>     
>     Pair-Programmed-With: Andreas Schneider <asn at samba.org>
>     Signed-off-by: Guenther Deschner <gd at samba.org>
>     Signed-off-by: Andreas Schneider <asn at samba.org>
>     
>     Autobuild-User(master): Günther Deschner <gd at samba.org>
>     Autobuild-Date(master): Wed Jun 15 20:28:31 CEST 2016 on sn-devel-144

I have also been thinking about this and would have liked
this to have been presented to the ML first. :-)
At least I observed this with my own patch failing in rebase.
So accept my comments after the fact. ;-)
Note: I after careful contemplation, think the patch is wrong.

> diff --git a/source3/libnet/libnet_join.c b/source3/libnet/libnet_join.c
> index c007183..abb9cff 100644
> --- a/source3/libnet/libnet_join.c
> +++ b/source3/libnet/libnet_join.c
> @@ -2367,9 +2367,26 @@ static WERROR libnet_join_check_config(TALLOC_CTX *mem_ctx,
>  			W_ERROR_HAVE_NO_MEMORY(wrong_conf);
>  		}
>  
> +		/*
> +		 * We should generate the warning for the special case when
> +		 * domain is AD, "security = domain" and the realm parameter is
> +		 * not set.
> +		 */
> +		if (lp_security() == SEC_DOMAIN &&
> +		    r->out.domain_is_ad &&
> +		    !valid_realm) {
> +			libnet_join_set_error_string(mem_ctx, r,
> +				"Warning: when joining AD domains with security=domain, "
> +				"\"realm\" should be defined in the configuration (%s) "
> +				"and configuration modification was not requested",
> +				wrong_conf);
> +			return WERR_OK;
> +		}

- The place for early-success-out in the flow of
  this function is further up where valid_realm is set. This
  is the error treatment path of the function.

  ==> So this patch is *wrong* since it will let the config check
      succeed when the workgroup is wrong!

- The comment is kind-of redundant: the actual code says it all!  :-)

- The comment and debug msg are not quite correct:
  This will not only warn when realm is unset, but also
  when it is set to a different value than the detected
  dns domain name (r->out.dns_domain_name), which I think
  is OK.

- I question the content of actual warning message:
  Why *should* I configure a realm when using 'security=domain' ?
  I could buy this recommendation when also 'winbind rpc only = no'.
  So if we add a warning, we should make it dependent on the
  value of this parameter, imho.

  ==> this makes me remember that I would like to change the
      default of 'winbind rpc only' to 'yes'.
      Sending a patch separately...


>  		libnet_join_set_error_string(mem_ctx, r,
>  			"Invalid configuration (%s) and configuration modification "
>  			"was not requested", wrong_conf);
> +

Sneaking in an extra blank line? :-)

>  		return WERR_CAN_NOT_COMPLETE;
>  	}


Attached find 3 patches I propose for the fix:

#1 reverts this patch
#2 achieves the same intended difference in behavior
#3 adds the debug message in early-success path

Cheers - Michael
-------------- next part --------------
From 6b9c9d9392b209f7d57a171489c1a2c5b30fb859 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 16 Jun 2016 11:20:15 +0200
Subject: [PATCH 1/3] Revert "s3:libnet: accept empty realm for AD domains when
 only security=domain is set."

This reverts commit 234a470f198f8f09f46aaeaf58f966faccedef18.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/libnet/libnet_join.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/source3/libnet/libnet_join.c b/source3/libnet/libnet_join.c
index abb9cff..c007183 100644
--- a/source3/libnet/libnet_join.c
+++ b/source3/libnet/libnet_join.c
@@ -2367,26 +2367,9 @@ static WERROR libnet_join_check_config(TALLOC_CTX *mem_ctx,
 			W_ERROR_HAVE_NO_MEMORY(wrong_conf);
 		}
 
-		/*
-		 * We should generate the warning for the special case when
-		 * domain is AD, "security = domain" and the realm parameter is
-		 * not set.
-		 */
-		if (lp_security() == SEC_DOMAIN &&
-		    r->out.domain_is_ad &&
-		    !valid_realm) {
-			libnet_join_set_error_string(mem_ctx, r,
-				"Warning: when joining AD domains with security=domain, "
-				"\"realm\" should be defined in the configuration (%s) "
-				"and configuration modification was not requested",
-				wrong_conf);
-			return WERR_OK;
-		}
-
 		libnet_join_set_error_string(mem_ctx, r,
 			"Invalid configuration (%s) and configuration modification "
 			"was not requested", wrong_conf);
-
 		return WERR_CAN_NOT_COMPLETE;
 	}
 
-- 
2.5.5


From df4b5de43b24df6f9b5bd2919386f55ec1ca539e Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Wed, 15 Jun 2016 23:03:32 +0200
Subject: [PATCH 2/3] libnet: don't check realm setting for domain security
 joins to AD domains

Inspired by initial patch from Matt Rogers @ RedHat.

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

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/libnet/libnet_join.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/source3/libnet/libnet_join.c b/source3/libnet/libnet_join.c
index c007183..35d4bb0 100644
--- a/source3/libnet/libnet_join.c
+++ b/source3/libnet/libnet_join.c
@@ -2322,6 +2322,7 @@ static WERROR libnet_join_check_config(TALLOC_CTX *mem_ctx,
 			valid_realm = strequal(lp_realm(), r->out.dns_domain_name);
 			switch (lp_security()) {
 			case SEC_DOMAIN:
+				valid_realm = true;
 			case SEC_ADS:
 				valid_security = true;
 			}
-- 
2.5.5


From ccebf48fab08412b4794d919f60f791146d9dce9 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 16 Jun 2016 11:18:12 +0200
Subject: [PATCH 3/3] libnet: warn if realm is not set correctly in sec=domain
 joins against AD

if 'winbind rpc only' is false (otherwise we don't care).

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

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/libnet/libnet_join.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/source3/libnet/libnet_join.c b/source3/libnet/libnet_join.c
index 35d4bb0..77a57da 100644
--- a/source3/libnet/libnet_join.c
+++ b/source3/libnet/libnet_join.c
@@ -2303,6 +2303,7 @@ static WERROR libnet_join_check_config(TALLOC_CTX *mem_ctx,
 	bool valid_security = false;
 	bool valid_workgroup = false;
 	bool valid_realm = false;
+	bool ignored_realm = false;
 
 	/* check if configuration is already set correctly */
 
@@ -2323,11 +2324,25 @@ static WERROR libnet_join_check_config(TALLOC_CTX *mem_ctx,
 			switch (lp_security()) {
 			case SEC_DOMAIN:
 				valid_realm = true;
+				ignored_realm = true;
 			case SEC_ADS:
 				valid_security = true;
 			}
 
 			if (valid_workgroup && valid_realm && valid_security) {
+				if (ignored_realm &&
+				    !lp_winbind_rpc_only() &&
+				    !r->in.modify_config)
+				{
+					libnet_join_set_error_string(mem_ctx, r,
+						"Warning: when joining an AD "
+						"domain with 'security=domain' "
+						"and 'winbind rpc only = yes', "
+						"\"realm\" should be set to the"
+						" correct value (set to '%s', "
+						"should be '%s').", lp_realm(),
+						r->out.dns_domain_name);
+				}
 				/* nothing to be done */
 				return WERR_OK;
 			}
-- 
2.5.5

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160616/ee1b2111/signature.sig>


More information about the samba-technical mailing list