[PATCH] s3-winbindd: Use correct realm for trusted domains in idmap child
Christof Schmitt
cs at samba.org
Thu Aug 28 16:03:33 MDT 2014
On Thu, Aug 28, 2014 at 02:00:00PM -0700, Jeremy Allison wrote:
> On Thu, Aug 28, 2014 at 12:35:14PM -0700, Christof Schmitt wrote:
> >
> > It took also me a bit to figure this out, so any feedback is valuable.
> >
> > ads_cached_connection_connect takes two realm parameters (in the
> > second and sixth position). The one in the second position is used for
> > ads_init() and identifies the realm of the domain controller we want to
> > connect to. The parameter in the sixth position identifies our local
> > realm that is used for obtaining a kerberos ticket (and our auth realm
> > is the local domain).
> >
> > The logic after "if (IS_DC)" obtains the realm of the local/primary
> > domain, so that is correct for the parameter in the sixth position. It
> > should not be used for identifying the realm of the DC we want to
> > connect to, since that can also be a trusted domain.
> >
> > This is not entirely obvious, and it was also me who introduced this
> > problem. Maybe using better names for the variables and parameters would help
> > here (e.g. auth_realm, target_realm), but i would like to get this
> > problem addressed first.
>
> Thanks a *LOT* for the explaination - makes sense to me, thanks.
>
> Given that - how about the following version of the patch.
> Might help make the code a bit more self-documenting ? If
> you're happy with it, feel free to push with my 'Reviewed-by'.
>
> Jeremy.
> From a72150272091a0ffb86cf1745814c297a2f83510 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 28 Aug 2014 13:53:26 -0700
> Subject: [PATCH] s3-winbindd: Use correct realm for trusted domains in idmap
> child
>
> When authenticating users in a trusted domain, the idmap_ad module
> always connects to a local DC instead of one in the trusted domain.
>
> Fix this by passing the correct realm to connect to.
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> Reviewed-by: Jeremy Allison <jra at samba.org>
> ---
> source3/winbindd/winbindd_ads.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/source3/winbindd/winbindd_ads.c b/source3/winbindd/winbindd_ads.c
> index a869ff5..c247ae0 100644
> --- a/source3/winbindd/winbindd_ads.c
> +++ b/source3/winbindd/winbindd_ads.c
> @@ -188,8 +188,13 @@ ADS_STATUS ads_idmap_cached_connection(ADS_STRUCT **adsp, const char *dom_name)
> }
> }
>
> - status = ads_cached_connection_connect(adsp, realm, dom_name, ldap_server,
> - password, realm, 0);
> + status = ads_cached_connection_connect(adsp, /* Returns ads struct. */
> + wb_dom->alt_name, /* realm to connect to. */
> + dom_name, /* 'workgroup' name for ads_init */
> + ldap_server, /* DNS name to connect to. */
> + password, /* password for auth realm. */
> + realm, /* realm used for krb5 ticket. */
> + 0); /* renewable ticket time. */
> SAFE_FREE(realm);
> TALLOC_FREE(ldap_server);
>
> --
> 2.1.0.rc2.206.gedb03e5
I would still like to have this documented in
ads_cached_connection_connect. What do you think of the attached patches
to clarify this function call? Feel free to squash them.
Christof
-------------- next part --------------
From 93f5b3d83449c048d9ed9a9df4266f202cdf7626 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 22 Aug 2014 09:15:59 -0700
Subject: [PATCH 1/4] s3-winbindd: Use correct realm for trusted domains in idmap child
When authenticating users in a trusted domain, the idmap_ad module
always connects to a local DC instead of one in the trusted domain.
Fix this by passing the correct realm to connect to.
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/winbindd/winbindd_ads.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/source3/winbindd/winbindd_ads.c b/source3/winbindd/winbindd_ads.c
index a869ff5..1b9dadb 100644
--- a/source3/winbindd/winbindd_ads.c
+++ b/source3/winbindd/winbindd_ads.c
@@ -188,8 +188,8 @@ ADS_STATUS ads_idmap_cached_connection(ADS_STRUCT **adsp, const char *dom_name)
}
}
- status = ads_cached_connection_connect(adsp, realm, dom_name, ldap_server,
- password, realm, 0);
+ status = ads_cached_connection_connect(adsp, wb_dom->alt_name, dom_name,
+ ldap_server, password, realm, 0);
SAFE_FREE(realm);
TALLOC_FREE(ldap_server);
--
1.7.1
From f1d7f645b5a7871c9298560aeb7a97aeef9d48ca Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 28 Aug 2014 14:35:29 -0700
Subject: [PATCH 2/4] s3-winbindd: Comment parameters passed to ads_cached_connection_connect
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/winbindd/winbindd_ads.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/source3/winbindd/winbindd_ads.c b/source3/winbindd/winbindd_ads.c
index 1b9dadb..1da2462 100644
--- a/source3/winbindd/winbindd_ads.c
+++ b/source3/winbindd/winbindd_ads.c
@@ -188,8 +188,15 @@ ADS_STATUS ads_idmap_cached_connection(ADS_STRUCT **adsp, const char *dom_name)
}
}
- status = ads_cached_connection_connect(adsp, wb_dom->alt_name, dom_name,
- ldap_server, password, realm, 0);
+ status = ads_cached_connection_connect(
+ adsp, /* Returns ads struct. */
+ wb_dom->alt_name, /* realm to connect to. */
+ dom_name, /* 'workgroup' name for ads_init */
+ ldap_server, /* DNS name to connect to. */
+ password, /* password for auth realm. */
+ realm, /* realm used for krb5 ticket. */
+ 0); /* renewable ticket time. */
+
SAFE_FREE(realm);
TALLOC_FREE(ldap_server);
--
1.7.1
From b49b506e99d9eb42107fe5eee925da32e22bbcd1 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 28 Aug 2014 14:44:59 -0700
Subject: [PATCH 3/4] s3-winbindd: Use more descriptive parameter names in ads_cached_connection_connect
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/winbindd/winbindd_ads.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/source3/winbindd/winbindd_ads.c b/source3/winbindd/winbindd_ads.c
index 1da2462..1986dec 100644
--- a/source3/winbindd/winbindd_ads.c
+++ b/source3/winbindd/winbindd_ads.c
@@ -74,11 +74,11 @@ static void ads_cached_connection_reuse(ADS_STRUCT **adsp)
}
static ADS_STATUS ads_cached_connection_connect(ADS_STRUCT **adsp,
- const char *dom_name_alt,
- const char *dom_name,
+ const char *target_realm,
+ const char *target_dom_name,
const char *ldap_server,
char *password,
- char *realm,
+ char *auth_realm,
time_t renewable)
{
ADS_STRUCT *ads;
@@ -86,16 +86,16 @@ static ADS_STATUS ads_cached_connection_connect(ADS_STRUCT **adsp,
struct sockaddr_storage dc_ss;
fstring dc_name;
- if (realm == NULL) {
+ if (auth_realm == NULL) {
return ADS_ERROR_NT(NT_STATUS_UNSUCCESSFUL);
}
/* we don't want this to affect the users ccache */
setenv("KRB5CCNAME", WINBIND_CCACHE_NAME, 1);
- ads = ads_init(dom_name_alt, dom_name, ldap_server);
+ ads = ads_init(target_realm, target_dom_name, ldap_server);
if (!ads) {
- DEBUG(1,("ads_init for domain %s failed\n", dom_name));
+ DEBUG(1,("ads_init for domain %s failed\n", target_dom_name));
return ADS_ERROR(LDAP_NO_MEMORY);
}
@@ -105,7 +105,7 @@ static ADS_STATUS ads_cached_connection_connect(ADS_STRUCT **adsp,
ads->auth.renewable = renewable;
ads->auth.password = password;
- ads->auth.realm = SMB_STRDUP(realm);
+ ads->auth.realm = SMB_STRDUP(auth_realm);
if (!strupper_m(ads->auth.realm)) {
ads_destroy(&ads);
return ADS_ERROR_NT(NT_STATUS_INTERNAL_ERROR);
@@ -119,7 +119,7 @@ static ADS_STATUS ads_cached_connection_connect(ADS_STRUCT **adsp,
status = ads_connect(ads);
if (!ADS_ERR_OK(status)) {
DEBUG(1,("ads_connect for domain %s failed: %s\n",
- dom_name, ads_errstr(status)));
+ target_dom_name, ads_errstr(status)));
ads_destroy(&ads);
return status;
}
--
1.7.1
From d4bce6606bd59e319dcf19afaf5c2f4b3e357b87 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 28 Aug 2014 14:50:39 -0700
Subject: [PATCH 4/4] s3-winbindd: Document parameters in ads_cached_connection_reuse
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/winbindd/winbindd_ads.c | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/source3/winbindd/winbindd_ads.c b/source3/winbindd/winbindd_ads.c
index 1986dec..18fc843 100644
--- a/source3/winbindd/winbindd_ads.c
+++ b/source3/winbindd/winbindd_ads.c
@@ -73,6 +73,19 @@ static void ads_cached_connection_reuse(ADS_STRUCT **adsp)
}
}
+/**
+ * @brief Establish a connection to a DC
+ *
+ * @param[out] adsp ADS_STRUCT that will be created
+ * @param[in] target_realm Realm of domain to connect to
+ * @param[in] target_dom_name 'workgroup' name of domain to connect to
+ * @param[in] ldap_server DNS name of server to connect o
+ * @param[in] password Our machine acount secret
+ * @param[in] auth_realm Realm of local domain for creating krb token
+ * @param[in] renewable Renewable ticket time
+ *
+ * @return ADS_STATUS
+ */
static ADS_STATUS ads_cached_connection_connect(ADS_STRUCT **adsp,
const char *target_realm,
const char *target_dom_name,
--
1.7.1
More information about the samba-technical
mailing list