From 4a76efe6d51fe31dc5fa9b32f567f903e52db0ae Mon Sep 17 00:00:00 2001 From: Marc Hartmayer Date: Thu, 23 May 2024 16:32:57 +0000 Subject: [PATCH] rust: Use AsRef and PathBuf in libraries Use `AsRef` instead of `&Path`, &str, .... to be more versatile and accept more input types. In addition, use `PathBuf` and `Path` for paths instead of `String` and `str`. Signed-off-by: Marc Hartmayer Reviewed-by: Steffen Eiden Signed-off-by: Steffen Eiden --- rust/pv/src/error.rs | 4 +++- rust/pv/src/test_utils.rs | 6 +++--- rust/pv/src/utils.rs | 24 ++++++++++----------- rust/pv/src/verify.rs | 15 ++++++++----- rust/pv/src/verify/helper.rs | 28 ++++++++++++------------ rust/pv/src/verify/test.rs | 15 +++++++------ rust/pv/tests/cert_verifier.rs | 9 ++++---- rust/pv_core/src/error.rs | 6 ++++-- rust/pv_core/src/macros.rs | 9 +------- rust/pv_core/src/utils.rs | 38 ++++++++++++++++----------------- rust/pv_core/src/uvdevice.rs | 2 +- rust/pvsecret/src/cmd/verify.rs | 2 +- rust/utils/src/cli.rs | 14 ++++++------ 13 files changed, 88 insertions(+), 84 deletions(-) diff --git a/rust/pv/src/error.rs b/rust/pv/src/error.rs index c1540ae0..3d355c13 100644 --- a/rust/pv/src/error.rs +++ b/rust/pv/src/error.rs @@ -2,6 +2,8 @@ // // Copyright IBM Corp. 2023, 2024 +use std::path::PathBuf; + use crate::secret::UserDataType; /// Result type for this crate @@ -26,7 +28,7 @@ pub enum Error { #[error("Cannot load {ty} from {path}")] X509Load { - path: String, + path: PathBuf, ty: &'static str, source: openssl::error::ErrorStack, }, diff --git a/rust/pv/src/test_utils.rs b/rust/pv/src/test_utils.rs index ec43d227..ef7874e9 100644 --- a/rust/pv/src/test_utils.rs +++ b/rust/pv/src/test_utils.rs @@ -42,7 +42,7 @@ pub fn get_cert_asset_path>(path: P) -> PathBuf { /// TEST ONLY! Load an cert /// /// panic on errors -pub fn get_cert_asset(path: &'static str) -> Vec { +pub fn get_cert_asset>(path: P) -> Vec { let p = get_cert_asset_path(path); fs::read(p).unwrap() } @@ -50,7 +50,7 @@ pub fn get_cert_asset(path: &'static str) -> Vec { /// TEST ONLY! Load cert found in the asset path /// /// panic on errors -pub fn load_gen_cert(asset_path: &'static str) -> X509 { +pub fn load_gen_cert>(asset_path: P) -> X509 { let buf = get_cert_asset(asset_path); let mut cert = X509::from_der(&buf) .map(|crt| vec![crt]) @@ -63,7 +63,7 @@ pub fn load_gen_cert(asset_path: &'static str) -> X509 { /// TEST ONLY! Load the CRL found in the asset path /// /// panic on errors -pub fn load_gen_crl(asset_path: &'static str) -> X509Crl { +pub fn load_gen_crl>(asset_path: P) -> X509Crl { let buf = get_cert_asset(asset_path); X509Crl::from_der(&buf) diff --git a/rust/pv/src/utils.rs b/rust/pv/src/utils.rs index 33cd55fd..7d9e2e4b 100644 --- a/rust/pv/src/utils.rs +++ b/rust/pv/src/utils.rs @@ -13,11 +13,11 @@ use openssl::{ /// /// This function will return an error if the underlying OpenSSL implementation cannot parse `buf` /// as `DER` or `PEM`. -pub fn read_crls(buf: &[u8]) -> Result> { +pub fn read_crls>(buf: T) -> Result> { use crate::openssl_extensions::StackableX509Crl; - X509Crl::from_der(buf) + X509Crl::from_der(buf.as_ref()) .map(|crl| vec![crl]) - .or_else(|_| StackableX509Crl::stack_from_pem(buf)) + .or_else(|_| StackableX509Crl::stack_from_pem(buf.as_ref())) .map_err(Error::Crypto) } @@ -26,10 +26,10 @@ pub fn read_crls(buf: &[u8]) -> Result> { /// # Errors /// /// This function will return an error if the underlying OpenSSL implementation cannot parse `buf` -pub fn read_certs(buf: &[u8]) -> Result, ErrorStack> { - X509::from_der(buf) +pub fn read_certs>(buf: T) -> Result, ErrorStack> { + X509::from_der(buf.as_ref()) .map(|crt| vec![crt]) - .or_else(|_| X509::stack_from_pem(buf)) + .or_else(|_| X509::stack_from_pem(buf.as_ref())) } #[cfg(test)] @@ -41,9 +41,9 @@ mod tests { let crl = get_cert_asset("ibm.crl"); let crl_der = get_cert_asset("der.crl"); let fail = get_cert_asset("ibm.crt"); - assert_eq!(super::read_crls(&crl).unwrap().len(), 1); - assert_eq!(super::read_crls(&crl_der).unwrap().len(), 1); - assert_eq!(super::read_crls(&fail).unwrap().len(), 0); + assert_eq!(super::read_crls(crl).unwrap().len(), 1); + assert_eq!(super::read_crls(crl_der).unwrap().len(), 1); + assert_eq!(super::read_crls(fail).unwrap().len(), 0); } #[test] @@ -51,8 +51,8 @@ mod tests { let crt = get_cert_asset("ibm.crt"); let crt_der = get_cert_asset("der.crt"); let fail = get_cert_asset("ibm.crl"); - assert_eq!(super::read_certs(&crt).unwrap().len(), 1); - assert_eq!(super::read_certs(&crt_der).unwrap().len(), 1); - assert_eq!(super::read_certs(&fail).unwrap().len(), 0); + assert_eq!(super::read_certs(crt).unwrap().len(), 1); + assert_eq!(super::read_certs(crt_der).unwrap().len(), 1); + assert_eq!(super::read_certs(fail).unwrap().len(), 0); } } diff --git a/rust/pv/src/verify.rs b/rust/pv/src/verify.rs index 1e9bedeb..553eb7ac 100644 --- a/rust/pv/src/verify.rs +++ b/rust/pv/src/verify.rs @@ -173,12 +173,17 @@ impl CertVerifier { /// # Errors /// /// This function will return an error if the chain of trust could not be established. - pub fn new( - cert_paths: &[&Path], - crl_paths: &[&Path], - root_ca_path: Option<&Path>, + pub fn new( + cert_paths: &[P], + crl_paths: &[Q], + root_ca_path: Option, offline: bool, - ) -> Result { + ) -> Result + where + P: AsRef, + Q: AsRef, + R: AsRef, + { let mut store = helper::store_setup(root_ca_path, crl_paths, cert_paths)?; let mut untr_certs = Vec::with_capacity(cert_paths.len()); for path in cert_paths { diff --git a/rust/pv/src/verify/helper.rs b/rust/pv/src/verify/helper.rs index 1f25bd00..a57f39db 100644 --- a/rust/pv/src/verify/helper.rs +++ b/rust/pv/src/verify/helper.rs @@ -75,10 +75,10 @@ pub fn verify_crl(crl: &X509CrlRef, issuer: &X509Ref) -> Option<()> { } /// Setup the x509Store such that it can be used it for verifying certificates -pub fn store_setup( - root_ca_path: Option<&Path>, - crl_paths: &[&Path], - cert_w_crl_paths: &[&Path], +pub fn store_setup, Q: AsRef, R: AsRef>( + root_ca_path: Option

, + crl_paths: &[Q], + cert_w_crl_paths: &[R], ) -> Result { let mut x509store = X509StoreBuilder::new()?; @@ -89,7 +89,7 @@ pub fn store_setup( for crl in crl_paths { load_crl_to_store(&mut x509store, crl, true).map_err(|source| Error::X509Load { - path: crl.display().to_string(), + path: crl.as_ref().into(), ty: Error::CRL, source, })?; @@ -97,7 +97,7 @@ pub fn store_setup( for crl in cert_w_crl_paths { load_crl_to_store(&mut x509store, crl, false).map_err(|source| Error::X509Load { - path: crl.display().to_string(), + path: crl.as_ref().into(), ty: Error::CRL, source, })?; @@ -236,35 +236,35 @@ fn get_ibm_z_sign_key(certs: &[X509]) -> Result { } } -fn load_root_ca(path: &Path, x509_store: &mut X509StoreBuilder) -> Result<()> { +fn load_root_ca>(path: P, x509_store: &mut X509StoreBuilder) -> Result<()> { let lu = x509_store.add_lookup(X509Lookup::::file())?; // Try to load cert as PEM file - match lu.load_cert_file(path, SslFiletype::PEM) { + match lu.load_cert_file(&path, SslFiletype::PEM) { Ok(_) => lu - .load_crl_file(path, SslFiletype::PEM) + .load_crl_file(&path, SslFiletype::PEM) .map(|_| ()) .or(Ok(())), // Not a PEM file? try ASN1 Err(_) => lu - .load_cert_file(path, SslFiletype::ASN1) + .load_cert_file(&path, SslFiletype::ASN1) .map(|_| ()) .map_err(|source| Error::X509Load { - path: path.display().to_string(), + path: path.as_ref().into(), ty: Error::CERT, source, }), } } -fn load_crl_to_store( +fn load_crl_to_store>( x509_store: &mut X509StoreBuilder, - path: &Path, + path: P, err_out_empty_crl: bool, ) -> std::result::Result<(), ErrorStack> { let lu = x509_store.add_lookup(X509Lookup::::file())?; // Try to load cert as PEM file - if lu.load_crl_file(path, SslFiletype::PEM).is_err() { + if lu.load_crl_file(&path, SslFiletype::PEM).is_err() { // Not a PEM file? try read as ASN1 let res = lu.load_crl_file(path, SslFiletype::ASN1); if err_out_empty_crl { diff --git a/rust/pv/src/verify/test.rs b/rust/pv/src/verify/test.rs index 294d9646..71495626 100644 --- a/rust/pv/src/verify/test.rs +++ b/rust/pv/src/verify/test.rs @@ -14,7 +14,7 @@ use crate::test_utils::*; // Mock function pub fn download_first_crl_from_x509(cert: &X509Ref) -> Result>> { fn mock_download>(path: P) -> Result> { - read_crls(&std::fs::read(path)?) + read_crls(std::fs::read(path)?) } for dist_point in x509_dist_points(cert) { @@ -35,8 +35,9 @@ pub fn download_first_crl_from_x509(cert: &X509Ref) -> Result, &crls, &[&ibm_path, &inter_path]); assert!(store.is_ok()); } @@ -45,8 +46,9 @@ fn verify_chain_online() { let ibm_crt = get_cert_asset_path("ibm.crt"); let inter_crt = get_cert_asset_path("inter_ca.crt"); let root_crt = get_cert_asset_path("root_ca.chained.crt"); + let crls: [String; 0] = []; - let ret = CertVerifier::new(&[&ibm_crt, &inter_crt], &[], Some(&root_crt), false); + let ret = CertVerifier::new(&[&ibm_crt, &inter_crt], &crls, Some(&root_crt), false); assert!(ret.is_ok(), "CertVerifier::new failed: {ret:?}"); } @@ -56,8 +58,9 @@ fn verify_chain_offline() { let inter_crl = get_cert_asset_path("inter_ca.crl"); let inter_crt = load_gen_cert("inter_ca.crt"); let root_crt = get_cert_asset_path("root_ca.chained.crt"); + let certs: [String; 0] = []; - let store = helper::store_setup(Some(&root_crt), &[&inter_crl], &[]) + let store = helper::store_setup(Some(&root_crt), &[&inter_crl], &certs) .unwrap() .build(); @@ -84,10 +87,10 @@ fn verify(offline: bool, ibm_crt: &'static str, ibm_crl: &'static str, hkd: &'st let hkd_exp = load_gen_cert("host_crt_expired.crt"); let hkd = load_gen_cert(hkd); - let crls = &[ibm_crl.as_path(), inter_crl.as_path()]; + let crls = [&ibm_crl, &inter_crl]; let verifier = CertVerifier::new( &[&ibm_crt, &inter_crt], - if offline { crls } else { &[] }, + if offline { &crls } else { &[] }, Some(&root_crt), offline, ) diff --git a/rust/pv/tests/cert_verifier.rs b/rust/pv/tests/cert_verifier.rs index 6185a304..c99c128b 100644 --- a/rust/pv/tests/cert_verifier.rs +++ b/rust/pv/tests/cert_verifier.rs @@ -40,9 +40,10 @@ fn verifier_new() { let ibm_early_crt = get_cert_asset_path("ibm_outdated_early.crl"); let ibm_late_crt = get_cert_asset_path("ibm_outdated_late.crl"); let ibm_rev_crt = get_cert_asset_path("ibm_rev.crt"); + let empty: [String; 0] = []; // Too many signing keys - let verifier = CertVerifier::new(&[&ibm_crt, &ibm_rev_crt], &[], None, true); + let verifier = CertVerifier::new(&[&ibm_crt, &ibm_rev_crt], &empty, None::, true); assert!(matches!(verifier, Err(Error::HkdVerify(ManyIbmSignKeys)))); // No CRL for each X509 @@ -53,7 +54,7 @@ fn verifier_new() { false, ); verify_sign_error(3, verifier.unwrap_err()); - let verifier = CertVerifier::new(&[&inter_crt, &ibm_crt], &[], Some(&root_chn_crt), false); + let verifier = CertVerifier::new(&[&inter_crt, &ibm_crt], &empty, Some(&root_chn_crt), false); verify_sign_error(3, verifier.unwrap_err()); // Wrong intermediate (or ibm key) @@ -67,7 +68,7 @@ fn verifier_new() { verify_sign_error_slice(&[20, 30], verifier.unwrap_err()); // Wrong root ca - let verifier = CertVerifier::new(&[&inter_crt, &ibm_crt], &[&inter_crl], None, true); + let verifier = CertVerifier::new(&[&inter_crt, &ibm_crt], &[&inter_crl], None::, true); verify_sign_error(20, verifier.unwrap_err()); // Correct signing key + intermediate cert @@ -80,7 +81,7 @@ fn verifier_new() { .unwrap(); // No intermediate key - let verifier = CertVerifier::new(&[&ibm_crt], &[], Some(&root_chn_crt), false); + let verifier = CertVerifier::new(&[&ibm_crt], &empty, Some(&root_chn_crt), false); verify_sign_error(20, verifier.unwrap_err()); // IBM Sign outdated diff --git a/rust/pv_core/src/error.rs b/rust/pv_core/src/error.rs index 1a4c5b22..86094b23 100644 --- a/rust/pv_core/src/error.rs +++ b/rust/pv_core/src/error.rs @@ -2,6 +2,8 @@ // // Copyright IBM Corp. 2023, 2024 +use std::path::PathBuf; + /// Result type for this crate pub type Result = std::result::Result; @@ -24,13 +26,13 @@ pub enum Error { FileIo { ty: FileIoErrorType, ctx: String, - path: String, + path: PathBuf, source: std::io::Error, }, #[error("Cannot {ty} `{path}`")] FileAccess { ty: FileAccessErrorType, - path: String, + path: PathBuf, source: std::io::Error, }, diff --git a/rust/pv_core/src/macros.rs b/rust/pv_core/src/macros.rs index c904f544..2c211a9a 100644 --- a/rust/pv_core/src/macros.rs +++ b/rust/pv_core/src/macros.rs @@ -2,19 +2,12 @@ // // Copyright IBM Corp. 2023, 2024 -macro_rules! path_to_str { - ($path: expr) => { - $path.as_ref().to_str().unwrap_or("no UTF-8 path") - }; -} -pub(crate) use path_to_str; - macro_rules! file_error { ($ty: tt, $ctx: expr, $path:expr, $src: expr) => { $crate::Error::FileIo { ty: $crate::FileIoErrorType::$ty, ctx: $ctx.to_string(), - path: $path.to_string(), + path: $path.as_ref().to_path_buf(), source: $src, } }; diff --git a/rust/pv_core/src/utils.rs b/rust/pv_core/src/utils.rs index 26afe758..b22166ca 100644 --- a/rust/pv_core/src/utils.rs +++ b/rust/pv_core/src/utils.rs @@ -2,7 +2,7 @@ // // Copyright IBM Corp. 2023 use crate::{ - macros::{bail_spec, file_error, path_to_str}, + macros::{bail_spec, file_error}, Error, FileAccessErrorType, FileIoErrorType, Result, }; use std::{ @@ -187,7 +187,7 @@ pub fn try_parse_u64(hex_str: &str, ctx: &str) -> Result { pub fn open_file>(path: P) -> Result { File::open(&path).map_err(|e| Error::FileAccess { ty: FileAccessErrorType::Open, - path: path_to_str!(path).to_string(), + path: path.as_ref().to_path_buf(), source: e, }) } @@ -200,7 +200,7 @@ pub fn open_file>(path: P) -> Result { pub fn create_file>(path: P) -> Result { File::create(&path).map_err(|e| Error::FileAccess { ty: FileAccessErrorType::Create, - path: path_to_str!(path).to_string(), + path: path.as_ref().to_path_buf(), source: e, }) } @@ -226,7 +226,7 @@ pub fn read_exact_file, const COUNT: usize>( ) -> Result<[u8; COUNT]> { let mut f = File::open(&path).map_err(|e| Error::FileAccess { ty: FileAccessErrorType::Open, - path: path_to_str!(path).to_string(), + path: path.as_ref().to_path_buf(), source: e, })?; @@ -236,7 +236,7 @@ pub fn read_exact_file, const COUNT: usize>( let mut buf = [0; COUNT]; f.read_exact(&mut buf) - .map_err(|e| file_error!(Read, ctx, path_to_str!(path).to_string(), e))?; + .map_err(|e| file_error!(Read, ctx, path, e))?; Ok(buf) } @@ -249,14 +249,7 @@ pub fn read_exact_file, const COUNT: usize>( /// # Errors /// Passes through any kind of error `std::fs::read` produces pub fn read_file>(path: P, ctx: &str) -> Result> { - std::fs::read(&path).map_err(|e| { - file_error!( - Read, - ctx, - path.as_ref().to_str().unwrap_or("no UTF-8 path"), - e - ) - }) + std::fs::read(&path).map_err(|e| file_error!(Read, ctx, path, e)) } /// Reads all content from a [`std::io::Read`] and add context in case of an error @@ -267,12 +260,12 @@ pub fn read_file>(path: P, ctx: &str) -> Result> { /// /// # Errors /// Passes through any kind of error `std::fs::read` produces -pub fn read(rd: &mut R, path: &str, ctx: &str) -> Result> { +pub fn read>(rd: &mut R, path: P, ctx: &str) -> Result> { let mut buf = vec![]; rd.read_to_end(&mut buf).map_err(|e| Error::FileIo { ty: FileIoErrorType::Write, ctx: ctx.to_string(), - path: path.to_string(), + path: path.as_ref().to_path_buf(), source: e, })?; Ok(buf) @@ -286,11 +279,11 @@ pub fn read(rd: &mut R, path: &str, ctx: &str) -> Result> { /// /// # Errors /// Passes through any kind of error `std::fs::write` produces -pub fn write_file>(path: &str, data: D, ctx: &str) -> Result<()> { - std::fs::write(path, data.as_ref()).map_err(|e| Error::FileIo { +pub fn write_file, P: AsRef>(path: P, data: D, ctx: &str) -> Result<()> { + std::fs::write(path.as_ref(), data.as_ref()).map_err(|e| Error::FileIo { ty: FileIoErrorType::Write, ctx: ctx.to_string(), - path: path.to_string(), + path: path.as_ref().to_path_buf(), source: e, }) } @@ -303,11 +296,16 @@ pub fn write_file>(path: &str, data: D, ctx: &str) -> Result<()> /// /// # Errors /// Passes through any kind of error `std::fs::write` produces -pub fn write, W: Write>(wr: &mut W, data: D, path: &str, ctx: &str) -> Result<()> { +pub fn write, P: AsRef, W: Write>( + wr: &mut W, + data: D, + path: P, + ctx: &str, +) -> Result<()> { wr.write_all(data.as_ref()).map_err(|e| Error::FileIo { ty: FileIoErrorType::Write, ctx: ctx.to_string(), - path: path.to_string(), + path: path.as_ref().to_path_buf(), source: e, }) } diff --git a/rust/pv_core/src/uvdevice.rs b/rust/pv_core/src/uvdevice.rs index b7b7cbea..c3520531 100644 --- a/rust/pv_core/src/uvdevice.rs +++ b/rust/pv_core/src/uvdevice.rs @@ -208,7 +208,7 @@ impl UvDevice { .open(UvDevice::PATH) .map_err(|e| Error::FileAccess { ty: FileAccessErrorType::Open, - path: (UvDevice::PATH).to_string(), + path: (UvDevice::PATH).into(), source: e, })?, )) diff --git a/rust/pvsecret/src/cmd/verify.rs b/rust/pvsecret/src/cmd/verify.rs index d96ac8cf..bb75292a 100644 --- a/rust/pvsecret/src/cmd/verify.rs +++ b/rust/pvsecret/src/cmd/verify.rs @@ -10,7 +10,7 @@ use utils::{get_reader_from_cli_file_arg, get_writer_from_cli_file_arg}; /// read the content of a DER or PEM x509 and return the public key fn read_sgn_key(path: &str) -> Result> { - read_certs(&read_file(path, "user-signing key")?)? + read_certs(read_file(path, "user-signing key")?)? .first() .ok_or(anyhow!("File does not contain a X509 certificate"))? .public_key() diff --git a/rust/utils/src/cli.rs b/rust/utils/src/cli.rs index 6d149eea..970f9f66 100644 --- a/rust/utils/src/cli.rs +++ b/rust/utils/src/cli.rs @@ -104,9 +104,9 @@ impl CertificateOptions { Ok(Box::new(NoVerifyHkd)) } false => Ok(Box::new(CertVerifier::new( - &self.certs.iter().map(Path::new).collect::>(), - &self.crls.iter().map(Path::new).collect::>(), - self.root_ca.as_ref().map(Path::new), + &self.certs, + &self.crls, + self.root_ca.as_ref(), self.offline, )?)), } @@ -154,8 +154,8 @@ pub const STDOUT: &str = "-"; pub const STDIN: &str = "-"; /// Converts an argument value into a Writer. -pub fn get_writer_from_cli_file_arg(path: &str) -> Result> { - if path == STDOUT { +pub fn get_writer_from_cli_file_arg>(path: P) -> Result> { + if path.as_ref() == Path::new(STDOUT) { Ok(Box::new(std::io::stdout())) } else { Ok(Box::new(create_file(path)?)) @@ -163,8 +163,8 @@ pub fn get_writer_from_cli_file_arg(path: &str) -> Result> { } /// Converts an argument value into a Reader. -pub fn get_reader_from_cli_file_arg(path: &str) -> Result> { - if path == STDIN { +pub fn get_reader_from_cli_file_arg>(path: P) -> Result> { + if path.as_ref() == Path::new(STDIN) { Ok(Box::new(std::io::stdin())) } else { Ok(Box::new(open_file(path)?))