Skip to content

Commit

Permalink
Prefer partial success to failure
Browse files Browse the repository at this point in the history
If we read some certs from a file, but fail to read some from
a directory (or vv.) then return what we have.

This is good because it restores the crate's previous behaviour.
It also matches what (for example) golang crypto.x509 does.

This is bad because it hides a failure, which would be confusing
if a user relied on reading from _both_ a file and directory at
the same time.  Would someone do that?  The follow commit steps
towards ameliorating this, slightly.
  • Loading branch information
ctz committed Aug 28, 2024
1 parent 9832a72 commit 7ada714
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 7 deletions.
29 changes: 22 additions & 7 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,18 +157,33 @@ impl CertPaths {
return Ok(None);
}

let mut first_error = None;

let mut certs = match &self.file {
Some(cert_file) => {
load_pem_certs(cert_file).map_err(|err| Self::load_err(cert_file, "file", err))?
}
Some(cert_file) => match load_pem_certs(cert_file)
.map_err(|err| Self::load_err(cert_file, "file", err))
{
Ok(certs) => certs,
Err(err) => {
first_error = first_error.or(Some(err));
Vec::new()
}
},
None => Vec::new(),
};

if let Some(cert_dir) = &self.dir {
certs.append(
&mut load_pem_certs_from_dir(cert_dir)
.map_err(|err| Self::load_err(cert_dir, "dir", err))?,
);
match load_pem_certs_from_dir(cert_dir)
.map_err(|err| Self::load_err(cert_dir, "dir", err))
{
Ok(mut from_dir) => certs.append(&mut from_dir),
Err(err) => first_error = first_error.or(Some(err)),
}
}

// promote first error if we have no certs to return
if let (Some(error), []) = (first_error, certs.as_slice()) {
return Err(error);
}

certs.sort_unstable_by(|a, b| a.cmp(b));
Expand Down
50 changes: 50 additions & 0 deletions tests/smoketests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,53 @@ fn badssl_with_dir_from_env() {

check_site("self-signed.badssl.com").unwrap();
}

#[test]
#[serial]
#[ignore]
#[cfg(target_os = "linux")]
fn google_with_dir_but_broken_file() {
unsafe {
// SAFETY: safe because of #[serial]
common::clear_env();
}

env::set_var("SSL_CERT_DIR", "/etc/ssl/certs");
env::set_var("SSL_CERT_FILE", "not-exist");
check_site("google.com").unwrap();
}

#[test]
#[serial]
#[ignore]
#[cfg(target_os = "linux")]
fn google_with_file_but_broken_dir() {
unsafe {
// SAFETY: safe because of #[serial]
common::clear_env();
}

env::set_var("SSL_CERT_DIR", "/not-exist");
env::set_var("SSL_CERT_FILE", "/etc/ssl/certs/ca-certificates.crt");
check_site("google.com").unwrap();
}

#[test]
#[serial]
#[ignore]
#[cfg(target_os = "linux")]
fn nothing_works_with_broken_file_and_dir() {
unsafe {
// SAFETY: safe because of #[serial]
common::clear_env();
}

env::set_var("SSL_CERT_DIR", "/not-exist");
env::set_var("SSL_CERT_FILE", "not-exist");
assert_eq!(
rustls_native_certs::load_native_certs()
.unwrap_err()
.to_string(),
"could not load certs from file not-exist: No such file or directory (os error 2)"
);
}

0 comments on commit 7ada714

Please sign in to comment.