v0.101.5 preparation #170

cpu · 2023-09-08T15:45:41Z

Description

This branch targets a base of rel-0.101 to prepare a point release in the v0.101.x series.

It brings in:

I won't call out each individual commit this time since there are many :-)

In terms of required adjustments for backporting none of the commits in this branch required extensive changes. Principally much of the work was in avoiding the pki-types usages that are throughout main. The change in 65eb6a0 probably required the most adjustment to accommodate the difference in name iteration (this branch doesn't have a0e6434).

Proposed Release Notes

Path building complexity is now limited to a maximum budget of path finding operations, avoiding exponential processing time when encountering certificate chains containing many certificates with the same subject/issuer distinguished name but different subject public key information.
Name constraints evaluation is now limited to a maximum number of comparison operations, avoiding exponential processing time when encountering certificate chains containing many name constraints and subject alternate names.
Subject common names are no longer parsed for name iteration, or applying name constraints. Webpki only uses Subject Alternate Names when validating certificates, and the common name handling was buggy, producing Error::BadDer when iterating certificates with printable string subject common names, or omitted common names encoded as an empty sequence.

This wasn't an actual issue, but the fix is equivalent.

This allows new errors to have minimal diffs.

This is executing a TODO when the chain length exceeds 6 issuers deep.

This commit renames the `check_signatures` fn, as it is doing more than simply verifying signatures. It also checks revocation status w/ CRLs when appropriate.

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 adds a pair of tests reproducing issue rustls#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included.

@ctz

As suggested by @ctz in this comment: rustls#167 (comment).

This commit removes parsing of the subject common name field from `NameIterator`, since `rustls-webpki` does not actually verify subject common names except when enforcing name constraints. This fixes issues with common names in formats that `rustls-webpki` doesn't currently support, by removing this code entirely. Fixes rustls-webpki/webpki#167

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 · 2023-09-08T15:48:05Z

Codecov Report

Merging #170 (702d57f) into rel-0.101 (d7f6aa4) will increase coverage by 0.20%.
The diff coverage is 94.00%.

@@              Coverage Diff              @@
##           rel-0.101     #170      +/-   ##
=============================================
+ Coverage      94.89%   95.09%   +0.20%     
=============================================
  Files             15       16       +1     
  Lines           3504     3711     +207     
=============================================
+ Hits            3325     3529     +204     
- Misses           179      182       +3

Files Changed	Coverage Δ
src/der.rs	`98.37% <ø> (-0.02%)`	⬇️
src/error.rs	`56.89% <47.50%> (+4.72%)`	⬆️
src/verify_cert.rs	`97.24% <99.58%> (+0.76%)`	⬆️
src/crl.rs	`100.00% <100.00%> (ø)`
src/end_entity.rs	`77.24% <100.00%> (+8.97%)`	⬆️
src/signed_data.rs	`100.00% <100.00%> (ø)`
src/subject_name/verify.rs	`97.11% <100.00%> (+0.56%)`	⬆️
src/test_utils.rs	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

📢 Have feedback on the report? Share it here.

hawkw

thank you so much for the backports!

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency.

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <eliza@buoyant.io>

djc · 2023-09-12T08:08:21Z

Published 0.101.5 to crates.io and published release notes.

cpu · 2023-09-12T13:31:06Z

Thanks djc 👍

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency.

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <eliza@buoyant.io> Signed-off-by: Adam Shaw <adam.shaw@vipps.no>

ctz and others added 28 commits September 6, 2023 12:46

Pin specific rcgen commit

f475596

misc: clippy fixes

b9ec1f1

Fix expect_fun_call clippy lints

a65cc14

This wasn't an actual issue, but the fix is equivalent.

Track signature limit using Budget type

72b07cc

Add comment indicating source of signature budget

387afe2

Apply budget to number of calls to build_chain_inner

cece639

Make error ranks more sparse

575ab1f

This allows new errors to have minimal diffs.

Introduce and test for MaximumPathDepthExceeded error

2b2d5d6

This is executing a TODO when the chain length exceeds 6 issuers deep.

Import Default trait

92ad878

Force all signature verification operations to consume budget

667370a

Improve readability of build chain calls budget

f50e54a

subject_name: clarify var name in check_name_constraints

e0729cf

verify_cert: check_signatures -> check_signed_chain

3a07985

This commit renames the `check_signatures` fn, as it is doing more than simply verifying signatures. It also checks revocation status w/ CRLs when appropriate.

error: alpha sort

0ccf679

verify_cert: pull out make_issuer test helper

2c555d9

verify_cert: pull out make_end_entity test helper

315d816

verify_cert: pull out verify_chain test helper

e1423de

Remove tests for common name handling

9fe852e

As suggested by @ctz in this comment: rustls#167 (comment).

verify_cert: take references in verify_chain helper

6d86211

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.

Cargo: bump version 0.101.4 -> 0.101.5

702d57f

cpu mentioned this pull request Sep 8, 2023

Remove subject common name parsing #169

Merged

cpu mentioned this pull request Sep 8, 2023

subject_name: fix stale unused_imports allow. #171

Merged

hawkw approved these changes Sep 8, 2023

View reviewed changes

cpu mentioned this pull request Sep 8, 2023

v0.100.3 preparation #172

Merged

hawkw mentioned this pull request Sep 8, 2023

use rustls-webpki instead of linkerd/webpki linkerd/linkerd2-proxy#2465

Merged

ctz approved these changes Sep 11, 2023

View reviewed changes

hawkw mentioned this pull request Sep 11, 2023

proxy: v2.209.0 linkerd/linkerd2#11361

Merged

djc approved these changes Sep 12, 2023

View reviewed changes

djc added this pull request to the merge queue Sep 12, 2023

Merged via the queue into rustls:rel-0.101 with commit 7cb6c64 Sep 12, 2023
21 of 22 checks passed

cpu deleted the cpu-0.101.5-prep branch September 12, 2023 13:31

cpu mentioned this pull request Sep 20, 2023

v0.101.5 broke semver for CertificateRevocationList.verify_signature #185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v0.101.5 preparation #170

v0.101.5 preparation #170

cpu commented Sep 8, 2023 •

edited

codecov bot commented Sep 8, 2023

hawkw left a comment

djc commented Sep 12, 2023

cpu commented Sep 12, 2023

v0.101.5 preparation #170

v0.101.5 preparation #170

Conversation

cpu commented Sep 8, 2023 • edited

Description

Proposed Release Notes

codecov bot commented Sep 8, 2023

Codecov Report

hawkw left a comment

Choose a reason for hiding this comment

djc commented Sep 12, 2023

cpu commented Sep 12, 2023

cpu commented Sep 8, 2023 •

edited