[Freeipa-devel] [PATCH PoC] proper support of kerberos principal aliases

[Simo Sorce]

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