[Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
Sumit Bose
sbose at redhat.com
Tue Jul 28 10:15:15 UTC 2015
On Wed, Jul 22, 2015 at 09:41:51AM -0400, Simo Sorce wrote:
> ----- 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.
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
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.
I added a second patch which makes the unit test a bit more robust if
the krb5.conf on the system running the tests is broken.
bye,
Sumit
>
> > 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
-------------- next part --------------
From f75e334262219ca8d97fd09ece4ea31495e94ec9 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Tue, 28 Jul 2015 11:00:41 +0200
Subject: [PATCH 149/150] IPA KDB: allow case in-sensitive realm in AS request
If the canonicalization flag is set the realm of the client principal in
an AS request (kinit) is transformed into upper-case to match the IPA
convention for realm names.
Resolves https://fedorahosted.org/freeipa/ticket/4844
---
daemons/ipa-kdb/ipa_kdb.h | 2 +-
daemons/ipa-kdb/ipa_kdb_common.c | 41 +++++++++++++++++++++++++-
daemons/ipa-kdb/ipa_kdb_principals.c | 3 +-
daemons/ipa-kdb/ipa_kdb_pwdpolicy.c | 2 +-
daemons/ipa-kdb/tests/ipa_kdb_tests.c | 55 +++++++++++++++++++++++++++++++++++
5 files changed, 99 insertions(+), 4 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h
index 4abb7335d356f976eb5dc777c94b35c81655ad79..a9d36fe259b60fdc7d500c889b18c6a2e57a3f47 100644
--- a/daemons/ipa-kdb/ipa_kdb.h
+++ b/daemons/ipa-kdb/ipa_kdb.h
@@ -133,7 +133,7 @@ struct ipadb_context *ipadb_get_context(krb5_context kcontext);
int ipadb_get_connection(struct ipadb_context *ipactx);
/* COMMON LDAP FUNCTIONS */
-char *ipadb_filter_escape(const char *input, bool star);
+char *ipadb_filter_escape(const char *input, bool star, bool unify);
krb5_error_code ipadb_simple_search(struct ipadb_context *ipactx,
char *basedn, int scope,
char *filter, char **attrs,
diff --git a/daemons/ipa-kdb/ipa_kdb_common.c b/daemons/ipa-kdb/ipa_kdb_common.c
index 112086b57c9f83895589538b5494ae81fb14a948..07d8d47214fab673aecffeef0c59f0072a5c08bc 100644
--- a/daemons/ipa-kdb/ipa_kdb_common.c
+++ b/daemons/ipa-kdb/ipa_kdb_common.c
@@ -25,9 +25,39 @@
static struct timeval std_timeout = {300, 0};
-char *ipadb_filter_escape(const char *input, bool star)
+static char *unify_princ(const char *princ)
+{
+ int ret;
+ char *p;
+ uint8_t *uc_realm;
+ char *unify_princ;
+ size_t size;
+ uint8_t *buf;
+
+ p = strrchr(princ, '@');
+ if (p == NULL) {
+ return NULL;
+ }
+
+ size = strlen(p + 1);
+ /* Assume the worst-case. */
+ buf = calloc(size * 2 + 1, sizeof(uint8_t));
+ uc_realm = u8_toupper((const uint8_t *)( p + 1), size, NULL, NULL, buf,
+ &size);
+
+ ret = asprintf(&unify_princ, "%.*s@%s", (p - princ), princ, uc_realm);
+ free(buf);
+ if (ret == -1) {
+ return NULL;
+ }
+
+ return unify_princ;
+}
+
+char *ipadb_filter_escape(const char *input, bool star, bool unify)
{
char *output;
+ char *unified;
size_t i = 0;
size_t j = 0;
@@ -75,6 +105,15 @@ char *ipadb_filter_escape(const char *input, bool star)
}
output[j] = '\0';
+ if (unify) {
+ unified = unify_princ(output);
+ /* return output in case of an error */
+ if (unified != NULL) {
+ free(output);
+ output = unified;
+ }
+ }
+
return output;
}
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index b3f8b1ad7784f55f55b4d6edd05f778a9389de27..5fb280d6217c2957b9a554cc5fd3a027ddd729fa 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -788,7 +788,8 @@ static krb5_error_code ipadb_fetch_principals(struct ipadb_context *ipactx,
/* escape filter but do not touch '*' as this function accepts
* wildcards in names */
- esc_original_princ = ipadb_filter_escape(principal, false);
+ esc_original_princ = ipadb_filter_escape(principal, false,
+ (flags & KRB5_KDB_FLAG_ALIAS_OK));
if (!esc_original_princ) {
kerr = KRB5_KDB_INTERNAL_ERROR;
goto done;
diff --git a/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c b/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c
index 076314a12840881a340763ab5693131aaccafec6..875960c5d77984c111b197ba03ad7ba79705d6e7 100644
--- a/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c
+++ b/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c
@@ -151,7 +151,7 @@ krb5_error_code ipadb_get_pwd_policy(krb5_context kcontext, char *name,
return KRB5_KDB_DBNOTINITED;
}
- esc_name = ipadb_filter_escape(name, true);
+ esc_name = ipadb_filter_escape(name, true, false);
if (!esc_name) {
return ENOMEM;
}
diff --git a/daemons/ipa-kdb/tests/ipa_kdb_tests.c b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
index edd4ae0975628d6b3abe9bab2852c990c9a8c590..eea735b9d2d59650fc4e7bb9629e35393dc93f63 100644
--- a/daemons/ipa-kdb/tests/ipa_kdb_tests.c
+++ b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
@@ -465,6 +465,60 @@ void test_dom_sid_string(void **state)
str_sid = dom_sid_string(test_ctx, &test_sid);
}
+void test_ipadb_filter_escape(void **state)
+{
+ char *out;
+ size_t c;
+
+ struct test_data {
+ const char *in;
+ bool star;
+ bool unify;
+ const char *exp_out;
+ } test_data[] = {
+ {"abc", false, false, "abc"},
+ {"abc", false, true, "abc"},
+ {"abc", true, true, "abc"},
+ {"abc", true, false, "abc"},
+ {"abc at def", false, false, "abc at def"},
+ {"abc at def", false, true, "abc at DEF"},
+ {"abc at def", true, true, "abc at DEF"},
+ {"abc at def", true, false, "abc at def"},
+ {"abc at DEF", false, false, "abc at DEF"},
+ {"abc at DEF", false, true, "abc at DEF"},
+ {"abc at DEF", true, true, "abc at DEF"},
+ {"abc at DEF", true, false, "abc at DEF"},
+ {"ab*c at def", false, false, "ab*c at def"},
+ {"ab*c at def", false, true, "ab*c at DEF"},
+ {"ab*c at def", true, true, "ab\\2ac at DEF"},
+ {"ab*c at def", true, false, "ab\\2ac at def"},
+ {"\\a(b)c at def", false, false, "\\5ca\\28b\\29c at def"},
+ {"\\a(b)c at def", false, true, "\\5ca\\28b\\29c at DEF"},
+ {"\\a(b)c at def", true, true, "\\5ca\\28b\\29c at DEF"},
+ {"\\a(b)c at def", true, false, "\\5ca\\28b\\29c at def"},
+ {"abc at de*f", false, false, "abc at de*f"},
+ {"abc at de*f", false, true, "abc at DE*F"},
+ {"abc at de*f", true, true, "abc at DE\\2AF"},
+ {"abc at de*f", true, false, "abc at de\\2af"},
+ /* Special characters must be UTF-8 encoded, don't change encoding */
+ {"abc@???", false, false, "abc@???"},
+ {"abc@???", false, true, "abc@???"},
+ {"abc@???", true, true, "abc@???"},
+ {"abc@???", true, false, "abc@???"},
+ {NULL, false, false, NULL}
+ };
+
+ out = ipadb_filter_escape(NULL, false, false);
+ assert_null(out);
+
+ for (c = 0; test_data[c]. in != NULL; c++) {
+ out = ipadb_filter_escape(test_data[c].in, test_data[c].star,
+ test_data[c].unify);
+ assert_string_equal(out, test_data[c].exp_out);
+ free(out);
+ }
+}
+
int main(int argc, const char *argv[])
{
@@ -473,6 +527,7 @@ int main(int argc, const char *argv[])
unit_test_setup_teardown(test_filter_logon_info, setup, teardown),
unit_test(test_string_to_sid),
unit_test_setup_teardown(test_dom_sid_string, setup, teardown),
+ unit_test(test_ipadb_filter_escape),
};
return run_tests(tests);
--
2.4.3
-------------- next part --------------
From f52ead00a1d0600c71da4604755c3132c5423755 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Tue, 28 Jul 2015 10:56:26 +0200
Subject: [PATCH 150/150] IPA KDB: use empty profile to init krb5 context in
tests
If the systems /etc/krb5.conf contains some unexpected or broken
configuration the test might fail. With this patch the tests are run
with an empty configuration.
---
daemons/ipa-kdb/tests/ipa_kdb_tests.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/daemons/ipa-kdb/tests/ipa_kdb_tests.c b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
index eea735b9d2d59650fc4e7bb9629e35393dc93f63..1a5b6a7edb4c691e52a026866e983c60a3fb7800 100644
--- a/daemons/ipa-kdb/tests/ipa_kdb_tests.c
+++ b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
@@ -27,6 +27,7 @@
#include <stdint.h>
#include <setjmp.h>
#include <cmocka.h>
+#include <profile.h>
#include <talloc.h>
@@ -79,8 +80,13 @@ void setup(void **state)
krb5_error_code kerr;
struct ipadb_context *ipa_ctx;
struct test_ctx *test_ctx;
+ struct _profile_t *profile;
+ long perr;
- kerr = krb5_init_context(&krb5_ctx);
+ perr = profile_init(NULL, &profile);
+ assert_int_equal(perr, 0);
+
+ kerr = krb5_init_context_profile(profile, 0, &krb5_ctx);
assert_int_equal(kerr, 0);
kerr = krb5_db_setup_lib_handle(krb5_ctx);
assert_int_equal(kerr, 0);
--
2.4.3
More information about the Freeipa-devel
mailing list