Skip to content
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

return most specific error from path building failures #89

Merged
merged 3 commits into from
Jun 19, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Jun 16, 2023

This branch adopts the "specificity ranking" strategy discussed in #79. In particular this allows us to surface CRL revocation errors like CertRevoked without them being turned into generic UnknownIssuer errors.

Resolves #79.

error: introduce ranking of errors by specificity.

This commit introduces a error.most_specific function that can compare two Error instances, returning the one that is "most specific". This is primarily useful for the context of path building where we may encounter several errors while trying to build a path to a trust anchor, and want to return the one that will be most useful to an end user for diagnosing the problem at hand. Without this feature we always yield Error::UnknownIssuer, even when the issuer is known and some more specific problem has been detected.

Since introducing more specific errors has changed the expected error result for several of the generated tests we temporarily #ignore them so that this commit can build clean without needing to bundle in a large diff regenerating test content. This will be undone in subsequent commits to restore test coverage.

The code in this diff was originally proposed in briansmith/webpki#188 by @stepancheg - All credit to them for the idea and the base implementation I've extended/adapted.

tests: update generator expected errors.

Now that we yield more specific errors for some path building error conditions the generated test suite tests need to be updated to stop expecting the generic UnknownIssuer error and instead expect the more specific error relevant to the test case.

This also fixes a few bugs in the generated tests:

  • A couple of the CRL tests were using the wrong keys to sign the CRL. These mistakes were being masked by the unspecific errors returned previously.

  • The intermediate CRL wrong key usage tests wouldn't work as written! Since the chain was EE->intermediate->trust anchor the intermediate CRL was always signed by a trust anchor and we don't track key usage for those. To fix this we now generate a chain like EE->intermediate A->intermediate B->trust anchor and can test that a CRL revoking intermediate A when signed by intermediate B without the right KU errors.

tests: regen server_cert and client_auth_rev tests.

Generated with:

cd tests
./generate.py --tls-server-certs --no-signatures --no-client-auth --client-auth-revocation

@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #89 (885722f) into main (9577180) will decrease coverage by 0.53%.
The diff coverage is 67.14%.

@@            Coverage Diff             @@
##             main      #89      +/-   ##
==========================================
- Coverage   95.93%   95.40%   -0.53%     
==========================================
  Files          15       15              
  Lines        3219     3262      +43     
==========================================
+ Hits         3088     3112      +24     
- Misses        131      150      +19     
Impacted Files Coverage Δ
src/error.rs 42.22% <39.47%> (-14.93%) ⬇️
src/verify_cert.rs 95.08% <100.00%> (+1.77%) ⬆️

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

@cpu cpu force-pushed the cpu-crl-support-pt4-error-specificity branch from de68d04 to 0e3e576 Compare June 18, 2023 13:27
@cpu cpu marked this pull request as ready for review June 18, 2023 13:32
Copy link
Member

@ctz ctz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is pretty good -- making up ordinals for each error and comparing those is a bit around-the-houses but i can't think of a less complex option. and i prefer the option of having a explicit most_specific_error function rather than implementing PartialOrd on Error.

src/error.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Outdated Show resolved Hide resolved
cpu added 3 commits June 19, 2023 09:56
This commit introduces a `error.most_specific` function that can
compare two `Error` instances, returning the one that is "most
specific". This is primarily useful for the context of path building
where we may encounter several errors while trying to build a path to
a trust anchor, and want to return the one that will be most useful to
an end user for diagnosing the problem at hand. Without this feature we
always yield `Error::UnknownIssuer`, even when the issuer is known and
some more specific problem has been detected.

Since introducing more specific errors has changed the expected error
result for several of the generated tests we temporarily `#ignore` them
so that this commit can build clean without needing to bundle in a large
diff regenerating test content.
Now that we yield more specific errors for some path building error
conditions the generated test suite tests need to be updated to stop
expecting the generic `UnknownIssuer` error and instead expect the more
specific error relevant to their test case.

This also fixes a few bugs in the generated tests:

* A couple of the CRL tests were using the wrong keys to sign the CRL.
* The intermediate CRL wrong key usage tests wouldn't work! Since the
  chain was EE->intermediate->trust anchor the intermediate CRL was
  always signed by a trust anchor and we don't track key usage for
  those. To fix this we now generate a chain like EE->intermediate
  A->intermediate B->trust anchor and can test that a CRL revoking
  intermediate A when signed by intermediate B without the right KU
  errors.
Generated with:
```
cd tests
./generate.py --tls-server-certs --no-signatures --no-client-auth --client-auth-revocation
```
@cpu cpu force-pushed the cpu-crl-support-pt4-error-specificity branch from 0e3e576 to 885722f Compare June 19, 2023 13:56
@cpu cpu merged commit 36a989e into rustls:main Jun 19, 2023
@cpu cpu deleted the cpu-crl-support-pt4-error-specificity branch June 19, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Options for improving build_chain error specificity
3 participants