client certificate revocation checking support. #1326

cpu · 2023-06-26T17:55:49Z

Description

This branch builds on the webpki CRL support added in v0.101.0 to offer client certificate revocation checking with the Rustls AllowAnyAuthenticatedClient and AllowAnyAnonymousOrAuthenticatedClient implementations of the ClientCertVerifier trait.

This replaces #1322:

We're no longer parsing DER -> CRLs for each client cert validation. In this iteration we parse CRLs only once, when added, holding owned representations of the parsed CRLs.
We provide a builder-like with_crls fn on the verifiers to optionally provide CRLs.
The error handling has been revisited to provide more specificity around CRL related errors, particularly when parsing CRLs.
Test CRL generation and integration test coverage have been added.

deps: update rustls-webpki.

This commit updates the webpki dependency of rustls to v0.101.0, the release that adds CRL support.

verify: AllowAnyAuthenticatedClient CRLs.

This commit introduces a crls: Vec<webpki::OwnedCertRevocationList>> field to the AllowAnyAuthenticatedClient implementation of ClientCertVerifier in preparation of supporting revocation checking of client certs with CRLs.

Each impl gets a builder-style with_crls method that can be used to augment the verifier with CRLs to use for client cert revocation status checking.

The constructed cert revocation lists are provided to webpki at cert verification time to use for revocation lookup.

The tlsserver-mio example is updated with a --crl flag to the that can be used to optionally provide a series of DER encoded CRLs to use for client certificate revocation checking.

test-ca: generate demo CRLs for client certs.

This commit updates the build-a-pki.sh script to generate example certificate revocation lists (CRLs) that mark each of the client certificates as revoked. These can be used by server tests to ensure CRL validation works as expected.

The process of generating CRLs using openssl is... well... not great...

It can't be done without using openssl ca, which in turn requires using an openssl.cnf with all the associated warts. I've done my best to create the absolute minimum configuration that can be used for our purposes.

Using openssl ca also requires writing some intermediate state. The script is updated to create/delete this state through the process of generating the CRLs. This should be sufficient for our needs. Blech.

test-ca: regenerate certs, keys, crls.

Generated with:

cd test-ca
./build-a-pki.sh
git add .

tests: integration test for revoked client certs.

This commit introduces api.rs integration tests that verify a server configured with client authentication (mandatory or optional), and a CRL that specifies a client cert as revoked, will correctly return a revoked error for that client cert.

In addition to the test CRL data generated in a previous commit this work requires rustls-pemfile >= 1.0.3 to use unreleased CRL support in the rustls-pemfile crate. The rustls dev-dependency and the rustls example dependency on this crate are adjusted accordingly.

docs: update README changelog for 0.21.3.

Includes:

CRL feature for the client verifiers.
Certificate validation helpers when using dangerous_configuration
feature.

codecov · 2023-06-26T18:02:25Z

Codecov Report

Merging #1326 (8500e90) into main (777cc07) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1326      +/-   ##
==========================================
+ Coverage   95.88%   95.93%   +0.05%     
==========================================
  Files          61       61              
  Lines       14591    14750     +159     
==========================================
+ Hits        13990    14150     +160     
+ Misses        601      600       -1

Impacted Files	Coverage Δ
rustls/src/error.rs	`97.36% <100.00%> (+2.24%)`	⬆️
rustls/src/verify.rs	`80.87% <100.00%> (+2.57%)`	⬆️

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

rustls/src/verify.rs

rustls/tests/api.rs

rustls/src/error.rs

rustls/src/verify.rs

cpu · 2023-06-27T20:58:39Z

cpu added 3 commits 51 minutes ago

test-ca generator script updates to also produce CRLs.
re-generated test-ca files.
integration test using the test CRLs.

PR description updated. Also required a 2nd Cargo patch for rustls-pemfile until we cut a release w/ CRL support.

test-ca/build-a-pki.sh

cpu · 2023-06-28T13:25:18Z

cpu force-pushed the cpu-crl-support branch from 3346e76 to 2d75883

Just swapping the Cargo.toml patch from my pemfile fork/branch to main of rustls/pemfile.

cpu · 2023-06-28T14:41:22Z

cpu force-pushed the cpu-crl-support branch from 2d75883 to 68d4de0

Pemfile patch replaced with an update to the now published v1.0.3.

ctz · 2023-06-29T14:10:16Z

Please note the breaking change¹ in the README.md changelog.

which is limited to, I think, the additional parameter on AllowAnyAnonymousOrAuthenticatedClient::new ↩

cpu · 2023-06-29T14:13:30Z

which is limited to, I think, the additional parameter on AllowAnyAnonymousOrAuthenticatedClient::new leftwards_arrow_with_hook

In addition to the AllowAnyAnonymousOrAuthenticatedClient and AllowAnyAuthenticatedClient constructors getting a new arg, they're also now fallible and return a Result since CRL parsing can error.

I will note both aspects in the changelog 👍

cpu · 2023-06-29T18:37:35Z

cpu force-pushed the cpu-crl-support branch from 344d566 to 304c688

Rebased to resolve conflicts.

cpu · 2023-07-03T15:35:43Z

cpu force-pushed the cpu-crl-support branch from 13cdbe9 to 14ed8be

Rebased, updated the webpki patch to use main instead of my fork.

One other substantial change: based on experience integrating this work in rustls-ffi, and more thought on the API design in general, I've reworked how CRLs are provided to the AllowAnyAnonymousOrAuthenticatedClient and AllowAnyAuthenticatedClient impls of ClientCertVerifier.

Initially I provided Vec<UnparsedCertRevocationList> as a new constructor arg. That's yucky in practice for ~3 reasons (also outlined in the updated PR desc):

It makes the constructor fallible since we have to handle CRL parse errors. It's unfortunate to force that complexity onto users that are likely not going to provide any CRLs.
It's a breaking API change, where-as adding a new fn to do this isn't (since it's defined on the concrete types, not the trait).
It's a pain to work with a Vec of UnparsedCertRevocationList (e.g. a Vec<Vec<u8>> on the C-side, as opposed to being able to use add_crl with a single Vec<u8>.

Let me know what you think. I can go back to the previous approach if folks dislike this variation.

rustls/src/verify.rs

rustls/src/error.rs

examples/src/bin/tlsserver-mio.rs

test-ca/build-a-pki.sh

rustls/tests/common/mod.rs

djc · 2023-07-04T10:15:15Z

6D40

I commented on this before seeing this discussion:

One other substantial change: based on experience integrating this work in rustls-ffi, and more thought on the API design in general, I've reworked how CRLs are provided to the AllowAnyAnonymousOrAuthenticatedClient and AllowAnyAuthenticatedClient impls of ClientCertVerifier.

Initially I provided Vec<UnparsedCertRevocationList> as a new constructor arg. That's yucky in practice for ~3 reasons (also outlined in the updated PR desc):
1. It makes the constructor fallible since we have to handle CRL parse errors. It's unfortunate to force that complexity onto users that are likely not going to provide any CRLs.

2. It's a breaking API change, where-as adding a new fn to do this isn't (since it's defined on the concrete types, not the trait).

Not sure this is a great argument? One would argue that it might be useful for downstream consumers to notice they now have the option to provide a CRL.

3. It's a pain to work with a `Vec` of `UnparsedCertRevocationList` (e.g. a `Vec<Vec<u8>>` on the C-side, as opposed to being able to use `add_crl` with a single `Vec<u8>`.

Why is it a pain?

Maybe the right path forward is to provide a with_crls(self, impl Iterator<Item = UnparsedCertRevocationList>) -> Result<Self, CrlError> method on the ClientCertVerifier impls? That would avoid breaking the API and making the constructor fallible while still having a more ergonomic builder-like API. Not sure how it would impact the -ffi side.

cpu · 2023-07-04T15:09:03Z

Maybe the right path forward is to provide a with_crls(self, impl Iterator<Item = UnparsedCertRevocationList>) -> Result<Self, CrlError> method on the ClientCertVerifier impls? That would avoid breaking the API and making the constructor fallible while still having a more ergonomic builder-like API. Not sure how it would impact the -ffi side.

I'll give that a shot. I think it's a nicer API.

cpu · 2023-07-04T16:13:11Z

codecov/patch Failing after 1s — 35.82% of diff hit (target 96.32%)

I'll see about shoring up the test coverage. Otherwise I think this is ready for review with all existing review feedback addressed.

With the webpki 0.101.0 release being available I removed the cargo patch and moved this out of draft status.

ctz

lgtm! for the coverage gaps i'd consider some unit-test-style direct exercise of the error types.

cpu · 2023-07-04T18:55:16Z

codecov/patch — 100.00% of diff hit (target 96.32%)

Some more unit tests did the trick 👍

examples/src/bin/tlsserver-mio.rs

rustls/src/verify.rs

This commit updates the webpki dependency of rustls to v0.101.0, the release that adds CRL support.

This commit introduces a `crls: Vec<webpki::OwnedCertRevocationList>>` field to the `AllowAnyAuthenticatedClient` implementation of `ClientCertVerifier` in preparation of supporting revocation checking of client certs with CRLs. Each impl gets a builder-style `with_crls` method that can be used to augment the verifier with CRLs to use for client cert revocation status checking. The constructed cert revocation lists are provided to webpki at cert verification time to use for revocation lookup. The `tlsserver-mio` example is updated with a `--crl` flag to the that can be used to optionally provide a series of DER encoded CRLs to use for client certificate revocation checking.

This commit updates the `build-a-pki.sh` script to generate example certificate revocation lists (CRLs) that mark each of the client certificates as revoked. These can be used by server tests to ensure CRL validation works as expected. The process of generating CRLs using `openssl` is... well... not great... It can't be done without using `openssl ca`, which in turn requires using an `openssl.cnf` with all the associated warts. I've done my best to create the absolute minimum configuration that can be used for our purposes. Using `openssl ca` also requires writing some intermediate state. The script is updated to create/delete this state through the process of generating the CRLs. This should be sufficient for our needs. Blech.

Generated with: ``` cd test-ca ./build-a-pki.sh git add . ```

This commit introduces `api.rs` integration tests that verify a server configured with client authentication (mandatory or optional), and a CRL that specifies a client cert as revoked, will correctly return a revoked error for that client cert. In addition to the test CRL data generated in a previous commit this work requires `rustls-pemfile` >= 1.0.3 to use unreleased CRL support in the `rustls-pemfile` crate. The rustls dev-dependency and the rustls example dependency on this crate are adjusted accordingly.

Includes: * CRL feature for the client verifiers. * Certificate validation helpers when using `dangerous_configuration` feature.

cpu self-assigned this Jun 26, 2023

cpu commented Jun 26, 2023

View reviewed changes

rustls/src/verify.rs Outdated Show resolved Hide resolved

rustls/src/verify.rs Show resolved Hide resolved

rustls/tests/api.rs Outdated Show resolved Hide resolved

djc reviewed Jun 27, 2023

View reviewed changes

ctz reviewed Jun 27, 2023

View reviewed changes

rustls/src/verify.rs Outdated Show resolved Hide resolved

cpu mentioned this pull request Jun 27, 2023

Owned CRL representation and associated updates. rustls/webpki#100

Merged

cpu force-pushed the cpu-crl-support branch from e7e6fe6 to 8c89c5f Compare June 27, 2023 16:19

cpu commented Jun 27, 2023

View reviewed changes

test-ca/build-a-pki.sh Show resolved Hide resolved

cpu commented Jun 27, 2023

View reviewed changes

test-ca/build-a-pki.sh Outdated Show resolved Hide resolved

cpu force-pushed the cpu-crl-support branch from 3346e76 to 2d75883 Compare June 28, 2023 13:24

cpu force-pushed the cpu-crl-support branch from 2d75883 to 68d4de0 Compare June 28, 2023 14:40

cpu mentioned this pull request Jun 29, 2023

Track rustls 0.22 API changes rustls/rustls-ffi#320

Closed

cpu force-pushed the cpu-crl-support branch from 344d566 to 304c688 Compare June 29, 2023 18:37

cpu force-pushed the cpu-crl-support branch 2 times, most recently from 13cdbe9 to 14ed8be Compare July 3, 2023 15:29

cpu mentioned this pull request Jul 3, 2023

WIP: client certificate revocation checking support. rustls/rustls-ffi#323

Closed

8 tasks

djc reviewed Jul 4, 2023

View reviewed changes

cpu force-pushed the cpu-crl-support branch from 14ed8be to b9c2fba Compare July 4, 2023 16:01

cpu changed the title ~~WIP: client certificate revocation checking support.~~ client certificate revocation checking support. Jul 4, 2023

cpu force-pushed the cpu-crl-support branch from b9c2fba to 22295e0 Compare July 4, 2023 16:07

cpu marked this pull request as ready for review July 4, 2023 16:09

ctz approved these changes Jul 4, 2023

View reviewed changes

cpu force-pushed the cpu-crl-support branch 2 times, most recently from e018bfc to 39964f9 Compare July 4, 2023 18:49

cpu force-pushed the cpu-crl-support branch from 39964f9 to b9770ac Compare July 5, 2023 14:58

ctz approved these changes Jul 5, 2023

View reviewed changes

djc approved these changes Jul 5, 2023

View reviewed changes

examples/src/bin/tlsserver-mio.rs Outdated Show resolved Hide resolved

rustls/src/verify.rs Outdated Show resolved Hide resolved

cpu added 6 commits July 5, 2023 11:16

deps: update rustls-webpki.

0c04be4

This commit updates the webpki dependency of rustls to v0.101.0, the release that adds CRL support.

test-ca: regenerate certs, keys, crls.

36e8113

Generated with: ``` cd test-ca ./build-a-pki.sh git add . ```

docs: update README changelog for 0.21.3.

8500e90

Includes: * CRL feature for the client verifiers. * Certificate validation helpers when using `dangerous_configuration` feature.

cpu force-pushed the cpu-crl-support branch from b9770ac to 8500e90 Compare July 5, 2023 15:25

cpu merged commit 27907af into rustls:main Jul 5, 2023

cpu deleted the cpu-crl-support branch July 5, 2023 15:34

jsha mentioned this pull request Jul 13, 2023

Rustls 0.21.5, client verifier CRL support. rustls/rustls-ffi#324

Merged

client certificate revocation checking support. #1326

client certificate revocation checking support. #1326

Uh oh!

Conversation

Uh oh!

Description

deps: update rustls-webpki.

verify: AllowAnyAuthenticatedClient CRLs.

test-ca: generate demo CRLs for client certs.

test-ca: regenerate certs, keys, crls.

tests: integration test for revoked client certs.

docs: update README changelog for 0.21.3.

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Footnotes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!