search

rustls / rustls-platform-verifier

A certificate verification library for rustls that uses the operating system's verifier
Apache License 2.0
63 stars 20 forks source link

Consider adding Verifier::new_with_extra_roots implementation to other platforms #58

Open complexspaces opened 8 months ago

complexspaces commented 8 months ago

The functionality of new_with_extra_roots is primarily useful for Linux/WASM/BSD platforms that don't have a consistent source of trusted CA root/anchors available. However, many private/internal applications often use their own private CAs instead of publicly issued ones. This seems like a use case we could support without much burden, even if those users might be better off making their own webpki-based verifier instead.

Implementation details

It's worth noting that the Apple and Windows code for this already exists in a near-drop in form. Android would require more work to make a TrustManager that combined certificates.

macOS/iOS

We can call SecTrustSetAnchorCertificates to add additional roots to the evaluation, and then call SecTrustSetAnchorCertificatesOnly with false to trust both the custom roots and the default OS-provided ones.

Windows

We can create a custom CERT_CHAIN_ENGINE_CONFIG and set cAdditionalStore to a custom, in-memory certificate store. This engine and store are then included in our call to CertGetCertificateChain.

Android

I believe this can be done by using a PKIXParameters. I think the best idea is to create a custom TrustManager class that considers the system Keystore's trustmanager and then a custom Keystore containing the user-provided roots. I'm not yet sure about the exact implementation strategy though since the Android X509 and PKIX APIs are a handful. This and this blog may be helpful references.

blind-oracle commented 3 months ago

I second that. This also makes development easier since when using MacOS I've spent some time figuring out why the new_with_extra_roots method is missing :)

djc commented 3 months ago

Note that this is blocking (built-in) support for rustls-platform-verifier in reqwest: https://github.com/seanmonstar/reqwest/pull/2286#discussion_r1609846678.

complexspaces commented 1 month ago

@stormshield-gt Based on https://github.com/rustls/rustls-native-certs/issues/3#issuecomment-2298675787, I believe this is the issue would be good to help with to forward reqwest integration.

If you'd like to take my existing work in https://github.com/rustls/rustls-platform-verifier/tree/shared/extra-roots-additions and update it to use the new types provided in https://github.com/rustls/webpki-roots/pull/75, that would make progress on the iOS side (as macOS and iOS share a verifier impl).

stormshield-gt commented 1 month ago

Thanks for the hint, I will start here so.

stormshield-gt commented 1 month ago

I'm adding tests for the Verifier::new_extra_roots and I wonder if I should just replace the existing tests that override one CA. Does it make sense to keep them both ?

complexspaces commented 4 weeks ago

This is something I was also thinking about when working on the branch and I was undecided 😅. If its cleaner with tests to merge together the mock CA and extra roots, go ahead and do so.

The one important part we need to keep around though is the ability to tell the system verifier to only use those roots during testing, and not consider the wider system state. This isn't wanted in production but during testing its very valuable to keep things reproducible given the background-managed nature of trust roots on platforms.

stormshield-gt commented 2 weeks ago

I think we should reopen this while we don't support android and windows

stormshield-gt commented 2 weeks ago

I wonder if we should consider adding an option to configure if the extra roots must be exclusive or not. I mean if we should consider only the extra root or the extra root and the OS root store.

Reqwest has this flag in its client builder and if we want to integrate with it, I think we want to expose this functionality.

For instance, on MacOS, when reqwest use the native-tls feature, this flag is just propagated to secure transport crate which is then just used as an argument to the call of the SecTrustSetAnchorCertificatesOnly function.

We could easily replicate this in our implementation. Besides it will remove the need for having special field inside the Verifier only for testing.

complexspaces commented 2 weeks ago

It seems like an edge case, honestly, but maybe we do need to support it for reqwest to be happy.

IMO that flag doesn't make much sense to have in a platform verifier: why would you use this crate and only custom roots? You could just use rustls on its own with the webpki-based verifier if the roots are known to be good ahead-of-time. Usually the platform verifiers are better at handling "messy" certificate chains because of how much compat work the OSes put in since their code has to "just work."

stormshield-gt commented 2 weeks ago

I see your point in the case you want to use the extra root also on production, I also think you better off using the webpki verifier.

But in a case you use extra roots only for testing, I think it can be valuable to have the same verifier for testing and production, as the behavior might be slightly different. As for this crate, when doing the test you might want only the extra root for reproducibility.

In any case, as you said, we might just need this for reqwest support.

complexspaces commented 2 weeks ago

The callout about end user testing is something I hadn't considered, thanks for mentioning that. I can see how people would want to maybe do similar to us and isolate unit test to an exclusive root store and not let the OS do any certificate fetching on-the-fly etc for reproducibility.

cpu commented 3 days ago

cpu closed this as completed in #135 now

GitHub misunderstood "partially fix 135" as "fix 135" - I believe all that remains now is support for this in the Android verifier.