Enable certificate revocation #1116
Conversation
The ApplicatioinPkcs7MimeTest is one of the only test where the context is passed in by the test so we can load the crls. I added code to load crls in the SecureMimeTest but then realized the context is not passed to the ApplicationPkcs7Mime.Sign method so you can't inject the crls.
It seems to be something about some updates in the OS. Maybe OpenSSL changes or OS changes. It broke on the Build server and locally on my Ubuntu 22.04 that I recently updated. After hours of debugging and finally getting the debugger to attach to the test I could narrow down where the error was happening. Copilot helped me to find a solution. I included the explanation here: Certainly! The line parameters.Y = key.Parameters.G.ModPow(key.X, key.Parameters.P).ToByteArrayUnsigned(); is used to derive the public key component Y from the private key components in the DSA (Digital Signature Algorithm) parameters. Here's a detailed explanation: DSA Key Components • P: A large prime number. • Q: A prime divisor of P-1. • G: A generator of a subgroup of order Q in the multiplicative group of integers modulo P. • X: The private key, which is a randomly chosen integer such that 0 < X < Q. • Y: The public key, which is derived from the private key. Deriving the Public Key Y In DSA, the public key Y is derived from the private key X using the following formula: [ Y = G^X \mod P ] Where: • G is the generator. • X is the private key. • P is the prime modulus. Explanation of the Code • key.Parameters.G: This is the generator G. • key.X: This is the private key X. • key.Parameters.P: This is the prime modulus P. The method ModPow is used to perform modular exponentiation, which calculates ( G^X \mod P ). This operation is efficient and secure for large numbers, which are typical in cryptographic algorithms. • key.Parameters.G.ModPow(key.X, key.Parameters.P): This calculates ( G^X \mod P ), resulting in the public key Y as a BigInteger. • .ToByteArrayUnsigned(): This converts the resulting BigInteger to an unsigned byte array, which is the format required by the DSAParameters structure.
|
I'll try to look at this tomorrow |
| var intermediateNow = DateTime.UtcNow; | ||
| crlGenerator.SetIssuerDN (certificate.IssuerDN); | ||
| crlGenerator.SetThisUpdate (intermediateNow); | ||
| crlGenerator.SetNextUpdate (intermediateNow.AddDays (crlOptions.DaysValid)); |
There was a problem hiding this comment.
The problem I see is that we're generating these CRLs if they don't already exist and we are setting it to expire after 7 days.
That means, every 7 days or so, I will need to delete these CRLs and run the unit tests to generate a new batch of them.
The reason the Certificates are done that way is because I used to use OpenSSL commands to generate them locally, check them into the repo, and then be able to run the unit tests again.
When I wrote C# code to generate them instead, I probably should have abandoned the strategy of checking them into the repo, but with a 10-year lifespan, it's not a big deal and also means that I can more easily debug an issue that pops up in the CI builds (reproducible certificates).
The same could be argued for CRLs, I suppose, but 7 days is really short and it won't really test anything (or am I missing something?).
The current tests only really test importing CRLs (which is something we need to test, but I'm not sure if it makes sense to check them into the repo).
For actually testing certificate revocation works if a CRL is available for a particular certificate, what we really need to be able to do is generate one on-the-fly. And then what we would do is have the following tests:
- Check that ApplicationPkcs7Mime.Verify() fails (for the correct reason) if a CRL is available for:
a. the signer's certificate
b. the intermediate certificate
c. the CA certificate(?) - Check that MultipartSigned.Verify() (for the correct reason) if a CRL is available for:
a. the signer's certificate
b. the intermediate certificate
c. the CA certificate(?) - Check that ApplicationPkcs7Mime.Encrypt()/SignAndEncrypt() (for the correct reason) if a CRL is available for:
a. a recipient's certificate
b. the intermediate certificate of a recipient's certificate
c. the CA certificate of a recipient's certificate
I'm not sure what should happen if we try to Sign() with a revoked certificate, but arguably we should allow it and leave it up to a caller to make the decision not to. If we do this, we should probably have a public method on SecureMimeContext that can check revocation status for a particular certificate.
There was a problem hiding this comment.
There is a lot here to work through. I am not in a rush.
The 7-day choice was just an arbitrary decision. In other projects, I've set it to 10 years to avoid dealing with it. I also have automated builds that produce all the PKI without checking into source. I'm flexible on this. The rollover style integration tests are interesting, but I wasn't trying to solve that in this PR. Funny enough, the first time I saw those .cfg files, I thought they were OpenSSL config files.
I think you're leaning towards generating the CRLs on the fly. I'll update the code accordingly.
The current tests only really test importing CRLs (which is something we need to test, but I'm not sure if it makes sense to check them into the repo).
I've run this code against my test PKI with CRL Endpoints defined. If I don't load the CRLs, the chain build fails as expected, similar to using X509Chain.Build from System.Security.Cryptography. I didn't include that code in this PR because I thought it would be better suited for another PR focused on CRL and Intermediate resolution. I'll bring that in here if the tests keep passing to add to the conversation. I'm still waiting on the resolution process for a future PR.
To repeat myself in different words, If a certificate in the chain has a CRL or OCSP endpoint extension and CheckCertificateRevocation = true but the CRL isn't loaded, the x509Chain Build process will always fail. This is what I have tested with my PKI. So, the following comment might not be optional. The user can set CheckCertificateRevocation = false. Before this PR, IsRevocationEnabled was hard coded to false in two places and none of the tests certs had a CRLDistributionPoint to trigger this failure.
- Check that ApplicationPkcs7Mime.Verify() fails (for the correct reason) if a CRL is available for:
a. the signer's certificate
b. the intermediate certificate
a. the CA certificate(?)
...
I think creating a revoced cert is another missing item for testing.
Loading a new CRL while holding an expired one is also good but can be included in the CRL Resolution PR.
There was a problem hiding this comment.
To repeat myself in different words, If a certificate in the chain has a CRL or OCSP endpoint extension and CheckCertificateRevocation = true but the CRL isn't loaded, the x509Chain Build process will always fail.
D'oh, now I get it. Thanks for the explanation. If my memory is correct, I think I ran into this issue in testing years ago.
I'm really happy to have your help with this because it's not my area of expertise.
I think creating a revoced cert is another missing item for testing.
Er, yes, in my brain-fart comment above, what I had been intending to say is that we should check those scenarios for a certificate that has been revoked which we would know about based on the CRL that the tests will dynamically generate to let us know that the certificate has been revoked (at least I think that's how CRL's work).
I would actually love to work with you on this functionality. Would it make sense for me to create a feature branch?
There was a problem hiding this comment.
Yes, a feature branch sounds like a good way to go. I assume it will include everything I've hinted at, including the CRL and intermediate resolutions. I wonder if we should also change BouncyCastleSecureMimeContext.BuildCertificateChain to virtual. This is how I approached it the first time I started testing with my PKI. For example an implementor could choose the System.Security.Cryptography code to build the chain.
FYI, The MS stack resolves the Intermediates and CRLs for you. I think the only place it doesn't is on Android. Which was surprising, I would have expected it to me similar to the Linux experience. Also on Linux I would have to come up with a caching strategy for intermediates and crls. But your pattern here in MimeKit is very adaptable to caching the Intermediates and CRLs.
There was a problem hiding this comment.
I'll go create a feature branch.
There was a problem hiding this comment.
branch: smime-revocation-checks
|
I will open a new PR for the DSA fix |
The crls need the pfx to not exist to be generated. That is the logic at this time. Can't delet rsa/smime.pfx becauase the TestAutoUpgradeVersion0 and TestAutoUpgradeVersion1 () expect it to be specfice. I didn not chase this one down.
|
FWIW, I cherry-picked your DSA fix from your fork. You don't need to create a PR anymore. |
|
I also cherry-picked de80777 since that could be applied stand-alone without breaking anything |
I returned CheckCertificateRevocation to true in ApplicationPkcs7MimeTest as it asserts the CRL is present in the trust chain when it is built.
Created CRLs in the X509CertificateGenerator.
IsRevocationEnabled is set by the Context CheckCertificateRevocation property rather than set to false in the BouncyCastleSecureMimeContext.
Tests updated
Negative tests added in ApplicationPkcs7MimeTests
In SecureMimeTests CheckCertificateRevocation was set to false.
May need to add a test with a revoked certificate.
Have a different PR in the pipeline that write the crl distribution point to the certificates. Was waiting on this because I wanted to ensure I could test the resolution (download) of CRLs in unit tests. I was just focusing on the CRL creation and manual configuration. I could bring the part that generates these distribution points.