properly test build chain call budget #179
Merged
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
|
For fun (and a warm laptop) you can apply this commit to see the quadratic runtime when the build chain budget is neutered (but the signature validation limit left in-effect). The pathological chain size is also increased for maximum effect. |
djc
approved these changes
Sep 18, 2023
Presently all of the helpers in this module require the `alloc` feature to be present. Rather than sprinkling the feature requirement on all members let's just put at the module level. If we add helpers that don't require `alloc` we can revisit.
Rather than having `make_issuer` support all possible parameters as arguments we should have a helper that provides a base `CertificateParams` with defaults that are sensible for an issuer, and then customize it as required. This commit adds `issuer_params` for generating the base `rcgen::CertificateParams` for an issuer, and then updates `make_issuer` to use it.
This was only used by one test, and now that there's a convenient way to get a `rcgen::CertificateParams` that describes sensible issuer defaults we can use that to customize the name constraints in only that one test, avoiding passing `None` from many others.
This commit updates `make_end_entity` similar to `make_issuer`: there's a new `end_entity_params` helper that returns sensible default `rcgen::CertificateParams` for an end-entity certificate when further customization is helpful, and leaves `make_end_entity` for when any old `rcgen::Certificate` for an end-entity issued by a specific issuer is required.
The `printable_string_common_name` unit test only needs to customize the end entity distinguished name, so let's use the new `test_utils::end_entity_params` fn and only provide that customization.
There aren't any consumers outside of the `test_utils` module and now that we're exposing `rcgen::CertificateParams` it's unlikely there will be new usages for creating params from scratch. For other use-cases it's possible to dig the value out of `rcgen::Certificate` and `rcgen::CertificateParams` instances.
This commit updates the `build_degenerate_chain` helper to properly reproduce the path building complexity budget issue that was reported upstream in briansmith/webpki. The previous implementation only reproduced the issue when the signature validation budget was artificially inflated. The new formulation is able to reproduce the issue with the default signature validation budget, and default build chain call budget (and completes in reasonable time). This better demonstrates the both limits are needed as its possible to make pathological certificate chains that avoid the one limit and are caught by the other.
The budget argument for `build_degenerate_chain` is no longer necessary. We always use the default budget with this helper.
|
Going to administratively merge this while the nightly docs & coverage CI is borked pending upstream updates. |
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.
Previously (#168) we fixed the test case that used the
build_degenerate_chainhelper to test the build chain call budget (introduced in #163) by artificially inflating the signature validation limit. This was crummy for a few reasons:This branch reworks
build_degenerate_chainso that it can reproduce a pathological chain that only has quadratic runtime prevented by the build chain call budget. E.g. the signature validation limit is not sufficient for this test case. The test demonstrates this with the default budget for both signature validations and build chain calls.Along the way I did some small refactoring of the
test_utilsto make it easier to adjust certificate parameters for tests that require that without burdening tests that don't.