Re: [Freeipa-devel] [PATCH PoC] proper support of kerberos principal aliases
On Wed, 2015-09-09 at 16:21 +0200, David Kupka wrote:
> On 09/09/15 15:59, Simo Sorce wrote:
> > On Wed, 2015-09-09 at 10:52 +0200, Martin Babinsky wrote:
> >> if (found) {
> >> +/* replace the incoming principal with the value got
> >> from LDAP
> >> + * search. This is needed so that correctly case
> >> principal is
> >> + * returned in the case when canonicalization is
> >> switched on
> >> + * and no krbcanonicalname attribute is present in
> >> the entry.
> >> + */
> >> +free(*principal);
> >> +*principal = strdup(vals[i]->bv_val);
> >> +if (!(*principal)) {
> >> +return KRB5_KDB_INTERNAL_ERROR;
> >> +}
> >> break;
> >
> >
> > This unconditionally replaces the principal even when canonicalization
> > is not requested. Shouldn't this replace be conditional on
> > KRB5_KDB_FLAGS_ALIAS_OK being set ?
> >
> > Simo.
> >
>
> It's not obvious from first look but it actually depends on the
> KRB5_KDB_FLAGS_ALIAS_OK.
> When KRB5_KDB_FLAGS_ALIAS_OK is true the 'found' variable is the result
> of case-insensitive comparison.
> When it's false 'found' variable is the result of case-sensitive comparison.
> In case of case-sensitive match we're replacing the principal with the
> exactly same value though effectively not changing it.
>
Yeah I saw that, but you just said it, it is not obvious.
You should just move this chunk of code, as is, 2 blocks above where the
insensitive comparison is done.
This will make it clear that it is done only for canonicalization and
following changes won't break stuff. It will also avoid unnecessry free
and replace with the same value when canonicalization is not used.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH PoC] proper support of kerberos principal aliases
On 09/09/15 15:59, Simo Sorce wrote:
On Wed, 2015-09-09 at 10:52 +0200, Martin Babinsky wrote:
if (found) {
+/* replace the incoming principal with the value got
from LDAP
+ * search. This is needed so that correctly case
principal is
+ * returned in the case when canonicalization is
switched on
+ * and no krbcanonicalname attribute is present in
the entry.
+ */
+free(*principal);
+*principal = strdup(vals[i]->bv_val);
+if (!(*principal)) {
+return KRB5_KDB_INTERNAL_ERROR;
+}
break;
This unconditionally replaces the principal even when canonicalization
is not requested. Shouldn't this replace be conditional on
KRB5_KDB_FLAGS_ALIAS_OK being set ?
Simo.
It's not obvious from first look but it actually depends on the
KRB5_KDB_FLAGS_ALIAS_OK.
When KRB5_KDB_FLAGS_ALIAS_OK is true the 'found' variable is the result
of case-insensitive comparison.
When it's false 'found' variable is the result of case-sensitive comparison.
In case of case-sensitive match we're replacing the principal with the
exactly same value though effectively not changing it.
--
David Kupka
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH PoC] proper support of kerberos principal aliases
On Wed, 2015-09-09 at 10:52 +0200, Martin Babinsky wrote:
> if (found) {
> +/* replace the incoming principal with the value got
> from LDAP
> + * search. This is needed so that correctly case
> principal is
> + * returned in the case when canonicalization is
> switched on
> + * and no krbcanonicalname attribute is present in
> the entry.
> + */
> +free(*principal);
> +*principal = strdup(vals[i]->bv_val);
> +if (!(*principal)) {
> +return KRB5_KDB_INTERNAL_ERROR;
> +}
> break;
This unconditionally replaces the principal even when canonicalization
is not requested. Shouldn't this replace be conditional on
KRB5_KDB_FLAGS_ALIAS_OK being set ?
Simo.
--
Simo Sorce * Red Hat, Inc * New York
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH PoC] proper support of kerberos principal aliases
On Wed, 2015-09-09 at 10:52 +0200, Martin Babinsky wrote: > Work-in-progress patchset for https://fedorahosted.org/freeipa/ticket/3864 > > I didn't even format the patches according to guidelines since I will > certainly get many comments from Simo/Alexander and do a lot of > reworking. But I hope I'm at least on a right track. The direction looks good, keep in mind you have to change also install/share/modrdn-krbprinc.ldif (and related update file) to add a 'cn=Kerberos Canonical Name' rule. Simo. -- Simo Sorce * Red Hat, Inc * New York -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
