Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Support AS-REQ with NT-X500-PRINCIPAL PKINIT #1117

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

svmhdvn
Copy link
Contributor

@svmhdvn svmhdvn commented May 26, 2023

WIP

@@ -2115,8 +2286,23 @@ _kdc_as_rep(astgs_request_t r)
goto out;
}

ret = _krb5_principalname2krb5_principal(r->context, &r->client_princ,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heimdal does support PKINIT SANs (I use that all the time). So you don't have to write code to extract the SANs from the client's PKINIT certificate. What Heimdal doesn't support is X.500-style principal names. So I think what we need here is to inspect the cname from the AS-REQ and decide that what we should do is take the Kerberos principal name from the certificate (if it's present there).

But I think too X.500 names could be checked against the DN of the certificate if it doesn't have a PKINIT SAN, and then maybe we issue the ticket with the requested name (but, uh, no Heimdal or MIT services will understand what to do with it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you don't have to write code to extract the SANs from the client's PKINIT certificate.

The reason I duplicated the code to extract the SAN is because the PADATA parsing and validation code happens after the "AS-REQ_cname to principal_name" code. I initially tried re-using the same PA-DATA extraction code earlier, but it depends on the client_principal to already have been fetched from the database. I'm new to this code though, so is there an easier way of grabbing the client_cert from the PADATA earlier than the "cname to principal" code? I am already reusing functions to extract the SAN from the client PKINIT certificate, but I'm just not able to find an easy way to get the client_cert struct in the first place.

But I think too X.500 names could be checked against the DN of the certificate if it doesn't have a PKINIT SAN, and then maybe we issue the ticket with the requested name

That was my thought. As a first step (since it's already current behaviour right now), we can prioritize the SAN as the principal name (if it exists), and otherwise fallback to using the cname verbatim. Then in future commits, we can allow a configuration option to run a regexp pattern match against the cert's DN and extract whatever key=value pair as the principal name.

Copy link
Contributor

@nicowilliams nicowilliams May 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm new to this code though,

And because of that I'm quite impressed with your PR, so please don't be discouraged.

so is there an easier way of grabbing the client_cert from the PADATA earlier than the "cname to principal" code?

I see your problem now, yes: we look up the requested client principal name way too early to hook this into the existing pre-auth code.

So, yeah, I think we may want to go with roughly what you sketched. Though we'll want to check whether the requested cname is of X.500 name-type and only then do this check. By the way, this is a good time to point out that we do support using PKINIT w/ PKINIT SANs in certificates even when the named principals don't exist in the HDB -- all you have to do is enable synthetic principals (currently set synthetic_clients = true in [kdc] in krb5.conf, though I'll be moving that into [hdb] soon).

An alternative sketch would be to a) parse X.500 names, b) treat them as "synthetic" so we can get past the early cname lookup in the HDB, c) later in the PKINIT code (specifically in _kdc_pk_check_client()) replace the requested cname with the first PKINIT SAN in the client's certificate. Now, this does seem a bit hackish, but so does trying to extract the SAN very early, so I'd be happy with either approach, and this alternative would be harder for you to write, so let's go with your approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess what I'd like to see here is a change where we call siva_extract_san_from_padata() only if b->cname is of X.500 name-type :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually turns out after further testing, none of this is needed. On windows, only the first AS-REQ is made with the NT-X500-PRINCIPAL, and it contains no PADATA. I have some local code to simply run a POSIX regex against a specific attribute in the subject DN (common name seemed like a good start) and extract the value as the principal name. This sent back an ERR_PREAUTH_REQUIRED to Windows, which then sent a subsequent AS-REQ with PADATA and a normal principal entry. The Heimdal KDC returned back a successful AS-REP.

However, windows still doesn't want to consider that as a successful login attempt for some reason, so I'm looking into it further. I'll update this draft PR soon with what I have. In my local changes, I am only running this when the cname is of type NT-X500-PRINCIPAL.

@nicowilliams
Copy link
Contributor

Looking at MIT code, all the MIT KDC does with X.500-style names is ignore them and extract the PKINIT SAN from the client's certificate, like what you're doing, and only when using KRB5_PADATA_S4U_X509_USER as opposed to PKINIT.

So, first off, if we get a PKINIT request w/ an X.500-style cname in the request, let's indeed just extract the first PKINIT SAN from the certificate and use that and ignore the cname (maybe only if it's empty?).

Second, do we need to implement KRB5_PADATA_S4U_X509_USER?

@svmhdvn
Copy link
Contributor Author

svmhdvn commented May 27, 2023

So, first off, if we get a PKINIT request w/ an X.500-style cname in the request, let's indeed just extract the first PKINIT SAN from the certificate and use that and ignore the cname (maybe only if it's empty?).

I was thinking to always ignore the cname if we get an AS-REQ with NT-X500-PRINCIPAL and the SAN exists. As I understand it, this is recommended behaviour by rfc4556 section 3.2.2, bullet #2. If the SAN doesn't exist, go ahead with the cname.

Second, do we need to implement KRB5_PADATA_S4U_X509_USER?

I'm not sure right now as I'm just focused on getting this feature out for the sake of enabling windows smartcard login support with PKINIT.

kdc/kerberos5.c Outdated
*
*/
static int
siva_any_to_certs(hx509_context context, const SignedData *sd, hx509_certs certs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the siva_ prefix. You'll get credit in the commit message, and if you'd like to provide a copyright notice with your name and the same license as in LICENSE then we'd add that to LICENSE.

Copy link
Contributor Author

@svmhdvn svmhdvn May 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha I litter this all across my WIP code to make it really easy to grep and search inside vim for things I've modified. Not at all for the attribution or anything :)

I will clean up my PR once it's ready for submission.

kdc/kerberos5.c Outdated
return 0;
}

static int siva_extract_san_from_padata(astgs_request_t r, PrincipalName *princname) {
Copy link
Contributor

@nicowilliams nicowilliams May 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a block comment here to explain why this code exists at all.

Also, do please run the KDC with valgrind or other memory debugger to make sure there's no memory errors here when exercising this code.

Ah, we do need to write a test. Hmmm, to write a test we'll need a way to generate an AS-REQ with an X.500 cname. That does make this more involved... We'll need to add X.500 support in lib/krb5/principal.c, and then we'd need a command-line option to kuser/kinit.c to indicate that we want an X.500 style client principal name. Since we'd have to add X.500 support in lib/krb5/principal.c that means we could use the alternative sketch for implementing this feature that I mentioned earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most probably, I will be removing this code, since the test I'm doing doesn't seem to require it. I'll update the draft PR with what I have so far, but may need further help in debugging why exactly Windows doesn't consider the successful AS-REP "a successful authentication attempt".

@svmhdvn
Copy link
Contributor Author

svmhdvn commented May 28, 2023

Updated the PR to change the method. On windows with an interactive smartcard login to a kerberos enabled samba CIFS share, an initial AS-REQ with no PA-DATA is sent with a name type of NT-X500-PRINCIPAL and an x509 style subject DN as the name string. Then, further AS-REQs are sent with the PA-DATA and NT-PRINCIPAL name type. This change now just runs a regex to extract a specific KV pair as the krb5 principal name. I will be adding a configuration option to choose the atttribute soon, currently hardcoding it to CN.

This results in a successful AS-REP being sent back to Windows, but I can't figure out any further why the login attempt is unsuccessful. @nicowilliams do you have any expertise in the Windows side of things? One thing I noticed with this code is that the returned cname field in the AS-REP has a name type of NT-X500-PRINCIPAL, but changed the name string to be the actual extracted principal name (without the x509 attributes). Will this cause a mismatch on the Windows side? It just silently fails without any indication about why the login was blocked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants