PKINIT certificate authorization pluggable interface #610

mrogers950 · 2017-02-17T21:13:54Z

This PR implements the certauth plugin interface and builtin modules as outlined in https://k5wiki.kerberos.org/wiki/Projects/Certificate_authorization_pluggable_interface

greghudson · 2017-02-18T00:26:17Z

I will have more later, but from a quick look:

It looks like auth indicator strings are allocated by the certauth module and freed by the PKINIT module. That won't work if they don't use the same allocator.
I'd be happier if the test module didn't use the PKINIT object files. It could use OpenSSL directly, or its own parsing, or it could do something zany like look for a binary substring of the DER.
There is no point in having per-request moddata in this interface; a module gets no benefit out of receiving the same fixed sequence of req_init, authorize, and req_fini every time a cert comes in. There should instead be a moddata object which lasts the lifetime of the KDC process, or perhaps one per realm.
The free method for the moddata object should accept the moddata object, not a pointer to it.

greghudson · 2017-02-20T15:27:37Z

There should instead be a moddata object which lasts the lifetime of the KDC process, or perhaps one per realm.

To provide a little more guidance here: certauth_modules should not be a global variable; it should be a linked from the PKINIT kdcpreauth moddata, and should be cleaned up when that moddata is finalized. The PKINIT kdcpreauth moddata is currently an array of pkinit_kdc_context objects, one per realm. So there are two options:

For each configured realm, load the certauth modules and create an array of certauth_handle objects with a long-term moddata for each module. The module receives a moddata initialization for each configured realm.
Extend the PKINIT kdcpreauth moddata to be a structure containing the array of pkinit_kdc_context objects and also the array of certauth_handle objects. The module receives a single moddata initialization for the whole KDC process.

Option 1 may seem inefficient, but the overwhelmingly common case is for there only to be one realm served by a KDC, and even when there are multiple realms, they are relatively heavyweight objects.

Option 2 means that any per-realm data (currently represented in "opts") needs to be passed to the authorize method rather than a moddata initialization method. That might be more convenient for modules anyway.

mrogers950 · 2017-02-20T20:45:56Z

It looks like auth indicator strings are allocated by the certauth module and freed by the PKINIT module. That won't work if they don't use the same allocator.

Would it be preferable to add a "free_indicator" method for this, or provide a buffer allocated by the PKINIT module?

I'd be happier if the test module didn't use the PKINIT object files. It could use OpenSSL directly, or its own parsing, or it could do something zany like look for a binary substring of the DER.

That's fair. I don't think that we need to go too far into proper decoding and picking attributes out of the cert in the test module anyways, so checking the DER directly for the subject seems sufficient.

Option 2 means that any per-realm data (currently represented in "opts") needs to be passed to the authorize method rather than a moddata initialization method. That might be more convenient for modules anyway.

This sounds like the better option to me as well.

greghudson · 2017-02-20T21:26:25Z

Would it be preferable to add a "free_indicator" method for this, or provide a buffer allocated by the PKINIT module?

Probably the former.

Another option is for the module to just call the add_auth_indicator callback in the kdcpreauth callbacks structure. There is some potential for bad behavior there if one module adds an authentication indicator and another module says no. At that point it's unlikely that the KDC would issue a ticket because PKINIT will fail, so I'm not sure it's worth worrying about.

On an unrelated note, I was looking at the PR's server_authorize_cert() and it doesn't seem like it provides a way for a module to say "pass" as described in the wiki page.

mrogers950 · 2017-02-20T21:46:09Z

On an unrelated note, I was looking at the PR's server_authorize_cert() and it doesn't seem like it provides a way for a module to say "pass" as described in the wiki page.

If failure happens with any of the modules returning a FALSE status, is there a reason to make a distinction between a TRUE status and "pass"? I understood "pass" as meaning the module did not perform any authorization checks, for which it can just return TRUE.

greghudson · 2017-02-20T22:08:19Z

I understood "pass" as meaning the module did not perform any authorization checks, for which it can just return TRUE.

The desired accumulator behavior (which we use for several existing authorization pluggable interfaces) is that at least one module must say yes, and zero modules must say no. If a module can't give distinct "yes" and "pass" results, then there is no way to know whether any of the modules had a positive authorization result, or if they all just had nothing to say about the cert.

mrogers950 · 2017-03-01T02:05:28Z

Updated with a round of changes:

Changed the kdcpreauth_moddata to hold realm_contexts and certauth_modules, and load certauth module handles into the moddata. Some of the existing init functions were revised to accommodate this.
Changed req_init and req_fini methods to load-and-unload time fini/init. The builtin specific void *opts for the realm and request data is passed to the authorize function.
With regards to a module’s use of add_auth_indicator(), It seemed to be simpler to provide an indicator freeing method, rather than require a custom module to depend on kdcpreauth callbacks if it wants to add an indicator. So I changed server_authorize_cert() to use cb->add_auth_indicator() after the authorize method call for gathering module indicators, and then call the free_ind method each time.
Use the accumulator pattern for calling the authorize method: authorize no longer has *status and returns 0 for “yes”, KRB5_PLUGIN_NO_HANDLE for “pass”, and KRB5KDC_ERR_CERTIFICATE_MISMATCH/KRB5KDC_ERR_CLIENT_NAME_MISMATCH or other errors for “no”.
The test module now checks a certificate using a DER substring.
Combined the pkinit_srv.c module commits (made change rebasing easier), and reworded doc text and commits.

mrogers950 · 2017-03-02T17:53:39Z

Just fixed a typo and made some alignment adjustments in plugins/certauth/test/main.c.

greghudson

I expect to have more review comments, but here are some for now.

greghudson · 2017-03-02T19:08:04Z

src/plugins/certauth/test/main.c

+    c[sizeof(cntag)] = strlen(name);
+    memcpy(c + sizeof(cntag) + 1, name, strlen(name));
+
+    cc = memmem(cert.data, cert.length, c, sizeof(cntag) + strlen(name) + 1);


memmem() is a Gnu libc extension. I'm not sure what you can use instead; I will think about it.

I think you'll want to use memchr() and memcmp() instead. There will be a fiddly length check involved, but it shouldn't be a lot of code.

greghudson · 2017-03-02T19:08:49Z

src/plugins/certauth/test/main.c

+}
+
+static krb5_boolean
+has_cn(krb5_context context, krb5_data cert, char *name)


const char *name here, probably.

greghudson · 2017-03-02T19:09:40Z

src/plugins/certauth/test/main.c

+
+    /* Construct a DER search string of CN tag + length tag + name to
+     * check for princ as CN. */
+    c = malloc(sizeof(cntag) +  strlen(name) + 1);


Using k5buf here might not be any less code, but it is probably less error-prone. asprintf() is also an option but tends to be awkward for composing binary strings.

greghudson · 2017-03-02T19:12:29Z

src/plugins/preauth/pkinit/pkinit_srv.c

+    return ret;
+}
+
+krb5_error_code


This function can be static (and its declaration removed, if the new code is ordered to allow that).

greghudson · 2017-03-02T19:12:59Z

src/plugins/preauth/pkinit/pkinit_srv.c

+    return ret;
+}
+
+krb5_error_code


This function can be static (and its declaration removed).

greghudson · 2017-03-02T20:16:30Z

src/plugins/preauth/pkinit/pkinit_srv.c

+}
+
+static krb5_error_code
+certauth_authorize(krb5_context context, krb5_certauth_moddata moddata,


This function name seems off; it's for the pkinitsrv certauth module but its name suggests it's associated with the certauth interface itself.

greghudson · 2017-03-02T20:18:19Z

src/plugins/preauth/pkinit/pkinit_srv.c

+ * Do certificate auth based on match expressions in the pkinit_cert_match
+ * attribute string.  An expression should be in the same form as those used for
+ * the pkinit_cert_match configuration option.  Multiple expressions are
+ * separated by ':'.  The certificate is authorized if all expressions are


Do we need support for multiple expressions? An expression can already contain multiple rules whose results can either be and-ed or or-ed together. If we have a use case for an and-of-ors, that functionality is probably best moved into pkinit_matching.c.

greghudson · 2017-03-02T20:19:53Z

src/plugins/preauth/pkinit/pkinit_srv.c

+        ret = KRB5_PLUGIN_NO_HANDLE;
+        goto out;
+    }
+    matchcpy = k5memdup0(match, strlen(match), &ret);


Possibly moot, but this is more clearly written using strdup(), even though that doesn't set ret. (I've considered adding a k5strdup helper for that reason, but haven't done so yet.)

greghudson · 2017-03-02T20:22:18Z

src/include/krb5/certauth_plugin.h

+ * - KRB5_PLUGIN_NO_HANDLE - The module answers "passed" and had nothing to say
+ *   about the cert.
+ *
+ * opts is used by the builtin kdc modules.  db_entry receives the client


We need a little more verbiage here. "opts is used by built-in modules and should be ignored by other modules."

greghudson · 2017-03-02T20:22:55Z

src/include/krb5/certauth_plugin.h

+/* Abstract module data type. */
+typedef struct krb5_certauth_moddata_st *krb5_certauth_moddata;
+
+/*


Every pluggable interface method description should say whether it is optional or mandatory to implement. See other interface headers for examples.

mrogers950 · 2017-03-03T20:26:07Z

Rewrote has_cn() with k5buf and replaced memmem() with memchr/memcmp.
Changed declarations of certauth initvt functions to static.
Renamed certauth_authorize to pkinitsrv_authorize.
Changed the suggested k5memdup0 to strdup. A k5strdup() would be nice for clarity over k5memdup0().
Adjustments to certauth_plugin.h comments.

Since the pkinit_cert_match option allowed multiple expressions I just tried to keep the same behavior for the string attribute in some way, so there is no explicit requirement. Does the client certificate matching require multiple statements in a way that would not fit for the server's checking purposes? I'm OK with changing it to just use one expression if multiple are unnecessary.

greghudson · 2017-03-04T15:54:21Z

src/plugins/certauth/test/main.c

+    c_len = cert.length;
+
+    /* Check for the CN needle in the certificate haystack. */
+    c = memchr(cert.data, b_start, c_len);


What happens in the loop below if memchr() finds b_start at the last character of the cert?

greghudson · 2017-03-04T15:56:32Z

I think pkinit_cert_matching() only supports multiple rule sets because the profile library naturally produces multiple results. Since you can have several components in a rule set, I would say that we should start with support for just a single rule set in the string attribute.

mrogers950 · 2017-03-06T19:27:44Z

Looking closer at the case of b_start at the end of a cert, this led to a memcmp() past bounds and an overflowed value passed to memchr(). Changing this to figure out the amount of certificate left to check (variable renamed to c_left) and comparing with b_len before doing the memcmp() handles this situation cleaner.
Changed the dbmatch module to use just a single expression, and updated the related t_pkinit.py tests to exercise the matching code a bit further. Updated docs.

greghudson · 2017-03-06T21:06:04Z

More partial feedback (sorry): we will need documentation in pkinit.rst describing how to use pkinit_cert_match (it can reference pkinit_cert_match in krb5_conf.rst but should give a simple example using a subject match). The pkinit_cert_match string attribute should also be briefly described in kadmin_local.rst under set_string.

mrogers950 · 2017-03-07T18:31:54Z

While working on the doc text I thought it might make sense for the dbmatch module to have an option for a default matching expression that it could apply to all principals in a realm. This way when using the dbmatch module solo as enable_only, you would not need to apply a match expression attribute to each principal. Thoughts on this?

greghudson · 2017-03-07T18:34:16Z

Wouldn't you need some way to substitute the principal name into a matching pattern, for that to be useful? That starts to get complicated.

On a related note, I think the dbmatch module, as well as its documentation and tests, should be moved into a second commit. That can happen after we've finished review on the code changes to simplify development.

mrogers950 · 2017-03-07T19:11:38Z

Wouldn't you need some way to substitute the principal name into a matching pattern, for that to be useful? That starts to get complicated.

Without that, it could be useful for some broader cases, like say if you wanted the only cert restriction to be that the subject mentions a certain realm. I figure that using dbmatch alone would be used as a common way around the standard policy imposed by the pkinitsrv module, and those doing so may see this as a problem.

On a related note, I think the dbmatch module, as well as its documentation and tests, should be moved into a second commit. That can happen after we've finished review on the code changes to simplify development.

That's fine with me.

mrogers950 · 2017-03-07T21:17:42Z

Updated with added doc text.

greghudson · 2017-03-07T21:35:58Z

Without that, it could be useful for some broader cases, like say if you wanted the only cert restriction to be that the subject mentions a certain realm. I figure that using dbmatch alone would be used as a common way around the standard policy imposed by the pkinitsrv module, and those doing so may see this as a problem.

If you were to set a default match expression of, say, ".*@myrealm", then the dbmatch module would say "yes" for any cert with a subject containing MYREALM, even if the user in the cert bears no relationship to the user in the request, wouldn't it? That doesn't seem like a useful policy.

I could see it being useful to be able to supply a default expression which is necessary to match but not sufficient, but then you would still need some criteria to judge a certificate as sufficient.

greghudson

More partial feedback since I ran out of time.

greghudson · 2017-03-08T16:41:04Z

doc/admin/pkinit.rst

+**pkinit_cert_match** can be specified by the **pkinit_cert_match**
+attribute::
+
+    kadmin -q 'set_string user@REALM pkinit_cert_match "<SUBJECT>.*user@REALM.*"'


I'm not sure why one would want to put wildcards in this regexp. And actually, I think the regexp match isn't anchored (can you confirm whether or not that's the case?) so I think you would want <SUBJECT>^user@REALM$ .

greghudson · 2017-03-08T16:43:24Z

doc/admin/admin_commands/kadmin_local.rst

@@ -661,6 +661,13 @@ KDC:
    *principal*.  The *value* is a JSON string representing an array
    of objects, each having optional ``type`` and ``username`` fields.

+**pkint_cert_match**


Typo here (should be pkinit, not pkint).

greghudson · 2017-03-08T16:45:28Z

doc/admin/pkinit.rst

+contain "user@REALM" in the subject in order to authenticate with
+PKINIT.  By default, if **pkinit_cert_match** is set on a principal
+the KDC performs the match in addition to checking for the attributes
+outlined in the above PKINIT client certificate configuration.


It isn't clear from this documentation what certificate criteria are necessary and sufficient when a pkinit_cert_match attribute is present.

greghudson · 2017-03-08T16:47:34Z

src/include/krb5/certauth_plugin.h

+ * after the certificate signature has been verified.  A module can be
+ * implemented in the same shared object as a kdb module and define an
+ * authorization policy on top of the builtin module, or override the builtin
+ * module with enable_only.


I would strike the last sentence in this comment.

greghudson · 2017-03-08T16:48:31Z

src/include/krb5/certauth_plugin.h

+/*
+ * Mandatory:
+ * Return 0 if the DER encoded cert is authorized for PKINIT
+ * authentication by princ, otherwise return one of the following error codes:


Use a semicolon here; it's a run-on setence with a comma.

greghudson · 2017-03-08T17:03:56Z

src/plugins/preauth/pkinit/pkinit_crypto.h

+                         pkinit_plg_crypto_context plgctx,
+                         pkinit_req_crypto_context reqctx,
+                         const char *match_rule,
+                         krb5_boolean *matched);


The use of blank lines in these prototypes is inconsistent.

greghudson · 2017-03-08T17:07:02Z

src/plugins/preauth/pkinit/pkinit_srv.c

+ * defined by modules.
+ */
+static krb5_error_code
+server_authorize_cert(krb5_context context, certauth_handle *certauth_modules,


Do we need the server_ prefix on this name? We're in pkinit_srv.c and this is a static function.

greghudson · 2017-03-08T17:13:01Z

src/plugins/preauth/pkinit/pkinit_srv.c

+static krb5_error_code
+pkinitsrv_authorize(krb5_context context, krb5_certauth_moddata moddata,
+                   krb5_data cert, krb5_principal princ, void *opts,
+                   void *db_entry, char ***authinds_out)


I don't think we're feeding the right things into the accumulator here, with the result that administrators will have to use module configuration to disable the pkinitsrv module when they shouldn't have to.

If the EKU is not found, I think we should say no (KRB5KDC_ERR_INCONSISTENT_KEY_PURPOSE). An admin can disable EKU checking in configuration if desired.

If the SAN is not found, I think we should pass. The certificate may still match the client principal according to another module's criteria, and there is no way to disable SAN checking in configuration.

greghudson · 2017-03-08T17:16:46Z

src/plugins/preauth/pkinit/pkinit_srv.c

+    }
+
+    if (accepted)
+        ret = 0;


And otherwise we return KRB5_PLUGIN_NO_HANDLE? I don't think that's meaningful to the caller. I think we should return KRB5KDC_ERR_CLIENT_NAME_MISMATCH, in that no module has asserted a match between the certificate and the client principal name.

greghudson · 2017-03-08T17:17:49Z

src/plugins/preauth/pkinit/pkinit_srv.c

+        goto out;
+
+    if (!matched)
+        ret = KRB5KDC_ERR_CERTIFICATE_MISMATCH;


I'm not sure if this should be a "no", but leave it alone for now; there's a reasonable argument that if the string attribute is present, it should be necessary as well as sufficient.

mrogers950 · 2017-03-09T02:50:07Z

In certauth_plugin.h, add the krb5_db_entry typedef, change method arguments and comments.
Fixed certauth/test/Makefile.in
Use assert() in test modules, fix top comment.
Indentation (my own fault rather than vim) and space fixes.
Renamed server_authorize_cert() to authorize_cert()
Make authorize_cert() return KRB5KDC_ERR_CLIENT_NAME_MISMATCH on no module match.

I'm not sure why one would want to put wildcards in this regexp. And actually, I think the regexp match isn't anchored (can you confirm whether or not that's the case?) so I think you would want <SUBJECT>^user@REALM$.

You might be confusing it with <SAN> which would make sense with an anchored match. For that example I was thinking of a cert subject that also contains OU, CN=, etc., the tests also run this same <SUBJECT> regexp.

It isn't clear from this documentation what certificate criteria are necessary and sufficient when a pkinit_cert_match attribute is present.

I revised this part a bit. If it’s still not clear some suggestions would be welcome.

Does this work if the needle is present at the last possible position in the haystack?

Actually no; the offset-by-one was wrong between the c_left assignment and the in-loop memchr(), so my update corrects this (although I don't think we would see this with our test certs).

I don't think we're feeding the right things into the accumulator here, with the result that administrators will have to use module configuration to disable the pkinitsrv module when they shouldn't have to.
If the EKU is not found, I think we should say no (KRB5KDC_ERR_INCONSISTENT_KEY_PURPOSE). An admin can disable EKU checking in configuration if desired.
If the SAN is not found, I think we should pass. The certificate may still match the client principal according to another module's criteria, and there is no way to disable SAN checking in configuration.

In order for the SAN check to be able to return pass independently from the EKU check, I split them into separate pkinitsrv modules, and made verify_client_san() return ENOENT when no SAN exists.

greghudson · 2017-03-09T16:03:40Z

You might be confusing it with which would make sense with an anchored match. For that example I was thinking of a cert subject that also contains OU, CN=, etc., the tests also run this same regexp.

If I am right that the code does an unanchored regexp match, then the leading and trailing .* wildcards add nothing to the regexp.

More importantly, simply looking for the substring user@realm in the subject would also match anotheruser@realm which is a clear escalation of privilege issue.

mrogers950 · 2017-03-09T17:19:10Z

If I am right that the code does an unanchored regexp match, then the leading and trailing .* wildcards add nothing to the regexp.
More importantly, simply looking for the substring user@realm in the subject would also match anotheruser@realm which is a clear escalation of privilege issue.

I see, so I've updated the doc text and the test regexps to what should be more sensible examples.

greghudson · 2017-03-10T16:33:35Z

I revised this part a bit. If it’s still not clear some suggestions would be welcome.

I suggest the following (this is not necessarily wrapped correctly for pkinit.rst):

By default, the KDC requires PKINIT client certificates to have the standard Extended Key
Usage and Subject Alternative Name attributes for PKINIT.  Starting in release 1.16, it is
possible to authorize client certificates based on the subject or other criteria instead of the
standard PKINIT Subject Alternative Name, by setting the **pkinit_cert_match** string
attribute on each client principal entry.  For example::

    kadmin set_string user@REALM pkinit_cert_match "<SUBJECT>CN=user@REALM$"

The **pkinit_cert_match** string attribute follows the syntax used by the :ref:`krb5.conf(5)`
**pkinit_cert_match** relation.  To allow the use of non-PKINIT client certificates, it will also
be necessary to disable key usage checking using the **pkinit_eku_checking** relation; for
example::

    [kdcdefaults]
        pkinit_eku_checking = none

greghudson · 2017-03-10T16:41:55Z

src/include/krb5/certauth_plugin.h

+ */
+typedef krb5_error_code
+(*krb5_certauth_authorize_fn)(krb5_context context,
+                              krb5_certauth_moddata moddata, krb5_data cert,


We don't like passing structure types as function parameters or return values (as stated in http://k5wiki.kerberos.org/wiki/Coding_style/Practices). I recommend using separate "const uint8_t *cert" and "size_t len" parameters; I think that will be easier for most consumers to deal with than a krb5_data object anyway.

(static inline functions such as those in k5-int.h are an exception to this rule.)

greghudson

A little more partial feedback; I will have more later.

greghudson · 2017-03-10T16:45:50Z

src/plugins/certauth/test/main.c

+    k5_buf_init_dynamic(&buf);
+    k5_buf_add_len(&buf, cntag, sizeof(cntag));
+    name_len = strlen(name);
+    k5_buf_add_len(&buf, &name_len, 1);


This line seems to assume a little-endian platform. name_len should be of type uint8_t. You should also add "assert(strlen(name) < 128);".

greghudson · 2017-03-10T16:57:50Z

src/plugins/certauth/test/main.c

+{
+    krb5_error_code ret;
+    krb5_boolean match = FALSE;
+    uint8_t cntag[6] = "\x06\x03\x55\x04\x03\x0c";


I think it would be a little clearer to a reader versed in DER to trim the last byte here and add it separately (with k5_buf_add(&buf, "\x0C");) before adding the length byte. That way the five bytes of cntag are the encoding of the type, while 0C <len> <len bytes> is the encoding of the value.

(As an aside, we usually use uppercase hex constants, or at least I do, because it's easier to visually distinguish the lowercase x from the uppercase hex digits.)

greghudson · 2017-03-10T17:00:59Z

src/include/krb5/certauth_plugin.h

+(*krb5_certauth_authorize_fn)(krb5_context context,
+                              krb5_certauth_moddata moddata, krb5_data cert,
+                              krb5_principal princ, void *opts,
+                              krb5_db_entry *db_entry,


db_entry should probably be of type const krb5_db_entry *. princ should also be of type krb5_const_principal, and opts should be of type const void * (which means that in the built-in consumers, it should be cast to a const pointer to the options structure).

greghudson · 2017-03-10T17:01:50Z

src/plugins/certauth/test/main.c

+
+    if (authinds == NULL)
+        return;
+    for (i = 0; authinds[i] != NULL; i++) {


No braces here.

greghudson

Still wrapping my head around the pkinit_srv.c changes, so this is more partial feedback. I do have one question: I get why it's not trivial to extract the DER-encoded cert from the crypto context because it was contained within a CMS ContentInfo object that we decoded in one step. That accounts for crypto_encode_der_cert(). But if the cert we're authorizing is in received_cert, why decode it again in each built-in callback? Why not just use received_cert as is?

greghudson · 2017-03-11T16:39:11Z

doc/admin/admin_commands/kadmin_local.rst

+    attributes required for the client certificate used by the
+    principal during PKINIT authentication.  The matching expression
+    is in the same format as those used by the **pkinit_cert_match**
+    option in :ref:`kdc.conf(5)`.  (New in release 1.16.)


krb5.conf, not kdc.conf

greghudson · 2017-03-11T17:12:40Z

doc/plugindev/certauth.rst

+A certauth module implements the **authorize** method to determine if
+a client's certificate is authorized to authenticate a client
+principal.  **authorize** receives the DER encoded certificate, client
+request principal, and an opaque pointer to the client's krb5_db_entry


The DB entry pointer isn't opaque.

greghudson · 2017-03-11T17:14:04Z

doc/plugindev/certauth.rst

+from **authorize**, it should also implement the **free_ind** method
+to free the list.
+
+Two builtin modules are provided with the certauth interface.


greghudson · 2017-03-11T17:19:52Z

src/plugins/certauth/test/main.c

+    /* Create an indicator list with the module name and CN. */
+    assert(ais = calloc(3, sizeof(*ais)));
+    assert(ai1 = strdup("test2"));
+    assert(ai2 = strdup(name));


Don't put expressions with side-effects inside assert(), as asserts can be compiled out. Separately assert(ais != NULL); etc. after each allocation.

You can also simplify this code by getting rid of ai1 and ai2 and just assigning to ais[0] and ais[1]. We don't really need the logic for freeing ais on failure because we're just going to abort on allocation failure anyway.

mrogers950 · 2017-03-13T15:44:44Z

Updated:

Constify method arguments. Use separate *cert and cert_len arguments and make related adjustments in pkinit_srv.c.
Make buf and data type adjustments to has_cn(), add name length assert().
Fix testing asserts.
Correct the test1 module to return pass now that the framework has the accumulator behavior, since test1 is intended to test the behavior of a passed module asserting indicators.
Use suggested text for pkinit.rst.
Fix manpage ref in kadmin_local.rst.
Remove referring to pointer as opaque, changed "KDB" to say "libkdb5" and adjusted the builtin module text in certauth.rst.

But if the cert we're authorizing is in received_cert, why decode it again in each built-in callback? Why not just use received_cert as is?

Good point, thinking about it now this was only due to my initial idea of how the interface was to be designed (and that initially I used the pkinit objects during testing). Now that it has changed, I see the decoding part is unnecessary. I've removed the decoding step from the builtins and assuming we do not care to export any OpenSSL decoding for use by custom modules, I removed the decoding functions.

greghudson · 2017-03-15T15:55:30Z

On a related note, I think the dbmatch module, as well as its documentation and tests, should be moved into a second commit. That can happen after we've finished review on the code changes to simplify development.

What I see now is a breakdown into five commits, but no separation of dbmatch from the certauth interface. The second commit contains support functions used only by dbmatch as well as one used by the other certauth modules; the third commit contains the dbmatch module along with the certauth interface consuming code; the fourth commit contains dbmatch tests along with certauth tests; the fifth commit contains dbmatch documentation mixed in with certauth documentation.

What I would like to see is a first commit containing the certauth interface, with its tests and its documentation, as if we had never conceived of dbmatch as a clever piece of added functionality, and then a second commit containing the dbmatch feature, with its tests and its documentation.

mrogers950 · 2017-03-16T01:05:38Z

Updated with revised commits.

greghudson

This feedback should be approaching comprehensive. I still need to delve into the details of the new matching functions.

The style checker finds a number of problems with both commits. Some of them are slightly overlong lines; others arise from pkinit_crypto.h still using tabs (I'm not sure why that file was never converted to use spaces). Please fix all of the issues it finds.
I'd like the module names pkinitsrv_san and pkinitsrv_eku renamed to pkinit_san and pkinit_eku. "pkinitsrv_san" seems like it refers to part of our implementation, while "pkinit_san" naturally suggests checking for the standard PKINIT SAN.
crypto_encode_der_cert() should output a uint8_t * and a size_t, since that is what we now desire in its caller.

greghudson · 2017-03-16T15:05:34Z

src/plugins/preauth/pkinit/pkinit_srv.c

+        if (hd->vt.authorize == NULL)
+            continue;
+
+        ret = hd->vt.authorize(context, hd->moddata, (const uint8_t *)cert.data,


Likely moot due to other changes, but it's only necessary to cast to (uint8_t *) here.

greghudson · 2017-03-16T15:07:20Z

src/plugins/preauth/pkinit/pkinit_srv.c

+                    goto cleanup;
+            }
+
+            if (hd->vt.free_ind != NULL)


This method is documented as mandatory if indicators are returned; providing a fallback doesn't seem like a good idea.

greghudson · 2017-03-16T15:09:56Z

src/plugins/preauth/pkinit/pkinit_srv.c

+    size_t i;
+    if (ai_total == NULL)
+        return;
+    for (i = 0; ai_total[i] != NULL; i++) {


No braces here.

greghudson · 2017-03-16T15:10:18Z

src/plugins/preauth/pkinit/pkinit_srv.c

+static void
+free_ai_list(char **ai_total)
+{
+    size_t i;


Add a blank line after declarations.

greghudson · 2017-03-16T15:10:55Z

src/plugins/preauth/pkinit/pkinit_srv.c

@@ -51,6 +79,47 @@ pkinit_find_realm_context(krb5_context context,
                          krb5_kdcpreauth_moddata moddata,
                          krb5_principal princ);

+static void
+free_ai_list(char **ai_total)


"ai_total" seems like a relic from when this was part of another piece of code. "list" would be more natural.

greghudson · 2017-03-16T15:27:41Z

src/plugins/preauth/pkinit/pkinit_srv.c

+        goto out;
+
+    if (!valid_san) {
+        pkiDebug("%s: did not find an acceptable SAN in user "


This string constant now fits on one line. (Also applies to EKU module.)

greghudson · 2017-03-16T15:28:33Z

src/plugins/preauth/pkinit/pkinit_srv.c

+        ret = KRB5KDC_ERR_CLIENT_NAME_MISMATCH;
+    }
+
+out:


If there's nothing to put in the cleanup handler, don't use a cleanup label; just have earlier return points. (Also applies to EKU module.)

greghudson · 2017-03-16T15:31:19Z

src/plugins/preauth/pkinit/pkinit_srv.c

+    vt->authorize = pkinitsrv_san_authorize;
+    vt->init = NULL;
+    vt->fini = NULL;
+    vt->free_ind = NULL;


It's not our practice to explicitly set methods to NULL in initvt functions; not having to do so is a nice benefit of using a zero-filled vtable provided by the caller. (Also applies to the EKU module, the test1 module, and the dbmatch module.)

greghudson · 2017-03-16T15:31:59Z

src/plugins/preauth/pkinit/pkinit_srv.c

+ * Do certificate auth based on match expressions in the pkinit_cert_match
+ * attribute string.  An expression should be in the same form as those used for
+ * the pkinit_cert_match configuration option.  Multiple expressions are
+ * separated by ':'.  The certificate is authorized if all expressions are


This comment still needs to be updated now that the code supports only one expression.

greghudson · 2017-03-16T15:35:28Z

src/plugins/preauth/pkinit/pkinit_srv.c

+        h->moddata = NULL;
+        if (h->vt.init != NULL) {
+            ret = h->vt.init(context, &h->moddata);
+            if (ret) {


I'd like to see a trace log here; see the localauth, hostrealm, or ccselect consumers for examples. (Note that PKINIT trace log definitions are in pkinit_trace.h, though.)

mrogers950 · 2017-03-17T00:34:03Z

Update pkinit_crypto.h as a whole to conform to current style with spaced tabs. I made this a separate commit following the first two since it's a large formatting-only change, if that's OK to throw in here.
Rename and revise SAN and EKU modules as suggested.
Changed crypto_encode_der_cert() output arguments.
Remove the free_ai_list() fallback and function.
Remove initvt NULL assignments.
Update dbmatch comment.
Fix free_realm_contexts() declarations.
Add trace functions and use them during module loading.

greghudson · 2017-03-20T15:35:38Z

I'm going to push some adjusted commits to the PR branch to, among various miscellaneous edits:

change has_cn() to return a boolean value
move documentation for the builtin modules to krb5_conf.rst for consistency with other interfaces
use expected_msg in t_certauth.py
reorder the new pkinit_srv.c code to avoid needing forward prototypes
allocate our own memory in crypto_encode_der_cert() so we can free it with free(), as we do in all the other places where we use OpenSSL i2d functions
eliminate a copy in dbmatch_authorize() which is no longer needed
prototype pkinit_client_cert_match() in pkinit.h instead of pkinit_crypto.h
avoid using k5-int.h in pkinit_crypto_openssl.c

I will also be removing the third reformatting commit. It changes the license statement, which is not necessary okay, and this PR is too involved to include out-of-scope commits.

greghudson · 2017-03-20T15:39:00Z

I still haven't delved fully into the new matching code, but I am concerned about the memory management in crypto_req_cert_matching_data(). It allocates cd, fills it in, passes it to crypto_cert_get_matching_data() which makes an alias within the resulting md, and then frees it while md is still live. Am I missing something or does this create a dangling pointer?

mrogers950 · 2017-03-20T17:12:56Z

I still haven't delved fully into the new matching code, but I am concerned about the memory management in crypto_req_cert_matching_data(). It allocates cd, fills it in, passes it to crypto_cert_get_matching_data() which makes an alias within the resulting md, and then frees it while md is still live. Am I missing something or does this create a dangling pointer?

Yes, so technically there is a dangling pointer in md (and this would also apply for the ci pointer) however those two aliases of md->ch and md->cred are not used during the component_match() calls in pkinit_client_cert_match(), and md lasts until crypto_cert_free_matching_data(). Note that crypto_cert_free_matching_data() does not clean up md->ch or md->cred. There may be a cleaner way to handle the md here, it might need changing crypto_cert_free_matching_data() to be able to (optionally) release md->ch/md->cred.

greghudson · 2017-03-20T22:36:08Z

I see how that pointer is only used by crypto_cert_select() which we aren't calling. I think the way this code uses the existing matching interfaces creates too much technical debt. To make the matching interface properly usable for the server code, I think we need to:

Remove the ch field from pkinit_cert_matching_data and make the existing caller track the cert handle separately.
Change crypto_cert_select() to take a pkinit_cert_handle.
From crypto_cert_get_matching_data(), factor out a function which produces matching data from a cert (given a plgctx and reqctx object as separate parameters) and use that interface in crypto_req_cert_matching_data().

For step 1, there are a couple of approaches. We could create an array of cert_handle/matching_data pairs in obtain_all_cert_matching_data(). Another approach might be to replace crypto_cert_iteration_begin()/crypto_cert_iteration_next() with a function that returns an array of cert handles (this would be substantially less code) and maintain two parallel arrays with the same indexes.

These internal changes should be in a separate commit that doesn't introduce any new functionality or change any existing behavior.

greghudson · 2017-03-21T14:34:38Z

For step 1, there are a couple of approaches. We could create an array of cert_handle/matching_data pairs in obtain_all_cert_matching_data(). Another approach might be to replace crypto_cert_iteration_begin()/crypto_cert_iteration_next() with a function that returns an array of cert handles (this would be substantially less code) and maintain two parallel arrays with the same indexes.

Actually, I think we can go simpler still: get rid of cert handles entirely, and just ask the crypto code for an array of matching data objects for an id_cryptoctx (with a plgctx and reqctx as additional parameters for for the SAN/EKU functions). Then select a cert by id_cryptoctx and index.

mrogers950 · 2017-03-22T01:30:25Z

I've updated with a new commit implementing those changes.

greghudson · 2017-03-22T16:28:47Z

I flipped the second and third commits, and made some minor edits. I will make a final review pass probably tomorrow, and hopefully merge this.

Add the header include/krb5/certauth_plugin.h, defining a pluggable interface to control authorization of PKINIT client certificates. Add the "pkinit_san" and "pkinit_eku" builtin certauth modules and related PKINIT crypto X.509 helper functions. Add authorize_cert() as the entry function for certauth plugin module checks called in pkinit_server_verify_padata(). Modify kdcpreauth_moddata to hold the list of certauth module handles, and load the modules when the PKINIT kdcpreauth server plugin is initialized. Change crypto_retrieve_X509_sans() to return ENOENT when no SAN is found. Add test modules in plugins/certauth/test. Create t_certauth.py with basic certauth tests. Add plugin interface documentation in doc/plugindev/certauth.rst and doc/admin/krb5_conf.rst. [ghudson@mit.edu: simplified code, edited docs] ticket: 8561 (new)

Remove the pkinit_cert_handle structures and iteration functions used during certificate matching. Instead, make pkinit_matching.c obtain a list of matching data objects from the crypto code, and then select a cert based on the index into that list. Also fix a typo in the name of crypto_retrieve_X509_key_usage(). [ghudson@mit.edu: simplified code]

Add and enable the "dbmatch" builtin module. Add the pkinit_client_cert_match() and crypto_req_cert_matching_data() helper functions. Add dbmatch tests to t_pkinit.py. Add documentation to krb5_conf.rst, pkinit.rst, and kadmin_local.rst. [ghudson@mit.edu: simplified code, edited docs] ticket: 8562 (new)

mrogers950 force-pushed the certauth_plugin branch from 69a98d2 to 58ed85f Compare March 1, 2017 02:03

mrogers950 force-pushed the certauth_plugin branch from 58ed85f to 0cc3136 Compare March 2, 2017 17:42

greghudson requested changes Mar 2, 2017

View reviewed changes

mrogers950 force-pushed the certauth_plugin branch from 0cc3136 to ddadd91 Compare March 3, 2017 20:04

greghudson reviewed Mar 4, 2017

View reviewed changes

mrogers950 force-pushed the certauth_plugin branch from ddadd91 to 4a3cf4c Compare March 6, 2017 19:19

mrogers950 force-pushed the certauth_plugin branch from 4a3cf4c to d88cca3 Compare March 7, 2017 21:12

greghudson requested changes Mar 8, 2017

View reviewed changes

mrogers950 force-pushed the certauth_plugin branch from d88cca3 to ff0e1db Compare March 9, 2017 02:35

mrogers950 force-pushed the certauth_plugin branch 2 times, most recently from 434cbb9 to 0b686fe Compare March 9, 2017 17:15

greghudson reviewed Mar 10, 2017

View reviewed changes

greghudson requested changes Mar 11, 2017

View reviewed changes

sumit-bose mentioned this pull request Mar 13, 2017

IPA certauth plugin freeipa/freeipa#575

Closed

mrogers950 force-pushed the certauth_plugin branch from 0b686fe to 5b78523 Compare March 13, 2017 15:42

mrogers950 force-pushed the certauth_plugin branch from 5b78523 to e1b0843 Compare March 16, 2017 01:04

greghudson requested changes Mar 16, 2017

View reviewed changes

mrogers950 force-pushed the certauth_plugin branch from e1b0843 to b972580 Compare March 17, 2017 00:32

greghudson force-pushed the certauth_plugin branch from b972580 to 3f3408a Compare March 20, 2017 15:36

greghudson force-pushed the certauth_plugin branch from fbb87b5 to bc3c3c5 Compare March 22, 2017 16:28

Matt Rogers added 3 commits March 23, 2017 13:11

greghudson force-pushed the certauth_plugin branch from bc3c3c5 to 89634ca Compare March 23, 2017 17:12

greghudson approved these changes Mar 23, 2017

View reviewed changes

greghudson merged commit 89634ca into krb5:master Mar 23, 2017

greghudson mentioned this pull request Mar 23, 2017

Correct error handling bug in prior commit #617

Merged

PKINIT certificate authorization pluggable interface #610

PKINIT certificate authorization pluggable interface #610

Conversation

mrogers950 commented Feb 17, 2017

greghudson commented Feb 18, 2017 • edited

greghudson commented Feb 20, 2017

mrogers950 commented Feb 20, 2017

greghudson commented Feb 20, 2017

mrogers950 commented Feb 20, 2017

greghudson commented Feb 20, 2017

mrogers950 commented Mar 1, 2017

mrogers950 commented Mar 2, 2017

greghudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrogers950 commented Mar 3, 2017

Choose a reason for hiding this comment

greghudson commented Mar 4, 2017

mrogers950 commented Mar 6, 2017

greghudson commented Mar 6, 2017

mrogers950 commented Mar 7, 2017

greghudson commented Mar 7, 2017

mrogers950 commented Mar 7, 2017

mrogers950 commented Mar 7, 2017

greghudson commented Mar 7, 2017

greghudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrogers950 commented Mar 9, 2017

greghudson commented Mar 9, 2017

mrogers950 commented Mar 9, 2017

greghudson commented Mar 10, 2017 • edited

greghudson Mar 10, 2017 • edited

Choose a reason for hiding this comment

greghudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greghudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrogers950 commented Mar 13, 2017

greghudson commented Mar 15, 2017 • edited

mrogers950 commented Mar 16, 2017

greghudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greghudson commented Feb 18, 2017 •

edited

greghudson commented Mar 10, 2017 •

edited

greghudson Mar 10, 2017 •

edited

greghudson commented Mar 15, 2017 •

edited

greghudson commented Mar 22, 2017 •

edited