v0.100.3 preparation #172
Merged
v0.100.3 preparation #172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is executing a TODO when the chain length exceeds 6 issuers deep.
This commit renames the `check_signatures` fn, as it will soon be used to do more than simply verifying signatures.
This commit updates the name constraint validation done during path building to apply a budget for the maximum allowed number of name constraint checks. We use the same limit that golang crypto/x509 applies by default: 250,000 comparisons. Note: this commit applies the budget during path building in a manner that means certificates _not_ part of the built path can consume comparisons from the budget even though they will not be present in the complete validated path. Similarly name constraints are evaluated before signatures, meaning a certificate that doesn't verify to a trusted root still has its constraints parsed and evaluated. A subsequent commit will adjust these shortcomings.
This commit updates the path building process such that name constraints are only evaluated against a complete path where signatures on the chain have been checked successfully to a trust anchor. This avoids: * Parsing name constraints before signatures are validated. * Evaluating name constraints and consuming name constraint comparison budget for certificates that are not part of the built path. In the future it could be possible to interleave the name constraint checking with the signature checking, however the logic for this is more complicated. For an initial fix let's prefer a simpler solution that walks the built + validated path to check name constraints from the trust anchor to the end entity certificate.
This commit adjusts the arguments to the `verify_chain` test helper to take references instead of moving the arguments. This makes it easier to use the same inputs for multiple `verify_chain` invocations.
This commit updates the `verify_chain` helper to allow providing an optional `Budget` argument (using the default if not provided). This makes it easier to write tests that need to customize the path building budget (e.g. `name_constraint_budget`).
This commit adds a method to `Error` for testing whether an error should be considered fatal, e.g. should stop any further path building progress. The existing consideration of fatal errors in `loop_while_non_fatal_error` is updated to use the `is_fatal` fn. Having this in a central place means we can avoid duplicating the match arms in multiple places, where they are likely to fall out-of-sync.
Previously the handling of fatal path building errors (e.g. those that should halt all further exploration of the path space) was mishandled such that we could hit the maximum signature budget and still pursue additional path building. This was demonstrated by the `test_too_many_path_calls` unit test which was hitting a `MaximumSignatureChecksExceeded` error, but yet proceeding until hitting a `MaximumPathBuildCallsExceeded` error. This commit updates the error handling between the first and second `loop_while_non_fatal_error` calls to properly terminate the search when a fatal error is encountered, instead of proceeding with further search. The existing `test_too_many_path_calls` test is updated to use an artificially large signature check budget so that we can focus on testing the limit we care about for that test without needing to invest in more complicated test case generation. This avoids hitting a `MaximumSignatureChecksExceeded` error early in the test (which now terminates further path building), instead allowing execution to continue until the maximum path building call budget is expended (matching the previous behaviour and intent of the original test).
The `loop_while_non_fatal_error` helper can return one of three things: * success, when a validated chain to a trust anchor was built. * a fatal error, e.g. when a `Budget` has been exceeded and no further path building should occur because we've exhausted a budget. * a non-fatal error, when a candidate chain results in an error condition, but other paths could be considered if the options are not exhausted. This commit attempts to express this in the type system, centralizing a check for what is/isn't a fatal error and ensuring that downstream callers to `loop_while_non_fatal_error` handle the fatal case appropriately.
Codecov Report
@@ Coverage Diff @@
## rel-0.100 #172 +/- ##
=============================================
+ Coverage 94.59% 95.11% +0.51%
=============================================
Files 13 13
Lines 2573 2783 +210
=============================================
+ Hits 2434 2647 +213
+ Misses 139 136 -3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more 📢 Have feedback on the report? Share it here. |
cpu
commented
Sep 11, 2023
ctz
approved these changes
Sep 12, 2023
djc
approved these changes
Sep 12, 2023
|
Thanks for doing all the backporting legwork! |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This branch targets a base of rel-0.100 to prepare a point release in the v0.100.x series.
It brings in:
Unlike #170 we don't bring in clippy fixes (they applied to CRL code that isn't present in this branch), and we don't bring in the subject common name handling changes (since the problematic parsing isn't exposed to users via the name iteration code that's only in 0.101+ it didn't seem worthwhile).
I won't call out each individual commit this time since there are many :-) In terms of required adjustments for backporting there were more substantial changes in this branch than #170, largely due to the drift in APIs between releases.
pki-typesabstractions had to be replaced with their legacy equivalents.rank_errornotion. This is also reflected in 0b139ab having to adjust its expected error type in a unit test to the catch-all "UnknownIssuer" error.Proposed Release Notes