Rustls 0.21.5, client verifier CRL support.

[cpu]

chore: ignore clion/jetbrains dir, venv dir.

Small quality of life improvement for developers using clion and a venv for the Python tests.

deps: update rustls ecosystem dependencies.

Brings in the latest Rustls and supporting libraries.

• Rustls 0.21.0 -> 0.21.5
• Webpki 0.22 -> Rustls webpki 0.101.0
• Rustls pemfile 0.2.1 -> 1.0.3

cipher: rename verifier types to match upstream.

This commit renames the rustls_client_cert_verifier and rustls_client_cert_verifier_optional types to better match their upstream Rustls equivalents: rustls_allow_any_authenticated_client_verifier and rustls_allow_any_anonymous_or_authenticated_client_verifier. While wordier, these names are easier to understand and to map back to their Rust equivalents.

Associated functions (_new and _free) are updated to match the new naming scheme.

cipher: client cert verifier w/ CRLs support.

This commit reworks both the rustls_client_cert_verifier_optional and rustls_client_cert_verifier structs to use a separate builder pattern. This enables optionally providing the builder CRLs from PEM files. Afterwards the intermediate builder can be turned into the standard const * verifiers to provide to the client configuration.

Thanks to @ctz for suggesting this approach and sketching it out with me in a draft branch I've tided up and extended here.

server: support reading CRL PEM for client auth.

This commit updates the tests/server.c example program to support reading one or more CRLs from a single PEM encoded CRL file, provided via AUTH_CRL. This option is only processed when the server is performing mandatory client authentication (e.g. AUTH_CERT was provided).

tests: add CRL mTLS test.

This commit adds a simple test CRL (testdata/test.crl.pem) that lists the testdata/localhost/cert.pem certificate as revoked, but not the testdata/example.com/cert.pem certificate.

The client-server.py integration test driver is then updated with a suite that will start the server binary in a mode that requires mTLS, and that loads the test crl. Two connection attempts are made with the client binary: one using the example.com client cert that isn't expected to error, and one using the localhost client cert that is expected to error (since it's revoked).

[cpu]

rustls-ffi / Clippy nightly (optional) (pull_request)

This failure is nightly clippy saying:

error: usage of `Arc<T>` where `T` is not `Send` or `Sync`
Error:    --> src/lib.rs:396:23
    |
396 |         Arc::into_raw(Arc::new(src)) as *const _
    |                       ^^^^^^^^^^^^^
    |
    = help: consider using `Rc<T>` instead or wrapping `T` in a std::sync type like `Mutex<T>`

Ctz spotted that:

1. This is also happening on main.
2. It looks to be this false positive issue: arc_with_non_send_sync false positive in generic code rust-lang/rust-clippy#11076

[cpu]

cpu force-pushed the jbp-cpu-rustls-0.21.3 branch from bf791cd to 536ddce

Updated to Rustls 0.21.5 (ignore the branch name 😅), addressed the TODO's I left RE: error specificity now that the CRL error sub-variants are properly exported. I think this is ready for review.

[ctz]

i'm perhaps a bit compromised on this, but i think this looks good.

reviewing this makes me think the rustls api for choosing between mandatory and optional client auth is a bit inflexible. but that's not an issue for here.

[cpu]

@jsha Is this a PR you would be interested in reviewing? Since both Ctz and myself contributed code it would be nice to have an independent set of eyes on it if you're available.

[jsha]

Yep! Planning to check it out a little now and a little tomorrow.

[jsha]

Generally looks great, thanks so much for working on this.

I agree with ctz that the upstream API might be better; maybe it would even benefit from a builder pattern? E.g.:

let client_cert_verifier = VerifierBuilder::new().with_crl(xyz).allow_anonymous();
let client_cert_verifier2 = VerifierBuilder::new().with_crl(xyz).require_certificate();

In terms of how this would be used, I'd expect the CRL(s) to be updated with some frequency, but the comments here and on rustls/rustls#1326 don't make it immediately clear how to do that. Is the idea that you're always using the Acceptor API, and each time you select / build a ServerConfig, you provide it with a client cert verifier that is provided with the most up-to-date CRLs you've got?

[cpu]

Generally looks great, thanks so much for working on this.

Thanks for the review! Much appreciated. I'll work through the feedback ASAP.

I agree with ctz that the upstream API might be better; maybe it would even benefit from a builder pattern?

That does seem better. I think my preference would be to try and land this branch as-is and then look at iterating on the Rustls-side API as part of the next release since I suspect it will be a breaking change and we're queuing a few of those for a 0.22.x release.

Does that seem reasonable or would you prefer we hold this work to avoid reworking the API twice on the ffi side?

In terms of how this would be used, I'd expect the CRL(s) to be updated with some frequency, but the comments here and on rustls/rustls#1326 don't make it immediately clear how to do that. Is the idea that you're always using the Acceptor API, and each time you select / build a ServerConfig, you provide it with a client cert verifier that is provided with the most up-to-date CRLs you've got?

Yes I think that's the best approach with the existing implementation 👍

My original goal was to have the verifier configured with a CrlProvider trait (https://github.com/rustls/webpki/blob/3f57243a26b80bfebba6b25d07b914bc49ed813e/src/verify_cert.rs#L21-L29) that could abstract the process of providing CRLs as needed. That ended up being a dead end; I couldn't find a good way to express that the lifetime of the produced CRL needed to last the duration of the overall verification. Changing things so that the verifier provided the CRLs as an argument to webpki when initiating the verification resolved this very naturally (and matches how intermediates/trust anchors were handled).

I suspect there's room for improvements here but it was starting to feel like the energy folks had to dedicate to this effort was waning and the simpler approach was something we could get out the door.

Does that make sense? I'm hopeful there's still utility to this feature even with that limitation in mind.

[jsha]

my preference would be to try and land this branch as-is and then look at iterating on the Rustls-side API as part of the next release

Yep, very agree on this.

always using the Acceptor API, and each time you select / build a ServerConfig, you provide it with a client cert verifier that is provided with the most up-to-date CRLs you've got?
Yes I think that's the best approach with the existing implementation

Sounds good. This does mean the server needs to know in advance all the CRLs it could possibly expect to see in any client certificate, but I think that is probably a reasonable requirement given how I expect client certificates are used in practice.

[cpu]

@github-actions rustls-ffi / ensure-header-updated (pull_request) Failing after 28s

Oops, forgot to regenerate the .h after going back to fix-up a comment review point I missed.

I'll update + push.

[jsha]

Looking great! Almost there.

[cpu]

rustls-ffi / ensure-header-updated (pull_request) Failing

D'oh. cbindgen version mismatch between local (0.24.5) and CI (0.24.3). I'll switch to matching CI and regenerate.

[cpu]

D'oh. cbindgen version mismatch between local (0.24.5) and CI (0.24.3). I'll switch to matching CI and regenerate.

@github-actions rustls-ffi / ensure-header-updated (pull_request) Failing

I regenerated with 0.24.3 and on the next run CI was using cbindgen 0.24.5! I think separate from this PR we might need to explicitly pin cbindgen to one version. I'm not sure why it's changing between runs but it seems to require some luck to match up.

[jsha]

🎸

[cpu]

I can open a PR updating the Cargo.toml version to 0.11.0 and updating the CHANGELOG.md tomorrow morning. I want to make sure I take some care capturing the changes for the changelog.

Edit: and thanks again for the reviews/help!

[cpu]

I can open a PR updating the Cargo.toml version to 0.11.0 and updating the CHANGELOG.md tomorrow morning.

#338