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

Michael Adam obnox at samba.org
Tue Jun 21 15:21:20 UTC 2016


After longer discussions with Günther and Andreas,
here are our final addon-patches for bug #11977.

This is the resulting behavior, when joining an
AD domain:

- If security = domain and winbind rpc only = no,
  we keep the new behavior of failing the join
  if realm is not set properly. (This is the right
  thing to do because samba will use AD methods
  and hence require the realm setting.)

- If security = domain and winbind rpc only = yes,
  then we igore the realm setting but print a
  warning message.

This seems to be the correct way to handle this.

It also seems there is still a discrepancy between
the behaviors of security = domain and security =ads,
because several places do check for SEC_ADS, and
if we want to have security=domain synonymous to
security=ads (against ad domains at least, as discussed
in another thread), we'll have to audit and adapt the
code to make that true... But this is for future patches.

I will push this already reviewed patchset soon
unless I get vetos, and proceed with the bug#11977
accordingly.

Cheers - Michael


On 2016-06-17 at 14:53 +0200, Michael Adam wrote:
> On 2016-06-17 at 13:59 +0200, Günther Deschner wrote:
> > Hi Michael,
> > 
> > On 16/06/16 11:30, Michael Adam wrote:
> > > - 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!
> > 
> > Good catch, thanks! We certainly need to stop once lp_workgroup does
> > not match.
> > 
> > > - The comment is kind-of redundant: the actual code says it all!
> > > :-)
> > 
> > Well, kind of yes. But I think it is fine to add an additional comment
> > to explain why this is done.
> 
> A comment that adds extra reasoning or explanation might be
> good, but the current comment is redundant. :-)
> 
> > > - 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.
> > 
> > Yes, this is intended, and I think it is OK as well :)
> 
> Hm? The incorrect comment and debug msg are intended?
> 
> > > - I question the content of actual warning message: Why *should* I
> > > configure a realm when using 'security=domain' ?
> > 
> > Because a lot of code relies on lp_realm() when talking to AD since we
> > initially moved all AD and RPC join routines to the libnetjoin
> > interfaces (e.g. the gensec gse_krb5 backend, some of the namelookup
> > code, etc.).
> 
> So as explained in the thread about 'winbind rpc only',
> I was confused about what 'security = domain' is intended to do,
> so that's the reason behind this question.
> 
> Rephrasing it : Why would I configure a realm when doing
> 'winbind rpc only = yes' ?
> 
> > > 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...
> > 
> > ??? Sorry, but why should we change the default of this ?
> 
> ==> see the other thread. Ancient misunderstanding on my end. ;-)
> 
> > >> 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? :-)
> > 
> > I think it reads nicer with having a newline here.
> 
> I was commenting on patch hygiene here.
> IMHO, that should have been omitted or put up as a separate
> patch, since it is not in the lines changed anyways.
> But that's just me and maybe I am too priestly... ;-)
> 
> > > 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
> > 
> > I think the other proposed patchset is not correct as it would then
> > emit the warning regardless whether we deal with AD or not, right?
> 
> What is 'the other proposed patchset' ?
> 
> My patchset would only print the warning
> if the domain is AD, and security is domain
> and the ream has been ignored.
> 
> 
> I guess in my patchset, the possible improvement I currently see
> based on the increased understanding I have gained
> through the discussion about security=domain and
> winbind rpc only is to make the valid_realm = true
> dependent on if (lp_winbind_rpc_only())
> 
> so :
> 
> instead of:
> 
>  	case SEC_DOMAIN:
> +		valid_realm = true;
>  	case SEC_ADS:
> 
> I might propose:
> 
>  	case SEC_DOMAIN:
> +		if (lp_winbind_rpc_only()) {
> +			valid_realm = true;
> +		}
>  	case SEC_ADS:
> 
> 
> Cheers - Michael


-------------- next part --------------
From 70e37fc4e40cc3f51fdc295ac1c072dc7eb8169b 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/2] 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>
Reviewed-by: Guenther Deschner <gd 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 0a23e0d..c98862a 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 a55921fe3ce352269e8d5ba13068e5955711a7eb 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/2] libnet: ignore realm setting for domain security joins to
 AD domains if 'winbind rpc only = true'

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>
Reviewed-by: Guenther Deschner <gd at samba.org>
---
 source3/libnet/libnet_join.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/source3/libnet/libnet_join.c b/source3/libnet/libnet_join.c
index c98862a..df4fa72 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 */
 
@@ -2322,11 +2323,26 @@ 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:
+				if (!valid_realm && lp_winbind_rpc_only()) {
+					valid_realm = true;
+					ignored_realm = true;
+				}
 			case SEC_ADS:
 				valid_security = true;
 			}
 
 			if (valid_workgroup && valid_realm && valid_security) {
+				if (ignored_realm && !r->in.modify_config)
+				{
+					libnet_join_set_error_string(mem_ctx, r,
+						"Warning: ignoring realm when "
+						"joining AD domain with "
+						"'security=domain' and "
+						"'winbind rpc only = yes'. "
+						"(realm 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/20160621/890b2818/signature.sig>


More information about the samba-technical mailing list