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 <jhro...@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 > and > + defaults to the value of > <quote>ipa_domain</quote>. > + </para> > + <para> > + The name of the Kerberos realm has a special > + meaning in IPA - it is converted into the base > + DN 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); > if (ret != EOK) { > DEBUG(1, ("domain_to_basedn failed.\n")); > 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); > if (ret != EOK) { > goto done; > @@ -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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
