Add cargo features to control root certs of the rustls backend #1058
Conversation
|
What happens if both features get enabled? Cargo features are supposed to be additive... Does it just merge both certs in? |
|
@seanmonstar yeah that's the behaviour. Is it a problem? Root certificates are as additive as cargo features are. |
|
I don't know, I haven't thought about it really. I don't think it's a problem, just wanted to ask if that sounds right. |
est31
force-pushed
the
rustls-roots
branch
2 times, most recently
from
bdec612
to
8b6b4a2
Compare
|
@seanmonstar in general, more certs in root store == ability to connect to more websites. That being said, it might be important for some users to have precise control over the root store. For them it might be helpful to have a pub enum RootStore {
#[cfg(...)]
WebpkiRoots,
#[cfg(...)]
NativeRoots,
// Always available
ManualRoots,
// Always available
Default,
}where |
est31
changed the title
est31
changed the title
|
Should be ready now for review. |
Running and testing should be working on the three platforms, but there's still a fair bit that needs to be done: - Wheel building + testing in a venv still needs to be implemented. - Python requirements still need to be compiled with piptool and pinned; need to compile on all platforms then merge - Cargo deps in cargo/ and rslib/ need to be cleaned up, and ideally unified into one place - Currently using rustls to work around openssl compilation issues on Linux, but this will break corporate proxies with custom SSL authorities; need to conditionally use openssl or use seanmonstar/reqwest#1058 - Makefiles and docs still need cleaning up - It may make sense to reparent ts/* to the top level, as we don't nest the other modules under a specific language. - rspy and pylib must always be updated in lock-step, so merging rspy into pylib as a private module would simplify things. - Merging desktop-ftl and mobile-ftl into the core ftl would make managing and updating translations easier. - Obsolete scripts need removing. - And probably more.
Now, callers have more control over the set of roots. Note that, due to cargo unification, other dependencies in the dependency tree might enable rustls-tls-webpki-roots or rustls-tls. This will affect connections initiated by code that explicitly enabled rustls-tls-manual-roots. So for now, the choice is done once per entire cargo dependency graph. If people want more precise control over things, they can add methods that allow controlling this on a per-connection level. Even if such methods are available, the *-manual-roots feature will still be helpful with eliminating the webpki-roots dependency for those cargo graphs where there is no unification.
Adds an optional cargo feature to load certificates from the OS native certificate store.
|
I've rebased the PR. @seanmonstar would you like to review? |
|
I'm prepping a release right now, should be ready in a few minutes. |
|
What happens if loading the native certificate store fails?, will reqwest know to ignore it or will it fail the client builder? |
This PR adds three new public cargo features:
rustls-tls-webpki-roots: equivalent torustls-tlsand using thewebpki-rootscrate to obtain the root certificates.rustls-tls-native-roots: using therustls-native-certscrate to obtain root certificates.rustls-tls-manual-roots: enabling only raw rustls support without setting any root certificates. Users have to set them manually, or unify with a crate that enables one of the builtin root certificate features.Furthermore a private cargo feature
__rustlsis added that is equivalent withrustls-tls-manual-rootsbut shorter to type :).Cargo's model allows a combination of features to be enabled, at which point a combination of the root stores is used.
I'm filing this PR because ability to load OS native roots is a blocker for rustup's adoption of rustls (see rust-lang/rustup#2517 (comment)). Currently, the public API of reqwest is incompatible with using
rustls-native-certsfrom the outside, and even if it were possible to use it, something likerustls-tls-manual-rootswould be needed to turn offwebpki-rootsloading.