From 96cfb71e68452b53bbf389c7b3cf5e42d31fe4dc Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Mon, 26 Aug 2024 17:02:48 +0200 Subject: [PATCH] Return all errors to caller --- examples/google.rs | 2 +- examples/print-trust-anchors.rs | 2 +- src/lib.rs | 265 +++++++++++++++++++++----------- src/macos.rs | 36 +++-- src/unix.rs | 9 +- src/windows.rs | 21 ++- tests/compare_mozilla.rs | 17 +- tests/smoketests.rs | 2 +- 8 files changed, 227 insertions(+), 127 deletions(-) diff --git a/examples/google.rs b/examples/google.rs index 5b7baac..06a0dc4 100644 --- a/examples/google.rs +++ b/examples/google.rs @@ -4,7 +4,7 @@ use std::sync::Arc; fn main() { let mut roots = rustls::RootCertStore::empty(); - for cert in rustls_native_certs::load_native_certs().expect("could not load platform certs") { + for cert in rustls_native_certs::load_native_certs().certs { roots.add(cert).unwrap(); } diff --git a/examples/print-trust-anchors.rs b/examples/print-trust-anchors.rs index 5609ee1..010000a 100644 --- a/examples/print-trust-anchors.rs +++ b/examples/print-trust-anchors.rs @@ -4,7 +4,7 @@ use std::error::Error; use x509_parser::prelude::*; fn main() -> Result<(), Box> { - for cert in rustls_native_certs::load_native_certs()? { + for cert in rustls_native_certs::load_native_certs().certs { match parse_x509_certificate(cert.as_ref()) { Ok((_, cert)) => println!("{}", cert.tbs_certificate.subject), Err(e) => eprintln!("error parsing certificate: {}", e), diff --git a/src/lib.rs b/src/lib.rs index 47e961b..18ff197 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -21,12 +21,12 @@ // Enable documentation for all features on docs.rs #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] -use std::env; +use std::error::Error as StdError; use std::ffi::OsStr; use std::fs::{self, File}; -use std::io::BufReader; -use std::io::{Error, ErrorKind}; +use std::io::{self, BufReader}; use std::path::{Path, PathBuf}; +use std::{env, fmt}; use pki_types::CertificateDer; @@ -117,10 +117,53 @@ use macos as platform; /// this sparingly. /// /// [c_rehash]: https://www.openssl.org/docs/manmaster/man1/c_rehash.html -pub fn load_native_certs() -> Result>, Error> { - match CertPaths::from_env().load()? { - Some(certs) => Ok(certs), - None => platform::load_native_certs(), +pub fn load_native_certs() -> CertificateResult { + match CertPaths::from_env().load() { + out if !out.certs.is_empty() => out, + _ => platform::load_native_certs(), + } +} + +#[derive(Debug, Default)] +pub struct CertificateResult { + pub certs: Vec>, + pub errors: Vec, +} + +impl CertificateResult { + pub fn expect(self, msg: &str) -> Vec> { + match self.errors.is_empty() { + true => self.certs, + false => panic!("{msg}: {:?}", self.errors), + } + } + + pub fn unwrap(self) -> Vec> { + match self.errors.is_empty() { + true => self.certs, + false => panic!( + "errors occurred while loading certificates: {:?}", + self.errors + ), + } + } + + fn io_error(&mut self, err: io::Error, path: &Path, context: &'static str) { + self.errors.push(Error { + context, + kind: ErrorKind::Io { + inner: err, + path: path.to_owned(), + }, + }); + } + + #[cfg(any(windows, target_os = "macos"))] + fn os_error(&mut self, err: Box, context: &'static str) { + self.errors.push(Error { + context, + kind: ErrorKind::Os(err), + }); } } @@ -152,40 +195,24 @@ impl CertPaths { /// [hash files](`is_hash_file_name()`) contained in it must be loaded successfully, /// subject to the rules outlined above for `self.file`. The directory is not /// scanned recursively and may be empty. - fn load(&self) -> Result>>, Error> { + fn load(&self) -> CertificateResult { + let mut out = CertificateResult::default(); if self.file.is_none() && self.dir.is_none() { - return Ok(None); + return out; } - let mut certs = match &self.file { - Some(cert_file) => { - load_pem_certs(cert_file).map_err(|err| Self::load_err(cert_file, err))? - } - None => Vec::new(), - }; + if let Some(cert_file) = &self.file { + load_pem_certs(cert_file, &mut out); + } 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, err))?, - ); + load_pem_certs_from_dir(cert_dir, &mut out); } - certs.sort_unstable_by(|a, b| a.cmp(b)); - certs.dedup(); - - Ok(Some(certs)) - } - - fn load_err(path: &Path, err: Error) -> Error { - Error::new( - err.kind(), - format!( - "could not load certs from {} {}: {err}", - if path.is_file() { "file" } else { "dir" }, - path.display() - ), - ) + out.certs + .sort_unstable_by(|a, b| a.cmp(b)); + out.certs.dedup(); + out } } @@ -196,11 +223,24 @@ impl CertPaths { /// isn't a valid certificate, we limit ourselves to loading those files /// that have a hash-based file name matching the pattern used by OpenSSL. /// The hash is not verified, however. -fn load_pem_certs_from_dir(dir: &Path) -> Result>, Error> { - let dir_reader = fs::read_dir(dir)?; - let mut certs = Vec::new(); +fn load_pem_certs_from_dir(dir: &Path, out: &mut CertificateResult) { + let dir_reader = match fs::read_dir(dir) { + Ok(reader) => reader, + Err(err) => { + out.io_error(err, dir, "opening directory"); + return; + } + }; + for entry in dir_reader { - let entry = entry?; + let entry = match entry { + Ok(entry) => entry, + Err(err) => { + out.io_error(err, dir, "reading directory entries"); + continue; + } + }; + let path = entry.path(); let file_name = path .file_name() @@ -213,21 +253,37 @@ fn load_pem_certs_from_dir(dir: &Path) -> Result>, E // make sure we resolve them. let metadata = match fs::metadata(&path) { Ok(metadata) => metadata, - Err(e) if e.kind() == ErrorKind::NotFound => { + Err(e) if e.kind() == io::ErrorKind::NotFound => { // Dangling symlink continue; } - Err(e) => return Err(e), + Err(e) => { + out.io_error(e, &path, "failed to open file"); + continue; + } }; + if metadata.is_file() && is_hash_file_name(file_name) { - certs.append(&mut load_pem_certs(&path)?); + load_pem_certs(&path, out); } } - Ok(certs) } -fn load_pem_certs(path: &Path) -> Result>, Error> { - rustls_pemfile::certs(&mut BufReader::new(File::open(path)?)).collect() +fn load_pem_certs(path: &Path, out: &mut CertificateResult) { + let reader = match File::open(path) { + Ok(file) => BufReader::new(file), + Err(err) => { + out.io_error(err, path, "failed to open file"); + return; + } + }; + + for result in rustls_pemfile::certs(&mut BufReader::new(reader)) { + match result { + Ok(cert) => out.certs.push(cert), + Err(err) => out.io_error(err, path, "failed to parse PEM"), + } + } } /// Check if this is a hash-based file name for a certificate @@ -259,6 +315,41 @@ fn is_hash_file_name(file_name: &OsStr) -> bool { && matches!(iter.next(), Some(c) if c.is_ascii_digit()) } +#[derive(Debug)] +pub struct Error { + pub context: &'static str, + pub kind: ErrorKind, +} + +impl StdError for Error { + fn source(&self) -> Option<&(dyn StdError + 'static)> { + Some(match &self.kind { + ErrorKind::Io { inner, .. } => inner, + ErrorKind::Os(err) => &**err, + }) + } +} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.context)?; + f.write_str(": ")?; + match &self.kind { + ErrorKind::Io { inner, path } => { + write!(f, "{inner} in {}", path.display()) + } + ErrorKind::Os(err) => err.fmt(f), + } + } +} + +#[non_exhaustive] +#[derive(Debug)] +pub enum ErrorKind { + Io { inner: io::Error, path: PathBuf }, + Os(Box), +} + const ENV_CERT_FILE: &str = "SSL_CERT_FILE"; const ENV_CERT_DIR: &str = "SSL_CERT_DIR"; @@ -328,64 +419,65 @@ mod tests { write!(file, "{}", &cert2).unwrap(); } - let certs_from_file = CertPaths { + let result = CertPaths { file: Some(file_path.clone()), dir: None, } - .load() - .unwrap(); - assert_eq!(certs_from_file.unwrap().len(), 2); + .load(); + assert_eq!(result.certs.len(), 2); - let certs_from_dir = CertPaths { + let result = CertPaths { file: None, dir: Some(dir_path.clone()), } - .load() - .unwrap(); - assert_eq!(certs_from_dir.unwrap().len(), 2); + .load(); + assert_eq!(result.certs.len(), 2); - let certs_from_both = CertPaths { + let result = CertPaths { file: Some(file_path), dir: Some(dir_path), } - .load() - .unwrap(); - assert_eq!(certs_from_both.unwrap().len(), 2); + .load(); + assert_eq!(result.certs.len(), 2); } #[test] fn malformed_file_from_env() { // Certificate parser tries to extract certs from file ignoring // invalid sections. - let certs = load_pem_certs(Path::new(file!())).unwrap(); - assert_eq!(certs.len(), 0); + let mut result = CertificateResult::default(); + load_pem_certs(Path::new(file!()), &mut result); + assert_eq!(result.certs.len(), 0); + assert!(result.errors.is_empty()); } #[test] fn from_env_missing_file() { - assert_eq!( - load_pem_certs(Path::new("no/such/file")) - .unwrap_err() - .kind(), - ErrorKind::NotFound - ); + let mut result = CertificateResult::default(); + load_pem_certs(Path::new("no/such/file"), &mut result); + match &first_error(&result).kind { + ErrorKind::Io { inner, .. } => assert_eq!(inner.kind(), io::ErrorKind::NotFound), + _ => panic!("unexpected error {:?}", result.errors), + } } #[test] fn from_env_missing_dir() { - assert_eq!( - load_pem_certs_from_dir(Path::new("no/such/directory")) - .unwrap_err() - .kind(), - ErrorKind::NotFound - ); + let mut result = CertificateResult::default(); + load_pem_certs_from_dir(Path::new("no/such/directory"), &mut result); + match &first_error(&result).kind { + ErrorKind::Io { inner, .. } => assert_eq!(inner.kind(), io::ErrorKind::NotFound), + _ => panic!("unexpected error {:?}", result.errors), + } } #[test] #[cfg(unix)] fn from_env_with_non_regular_and_empty_file() { - let certs = load_pem_certs(Path::new("/dev/null")).unwrap(); - assert_eq!(certs.len(), 0); + let mut result = CertificateResult::default(); + load_pem_certs(Path::new("/dev/null"), &mut result); + assert_eq!(result.certs.len(), 0); + assert!(result.errors.is_empty()); } #[test] @@ -420,24 +512,23 @@ mod tests { #[cfg(unix)] fn test_cert_paths_bad_perms(cert_paths: CertPaths) { - let err = cert_paths.load().unwrap_err(); + let result = cert_paths.load(); - let affected_path = match (cert_paths.file, cert_paths.dir) { - (Some(file), None) => file, - (None, Some(dir)) => dir, - _ => panic!("only one of file or dir should be set"), + if let (None, None) = (cert_paths.file, cert_paths.dir) { + panic!("only one of file or dir should be set"); }; - let r#type = match affected_path.is_file() { - true => "file", - false => "dir", + + let error = first_error(&result); + match &error.kind { + ErrorKind::Io { inner, .. } => { + assert_eq!(inner.kind(), io::ErrorKind::PermissionDenied); + inner + } + _ => panic!("unexpected error {:?}", result.errors), }; + } - assert_eq!(err.kind(), ErrorKind::PermissionDenied); - assert!(err - .to_string() - .contains(&format!("certs from {type}"))); - assert!(err - .to_string() - .contains(&affected_path.display().to_string())); + fn first_error(result: &CertificateResult) -> &Error { + result.errors.first().unwrap() } } diff --git a/src/macos.rs b/src/macos.rs index 71f6313..6b61bdb 100644 --- a/src/macos.rs +++ b/src/macos.rs @@ -1,10 +1,11 @@ use std::collections::HashMap; -use std::io::{Error, ErrorKind}; use pki_types::CertificateDer; use security_framework::trust_settings::{Domain, TrustSettings, TrustSettingsForCertificate}; -pub fn load_native_certs() -> Result>, Error> { +use super::CertificateResult; + +pub fn load_native_certs() -> CertificateResult { // The various domains are designed to interact like this: // // "Per-user Trust Settings override locally administered @@ -16,13 +17,17 @@ pub fn load_native_certs() -> Result>, Error> { // overwrite existing elements, which mean User settings // trump Admin trump System, as desired. + let mut result = CertificateResult::default(); let mut all_certs = HashMap::new(); - for domain in &[Domain::User, Domain::Admin, Domain::System] { let ts = TrustSettings::new(*domain); - let iter = ts - .iter() - .map_err(|err| Error::new(ErrorKind::Other, err))?; + let iter = match ts.iter() { + Ok(iter) => iter, + Err(err) => { + result.os_error(err.into(), "failed to iterate trust settings"); + continue; + } + }; for cert in iter { let der = cert.to_der(); @@ -33,25 +38,28 @@ pub fn load_native_certs() -> Result>, Error> { // // "Note that an empty Trust Settings array means "always trust this cert, // with a resulting kSecTrustSettingsResult of kSecTrustSettingsResultTrustRoot". - let trusted = ts - .tls_trust_settings_for_certificate(&cert) - .map_err(|err| Error::new(ErrorKind::Other, err))? - .unwrap_or(TrustSettingsForCertificate::TrustRoot); + let trusted = match ts.tls_trust_settings_for_certificate(&cert) { + Ok(trusted) => trusted.unwrap_or(TrustSettingsForCertificate::TrustRoot), + Err(err) => { + result.os_error(err.into(), "certificate not trusted"); + continue; + } + }; all_certs.entry(der).or_insert(trusted); } } - let mut certs = Vec::new(); - // Now we have all the certificates and an idea of whether // to use them. for (der, trusted) in all_certs.drain() { use TrustSettingsForCertificate::*; if let TrustRoot | TrustAsRoot = trusted { - certs.push(CertificateDer::from(der)); + result + .certs + .push(CertificateDer::from(der)); } } - Ok(certs) + result } diff --git a/src/unix.rs b/src/unix.rs index 7c39e3b..7bb869f 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -1,15 +1,10 @@ -use std::io::Error; +use crate::{CertPaths, CertificateResult}; -use pki_types::CertificateDer; - -use crate::CertPaths; - -pub fn load_native_certs() -> Result>, Error> { +pub fn load_native_certs() -> CertificateResult { let likely_locations = openssl_probe::probe(); CertPaths { file: likely_locations.cert_file, dir: likely_locations.cert_dir, } .load() - .map(|certs| certs.unwrap_or_default()) } diff --git a/src/windows.rs b/src/windows.rs index f20635f..666ccf1 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -1,21 +1,28 @@ -use std::io::Error; - use pki_types::CertificateDer; use schannel::cert_context::ValidUses; use schannel::cert_store::CertStore; -pub fn load_native_certs() -> Result>, Error> { - let mut certs = Vec::new(); +use super::CertificateResult; - let current_user_store = CertStore::open_current_user("ROOT")?; +pub fn load_native_certs() -> CertificateResult { + let mut result = CertificateResult::default(); + let current_user_store = match CertStore::open_current_user("ROOT") { + Ok(store) => store, + Err(err) => { + result.os_error(err.into(), "failed to open current user certificate store"); + return result; + } + }; for cert in current_user_store.certs() { if usable_for_rustls(cert.valid_uses().unwrap()) && cert.is_time_valid().unwrap() { - certs.push(CertificateDer::from(cert.to_der().to_vec())); + result + .certs + .push(CertificateDer::from(cert.to_der().to_vec())); } } - Ok(certs) + result } fn usable_for_rustls(uses: ValidUses) -> bool { diff --git a/tests/compare_mozilla.rs b/tests/compare_mozilla.rs index 64522d1..aa7666a 100644 --- a/tests/compare_mozilla.rs +++ b/tests/compare_mozilla.rs @@ -58,7 +58,7 @@ fn stringify_x500name(subject: &Der<'_>) -> String { #[test] fn test_does_not_have_many_roots_unknown_by_mozilla() { - let native = rustls_native_certs::load_native_certs().unwrap(); + let native = rustls_native_certs::load_native_certs(); let mozilla = webpki_roots::TLS_SERVER_ROOTS .iter() .map(|ta| (ta.subject_public_key_info.as_ref(), ta)) @@ -66,7 +66,7 @@ fn test_does_not_have_many_roots_unknown_by_mozilla() { let mut missing_in_moz_roots = 0; - for cert in &native { + for cert in &native.certs { let cert = anchor_from_trusted_cert(cert).unwrap(); if let Some(moz) = mozilla.get(cert.subject_public_key_info.as_ref()) { assert_eq!(cert.subject, moz.subject, "subjects differ for public key"); @@ -88,7 +88,7 @@ fn test_does_not_have_many_roots_unknown_by_mozilla() { let diff = (missing_in_moz_roots as f64) / (mozilla.len() as f64); println!("mozilla: {:?}", mozilla.len()); - println!("native: {:?}", native.len()); + println!("native: {:?}", native.certs.len()); println!( "{:?} anchors present in native set but not mozilla ({}%)", missing_in_moz_roots, @@ -99,10 +99,10 @@ fn test_does_not_have_many_roots_unknown_by_mozilla() { #[test] fn test_contains_most_roots_known_by_mozilla() { - let native = rustls_native_certs::load_native_certs().unwrap(); + let native = rustls_native_certs::load_native_certs(); let mut native_map = HashMap::new(); - for anchor in &native { + for anchor in &native.certs { let cert = anchor_from_trusted_cert(anchor).unwrap(); let spki = cert.subject_public_key_info.as_ref(); native_map.insert(spki.to_owned(), anchor); @@ -129,7 +129,7 @@ fn test_contains_most_roots_known_by_mozilla() { let diff = (missing_in_native_roots as f64) / (mozilla.len() as f64); println!("mozilla: {:?}", mozilla.len()); - println!("native: {:?}", native.len()); + println!("native: {:?}", native.certs.len()); println!( "{:?} anchors present in mozilla set but not native ({}%)", missing_in_native_roots, @@ -146,9 +146,8 @@ fn util_list_certs() { common::clear_env(); } - let native = rustls_native_certs::load_native_certs().unwrap(); - - for (i, cert) in native.iter().enumerate() { + let native = rustls_native_certs::load_native_certs(); + for (i, cert) in native.certs.iter().enumerate() { let cert = anchor_from_trusted_cert(cert).unwrap(); println!("cert[{i}] = {}", stringify_x500name(&cert.subject)); } diff --git a/tests/smoketests.rs b/tests/smoketests.rs index 9b03889..0a1376f 100644 --- a/tests/smoketests.rs +++ b/tests/smoketests.rs @@ -23,7 +23,7 @@ use serial_test::serial; /// certificate loading, or connecting via TCP. fn check_site(domain: &str) -> Result<(), ()> { let mut roots = rustls::RootCertStore::empty(); - for cert in rustls_native_certs::load_native_certs().unwrap() { + for cert in rustls_native_certs::load_native_certs().certs { roots.add(cert).unwrap(); }