On Fri, 2015-08-07 at 23:56 +0300, Alexander Bokovoy wrote: > On Tue, 28 Jul 2015, Sumit Bose wrote: > >On Tue, Jul 28, 2015 at 01:42:29PM +0200, Sumit Bose wrote: > >> On Tue, Jul 28, 2015 at 02:26:34PM +0300, Alexander Bokovoy wrote: > >> > On Tue, 28 Jul 2015, Simo Sorce wrote: > >> > >On Tue, 2015-07-28 at 12:15 +0200, Sumit Bose wrote: > >> > >>On Wed, Jul 22, 2015 at 09:41:51AM -0400, Simo Sorce wrote: > >> > >>> ----- Original Message ----- > >> > >>> > From: "Sumit Bose" <sb...@redhat.com> > >> > >>> > To: "freeipa-devel" <freeipa-devel@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. > >> > >> > >> > >>Thank for for the review and the comments. > >> > >> > >> > >>I changed the patch as you suggested to upper-case the realm in the > >> > >>escape function if the flag is set. > >> > >> > >> > >>I didn't add any checks to make sure that the realm of newly added > >> > >>principal attributes is always upper case. Since the attributes can be > >> > >>added via various ways I think the check should happen on the DS level > >> > > > >> > >We should indeed intercept add/modify operations and see if they try to > >> > >set krbPrincipalName/krbCanonicalName and then validate the name. > >> > >Return unwilling to perform if the case of the realm is different (or > >> > >fix it on the fly, up for discussion) from the default case as > >> > >configured in the server. > >> > Will break trusts -- ipasam does add these principals for krbtgt/IPA@AD. > >> > > >> > >>but I see this more in the context of full canonicalization fix covered > >> > >>by https://fedorahosted.org/freeipa/ticket/3864 . If you think this is > >> > >>a > >> > >>requirement for the patch attached I would suggest to drop > >> > >>https://fedorahosted.org/freeipa/ticket/4844 and solve it together with > >> > >>#3864. > >> > > > >> > >We should clsoe 4844 as fixed upstream (there *was* a bug in libkrb5). > >> > >I commented on #3864 about what we can do, and we can also avoid > >> > >changing the schema. > >> > Yep. > >> > > >> > >So on the new patches, what does "unify" means ? I do not get what it > >> > >means (so probably it is a poor name), I guess you may want to call it > >> > >"canonicalization" ? (or even 'canon' to shorten it a bit). > >> > I have same question. I tried to understand why it is called unify and > >> > failed. > >> > >> I didn't want to use 'canonical' because the result will not be the > >> canonical name in the general case but only a name we use for searching. > >> I was thinking about 'normalized' bit this has a special meaning with > >> unicode. So I came up with 'unify'. But if you prefer 'canon' I can > >> change it. > >> > >> > > >> > >I think the worst case for a utf8 string is more then length*2, probably > >> > >more like length*6, unless there is some guarantee around case changes > >> > >that I am not aware of, that said we could probably just allocate on the > >> > >stack a fixed size string of a KiB or so, the longest DNS name is 256 > >> > >chars IIRC and a service name can't be that much longer, also usernames > >> > >can't be arbitrarily long. So 1/2 KiB should probably be fine for a full > >> > >principal name. (avoids a malloc too which is good). > >> > Yes, sounds good. A hostname label can be up to 63 characters and full > >> > domain name including dots would be 253 characters. At the same time, a > >> > a component of the principal may be of arbitrary length. From practical > >> > perspective it would probably be enough to go with a static buffer of > >> > 1/2 KiB for the quickest case and fall back to malloc() if the size is > >> > bigger than that one. > >> > >> ok, I will change this. > > > >new version with changed name and 1/2 KiB buffer attached. No changes to > >the 2nd patch. > Thanks. > > Patches look good to me. I, perhaps, would have added > char *canon_princ = NULL; > > in the definition of canon_princ but as you never access it in case > asprintf() failed, that's fine. > > Simo? >
LGTM. Simo. -- 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
