pkinit plugin logic in pkinit_srv.c

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

pkinit plugin logic in pkinit_srv.c

Craig Huckabee-2

While running some tests with the latest development builds, I noticed that the plugin test logic in pkinit_srv.c might be flawed.  The comment in the plugin check codes says:

     /*
     * Check the certificate against each certauth module.  For the certificate
     * to be authorized at least one module must return 0, and no module can an
     * error code other than KRB5_PLUGIN_NO_HANDLE (pass).  Add indicators from
     * modules that return 0 or pass.
     */

but that’s not really true as each plugin returns KRB5KDC_ERR_CLIENT_NAME_MISMATCH when a match is not found.  This means the first plugin that fails kicks out of that loop and no other checks are performed.  I noticed this specifically because we were testing with certs that need the dbmatch module to work but it was never being called.

Attached is a small patch that allows KRB5KDC_ERR_CLIENT_NAME_MISMATCH to be ignored and that will jump out of the loop on the first accepted match.

—Craig





_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev

smime.p7s (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pkinit plugin logic in pkinit_srv.c

Greg Hudson
On 08/24/2017 09:39 AM, Craig Huckabee wrote:

> While running some tests with the latest development builds, I noticed that the plugin test logic in pkinit_srv.c might be flawed.  The comment in the plugin check codes says:
>
>      /*
>      * Check the certificate against each certauth module.  For the certificate
>      * to be authorized at least one module must return 0, and no module can an
>      * error code other than KRB5_PLUGIN_NO_HANDLE (pass).  Add indicators from
>      * modules that return 0 or pass.
>      */
>
> but that’s not really true as each plugin returns KRB5KDC_ERR_CLIENT_NAME_MISMATCH when a match is not found.  This means the first plugin that fails kicks out of that loop and no other checks are performed.  I noticed this specifically because we were testing with certs that need the dbmatch module to work but it was never being called.

Can you elaborate on "each plugin returns
KRB5KDC_ERR_CLIENT_NAME_MISMATCH when a match is not found"?  Of the
built-in modules, I think only the san module would ever return that
code, and should only return it when it finds PKINIT or UPN SANs in the
certificate and they don't match.  It should return
KRB5_PLUGIN_NO_HANDLE if the cert doesn't have either of those SAN types.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: pkinit plugin logic in pkinit_srv.c

Robbie Harwood
In reply to this post by Craig Huckabee-2
Craig Huckabee <[hidden email]> writes:

> While running some tests with the latest development builds, I noticed
> that the plugin test logic in pkinit_srv.c might be flawed.  The
> comment in the plugin check codes says:
>
>      /*
>      * Check the certificate against each certauth module.  For the certificate
>      * to be authorized at least one module must return 0, and no module can an
>      * error code other than KRB5_PLUGIN_NO_HANDLE (pass).  Add indicators from
>      * modules that return 0 or pass.
>      */
>
> but that’s not really true as each plugin returns
> KRB5KDC_ERR_CLIENT_NAME_MISMATCH when a match is not found.  This
> means the first plugin that fails kicks out of that loop and no other
> checks are performed.  I noticed this specifically because we were
> testing with certs that need the dbmatch module to work but it was
> never being called.
>
> Attached is a small patch that allows KRB5KDC_ERR_CLIENT_NAME_MISMATCH
> to be ignored and that will jump out of the loop on the first accepted
> match.
Hi, I'm not seeing a patch attached.  Also, if you prefer, we do accept
PRs on github: https://github.com/krb5/krb5/

Thanks,
--Robbie

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pkinit plugin logic in pkinit_srv.c

Craig Huckabee-2
In reply to this post by Greg Hudson

> On Aug 24, 2017, at 11:02 AM, Greg Hudson <[hidden email]> wrote:
>
> On 08/24/2017 09:39 AM, Craig Huckabee wrote:
>> While running some tests with the latest development builds, I noticed that the plugin test logic in pkinit_srv.c might be flawed.  The comment in the plugin check codes says:
>>
>>     /*
>>     * Check the certificate against each certauth module.  For the certificate
>>     * to be authorized at least one module must return 0, and no module can an
>>     * error code other than KRB5_PLUGIN_NO_HANDLE (pass).  Add indicators from
>>     * modules that return 0 or pass.
>>     */
>>
>> but that’s not really true as each plugin returns KRB5KDC_ERR_CLIENT_NAME_MISMATCH when a match is not found.  This means the first plugin that fails kicks out of that loop and no other checks are performed.  I noticed this specifically because we were testing with certs that need the dbmatch module to work but it was never being called.
>
> Can you elaborate on "each plugin returns
> KRB5KDC_ERR_CLIENT_NAME_MISMATCH when a match is not found"?  Of the
> built-in modules, I think only the san module would ever return that
> code, and should only return it when it finds PKINIT or UPN SANs in the
> certificate and they don't match.  It should return
> KRB5_PLUGIN_NO_HANDLE if the cert doesn't have either of those SAN types.
>
Right - the san module returns it, but the dbmatch module also returns it, and the eku check returns KRB5KDC_ERR_INCONSISTENT_KEY_PURPOSE.

In my use case, I’m working with a DoD CAC so I have a SAN that looks like this "1234567890@mil” - so to work around that I wanted to use the dbmatch feature to add the relation between that SAN and my actual principal.  

But when the san module fails out with KRB5KDC_ERR_CLIENT_NAME_MISMATCH, the rest of the possible plugins are not called so I’ll never reach that dbmatch.
Anything other than KRB5_PLUGIN_NO_HANDLE kicks it out of the loop.  I assume the intent is to process all possible plugins until a match is found so that check needs to be adjusted or maybe just removed.

Attempting to attach the patch a second time - this probably isn’t the “right” fix but helps illustrate what makes it work.


—Craig


diff --git a/src/plugins/preauth/pkinit/pkinit_srv.c b/src/plugins/preauth/pkinit/pkinit_srv.c
index 5da8892..303b49d 100644
--- a/src/plugins/preauth/pkinit/pkinit_srv.c
+++ b/src/plugins/preauth/pkinit/pkinit_srv.c
@@ -370,7 +370,7 @@ authorize_cert(krb5_context context, certauth_handle *certauth_modules,
                               &opts, db_ent, &ais);
         if (ret == 0)
             accepted = TRUE;
-        else if (ret != KRB5_PLUGIN_NO_HANDLE)
+        else if (ret != KRB5_PLUGIN_NO_HANDLE && ret != KRB5KDC_ERR_CLIENT_NAME_MISMATCH)
             goto cleanup;
 
         if (ais != NULL) {
@@ -383,6 +383,9 @@ authorize_cert(krb5_context context, certauth_handle *certauth_modules,
             h->vt.free_ind(context, h->moddata, ais);
             ais = NULL;
         }
+ if (accepted) {
+    break;
+ }
     }
 
     ret = accepted ? 0 : KRB5KDC_ERR_CLIENT_NAME_MISMATCH;






_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev

smime.p7s (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pkinit plugin logic in pkinit_srv.c

Greg Hudson
On 08/24/2017 01:42 PM, Craig Huckabee wrote:
> Right - the san module returns it, but the dbmatch module also returns
> it, and the eku check returns KRB5KDC_ERR_INCONSISTENT_KEY_PURPOSE.

The dbmatch module returns KRB5KDC_ERR_CERTIFICATE_MISMATCH (not the
same error code) if there is a "pkinit_cert_match" string attribute and
it does not match.  It passes if there is no such string attribute.

The eku module returns KRB5KDC_ERR_INCONSISTENT_KEY_PURPOSE if the cert
doesn't have the EKU for a PKINIT client cert, unless kdc.conf contains
"pkinit_eku_checking = none", in which case it passes.

> In my use case, I’m working with a DoD CAC so I have a SAN that looks
> like this "1234567890@mil” - so to work around that I wanted to use the
> dbmatch feature to add the relation between that SAN and my actual
> principal.

We expected some users to want to authorize client certs which weren't
issued for PKINIT at all (so no id-pkinit-san SAN and no
id-pkinit-KPClientAuth EKU).  In this use case, the san module would
return KRB5_PLUGIN_NO_HANDLE, the admin would disable EKU checking so
that the eku modules returns KRB5_PLUGIN_NO_HANDLE, and the dbmatch
module would control authorization.

We didn't expect people to want to authorize client certs with an
id-pkinit-san SAN containing the wrong principal name.  The only way to
make that work in the current code is to use module configuration to
disable the san certauth module, which would mean that you'd have to use
dbmatch for everyone.

> But when the san module fails out with KRB5KDC_ERR_CLIENT_NAME_MISMATCH,
> the rest of the possible plugins are not called so I’ll never reach that
> dbmatch.
> Anything other than KRB5_PLUGIN_NO_HANDLE kicks it out of the loop.  I
> assume the intent is to process all possible plugins until a match is
> found so that check needs to be adjusted or maybe just removed.

It is intentional that certauth modules can authorize, explicitly deny,
or pass.  In this case the san module is explicitly denying the cert
because it found a PKINIT SAN for a different principal than the request
client principal.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: pkinit plugin logic in pkinit_srv.c

Craig Huckabee-2

> On Aug 24, 2017, at 2:47 PM, Greg Hudson <[hidden email]> wrote:
>
> On 08/24/2017 01:42 PM, Craig Huckabee wrote:
>
>> In my use case, I’m working with a DoD CAC so I have a SAN that looks
>> like this "1234567890@mil” - so to work around that I wanted to use the
>> dbmatch feature to add the relation between that SAN and my actual
>> principal.
>
> We expected some users to want to authorize client certs which weren't
> issued for PKINIT at all (so no id-pkinit-san SAN and no
> id-pkinit-KPClientAuth EKU).  In this use case, the san module would
> return KRB5_PLUGIN_NO_HANDLE, the admin would disable EKU checking so
> that the eku modules returns KRB5_PLUGIN_NO_HANDLE, and the dbmatch
> module would control authorization.
>
> We didn't expect people to want to authorize client certs with an
> id-pkinit-san SAN containing the wrong principal name.  The only way to
> make that work in the current code is to use module configuration to
> disable the san certauth module, which would mean that you'd have to use
> dbmatch for everyone.
>
Unfortunately the SAN present isn’t a id-pkinit-san, these cards are only issued with the NT-Principal type SAN, so that is what I have to work with.

I’ll revert my build and try disabling the plugin to see if that still works.  Thanks for the context.

—Craig


_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev

smime.p7s (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pkinit plugin logic in pkinit_srv.c

Greg Hudson
On 08/24/2017 03:09 PM, Craig Huckabee wrote:
>> We didn't expect people to want to authorize client certs with an
>> id-pkinit-san SAN containing the wrong principal name.  The only way to
>> make that work in the current code is to use module configuration to
>> disable the san certauth module, which would mean that you'd have to use
>> dbmatch for everyone.

> Unfortunately the SAN present isn’t a id-pkinit-san, these cards are
> only issued with the NT-Principal type SAN, so that is what I have to
> work with.

By NT-Principal type SAN, you mean 1.3.6.1.4.1.311.20.2.3 (User
Principal Name)?  We can handle those, if you set
"pkinit_allow_upn = true" in kdc.conf in the realm configuration (or in
[kdcdefaults]).  The value has to match the request client principal, of
course.

It may be that we presently have the wrong behavior if the cert contains
a UPN SAN and pkinit_allow_upn = false (the default).  In that case the
upn module should probably return KRB5_PLUGIN_NO_HANDLE but might now be
returning a mismatch.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: pkinit plugin logic in pkinit_srv.c

Greg Hudson
On 08/24/2017 03:26 PM, Greg Hudson wrote:
> It may be that we presently have the wrong behavior if the cert contains
> a UPN SAN and pkinit_allow_upn = false (the default).  In that case the
> upn module should probably return KRB5_PLUGIN_NO_HANDLE but might now be
> returning a mismatch.

Just to close the loop on this: it turns out the san module was
explicitly rejecting certs with any SANs at all, even if they have
nothing to do with PKINIT.  This will be fixed soon.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev