-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add support for enforcing CRL expiration using nextUpdate field #227
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #227 +/- ##
==========================================
+ Coverage 97.21% 97.25% +0.03%
==========================================
Files 19 19
Lines 4100 4158 +58
==========================================
+ Hits 3986 4044 +58
Misses 114 114 ☔ View full report in Codecov by Sentry. |
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.
LGTM, thanks!
The policy of ignoring by default feels at odds a little with our usual default of failing closed... Do we have any context of why other implementations aren't enforcing this?
@jasperpatterson for the clippy lint, probably just suppress it for that function?
Personally, I agree. I expected this to be enforced and only caught that it wasn't because I wrote a test covering it. The closest explanation I could find was this comment (about OCSP, not CRL) in the Go library: golang/go#45244 (comment) I also was unsure about changing the default here since it could be a breaking change. |
It might be good to look at what other implementations (notably BoringSSL and the Go std library) do for this case? |
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 think this would benefit from some integration test cases to explore the actual behaviour we want:
- a single CRL authoritative for the certificate, and the verification epoch is past nextUpdate.
- two CRLs authoritative for the certificate, one past, one current. I think the code right now will select the first one, and ignore the other. That is not ideal in several ways: a) later updates to the CRL are ignored, b) it fixates on an expired CRL when a "better" one is available, c) the order of CRLs in our API is unspecified, I think.
I think maybe it would be a better idea to do the expiration inside authoritative()
which would work correctly for (2), while falling to status_policy
for rejecting the certificate.
AIUI the Go standard library only exposes creating CRLs and verifying their signatures, not processing them for revocation decisions so it's not a very helpful point of comparison. I looked at https://github.com/grpc/grpc-go's "advancedtls" package as one data point back when I was starting this work and found that it did check the CRL next update to decide if a cached CRL was appropriate or not, but if it was expired and the on-disk copy wasn't updated, it would proceed using the expired CRL. I emailed the maintainers at the time to see if they considered this a bug/security issue and they replied that it was by design saying:
After that I compared with other implementations and found they acted similar and I replied:
|
You're right, though this kinda feels like an independent issue to me. It already wouldn't be picking the "best" CRL, regardless of expiration enforcement. e.g. It doesn't choose the most recently issued one. Maybe filtering them based on the |
Just wanted to clarify what changes you all would be looking for on this PR in order to move it forward? |
@jasperpatterson Apologies for the delay responding, I had missed the GitHub notification for this comment. From my perspective I think this mainly needs the integration test coverage that @ctz mentioned in his earlier comment. Beyond that, it would be helpful to see a draft PR on the Rustls repo that updates the ServerCertVerifierBuilder and ClientCertVerifierBuilder to use the new option implemented here. It would be nice to see this working end-to-end before we lock in the |
@jasperpatterson Is this ready for review after the latest push or should we hold off for a draft PR in Rustls, or further adjustments here? |
@cpu This is now ready for review! I added in the integration tests as requested. That caused quite a large diff unfortunately, as I needed to force regenerate all the test CRLs and signatures, otherwise my new tests would fail with I've also opened a draft PR in rustls: rustls/rustls#1922 |
Awesome, thanks for iterating on this. I will review ASAP.
NP, that's expected in this case. |
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.
Looks good to me. Thank you for following up with the extra test coverage.
I had some nits/minor comments (and I think the tests/signatures/
changes need to be backed out) but the overall implementation seems correct.
tests/signatures/ed25519_key_and_ed25519_detects_bad_signature.sig.bin
Outdated
Show resolved
Hide resolved
ec565c5
to
d4b6e98
Compare
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.
LGTM (I did not review the integration tests).
@ctz I think this is ready for you to give a review pass when you have a chance. |
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.
Thanks!
@jasperpatterson Thanks for your patience with this one 🙇 I'm preparing a release with this changeset in #256 |
This adds support for enforcing the CRL nextUpdate field (i.e. expiration). Currently there is no way for a consumer of this crate to enforce expiration on a CRL since the nextUpdate field is not even exposed.
I considered simply exposing the
next_update
field on theCertRevocationList
for a consumer to use on their own, but opted for an expiration enforcement option which seems more convenient, and follows the pattern of other builder options that already exist onRevocationOptionsBuilder
.I found this idea was also mentioned in one of the original PRs that added support for CRL:
To enforce expiration, you pass configuration to the
RevocationOptionsBuilder
:The default behaviour, ignoring the nextUpdate field, remains unchanged.