[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