diff --git a/openssl/src/error.rs b/openssl/src/error.rs index e097ce688..aae2f140b 100644 --- a/openssl/src/error.rs +++ b/openssl/src/error.rs @@ -23,6 +23,7 @@ use std::convert::TryInto; use std::error; use std::ffi::CStr; use std::fmt; +use std::fmt::Formatter; use std::io; use std::ptr; use std::str; @@ -399,6 +400,49 @@ cfg_if! { } } +pub enum X509D2iError { + InternalOpenSSLError(ErrorStack), + ExtensionNotFoundError, + ExtensionAmbiguousError, +} + +impl X509D2iError { + pub fn internal_openssl_error(error: ErrorStack) -> Self { + Self::InternalOpenSSLError(error) + } + + pub fn extension_not_found_error() -> Self { + Self::ExtensionNotFoundError + } + + pub fn extension_ambiguous_error() -> Self { + Self::ExtensionAmbiguousError + } + + fn format(&self, fmt: &mut Formatter<'_>) -> fmt::Result { + match self { + Self::InternalOpenSSLError(stack) => + write!(fmt, "Error: Could not get X509 extension; {}", stack), + Self::ExtensionNotFoundError => + write!(fmt, "Error: Could not get X509 extension; Reason: Could not find any matching extension."), + Self::ExtensionAmbiguousError => + write!(fmt, "Error: Could not get X509 extension; Reason: Tried to read an extension, but found multiple."), + } + } +} + +impl fmt::Debug for X509D2iError { + fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result { + self.format(fmt) + } +} + +impl fmt::Display for X509D2iError { + fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result { + self.format(fmt) + } +} + #[cfg(test)] mod tests { #[cfg(not(ossl310))] diff --git a/openssl/src/ssl/connector.rs b/openssl/src/ssl/connector.rs index 66d1bd893..72f06edb2 100644 --- a/openssl/src/ssl/connector.rs +++ b/openssl/src/ssl/connector.rs @@ -417,7 +417,7 @@ cfg_if! { use std::str; use once_cell::sync::OnceCell; - use crate::error::ErrorStack; + use crate::error::{ErrorStack, X509D2iError}; use crate::ex_data::Index; use crate::nid::Nid; use crate::ssl::Ssl; @@ -460,8 +460,9 @@ cfg_if! { fn verify_hostname(domain: &str, cert: &X509Ref) -> bool { match cert.subject_alt_names() { - Some(names) => verify_subject_alt_names(domain, names), - None => verify_subject_name(domain, &cert.subject_name()), + Ok(names) => verify_subject_alt_names(domain, names), + Err(X509D2iError::ExtensionNotFoundError) => verify_subject_name(domain, &cert.subject_name()), + Err(e) => panic!("Error when fetching alt names from certificate: {}", e), } } diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index a64524cbe..0438c4fa5 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -29,7 +29,7 @@ use crate::asn1::{ }; use crate::bio::MemBioSlice; use crate::conf::ConfRef; -use crate::error::ErrorStack; +use crate::error::{ErrorStack, X509D2iError}; use crate::ex_data::Index; use crate::hash::{DigestBytes, MessageDigest}; use crate::nid::Nid; @@ -419,63 +419,85 @@ impl X509Ref { ffi::X509_issuer_name_hash(self.as_ptr()) as u32 } } + fn create_d2i_result(critical: i32, out: Option) -> Result { + match (critical, out) { + (0 | 1, Some(out)) => Ok(out), + // -1 means the extension wasn't found, -2 means multiple were found. + (-1, _) => Err(X509D2iError::extension_not_found_error()), + (-2, _) => Err(X509D2iError::extension_ambiguous_error()), + // A critical value of 0 or 1 suggests success, but a null pointer + // was returned so something went wrong. + (0 | 1, None) => Err(X509D2iError::internal_openssl_error(ErrorStack::get())), + (c_int::MIN..=-2 | 2.., _) => panic!("OpenSSL should only return -2, -1, 0, or 1 for an extension's criticality but it returned {}", critical), + } + } /// Returns this certificate's subject alternative name entries, if they exist. #[corresponds(X509_get_ext_d2i)] - pub fn subject_alt_names(&self) -> Option> { - unsafe { + pub fn subject_alt_names(&self) -> Result, X509D2iError> { + let mut critical = -1; + let out = unsafe { let stack = ffi::X509_get_ext_d2i( self.as_ptr(), ffi::NID_subject_alt_name, - ptr::null_mut(), + &mut critical as *mut _, ptr::null_mut(), ); Stack::from_ptr_opt(stack as *mut _) - } + }; + + Self::create_d2i_result(critical, out) } /// Returns this certificate's CRL distribution points, if they exist. #[corresponds(X509_get_ext_d2i)] - pub fn crl_distribution_points(&self) -> Option> { - unsafe { + pub fn crl_distribution_points(&self) -> Result, X509D2iError> { + let mut critical = -1; + let out = unsafe { let stack = ffi::X509_get_ext_d2i( self.as_ptr(), ffi::NID_crl_distribution_points, - ptr::null_mut(), + &mut critical as *mut _, ptr::null_mut(), ); Stack::from_ptr_opt(stack as *mut _) - } + }; + + Self::create_d2i_result(critical, out) } /// Returns this certificate's issuer alternative name entries, if they exist. #[corresponds(X509_get_ext_d2i)] - pub fn issuer_alt_names(&self) -> Option> { - unsafe { + pub fn issuer_alt_names(&self) -> Result, X509D2iError> { + let mut critical = -1; + let out = unsafe { let stack = ffi::X509_get_ext_d2i( self.as_ptr(), ffi::NID_issuer_alt_name, - ptr::null_mut(), + &mut critical as *mut _, ptr::null_mut(), ); Stack::from_ptr_opt(stack as *mut _) - } + }; + Self::create_d2i_result(critical, out) } /// Returns this certificate's [`authority information access`] entries, if they exist. /// /// [`authority information access`]: https://tools.ietf.org/html/rfc5280#section-4.2.2.1 #[corresponds(X509_get_ext_d2i)] - pub fn authority_info(&self) -> Option> { - unsafe { + pub fn authority_info(&self) -> Result, X509D2iError> { + let mut critical = -1; + let out = unsafe { let stack = ffi::X509_get_ext_d2i( self.as_ptr(), ffi::NID_info_access, - ptr::null_mut(), + &mut critical as *mut _, ptr::null_mut(), ); Stack::from_ptr_opt(stack as *mut _) - } + }; + Self::create_d2i_result(critical, out) } /// Retrieves the path length extension from a certificate, if it exists. @@ -814,9 +836,15 @@ impl fmt::Debug for X509 { debug_struct.field("signature_algorithm", &self.signature_algorithm().object()); debug_struct.field("issuer", &self.issuer_name()); debug_struct.field("subject", &self.subject_name()); - if let Some(subject_alt_names) = &self.subject_alt_names() { - debug_struct.field("subject_alt_names", subject_alt_names); - } + match &self.subject_alt_names() { + Ok(subject_alt_names) => { + debug_struct.field("subject_alt_names", subject_alt_names); + } + Err(X509D2iError::ExtensionNotFoundError) => { + // found nothing, but this is ok + } + Err(e) => panic!("{}", e), + }; debug_struct.field("not_before", &self.not_before()); debug_struct.field("not_after", &self.not_after()); @@ -1711,7 +1739,7 @@ impl X509RevokedRef { /// /// This returns None if the extension is not present or occurs multiple times. #[corresponds(X509_REVOKED_get_ext_d2i)] - pub fn extension(&self) -> Result, ErrorStack> { + pub fn extension(&self) -> Result<(bool, T::Output), X509D2iError> { let mut critical = -1; let out = unsafe { // SAFETY: self.as_ptr() is a valid pointer to an X509_REVOKED. @@ -1726,13 +1754,14 @@ impl X509RevokedRef { T::Output::from_ptr_opt(ext as *mut _) }; match (critical, out) { - (0, Some(out)) => Ok(Some((false, out))), - (1, Some(out)) => Ok(Some((true, out))), + (0, Some(out)) => Ok((false, out)), + (1, Some(out)) => Ok((true, out)), // -1 means the extension wasn't found, -2 means multiple were found. - (-1 | -2, _) => Ok(None), + (-1, _) => Err(X509D2iError::extension_not_found_error()), + (-2, _) => Err(X509D2iError::extension_ambiguous_error()), // A critical value of 0 or 1 suggests success, but a null pointer // was returned so something went wrong. - (0 | 1, None) => Err(ErrorStack::get()), + (0 | 1, None) => Err(X509D2iError::internal_openssl_error(ErrorStack::get())), (c_int::MIN..=-2 | 2.., _) => panic!("OpenSSL should only return -2, -1, 0, or 1 for an extension's criticality but it returned {}", critical), } } @@ -1947,7 +1976,7 @@ impl X509CrlRef { /// /// This returns None if the extension is not present or occurs multiple times. #[corresponds(X509_CRL_get_ext_d2i)] - pub fn extension(&self) -> Result, ErrorStack> { + pub fn extension(&self) -> Result<(bool, T::Output), X509D2iError> { let mut critical = -1; let out = unsafe { // SAFETY: self.as_ptr() is a valid pointer to an X509_CRL. @@ -1962,13 +1991,14 @@ impl X509CrlRef { T::Output::from_ptr_opt(ext as *mut _) }; match (critical, out) { - (0, Some(out)) => Ok(Some((false, out))), - (1, Some(out)) => Ok(Some((true, out))), + (0, Some(out)) => Ok((false, out)), + (1, Some(out)) => Ok((true, out)), // -1 means the extension wasn't found, -2 means multiple were found. - (-1 | -2, _) => Ok(None), + (-1, _) => Err(X509D2iError::extension_not_found_error()), + (-2, _) => Err(X509D2iError::extension_ambiguous_error()), // A critical value of 0 or 1 suggests success, but a null pointer // was returned so something went wrong. - (0 | 1, None) => Err(ErrorStack::get()), + (0 | 1, None) => Err(X509D2iError::internal_openssl_error(ErrorStack::get())), (c_int::MIN..=-2 | 2.., _) => panic!("OpenSSL should only return -2, -1, 0, or 1 for an extension's criticality but it returned {}", critical), } } diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 25c2da012..a15ea6499 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -28,6 +28,7 @@ use crate::x509::{ CrlStatus, X509Crl, X509Extension, X509Name, X509Req, X509StoreContext, X509VerifyResult, X509, }; +use crate::error::X509D2iError; #[cfg(ossl110)] use foreign_types::ForeignType; use hex::{self, FromHex}; @@ -280,7 +281,11 @@ fn test_aia_ca_issuer() { // Without AIA let cert = include_bytes!("../../test/cert.pem"); let cert = X509::from_pem(cert).unwrap(); - assert!(cert.authority_info().is_none()); + match cert.authority_info() { + Ok(_) => panic!("Should not find dist point"), + Err(X509D2iError::ExtensionNotFoundError) => { /* ok */ } + Err(e) => panic!("Wrong error: {}", e), + } } #[test] @@ -701,10 +706,7 @@ fn test_crl_entry_extensions() { let crl = include_bytes!("../../test/entry_extensions.crl"); let crl = X509Crl::from_pem(crl).unwrap(); - let (critical, access_info) = crl - .extension::() - .unwrap() - .expect("Authority Information Access extension should be present"); + let (critical, access_info) = crl.extension::().unwrap(); assert!( !critical, "Authority Information Access extension is not critical" @@ -724,7 +726,6 @@ fn test_crl_entry_extensions() { let (critical, issuer) = entry .extension::() - .unwrap() .expect("Certificate issuer extension should be present"); assert!(critical, "Certificate issuer extension is critical"); assert_eq!(issuer.len(), 1, "Certificate issuer should have one entry"); @@ -740,7 +741,6 @@ fn test_crl_entry_extensions() { #[allow(unused_variables)] let (critical, reason_code) = entry .extension::() - .unwrap() .expect("Reason code extension should be present"); assert!(!critical, "Reason code extension is not critical"); #[cfg(ossl110)] @@ -1175,7 +1175,11 @@ fn test_dist_point() { fn test_dist_point_null() { let cert = include_bytes!("../../test/cert.pem"); let cert = X509::from_pem(cert).unwrap(); - assert!(cert.crl_distribution_points().is_none()); + match cert.crl_distribution_points() { + Ok(_) => panic!("Should not find dist point"), + Err(X509D2iError::ExtensionNotFoundError) => { /* ok */ } + Err(e) => panic!("Wrong error: {}", e), + } } #[test]