-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
https://fedorahosted.org/sssd/ticket/807
On Wed, Feb 23, 2011 at 05:24:20PM +0100, Jakub Hrozek wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
NACK, please use the "new" option from 'Add krb5_realm to the basic IPA options'
bye, Sumit
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk1lNLQACgkQHsardTLnvCXwLQCeIyfH5p8m/uHFbRvlZuoU8pam alIAoJ80vVTY6RDOdt7DzSmtxTJpzhiZ =SuLO -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2011 05:33 PM, Sumit Bose wrote:
On Wed, Feb 23, 2011 at 05:24:20PM +0100, Jakub Hrozek wrote: https://fedorahosted.org/sssd/ticket/807
NACK, please use the "new" option from 'Add krb5_realm to the basic IPA options'
bye, Sumit
See, that's what I get for not reading today's patches.
New one attached. I also explicitly set IPA_KRB5_REALM for cases where we use the uppercase domain as realm value so it's always available.
On Wed, Feb 23, 2011 at 05:59:13PM +0100, Jakub Hrozek wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2011 05:33 PM, Sumit Bose wrote:
On Wed, Feb 23, 2011 at 05:24:20PM +0100, Jakub Hrozek wrote: https://fedorahosted.org/sssd/ticket/807
NACK, please use the "new" option from 'Add krb5_realm to the basic IPA options'
bye, Sumit
See, that's what I get for not reading today's patches.
New one attached. I also explicitly set IPA_KRB5_REALM for cases where we use the uppercase domain as realm value so it's always available.
Patch looks good, would you mind to add a paragraph to the man page of the IPA provider which explains the special role of krb5_realm for the IPA provider?
bye, Sumit
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk1lPOEACgkQHsardTLnvCXlnwCfbWtpQ85blO8m0HVGT7HPHW++ kQgAn2JIylwVY/L5CiQvm0fkkWqsy5Sz =nZtP -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2011 06:18 PM, Sumit Bose wrote:
On Wed, Feb 23, 2011 at 05:59:13PM +0100, Jakub Hrozek wrote: On 02/23/2011 05:33 PM, Sumit Bose wrote:
On Wed, Feb 23, 2011 at 05:24:20PM +0100, Jakub Hrozek wrote: https://fedorahosted.org/sssd/ticket/807
NACK, please use the "new" option from 'Add krb5_realm to the basic IPA options'
bye, Sumit
See, that's what I get for not reading today's patches.
New one attached. I also explicitly set IPA_KRB5_REALM for cases where we use the uppercase domain as realm value so it's always available.
Patch looks good, would you mind to add a paragraph to the man page of the IPA provider which explains the special role of krb5_realm for the IPA provider?
bye, Sumit
Of course. A new patch is attached. I did one more modification - the values of SDAP_KRB5_REALM and KRB5_REALM for ipa_id and ipa_auth respectively now default to IPA_KRB5_REALM, not IPA_DOMAIN.toupper(). It should technically be the same, but I think the new way is more consistent.
Jakub
On Thu, Feb 24, 2011 at 12:23:05PM +0100, Jakub Hrozek wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2011 06:18 PM, Sumit Bose wrote:
On Wed, Feb 23, 2011 at 05:59:13PM +0100, Jakub Hrozek wrote: On 02/23/2011 05:33 PM, Sumit Bose wrote:
On Wed, Feb 23, 2011 at 05:24:20PM +0100, Jakub Hrozek wrote: https://fedorahosted.org/sssd/ticket/807
NACK, please use the "new" option from 'Add krb5_realm to the basic IPA options'
bye, Sumit
See, that's what I get for not reading today's patches.
New one attached. I also explicitly set IPA_KRB5_REALM for cases where we use the uppercase domain as realm value so it's always available.
Sorry for not sseing this earlier, but would you mind to move the whole setting of IPA_KRB5_REALM from ipa_service_init() to ipa_get_options()? I think it it safer to set it here so that it is really always available.
Patch looks good, would you mind to add a paragraph to the man page of the IPA provider which explains the special role of krb5_realm for the IPA provider?
bye, Sumit
Of course. A new patch is attached. I did one more modification - the values of SDAP_KRB5_REALM and KRB5_REALM for ipa_id and ipa_auth respectively now default to IPA_KRB5_REALM, not IPA_DOMAIN.toupper(). It should technically be the same, but I think the new way is more consistent.
Good point. While reading 'toupper' here I start thinking if we should add a 'tolower' to domain_to_basedn() as realm_to_suffix() does in FreeIPA. This is a cosmetic change because the case shouldn't matter here, but since we print the baseDN in the debug logs it might irritate the untrained eye.
bye, Sumit
Jakub -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk1mP5kACgkQHsardTLnvCVjigCfXS0ioFiWeSzYG051H/13Ri7r iC4AnR9NNqcJ10gHZXNw09AgGdoYQC5R =HOE/ -----END PGP SIGNATURE-----
From 2b4c66952a353c0b352401b6035e73506b3f1ac7 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 23 Feb 2011 17:40:44 +0100 Subject: [PATCH] Use realm for basedn instead of IPA domain
https://fedorahosted.org/sssd/ticket/807
src/man/sssd-ipa.5.xml | 15 +++++++++++++++ src/providers/ipa/ipa_access.c | 2 +- src/providers/ipa/ipa_auth.c | 12 ++++++------ src/providers/ipa/ipa_common.c | 19 +++++++++---------- 4 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/src/man/sssd-ipa.5.xml b/src/man/sssd-ipa.5.xml index 606581d..4604c55 100644 --- a/src/man/sssd-ipa.5.xml +++ b/src/man/sssd-ipa.5.xml @@ -161,6 +161,21 @@ </listitem> </varlistentry>
<varlistentry><term>krb5_realm (string)</term><listitem><para>The name of the Kerberos realm. This is optional anddefaults to the value of <quote>ipa_domain</quote>.</para><para>The name of the Kerberos realm has a specialmeaning in IPA - it is converted into the baseDN to use for performing LDAP operations.</para></listitem></varlistentry></variablelist> </para></refsect1>
diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c index 02b0a77..f07eb7b 100644 --- a/src/providers/ipa/ipa_access.c +++ b/src/providers/ipa/ipa_access.c @@ -74,7 +74,7 @@ static char *get_hbac_search_base(TALLOC_CTX *mem_ctx, DEBUG(9, ("ipa_hbac_search_base not available, trying base DN.\n"));
ret = domain_to_basedn(mem_ctx,
dp_opt_get_string(ipa_options, IPA_DOMAIN),
dp_opt_get_string(ipa_options, IPA_KRB5_REALM), &base);diff --git a/src/providers/ipa/ipa_auth.c b/src/providers/ipa/ipa_auth.c index eb7f291..d8d8ad5 100644 --- a/src/providers/ipa/ipa_auth.c +++ b/src/providers/ipa/ipa_auth.c @@ -46,7 +46,7 @@ struct get_password_migration_flag_state { struct sdap_handle *sh; enum sdap_result result; struct fo_server *srv;
- char *ipa_domain;
- char *ipa_realm; bool password_migration;
};
@@ -56,13 +56,13 @@ static void get_password_migration_flag_done(struct tevent_req *subreq); static struct tevent_req *get_password_migration_flag_send(TALLOC_CTX *memctx, struct tevent_context *ev, struct sdap_auth_ctx *sdap_auth_ctx,
char *ipa_domain)
char *ipa_realm){ int ret; struct tevent_req *req, *subreq; struct get_password_migration_flag_state *state;
- if (sdap_auth_ctx == NULL || ipa_domain == NULL) {
- if (sdap_auth_ctx == NULL || ipa_realm == NULL) { DEBUG(1, ("Missing parameter.\n")); return NULL; }
@@ -80,7 +80,7 @@ static struct tevent_req *get_password_migration_flag_send(TALLOC_CTX *memctx, state->result = SDAP_ERROR; state->srv = NULL; state->password_migration = false;
- state->ipa_domain = ipa_domain;
state->ipa_realm = ipa_realm;
/* We request to use StartTLS here, because if password migration is
- enabled we will use this connection for authentication, too. */
@@ -126,7 +126,7 @@ static void get_password_migration_flag_auth_done(struct tevent_req *subreq) return; }
- ret = domain_to_basedn(state, state->ipa_domain, &ldap_basedn);
- ret = domain_to_basedn(state, state->ipa_realm, &ldap_basedn); if (ret != EOK) { DEBUG(1, ("domain_to_basedn failed.\n")); tevent_req_error(req, ret);
@@ -311,7 +311,7 @@ static void ipa_auth_handler_done(struct tevent_req *req) state->ipa_auth_ctx->sdap_auth_ctx, dp_opt_get_string( state->ipa_auth_ctx->ipa_options,
IPA_DOMAIN));
IPA_KRB5_REALM)); if (req == NULL) { DEBUG(1, ("get_password_migration_flag failed.\n")); goto done;diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c index 397e418..59b6929 100644 --- a/src/providers/ipa/ipa_common.c +++ b/src/providers/ipa/ipa_common.c @@ -273,7 +273,7 @@ int ipa_get_id_options(struct ipa_options *ipa_opts, }
ret = domain_to_basedn(tmpctx,
dp_opt_get_string(ipa_opts->basic, IPA_DOMAIN),
dp_opt_get_string(ipa_opts->basic, IPA_KRB5_REALM), &basedn);@@ -319,16 +319,13 @@ int ipa_get_id_options(struct ipa_options *ipa_opts,
/* set krb realm */ if (NULL == dp_opt_get_string(ipa_opts->id->basic, SDAP_KRB5_REALM)) {
realm = dp_opt_get_string(ipa_opts->basic, IPA_DOMAIN);
realm = dp_opt_get_string(ipa_opts->basic, IPA_KRB5_REALM); value = talloc_strdup(tmpctx, realm); if (value == NULL) { DEBUG(1, ("talloc_strdup failed.\n")); ret = ENOMEM; goto done; }
for (i = 0; value[i]; i++) {value[i] = toupper(value[i]);} ret = dp_opt_set_string(ipa_opts->id->basic, SDAP_KRB5_REALM, value); if (ret != EOK) {@@ -467,7 +464,6 @@ int ipa_get_auth_options(struct ipa_options *ipa_opts, char *value; char *copy = NULL; int ret;
int i;
/* self check test, this should never fail, unless someone forgot
- to properly update the code after new ldap options have been added */
@@ -501,7 +497,7 @@ int ipa_get_auth_options(struct ipa_options *ipa_opts,
/* set krb realm */ if (NULL == dp_opt_get_string(ipa_opts->auth, KRB5_REALM)) {
value = dp_opt_get_string(ipa_opts->basic, IPA_DOMAIN);
value = dp_opt_get_string(ipa_opts->basic, IPA_KRB5_REALM); if (!value) { ret = ENOMEM; goto done;@@ -512,9 +508,6 @@ int ipa_get_auth_options(struct ipa_options *ipa_opts, ret = ENOMEM; goto done; }
for (i = 0; copy[i]; i++) {copy[i] = toupper(copy[i]);} ret = dp_opt_set_string(ipa_opts->auth, KRB5_REALM, copy); if (ret != EOK) { goto done;@@ -673,6 +666,12 @@ int ipa_service_init(TALLOC_CTX *memctx, struct be_ctx *ctx, service->krb5_service->realm[i] = toupper(service->krb5_service->realm[i]); }
ret = dp_opt_set_string(options->basic, IPA_KRB5_REALM,service->krb5_service->realm);if (ret != EOK) {goto done;}}
if (!servers) {
-- 1.7.4
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/24/2011 01:07 PM, Sumit Bose wrote:
Sorry for not sseing this earlier, but would you mind to move the whole setting of IPA_KRB5_REALM from ipa_service_init() to ipa_get_options()? I think it it safer to set it here so that it is really always available.
OK. I think the code even reads better now.
> Patch looks good, would you mind to add a paragraph to the man page of > the IPA provider which explains the special role of krb5_realm for the > IPA provider?
> bye, > Sumit
Of course. A new patch is attached. I did one more modification - the values of SDAP_KRB5_REALM and KRB5_REALM for ipa_id and ipa_auth respectively now default to IPA_KRB5_REALM, not IPA_DOMAIN.toupper(). It should technically be the same, but I think the new way is more consistent.
Good point. While reading 'toupper' here I start thinking if we should add a 'tolower' to domain_to_basedn() as realm_to_suffix() does in FreeIPA. This is a cosmetic change because the case shouldn't matter here, but since we print the baseDN in the debug logs it might irritate the untrained eye.
Added and changed unit tests accordingly.
Jakub
On Thu, Feb 24, 2011 at 06:36:34PM +0100, Jakub Hrozek wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/24/2011 01:07 PM, Sumit Bose wrote:
Sorry for not sseing this earlier, but would you mind to move the whole setting of IPA_KRB5_REALM from ipa_service_init() to ipa_get_options()? I think it it safer to set it here so that it is really always available.
OK. I think the code even reads better now.
>> Patch looks good, would you mind to add a paragraph to the man page of >> the IPA provider which explains the special role of krb5_realm for the >> IPA provider?
>> bye, >> Sumit
Of course. A new patch is attached. I did one more modification - the values of SDAP_KRB5_REALM and KRB5_REALM for ipa_id and ipa_auth respectively now default to IPA_KRB5_REALM, not IPA_DOMAIN.toupper(). It should technically be the same, but I think the new way is more consistent.
Good point. While reading 'toupper' here I start thinking if we should add a 'tolower' to domain_to_basedn() as realm_to_suffix() does in FreeIPA. This is a cosmetic change because the case shouldn't matter here, but since we print the baseDN in the debug logs it might irritate the untrained eye.
Added and changed unit tests accordingly.
Thank you.
ACK
bye, Sumit
Jakub -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk1mlyIACgkQHsardTLnvCXgowCg0wpC6ZIL1XHuo1KGkXCfFmsz AeMAn1F7QJFY9SMDybj2GibmH5stsvw8 =ckjo -----END PGP SIGNATURE-----
On Fri, 25 Feb 2011 10:25:04 +0100 Sumit Bose sbose@redhat.com wrote:
Added and changed unit tests accordingly.
Thank you.
ACK
Pushed to master.
Simo.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce -
Sumit Bose