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
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@IPA.REALM -> success
> > kinit -C ipauser@IPA.REALM -> success
> > kinit    ipauser@ipa.realm -> failure
> > kinit -C ipauser@ipa.realm -> failure
> > 
> > With the patch
> > kinit    ipauser@IPA.REALM -> success
> > kinit -C ipauser@IPA.REALM -> success
> > kinit    ipauser@ipa.realm -> success
> > kinit -C ipauser@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
From f75e334262219ca8d97fd09ece4ea31495e94ec9 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@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@def", false, false, "abc@def"},
+        {"abc@def", false, true, "abc@DEF"},
+        {"abc@def", true, true, "abc@DEF"},
+        {"abc@def", true, false, "abc@def"},
+        {"abc@DEF", false, false, "abc@DEF"},
+        {"abc@DEF", false, true, "abc@DEF"},
+        {"abc@DEF", true, true, "abc@DEF"},
+        {"abc@DEF", true, false, "abc@DEF"},
+        {"ab*c@def", false, false, "ab*c@def"},
+        {"ab*c@def", false, true, "ab*c@DEF"},
+        {"ab*c@def", true, true, "ab\\2ac@DEF"},
+        {"ab*c@def", true, false, "ab\\2ac@def"},
+        {"\\a(b)c@def", false, false, "\\5ca\\28b\\29c@def"},
+        {"\\a(b)c@def", false, true, "\\5ca\\28b\\29c@DEF"},
+        {"\\a(b)c@def", true, true, "\\5ca\\28b\\29c@DEF"},
+        {"\\a(b)c@def", true, false, "\\5ca\\28b\\29c@def"},
+        {"abc@de*f", false, false, "abc@de*f"},
+        {"abc@de*f", false, true, "abc@DE*F"},
+        {"abc@de*f", true, true, "abc@DE\\2AF"},
+        {"abc@de*f", true, false, "abc@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

From f52ead00a1d0600c71da4604755c3132c5423755 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@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

-- 
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

Reply via email to