[Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request

[Simo Sorce]

----- Original Message -----
> From: "Sumit Bose" <sbose at redhat.com>
> To: "freeipa-devel" <freeipa-devel at redhat.com>
> Sent: Tuesday, July 21, 2015 7:41:14 AM
> Subject: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm	in AS request
> 
> Hi,
> 
> this patch is my suggestion to solve
> https://fedorahosted.org/freeipa/ticket/4844 .
> 
> The original issue in the ticket has two part. One is a loop in libkrb5
> which is already fixed. The other is to handle canonicalization better.

Sorry Sumit,
I see several issues with this patck.

first of all you should really not change ipadb_get_principal(), that's the
wrong place to apply your logic.

To support searching for the realm name case-insensitively all we should do
is to always forcibly upper case the realm name at the same time we build the
filter (in ipadb_fetch_principals(), if canonicalization was requested. 
Because we will never store (code to prevent that should probably be dded with
this patch) a realm name that is not all caps.
Then the post search matches should be done straight within ipadb_find_principal().

> The general way to allow canonicalization on a principal is to add the
> attributes 'krbcanonicalname'[1] and 'ipakrbprincipalalias' together
> with the objectclass 'ipaKrbPrincipal' to the user object.

We have already a ticket open since long to remove krbprincipalalias, it was
a mistake to add it and any patch that depends on it will be nacked by me.
We need to use krbPrincipalName and krbCanonicalName.

> Then the IPA
> KDB backend will use 'ipakrbprincipalalias' for case in-sensitive
> matches and  the principal from 'krbcanonicalname' will be the canonical
> principal used further on. The 'krbPrincipalName' is not suitable for
> either because it has caseExact* matching rules and is a multivalue
> attribute [2].

Case-exact match is a problem only if we do not canonicalize names when storing
them, otherwise all you need to do is store a "search form" in krbPrincipalName
and always change searches to that form (forcibly upper case realm, forcibly
lowercase components) when canonicalization is requested.

Additionally in the patch you are using stcasecmp(), that function is not
acceptable, look at ipadb_find_principal() and you'll see we use ulc_casecmp()
there.
Also modyfing the principal before searching is done wrong (you use strchr()
to find the @ sign, but you could find an @ in the components this way, you
should use strrchr() at the very least), and is dangerous if done outside of
the inner functions because then we never have a way to know the original
form should it be needed. In any case as said above realm should be forcibly
uppercase, given a flag in the escape function instead.

> What I got from the comments in the ticket and the related bugzilla
> ticket is that it should be possible to get a TGT for a user even if the
> realm is given in lower-case if canonicalization is enabled. Please note
> that the client can only send such request because we have
> 'dns_lookup_kdc = true' in krb.conf and DNS is case in-sensitive. If you
> set 'dns_lookup_kdc = false' the client will fail immediately without
> sending a request at all, because it is not able to find a KDC for the
> lower-case realm.
> 
> On the server-side the request is processed because of
> http://k5wiki.kerberos.org/wiki/Projects/Aliases which made parts of
> processing case in-sensitive.
> 
> With the attached patch a second lookup is done if the lookup with the
> original input returned no result, canonicalization is
> enabled and the realm from the original input matches the IPA realm case
> in-sensitive. For the second lookup the realm is replace with the IPA
> realm. This approach adds a bit redundant code but does not add extra
> processing requests which would be successful before.
> 
> Without the patch
> kinit    ipauser at IPA.REALM -> success
> kinit -C ipauser at IPA.REALM -> success
> kinit    ipauser at ipa.realm -> failure
> kinit -C ipauser at ipa.realm -> failure
> 
> With the patch
> kinit    ipauser at IPA.REALM -> success
> kinit -C ipauser at IPA.REALM -> success
> kinit    ipauser at ipa.realm -> success
> kinit -C ipauser at ipa.realm -> success
> 
> where 'ipa.realm' can be replace by mixed case version like 'iPa.ReAlM'
> as well.
> 
> bye,
> Sumit
> 
> [1] I was not able to add 'krbcanonicalname' as admin user because of an
> ACI denial. I wonder if this is expected or if the ACI rules should be
> extended here?

Yes, we need to fix this, it's a bug that admins can't set the canonical name.

> [2] We might to skip the requirement that 'krbcanonicalname' must exists
> if 'ipaKrbPrincipal' only has a single value but canonicalization will
> fail immediately if someone adds a second value so I guess it would be
> more safe to keep it as it is.

If someone adds a second value we must have code to set krbCanonicalName
anyway or we will not know anymore what is the canonical name. So this also
needs fixing in this patchset probably, by adding checks to the add/modify
principal functions.

HTH,
Simo.

-- 
Simo Sorce * Red Hat, Inc. * New York