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.

Sign up for GitHub

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

Add support for rustls-platform-verifier #2286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for rustls-platform-verifier #2286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ jobs:
features: "--no-default-features"
- name: "feat.: rustls-tls"
features: "--no-default-features --features rustls-tls"
- name: "feat.: rustls-tls-platform-verifier"
features: "--no-default-features --features rustls-tls-platform-verifier"
- name: "feat.: rustls-tls-manual-roots"
features: "--no-default-features --features rustls-tls-manual-roots"
- name: "feat.: rustls-tls-native-roots"
Expand Down
2 changes: 2 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ native-tls-vendored = ["native-tls", "native-tls-crate?/vendored"]

rustls-tls = ["rustls-tls-webpki-roots"]
rustls-tls-manual-roots = ["__rustls"]
rustls-tls-platform-verifier = ["dep:rustls-platform-verifier", "__rustls"]
rustls-tls-webpki-roots = ["dep:webpki-roots", "__rustls"]
rustls-tls-native-roots = ["dep:rustls-native-certs", "__rustls"]

Expand Down Expand Up @@ -140,6 +141,7 @@ rustls-pki-types = { version = "1.1.0", features = ["alloc"] ,optional = true }
tokio-rustls = { version = "0.25", optional = true }
webpki-roots = { version = "0.26.0", optional = true }
rustls-native-certs = { version = "0.7", optional = true }
rustls-platform-verifier = { version = "0.2", optional = true }

## cookies
cookie_crate = { version = "0.18.0", package = "cookie", optional = true }
Expand Down
18 changes: 15 additions & 3 deletions src/async_impl/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,10 +565,21 @@ impl ClientBuilder {
return Err(crate::error::builder("empty supported tls versions"));
}

#[cfg(feature = "rustls-tls-platform-verifier")]
let verifier = Arc::new(rustls_platform_verifier::Verifier::new());
#[cfg(not(feature = "rustls-tls-platform-verifier"))]
let verifier =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WebPkiServerVerifierBuilder::build() step yields Err if no roots have been passed in, meaning that a whole bunch of tests now panic when testing under rustls-tls-manual-roots (without specifying any roots). I've just disabled these tests when testing with this particular feature, which seems like the easiest approach?

Alternatively, we could have slightly more complex setup here which would avoid the error at builder time, but failing earlier actually seems desirable to me.

rustls::client::WebPkiServerVerifier::builder(Arc::new(root_cert_store))
.build()
.map_err(|_| {
crate::error::builder("no trust anchors have been provided")
})?;

// Build TLS config
let config_builder =
rustls::ClientConfig::builder_with_protocol_versions(&versions)
.with_root_certificates(root_cert_store);
.dangerous()
.with_custom_certificate_verifier(verifier);

// Finalize TLS config
let mut tls = if let Some(id) = config.identity {
Expand Down Expand Up @@ -1304,7 +1315,7 @@ impl ClientBuilder {
///
/// # Example
///
/// ```
/// ```no_run
/// use std::net::IpAddr;
/// let local_addr = IpAddr::from([12, 4, 1, 8]);
/// let client = reqwest::Client::builder()
Expand All @@ -1323,7 +1334,7 @@ impl ClientBuilder {
///
/// # Example
///
/// ```
/// ```no_run
/// let interface = "lo";
/// let client = reqwest::Client::builder()
/// .interface(interface)
Expand Down Expand Up @@ -2745,6 +2756,7 @@ fn add_cookie_header(headers: &mut HeaderMap, cookie_store: &dyn cookie::CookieS
}
}

#[cfg(not(feature = "rustls-tls-manual-roots"))] // Building a client fails without roots
#[cfg(test)]
mod tests {
#[tokio::test]
Expand Down
1 change: 1 addition & 0 deletions src/async_impl/request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,7 @@ impl TryFrom<Request> for HttpRequest<Body> {
}
}

#[cfg(not(feature = "rustls-tls-manual-roots"))] // Building a client fails without roots
#[cfg(test)]
mod tests {
use super::{Client, HttpRequest, Request, RequestBuilder, Version};
Expand Down
2 changes: 2 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@
//! while using root certificates from the `webpki-roots` crate.
//! - **rustls-tls-native-roots**: Enables TLS functionality provided by `rustls`,
//! while using root certificates from the `rustls-native-certs` crate.
//! - **rustls-tls-platform-verifier**: Enables TLS functionality provided by `rustls`,
//! while using the platform's native certificate verifier.
//! - **blocking**: Provides the [blocking][] client API.
//! - **charset** *(enabled by default)*: Improved support for decoding text.
//! - **cookies**: Provides cookie session support.
Expand Down
16 changes: 12 additions & 4 deletions tests/badssl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ async fn test_rustls_badssl_modern() {
assert!(text.contains("<title>mozilla-modern.badssl.com</title>"));
}

#[cfg(feature = "__tls")]
#[cfg(all(feature = "__tls", not(feature = "rustls-tls-manual-roots")))]
#[tokio::test]
async fn test_badssl_self_signed() {
let text = reqwest::Client::builder()
Expand All @@ -59,14 +59,22 @@ async fn test_badssl_self_signed() {
assert!(text.contains("<title>self-signed.badssl.com</title>"));
}

#[cfg(feature = "__tls")]
// For the platform verifier, there is no concept of available roots
#[cfg(all(feature = "__tls", not(feature = "rustls-tls-platform-verifier")))]
#[tokio::test]
async fn test_badssl_no_built_in_roots() {
let result = reqwest::Client::builder()
.tls_built_in_root_certs(false)
.no_proxy()
.build()
.unwrap()
.build();

// Some configurations will fail to build a client without roots
let client = match result {
Ok(client) => client,
Err(_) => return,
};

let result = client
.get("https://mozilla-modern.badssl.com/")
.send()
.await;
Expand Down
2 changes: 1 addition & 1 deletion tests/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![cfg(not(target_arch = "wasm32"))]
#![cfg(not(any(target_arch = "wasm32", feature = "rustls-tls-manual-roots")))]
mod support;

use support::server;
Expand Down
2 changes: 1 addition & 1 deletion tests/proxy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![cfg(not(target_arch = "wasm32"))]
#![cfg(not(any(target_arch = "wasm32", feature = "rustls-tls-manual-roots")))]
mod support;
use support::server;

Expand Down
2 changes: 1 addition & 1 deletion tests/redirect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![cfg(not(target_arch = "wasm32"))]
#![cfg(not(any(target_arch = "wasm32", feature = "rustls-tls-manual-roots")))]
mod support;
use http_body_util::BodyExt;
use reqwest::Body;
Expand Down
2 changes: 1 addition & 1 deletion tests/timeouts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![cfg(not(target_arch = "wasm32"))]
#![cfg(not(any(target_arch = "wasm32", feature = "rustls-tls-manual-roots")))]
Copy link
Owner

@seanmonstar seanmonstar May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I guess I haven't fully internalized the difference. Looking at the rest of this test file, it seems to just make a "normal" client. Why would that fail?

Is it that if only manual roots are enabled, but none are added, that's an error?

And, what if other rustls features are enabled too? It should work, then?

Copy link
Contributor Author

@djc djc May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I guess I haven't fully internalized the difference. Looking at the rest of this test file, it seems to just make a "normal" client. Why would that fail?

Is it that if only manual roots are enabled, but none are added, that's an error?

Yes, that's the problem.

And, what if other rustls features are enabled too? It should work, then?

Because we can't use the rustls-platform-verifier in conjunction with otherwise-supplied roots on many platforms (only on non-Apple Unix, see rustls/rustls-platform-verifier#58), this PR chooses to skip gathering roots on all platforms in favor of exclusively using the platform verifier (which comes down to using rustls-native certs on non-Apple Unix).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to disrupt the additivity of the features. If anywhere in the dependency tree enables this feature, everywhere else that was depending on detecting native roots will now start erroring. That doesn't sound like a great experience for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as discussed previously in #2159. Do you want to block this addition on rustls/rustls-platform-verifier#58, which would enable making it additive?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking from the point of view of users, I believe that is something they would expect to work. So yea, that seems like a requirement.

mod support;
use support::server;

Expand Down
2 changes: 1 addition & 1 deletion tests/upgrade.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![cfg(not(target_arch = "wasm32"))]
#![cfg(not(any(target_arch = "wasm32", feature = "rustls-tls-manual-roots")))]
mod support;
use support::server;
use tokio::io::{AsyncReadExt, AsyncWriteExt};
Expand Down