-
Notifications
You must be signed in to change notification settings - Fork 177
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
base: master
Are you sure you want to change the base?
Conversation
@@ -2115,8 +2286,23 @@ _kdc_as_rep(astgs_request_t r) | |||
goto out; | |||
} | |||
|
|||
ret = _krb5_principalname2krb5_principal(r->context, &r->client_princ, |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
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 So, first off, if we get a PKINIT request w/ an X.500-style Second, do we need to implement |
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.
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) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
071be2b
to
704d647
Compare
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 |
WIP