Skip to content

Commit f6d67d5

Browse files
zaniebzanieb
authored
Improve certificate loading error messages (#18924)
See #18890 Adds special-case validation for `SSL_CERT_FILE` and `SSL_CERT_DIR` where we actually check if webpki will accept the given certificates and, if not, emit a better error message about why. This means we perform eager validation of certificates, parsing them more than once since reqwest will parse them again on client build. Unfortunately, there's not a straight-forward way to provide our pre-parsed certificates to reqwest without doing a lot more work. Nor is there a clear way to retrieve the parsed certificates on error. We use https://github.com/rusticata/x509-parser for parsing which seems reputable. We may want to _drop_ all invalid certificates instead, but that can be a future decision and this machinery can be reused for warnings. Ideally webpki would just have better error messages, but that's a separate project.
1 parent 39b83c3 commit f6d67d5

12 files changed

Lines changed: 337 additions & 45 deletions

File tree

‎Cargo.lock‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,8 @@ rcgen = { version = "0.14.5", features = [
334334
], default-features = false }
335335
rustls = { version = "0.23.36", default-features = false }
336336
similar = { version = "2.6.0" }
337+
webpki = { package = "rustls-webpki", version = "0.103.10" }
338+
x509-parser = { version = "0.18.0" }
337339
temp-env = { version = "0.3.6", features = ["async_closure"] }
338340
test-case = { version = "3.3.1" }
339341
test-log = { version = "0.2.16", features = [

‎crates/uv-client/Cargo.toml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ rustc-hash = { workspace = true }
6363
rustls-native-certs = { workspace = true }
6464
rustls-pki-types = { workspace = true }
6565
serde = { workspace = true }
66+
webpki = { workspace = true }
67+
x509-parser = { workspace = true }
6668
serde_json = { workspace = true }
6769
uv-platform = { workspace = true }
6870
thiserror = { workspace = true }

‎crates/uv-client/src/base_client.rs‎

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ use uv_warnings::warn_user_once;
4747

4848
use crate::linehaul::LineHaul;
4949
use crate::middleware::OfflineMiddleware;
50-
use crate::tls::{Certificates, read_identity};
50+
use crate::tls::{Certificates, TlsConfigurationError, read_identity};
5151
use crate::{Connectivity, WrappedReqwestError};
5252

5353
pub const DEFAULT_RETRIES: u32 = 3;
@@ -71,6 +71,44 @@ pub const DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
7171
/// timeout on the entire upload.
7272
pub const DEFAULT_READ_TIMEOUT_UPLOAD: Duration = Duration::from_mins(15);
7373

74+
#[derive(Debug)]
75+
pub struct ClientBuildError(ClientBuildErrorKind);
76+
77+
#[derive(Debug, Error)]
78+
enum ClientBuildErrorKind {
79+
#[error(transparent)]
80+
Reqwest(reqwest::Error),
81+
#[error(transparent)]
82+
TlsConfiguration(TlsConfigurationError),
83+
}
84+
85+
impl std::fmt::Display for ClientBuildError {
86+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
87+
f.write_str("failed to build HTTP client")
88+
}
89+
}
90+
91+
impl std::error::Error for ClientBuildError {
92+
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
93+
match &self.0 {
94+
ClientBuildErrorKind::Reqwest(error) => Some(error),
95+
ClientBuildErrorKind::TlsConfiguration(error) => Some(error),
96+
}
97+
}
98+
}
99+
100+
impl From<reqwest::Error> for ClientBuildError {
101+
fn from(error: reqwest::Error) -> Self {
102+
Self(ClientBuildErrorKind::Reqwest(error))
103+
}
104+
}
105+
106+
impl From<TlsConfigurationError> for ClientBuildError {
107+
fn from(error: TlsConfigurationError) -> Self {
108+
Self(ClientBuildErrorKind::TlsConfiguration(error))
109+
}
110+
}
111+
74112
/// Selectively skip parts or the entire auth middleware.
75113
#[derive(Debug, Clone, Copy, Default)]
76114
pub enum AuthIntegration {
@@ -380,7 +418,7 @@ impl<'a> BaseClientBuilder<'a> {
380418
retry_policy(self.retries, self.no_retry_delay)
381419
}
382420

383-
pub fn build(&self) -> reqwest::Result<BaseClient> {
421+
pub fn build(&self) -> Result<BaseClient, ClientBuildError> {
384422
if let Some(name) = self.client_name {
385423
debug!(
386424
"Using request connect timeout of {}s and read timeout of {}s for {} client",
@@ -464,7 +502,7 @@ impl<'a> BaseClientBuilder<'a> {
464502
&self,
465503
read_timeout: Duration,
466504
connect_timeout: Duration,
467-
) -> reqwest::Result<(Client, Client)> {
505+
) -> Result<(Client, Client), ClientBuildError> {
468506
// Create user agent.
469507
let mut user_agent_string = format!("uv/{}", version());
470508

@@ -475,7 +513,7 @@ impl<'a> BaseClientBuilder<'a> {
475513
}
476514

477515
// Load custom CA certificates from `SSL_CERT_FILE` and `SSL_CERT_DIR`.
478-
let custom_certs = Certificates::from_env().map(|certs| certs.to_reqwest_certs());
516+
let custom_certs = Certificates::from_env()?.map(|certs| certs.to_reqwest_certs());
479517

480518
// Create a secure client that validates certificates.
481519
let raw_client = self.create_client(
@@ -508,7 +546,7 @@ impl<'a> BaseClientBuilder<'a> {
508546
custom_certs: Option<Vec<Certificate>>,
509547
security: Security,
510548
redirect_policy: RedirectPolicy,
511-
) -> reqwest::Result<Client> {
549+
) -> Result<Client, ClientBuildError> {
512550
// Configure the builder.
513551
let client_builder = ClientBuilder::new()
514552
.http1_title_case_headers()
@@ -574,7 +612,7 @@ impl<'a> BaseClientBuilder<'a> {
574612
client_builder = client_builder.proxy(proxy);
575613
}
576614

577-
client_builder.build()
615+
client_builder.build().map_err(Into::into)
578616
}
579617

580618
fn apply_middleware(&self, client: Client) -> ClientWithMiddleware {

‎crates/uv-client/src/lib.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
pub use base_client::{
2-
AuthIntegration, BaseClient, BaseClientBuilder, DEFAULT_CONNECT_TIMEOUT, DEFAULT_MAX_REDIRECTS,
3-
DEFAULT_READ_TIMEOUT, DEFAULT_READ_TIMEOUT_UPLOAD, DEFAULT_RETRIES, ExtraMiddleware,
4-
RedirectClientWithMiddleware, RedirectPolicy, RequestBuilder, RetriableError,
2+
AuthIntegration, BaseClient, BaseClientBuilder, ClientBuildError, DEFAULT_CONNECT_TIMEOUT,
3+
DEFAULT_MAX_REDIRECTS, DEFAULT_READ_TIMEOUT, DEFAULT_READ_TIMEOUT_UPLOAD, DEFAULT_RETRIES,
4+
ExtraMiddleware, RedirectClientWithMiddleware, RedirectPolicy, RequestBuilder, RetriableError,
55
RetryParsingError, RetryState, UvRetryableStrategy, fetch_with_url_fallback,
66
retryable_on_request_failure,
77
};

‎crates/uv-client/src/registry_client.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ use uv_redacted::DisplaySafeUrl;
3737
use uv_small_str::SmallString;
3838
use uv_torch::TorchStrategy;
3939

40-
use crate::base_client::{BaseClientBuilder, ExtraMiddleware, RedirectPolicy};
40+
use crate::base_client::{BaseClientBuilder, ClientBuildError, ExtraMiddleware, RedirectPolicy};
4141
use crate::cached_client::CacheControl;
4242
use crate::flat_index::FlatIndexEntry;
4343
use crate::html::SimpleDetailHTML;
@@ -163,7 +163,7 @@ impl<'a> RegistryClientBuilder<'a> {
163163
}
164164
}
165165

166-
pub fn build(mut self) -> reqwest::Result<RegistryClient> {
166+
pub fn build(mut self) -> Result<RegistryClient, ClientBuildError> {
167167
self.cache_index_credentials();
168168
let index_urls = self.index_locations.index_urls();
169169

0 commit comments

Comments
 (0)