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
[Freeipa-devel] [PATCH PoC] proper support of kerberos principal aliases
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.
--
Martin^3 Babinsky
From 913bc484c1d00d62a5ab7f31546fec87cc1b8c43 Mon Sep 17 00:00:00 2001
From: Martin Babinsky
Date: Tue, 8 Sep 2015 18:01:57 +0200
Subject: [PATCH 8/8] add case-insensitive matching rule to krbprincipalname
index
Part of https://fedorahosted.org/freeipa/ticket/3864
---
install/share/indices.ldif| 2 ++
install/updates/20-indices.update | 10 ++
2 files changed, 12 insertions(+)
diff --git a/install/share/indices.ldif b/install/share/indices.ldif
index 8c4913b569eb8be740090e1665349608be4ae932..081c15e48094f1df8cf6b8613730e6c56f9c16b9 100644
--- a/install/share/indices.ldif
+++ b/install/share/indices.ldif
@@ -6,6 +6,8 @@ cn:krbPrincipalName
nsSystemIndex:false
nsIndexType:eq
nsIndexType:sub
+nsMatchingRule: caseIgnoreIA5Match
+nsMatchingRule: caseExactIA5Match
dn: cn=ou,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
changetype: add
diff --git a/install/updates/20-indices.update b/install/updates/20-indices.update
index 9c12e0cb804066feaa7e9e3f93a06018a8d43ddd..f39310a304d37614526a425e43751bc492fa7c67 100644
--- a/install/updates/20-indices.update
+++ b/install/updates/20-indices.update
@@ -231,3 +231,13 @@ default:ObjectClass: top
default:ObjectClass: nsIndex
only:nsIndexType: eq
only:nsIndexType: pres
+
+dn: cn=krbPrincipalName,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
+default:cn: krbPrincipalName
+default:ObjectClass: top
+default:ObjectClass: nsIndex
+default:nsSystemIndex: false
+only: nsMatchingRule: caseIgnoreIA5Match
+only: nsMatchingRule: caseExactIA5Match
+only:nsIndexType: eq
+only:nsIndexType: sub
--
2.4.3
From 9a838dba6713d1b5eef73fd6c9dee210f1c86458 Mon Sep 17 00:00:00 2001
From: Martin Babinsky
Date: Tue, 8 Sep 2015 17:52:40 +0200
Subject: [PATCH 7/8] always set krbcanonicalname attribute on new principals
created using IPA API
Part of https://fedorahosted.org/freeipa/ticket/3864
---
ipalib/plugins/baseuser.py | 3 ++-
ipalib/plugins/host.py | 4 +++-
ipalib/plugins/service.py | 16
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/ipalib/plugins/baseuser.py b/ipalib/plugins/baseuser.py
index ed7c1a9d360a89ce0640dd63e748596993bb8b6c..ed880441b0f2bb768f957a7a31590b6b4d58cfc1 100644
--- a/ipalib/plugins/baseuser.py
+++ b/ipalib/plugins/baseuser.py
@@ -29,7 +29,7 @@ from ipalib import Flag, Int, Password, Str, Bool, StrEnum, DateTime, Bytes
from ipalib.plugable import Registry
from ipalib.plugins.baseldap import DN, LDAPObject, \
LDAPCreate, LDAPUpdate, LDAPSearch, LDAPDelete, LDAPRetrieve
-from ipalib.plugins.service import validate_certificate
+from ipalib.plugins.service import validate_certificate, set_krbcanonicalname
from ipalib.plugins import baseldap
from ipalib.request import context
from ipalib import _, ngettext
@@ -483,6 +483,7 @@ class baseuser_add(LDAPCreate):
"""
def pre_common_callback(self, ldap, dn, entry_attrs, **options):
assert isinstance(dn, DN)
+set_krbcanonicalname(entry_attrs)
self.obj.convert_usercertificate_pre(entry_attrs)
def post_common_callback(self, ldap, dn, entry_attrs, **options):
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 532ff66607911cfd8e1a3407b0f361641bb7992b..bf363017d4c42c058c9bc9f1dd1997d75591e3fb 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -33,7 +33,8 @@ from ipalib.plugins.baseldap import (LDAPQuery, LDAPObject, LDAPCreate,
from ipalib.plugins.service import (split_principal, validate_certificate,
set_certificate_attrs, ticket_flags_params, update_krbticketflags,
set_kerberos_attrs, rename_ipaallowedtoperform_from_ldap,
-rename_ipaallowedtoperform_to_ldap, revoke_certs)
+rename_ipaallowedtoperform_to_ldap, revoke_certs,
+set_krbcanonicalname)
from ipalib.plugins.dns import (dns_container_exists, _record_types,
add_records_for_host_validation, add_records_for_host,
get_reverse_zone)
@@ -633,6 +634,7 @@ class host_add(LDAPCreate):
entry_attrs['objectclass'].append('krbprincipalaux')
if 'krbprincipal' not in entry_attrs['objectclass']:
entry_attrs['objectclass'].append('krbprincipal')
+set_krbcanonicalname(entry_attrs)
else:
if 'krbprincipalaux' in entry_attrs['objectclass']:
entry_attrs['objectclass'].remove('krbprincipalaux')
diff --git a/ipalib/plugins/service.py b/ipalib/plugins/service.py
index 0e188dad4a215a6882cf2a458e0bd1de80f4c58a..74e0f0dc716534b7fd2eb476cff1d1e0f3ba0f1d 100644
--- a/ipalib/plugins/service.py
+++ b/ipalib/plugins/service.py
@@ -361,6 +361,19 @@ def set_kerberos_attrs(entry_attrs, options):
if name
