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

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

Open
complexspaces opened this issue Jan 5, 2024 · 19 comments · Fixed by #133 or #135
Open
Labels
enhancement New feature or request O-Android Work related to the Android verifier implementation O-Apple Work related to the Apple (macOS, iOS) verifier implementation O-Windows Work related to the Windows verifier implementation

Comments

@complexspaces
Copy link
Collaborator

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.

@complexspaces complexspaces added O-Android Work related to the Android verifier implementation O-Apple Work related to the Apple (macOS, iOS) verifier implementation O-Windows Work related to the Windows verifier implementation enhancement New feature or request labels Jan 5, 2024
@blind-oracle
Copy link

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
Copy link
Member

djc commented Jun 17, 2024

Note that this is blocking (built-in) support for rustls-platform-verifier in reqwest: seanmonstar/reqwest#2286 (comment).

@complexspaces
Copy link
Collaborator Author

complexspaces commented Aug 20, 2024

@stormshield-gt Based on rustls/rustls-native-certs#3 (comment), 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 rustls/webpki-roots#75, that would make progress on the iOS side (as macOS and iOS share a verifier impl).

@stormshield-gt
Copy link
Contributor

Thanks for the hint, I will start here so.

@stormshield-gt
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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

@ctz ctz reopened this Sep 2, 2024
@stormshield-gt
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

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 cpu closed this as completed in #135 Sep 18, 2024
@cpu cpu reopened this Sep 18, 2024
@cpu
Copy link
Member

cpu commented Sep 18, 2024

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.

@ofek
Copy link

ofek commented Oct 13, 2024

Has this non-Android functionality been released yet?

@cpu
Copy link
Member

cpu commented Oct 13, 2024

No: v/0.3.4...main

@complexspaces
Copy link
Collaborator Author

I can put up a PR to start the release process. I think the new APIs have sat for long enough in main without anyone finding an obvious limitation right away.

@ofek
Copy link

ofek commented Oct 13, 2024

That would be awesome, I'm very excited for reqwest to start using this!

@stormshield-gt
Copy link
Contributor

Maybe before publishing we can take a look one more time at this #133 (comment) ?
As we now also iterate in the macOS/IOS case, maybe impl IntoIterator<Item = pki_types::CertificateDer<'static>> would be slightly more flexible ?

@djc
Copy link
Member

djc commented Oct 14, 2024

Maybe before publishing we can take a look one more time at this #133 (comment) ? As we now also iterate in the macOS/IOS case, maybe impl IntoIterator<Item = pki_types::CertificateDer<'static>> would be slightly more flexible ?

Sounds good to me, want to submit a PR?

@stormshield-gt
Copy link
Contributor

That's done with #145

tsibley added a commit to nextstrain/nextclade that referenced this issue Oct 15, 2024
Allows adding additional CA certificates to the trust store specifically
for Nextclade, for when modifying the system's trust store isn't
desirable/possible.  Works across all platforms.

Currently requires using landed unreleased changes to
rustls-platform-verifier so that macOS and Windows have the needed
Verifier::new_with_extra_certs() API.¹  That API also has signature
changes proposed² which, while unmerged, seem likely to land, so use
those in anticipation.  They actually bring the signature back closer to
the latest release (0.3.4).

¹ <rustls/rustls-platform-verifier#58>
² <rustls/rustls-platform-verifier#145>

Related-to: <#1529>
Related-to: <#726>
tsibley added a commit to nextstrain/nextclade that referenced this issue Oct 15, 2024
Allows adding additional CA certificates to the trust store specifically
for Nextclade, for when modifying the system's trust store isn't
desirable/possible.  Works across all platforms.

Currently requires using landed but unreleased changes to
rustls-platform-verifier so that macOS and Windows have the needed
Verifier::new_with_extra_certs() API.¹  That API also has signature
changes proposed² which, while unmerged, seem likely to land, so use
those in anticipation.  They actually bring the signature back closer to
the latest release (0.3.4).

¹ <rustls/rustls-platform-verifier#58>
² <rustls/rustls-platform-verifier#145>

Related-to: <#1529>
Related-to: <#726>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request O-Android Work related to the Android verifier implementation O-Apple Work related to the Apple (macOS, iOS) verifier implementation O-Windows Work related to the Windows verifier implementation
Projects
None yet
7 participants