On Mon, Nov 19, 2012 at 10:54:43AM -0500, Stephen Gallagher wrote:
> On Mon 19 Nov 2012 10:35:43 AM EST, Jakub Hrozek wrote:
> >On Mon, Nov 19, 2012 at 02:29:59PM +0100, Pavel Březina wrote:
> >>On 11/19/2012 11:53 AM, Jakub Hrozek wrote:
> >>>The behaviour of ldap_sasl_authid was changed in
> >>>e81a816cddab4a62f263d1a0274d5d3f101e8e0f so that it no longer accepted full
> >>>principal, but only hostname. The realm was read from the (undocumented)
> >>>ldap_sasl_realm option.
> >>>
> >>>Moreover the "hostname" had to be specified in exactly the format that
> >>>would
> >>>match the keytab later, ie if the first match was host/hostname, the option
> >>>would have to be specified as host/hostname, otherwise the user got an
> >>>error.
> >>>
> >>>I also found out that the principal selection is only used in IPA and AD
> >>>providers, but I'll file a separate ticket to track that. Right now, I'm
> >>>mostly interested in fixing the regression.
> >>>
> >>>The attached patched modify that behaviour:
> >>>
> >>>[PATCH 1/3] MAN: document the ldap_sasl_realm option
> >>>The option was completely undocumented.
> >>>
> >>>[PATCH 2/3] LDAP: Provide a common sdap_set_sasl_options init function
> >>>The AD and IPA initialization functions shared the same code. This patch
> >>>moves the code into a common initialization function.
> >>>[PATCH 3/3] LDAP: Make it possible to use full principal in
> >>>ldap_sasl_authid again
> >>>When the guessing of principals was introduced, we changed the existing
> >>>behaviour and instead of allowing both host/hostname@REALM and
> >>>host/hostname, only the latter worked and the realm was always appended
> >>>from the (then undocumented) ldap_sasl_realm option.
> >>>
> >>>This patch changes the behaviour to be more backwards-compatible -- if
> >>>the ldap_sasl_authid contains the @-sign, then the ldap_sasl_realm is
> >>>not always appended.
> >>>
> >>>The strict requirement of checking the requested authid/realm against
> >>>the one found in keytab was also kind of relaxed because it didn't
> >>>really work. For example when hostname was requested and the keytab
> >>>matched host/hostname first, the code blew up.
> >>>
> >>>https://fedorahosted.org/sssd/ticket/1635
> >>
> >>Hi,
> >>
> >>>- if ((primary_requested && strcmp(desired_primary, sasl_primary) != 0)
> >>>||
> >>>- (realm_requested && strcmp(desired_realm, sasl_realm) != 0)) {
> >>>- DEBUG(SSSDBG_CRIT_FAILURE,
> >>>- ("Configured SASL auth ID/realm not found in keytab.\n"));
> >>>- ret = ENOENT;
> >>>- goto done;
> >>>+ if (primary_requested && strcmp(desired_primary, sasl_primary) != 0) {
> >>>+ DEBUG(SSSDBG_MINOR_FAILURE,
> >>>+ ("Configured SASL auth ID not found in keytab. "
> >>>+ "Requested %s, found %s\n", desired_primary,
> >>>sasl_primary));
> >>>+ }
> >>>+
> >>>+ if (realm_requested && strcmp(desired_realm, sasl_realm) != 0) {
> >>>+ DEBUG(SSSDBG_MINOR_FAILURE,
> >>>+ ("Configured SASL realm not found in keytab. "
> >>>+ "Requested %s, found %s\n", desired_realm, sasl_realm));
> >>> }
> >>
> >>Nack.
> >>
> >>I agree with splitting the condition but can you do it in patch #2?
> >
> >Done.
> >
>
> Sorry, I wish I had seen this request earlier. Please separate this
> into its own patch. Patches that move functions from one location to
> a common location should do that and nothing else. Moving and
> changing at the same time makes it difficult to review or examine
> later.
>
> >>
> >>Also you are not interpreting it as error any longer. Is it intentional?
> >
> >The check is too restrictive as the select_principal_from_keytab can
> >return something else than you requested right now.
> >
> >Consider that you query for host/myser...@example.com, then the
> >select_principal_from_keytab function will return "myserver" in primary
> >and "EXAMPLE.COM" in realm. So you'd need to add logic to also break
> >down the principal to get rid of the host/ part. I think the heuristics
> >would simply get too complex.
> >
> >select_principal_from_keytab will error out anyway if there's no
> >suitable principal at all.
>
>
> Yes, this is the correct behavior, though as I said above, please
> move functional changes into a separate patch from the
> de-duplication patch.
Thank you for the review. A new patchset is attached with the single
change in a separate patch.
>From c5fb79df4c70b69a613321e4d1f9034eb333b976 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Mon, 19 Nov 2012 10:32:53 +0100
Subject: [PATCH 1/4] MAN: document the ldap_sasl_realm option
The option was completely undocumented.
---
src/man/sssd-ldap.5.xml | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
index
70ada226298efda427b94c352e6e2352104a5a85..2d62c11f232c4f1fd15b547b9ac3f4cfb7c0e170
100644
--- a/src/man/sssd-ldap.5.xml
+++ b/src/man/sssd-ldap.5.xml
@@ -1426,6 +1426,19 @@
</varlistentry>
<varlistentry>
+ <term>ldap_sasl_realm (string)</term>
+ <listitem>
+ <para>
+ Specify the SASL realm to use. When not specified,
+ this option defaults to the value of krb5_realm.
+ </para>
+ <para>
+ Default: the value of krb5_realm.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term>ldap_sasl_canonicalize (boolean)</term>
<listitem>
<para>
--
1.8.0
>From 4f1c40e4ed27e4ad40c6bf0710103091583528f2 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Mon, 19 Nov 2012 10:26:18 +0100
Subject: [PATCH 2/4] LDAP: Provide a common sdap_set_sasl_options init
function
The AD and IPA initialization functions shared the same code. This patch
moves the code into a common initialization function.
---
src/providers/ad/ad_common.c | 52 +++++------------------------
src/providers/ipa/ipa_common.c | 55 +++++-------------------------
src/providers/ldap/ldap_common.c | 72 ++++++++++++++++++++++++++++++++++++++++
src/providers/ldap/ldap_common.h | 7 ++++
4 files changed, 95 insertions(+), 91 deletions(-)
diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index
21a7b53467cb6eeb426ce5266e771bd3fc5537d2..8600dab22a6392367fd6a4465177a49bffb81f8e
100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -422,13 +422,7 @@ ad_get_id_options(struct ad_options *ad_opts,
TALLOC_CTX *tmp_ctx;
struct sdap_options *id_opts;
char *krb5_realm;
- char *sasl_primary;
- char *desired_primary;
- char *sasl_realm;
- char *desired_realm;
char *keytab_path;
- bool primary_requested = true;
- bool realm_requested = true;
tmp_ctx = talloc_new(NULL);
if (!tmp_ctx) return ENOMEM;
@@ -478,19 +472,6 @@ ad_get_id_options(struct ad_options *ad_opts,
id_opts->basic[SDAP_KRB5_REALM].opt_name,
krb5_realm));
- /* Configuration of SASL auth ID and realm */
- desired_primary = dp_opt_get_string(id_opts->basic, SDAP_SASL_AUTHID);
- if (!desired_primary) {
- primary_requested = false;
- desired_primary = dp_opt_get_string(ad_opts->basic, AD_HOSTNAME);
- }
-
- desired_realm = dp_opt_get_string(id_opts->basic, SDAP_SASL_REALM);
- if (!desired_realm) {
- realm_requested = false;
- desired_realm = dp_opt_get_string(ad_opts->basic, AD_KRB5_REALM);
- }
-
keytab_path = dp_opt_get_string(ad_opts->basic, AD_KEYTAB);
if (keytab_path) {
ret = dp_opt_set_string(id_opts->basic, SDAP_KRB5_KEYTAB,
@@ -502,34 +483,17 @@ ad_get_id_options(struct ad_options *ad_opts,
keytab_path));
}
- ret = select_principal_from_keytab(tmp_ctx,
- desired_primary, desired_realm,
- keytab_path, NULL,
- &sasl_primary, &sasl_realm);
- if (ret != EOK) goto done;
-
- if ((primary_requested && strcmp(desired_primary, sasl_primary) != 0) ||
- (realm_requested && strcmp(desired_realm, sasl_realm) != 0)) {
- DEBUG(SSSDBG_FATAL_FAILURE,
- ("Configured SASL auth ID/realm not found in keytab.\n"));
- ret = ENOENT;
+ ret = sdap_set_sasl_options(id_opts,
+ dp_opt_get_string(ad_opts->basic,
+ AD_HOSTNAME),
+ dp_opt_get_string(ad_opts->basic,
+ AD_KRB5_REALM),
+ keytab_path);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, ("Cannot set the SASL-related options\n"));
goto done;
}
- ret = dp_opt_set_string(id_opts->basic, SDAP_SASL_AUTHID, sasl_primary);
- if (ret != EOK) goto done;
- DEBUG(SSSDBG_CONF_SETTINGS,
- ("Option %s set to %s\n",
- id_opts->basic[SDAP_SASL_AUTHID].opt_name,
- sasl_primary));
-
- ret = dp_opt_set_string(id_opts->basic, SDAP_SASL_REALM, sasl_realm);
- if (ret != EOK) goto done;
- DEBUG(SSSDBG_CONF_SETTINGS,
- ("Option %s set to %s\n",
- id_opts->basic[SDAP_SASL_REALM].opt_name,
- sasl_realm));
-
/* fix schema to AD */
id_opts->schema_type = SDAP_SCHEMA_AD;
diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c
index
db736921eaf02666fce24a57a297885fac23bfef..4c68f61d5c4f91f06c193ceb40631af2c242e332
100644
--- a/src/providers/ipa/ipa_common.c
+++ b/src/providers/ipa/ipa_common.c
@@ -168,14 +168,9 @@ int ipa_get_id_options(struct ipa_options *ipa_opts,
struct sdap_options **_opts)
{
TALLOC_CTX *tmpctx;
- char *primary;
char *basedn;
char *realm;
char *value;
- char *desired_realm;
- char *desired_primary;
- bool primary_requested = true;
- bool realm_requested = true;
int ret;
int i;
@@ -248,51 +243,17 @@ int ipa_get_id_options(struct ipa_options *ipa_opts,
dp_opt_get_string(ipa_opts->id->basic, SDAP_KRB5_REALM)));
}
- /* Configuration of SASL auth ID and realm */
- desired_primary = dp_opt_get_string(ipa_opts->id->basic, SDAP_SASL_AUTHID);
- if (!desired_primary) {
- primary_requested = false;
- desired_primary = dp_opt_get_string(ipa_opts->id->basic, IPA_HOSTNAME);
- }
- desired_realm = dp_opt_get_string(ipa_opts->id->basic, SDAP_SASL_REALM);
- if (!desired_realm) {
- realm_requested = false;
- desired_realm = dp_opt_get_string(ipa_opts->id->basic,
SDAP_KRB5_REALM);
- }
-
- ret = select_principal_from_keytab(tmpctx,
- desired_primary, desired_realm,
- dp_opt_get_string(ipa_opts->id->basic,
- SDAP_KRB5_KEYTAB),
- NULL, &primary, &realm);
- if (ret != EOK) {
- goto done;
- }
-
- if ((primary_requested && strcmp(desired_primary, primary) != 0) ||
- (realm_requested && strcmp(desired_realm, realm) != 0)) {
- DEBUG(1, ("Configured SASL auth ID/realm not found in keytab.\n"));
- ret = ENOENT;
- goto done;
- }
-
- ret = dp_opt_set_string(ipa_opts->id->basic,
- SDAP_SASL_AUTHID, primary);
- if (ret != EOK) {
- goto done;
- }
- DEBUG(6, ("Option %s set to %s\n",
- ipa_opts->id->basic[SDAP_SASL_AUTHID].opt_name,
- dp_opt_get_string(ipa_opts->id->basic, SDAP_SASL_AUTHID)));
-
- ret = dp_opt_set_string(ipa_opts->id->basic,
- SDAP_SASL_REALM, realm);
+ ret = sdap_set_sasl_options(ipa_opts->id,
+ dp_opt_get_string(ipa_opts->id->basic,
+ IPA_HOSTNAME),
+ dp_opt_get_string(ipa_opts->id->basic,
+ SDAP_KRB5_REALM),
+ dp_opt_get_string(ipa_opts->id->basic,
+ SDAP_KRB5_KEYTAB));
if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, ("Cannot set the SASL-related options\n"));
goto done;
}
- DEBUG(6, ("Option %s set to %s\n",
- ipa_opts->id->basic[SDAP_SASL_REALM].opt_name,
- dp_opt_get_string(ipa_opts->id->basic, SDAP_SASL_REALM)));
/* fix schema to IPAv1 for now */
ipa_opts->id->schema_type = SDAP_SCHEMA_IPA_V1;
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index
da5786fbf2acb89d3ffecfe2af1a4e035423bf4d..07e9c5d4fd09358e4157bf189703f7f7266bf164
100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -999,6 +999,78 @@ done:
return ret;
}
+errno_t
+sdap_set_sasl_options(struct sdap_options *id_opts,
+ char *default_primary,
+ char *default_realm,
+ const char *keytab_path)
+{
+ errno_t ret;
+ TALLOC_CTX *tmp_ctx;
+ char *sasl_primary;
+ char *desired_primary;
+ char *sasl_realm;
+ char *desired_realm;
+ bool primary_requested = true;
+ bool realm_requested = true;
+
+ tmp_ctx = talloc_new(NULL);
+ if (!tmp_ctx) return ENOMEM;
+
+ /* Configuration of SASL auth ID and realm */
+ desired_primary = dp_opt_get_string(id_opts->basic, SDAP_SASL_AUTHID);
+ if (!desired_primary) {
+ primary_requested = false;
+ desired_primary = default_primary;
+ }
+
+ desired_realm = dp_opt_get_string(id_opts->basic, SDAP_SASL_REALM);
+ if (!desired_realm) {
+ realm_requested = false;
+ desired_realm = default_realm;
+ }
+
+ ret = select_principal_from_keytab(tmp_ctx,
+ desired_primary, desired_realm,
+ keytab_path,
+ NULL, &sasl_primary, &sasl_realm);
+ if (ret != EOK) {
+ goto done;
+ }
+
+ if ((primary_requested && strcmp(desired_primary, sasl_primary) != 0) ||
+ (realm_requested && strcmp(desired_realm, sasl_realm) != 0)) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Configured SASL auth ID/realm not found in keytab.\n"));
+ ret = ENOENT;
+ goto done;
+ }
+
+ ret = dp_opt_set_string(id_opts->basic,
+ SDAP_SASL_AUTHID, sasl_primary);
+ if (ret != EOK) {
+ goto done;
+ }
+ DEBUG(SSSDBG_CONF_SETTINGS, ("Option %s set to %s\n",
+ id_opts->basic[SDAP_SASL_AUTHID].opt_name,
+ dp_opt_get_string(id_opts->basic, SDAP_SASL_AUTHID)));
+
+ ret = dp_opt_set_string(id_opts->basic,
+ SDAP_SASL_REALM, sasl_realm);
+ if (ret != EOK) {
+ goto done;
+ }
+
+ DEBUG(SSSDBG_CONF_SETTINGS, ("Option %s set to %s\n",
+ id_opts->basic[SDAP_SASL_REALM].opt_name,
+ dp_opt_get_string(id_opts->basic, SDAP_SASL_REALM)));
+
+ ret = EOK;
+done:
+ talloc_free(tmp_ctx);
+ return ret;
+}
+
static const char *
sdap_gssapi_get_default_realm(TALLOC_CTX *mem_ctx)
{
diff --git a/src/providers/ldap/ldap_common.h b/src/providers/ldap/ldap_common.h
index
034dc995bfa6969dd01955744735d9d6214b9c19..86079fa6ad2aedcf220d38251adc94d60d654bd1
100644
--- a/src/providers/ldap/ldap_common.h
+++ b/src/providers/ldap/ldap_common.h
@@ -228,4 +228,11 @@ sdap_attrs_get_sid_str(TALLOC_CTX *mem_ctx,
struct sysdb_attrs *sysdb_attrs,
const char *sid_attr,
char **_sid_str);
+
+errno_t
+sdap_set_sasl_options(struct sdap_options *id_opts,
+ char *default_primary,
+ char *default_realm,
+ const char *keytab_path);
+
#endif /* _LDAP_COMMON_H_ */
--
1.8.0
>From a2646b9067bffdb8f2203c1bad47d198acd951a1 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Mon, 19 Nov 2012 17:36:55 +0100
Subject: [PATCH 3/4] LDAP: Checking the principal should not be considered
fatal
The check is too restrictive as the select_principal_from_keytab can
return something else than user requested right now.
Consider that user query for host/myser...@example.com, then the
select_principal_from_keytab function will return "myserver" in primary and
"EXAMPLE.COM" in realm. So the caller needs to add logic to also break
down the principal to get rid of the host/ part. The heuristics would
simply get too complex.
select_principal_from_keytab will error out anyway if there's no
suitable principal at all.
---
src/providers/ldap/ldap_common.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index
07e9c5d4fd09358e4157bf189703f7f7266bf164..516ba179dd7ebf7559262bcda2832bca60360418
100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -1038,12 +1038,16 @@ sdap_set_sasl_options(struct sdap_options *id_opts,
goto done;
}
- if ((primary_requested && strcmp(desired_primary, sasl_primary) != 0) ||
- (realm_requested && strcmp(desired_realm, sasl_realm) != 0)) {
- DEBUG(SSSDBG_CRIT_FAILURE,
- ("Configured SASL auth ID/realm not found in keytab.\n"));
- ret = ENOENT;
- goto done;
+ if (primary_requested && strcmp(desired_primary, sasl_primary) != 0) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Configured SASL auth ID not found in keytab. "
+ "Requested %s, found %s\n", desired_primary, sasl_primary));
+ }
+
+ if (realm_requested && strcmp(desired_realm, sasl_realm) != 0) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Configured SASL realm not found in keytab. "
+ "Requested %s, found %s\n", desired_realm, sasl_realm));
}
ret = dp_opt_set_string(id_opts->basic,
--
1.8.0
>From febd0230f961356a1d02a645e464edaad7624a33 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Mon, 19 Nov 2012 17:34:56 +0100
Subject: [PATCH 4/4] LDAP: Make it possible to use full principal in
ldap_sasl_authid again
---
src/man/sssd-ldap.5.xml | 5 +++++
src/providers/ldap/ldap_common.c | 20 ++++++++++++++++----
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
index
2d62c11f232c4f1fd15b547b9ac3f4cfb7c0e170..b1be45fe2eb7c5900bcaca83d5d2ccb2335c8600
100644
--- a/src/man/sssd-ldap.5.xml
+++ b/src/man/sssd-ldap.5.xml
@@ -1418,6 +1418,9 @@
Specify the SASL authorization id to use.
When GSSAPI is used, this represents the Kerberos
principal used for authentication to the directory.
+ This option can either contain the full principal
(for
+ example host/myh...@example.com) or just the
principal name
+ (for example host/myhost).
</para>
<para>
Default: host/hostname@REALM
@@ -1431,6 +1434,8 @@
<para>
Specify the SASL realm to use. When not specified,
this option defaults to the value of krb5_realm.
+ If the ldap_sasl_authid contains the realm as well,
+ this option is ignored.
</para>
<para>
Default: the value of krb5_realm.
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index
516ba179dd7ebf7559262bcda2832bca60360418..f8b921adfe0aabc8f7ab70caab4b0201f7959c46
100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -1009,6 +1009,7 @@ sdap_set_sasl_options(struct sdap_options *id_opts,
TALLOC_CTX *tmp_ctx;
char *sasl_primary;
char *desired_primary;
+ char *primary_realm;
char *sasl_realm;
char *desired_realm;
bool primary_requested = true;
@@ -1024,12 +1025,23 @@ sdap_set_sasl_options(struct sdap_options *id_opts,
desired_primary = default_primary;
}
- desired_realm = dp_opt_get_string(id_opts->basic, SDAP_SASL_REALM);
- if (!desired_realm) {
- realm_requested = false;
- desired_realm = default_realm;
+ if ((primary_realm = strchr(desired_primary, '@'))) {
+ *primary_realm = '\0';
+ desired_realm = primary_realm+1;
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("authid contains realm [%s]\n", desired_realm));
+ } else {
+ desired_realm = dp_opt_get_string(id_opts->basic, SDAP_SASL_REALM);
+ if (!desired_realm) {
+ realm_requested = false;
+ desired_realm = default_realm;
+ }
}
+ DEBUG(SSSDBG_CONF_SETTINGS, ("Will look for %s@%s in %s\n",
+ desired_primary, desired_realm,
+ keytab_path ? keytab_path : "default keytab"));
+
ret = select_principal_from_keytab(tmp_ctx,
desired_primary, desired_realm,
keytab_path,
--
1.8.0
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
