Add support for X509 revocation checks on Unix #3047
Conversation
| if (!Interop.libcrypto.X509_STORE_add_crl(store, crl)) | ||
| { | ||
| //error:0B07D065:x509 certificate routines:X509_STORE_add_crl:cert already in hash table | ||
| //throw Interop.libcrypto.CreateOpenSslCryptographicException(); |
There was a problem hiding this comment.
Oh, right, this.
For full chains, when the root provides its own CDP extension it will be the same as the one from the intermediate. So there's one legit code that shouldn't throw. Other things will likely just cause the chain to fail, so not throwing here isn't the end of the world.
Or we could plumb the OpenSSL error code into the HResult property of the exception, and check it here. Exposing the code probably has positive utility, it just isn't really an "HResult", and I didn't want to be the first Unix thing to introduce that paradigm without a discussion.
There was a problem hiding this comment.
Or we could plumb the OpenSSL error code into the HResult property of the exception, and check it here
I'm confused... what code is creating the exception? You say "check it here", but it looks like the commented out code here is the one creating the exception.
There was a problem hiding this comment.
There are three options that I see:
- Remove the if, ignore the function result (effectively what I currently have)
- Try to determine the current OpenSSL error code without creating an exception
- The way that OpenSSL's error queue works that feels slightly problematic
- Attach the error code to the exception object, and here do something like
CryptographicException e = CreateException(); if (e.HResult != X509_R_CERT_ALREADY_IN_HASH_TABLE) { throw e; }
The first has a certain distaste to it, the second might be non-destructively-achievable with some native interop, and the third puts values into the HResult field that weren't HResult values.
And I'm sort of torn on which one I like the most...
There was a problem hiding this comment.
Oh, bah. There's no CryptographicException ctor that takes both a code and a message; so option 3 might not exist.
There was a problem hiding this comment.
Sometimes it doesn't occur to me that I have the power to change things CryptographicException 😄.
Currently we don't FormatMessage the HR-based CryptographicExceptions on Windows\CoreFX; but if we built Primitives in a platform-specific manner then we could use FormatMessage on Windows and the OpenSSL formatted message on Unix; then we'd just make CreateOpenSslCryptographicException always use the "HResult" constructor.
It's a bit heavy-handed for this change; so it might be a "make an issue for the messaging change, and a followup one for making this method utilize it" approach.
That really just comes down to "do we want to call OpenSSL's error codes HResults because they happen to both be 32-bit error values?". I know we have errno vs GetLastWin32Error; but how have we been addressing this in other areas?
There was a problem hiding this comment.
I think it's reasonable to carry the error codes in the otherwise defunct HResult field. We do the same thing in a bunch of places with IOException.
|
A few comments/questions, but LGTM. |
* Look up the CRL address via the CRL Distribution Point extension.
* Only "http" protocol addresses will be attempted (mostly this skips ldap addresses)
* Download the CRL using libcurl
* Save the result to ~/.dotnet/corefx/cryptography/crls/
* The filename is the hash value from libcrypto, plus ".crl"
* Later chains will use this same CRL for the remainder of its validity period (crl.NextUpdate)
* This matches the Windows CAPI behavior of caching it for the full lifetime. It's definitely the easiest to write.
OCSP is not supported with this change.
This test has the full certificate chain data available via the ExtraStore collection, and does not check for revocation. Since the chain can build up to a self-signed cert, it's all happy.
This necessitates building the X509 test library differently for Windows and Unix, because it needs to be capable of independently calculating the directory for the test (and we've learned not to trust $HOME!) This test does not test Online mode, because eventually the CRL it downloads will be not-yet-valid as of our validation time; at which point the test will be incapable of ever passing again.
bartonjs
force-pushed
the
x509_revocation
branch
from
8c6a6e6
to
345a5c4
Compare
bartonjs
changed the title
Add support for X509 revocation checks on Unix
|
@bartonjs is the CRL cache location documented/stable? I'm working on sharing this between containers and to do so I need to mount share the CRL cache directory from the container host to all the containers. |
No, it's considered an internal implementation detail.
It hasn't changed yet 😄 |
Add support for X509 revocation checks on Unix Commit migrated from dotnet/corefx@4ba5245
Add support for CRL checking in X509Chain on Unix.
** The filename is the hash value from libcrypto, plus ".crl"
** This matches the Windows CAPI behavior of caching it for the full lifetime. It's definitely the easiest to write.
OCSP is not supported with this change.
Fixes #2203.