Closed cpu closed 1 year ago
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:
main
.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 :sweatsmile:), 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.
@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.
Yep! Planning to check it out a little now and a little tomorrow.
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 https://github.com/rustls/rustls/pull/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 :+1:
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.
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.
@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.
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.
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.
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!
I can open a PR updating the Cargo.toml version to 0.11.0 and updating the CHANGELOG.md tomorrow morning.
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.
cipher: rename verifier types to match upstream.
This commit renames the
rustls_client_cert_verifier
andrustls_client_cert_verifier_optional
types to better match their upstream Rustls equivalents:rustls_allow_any_authenticated_client_verifier
andrustls_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
andrustls_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 standardconst *
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 viaAUTH_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 thetestdata/localhost/cert.pem
certificate as revoked, but not thetestdata/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 theexample.com
client cert that isn't expected to error, and one using thelocalhost
client cert that is expected to error (since it's revoked).