From 65e7ed01511de82d3dd515077015d843d69cb486 Mon Sep 17 00:00:00 2001 From: Steven Malis Date: Fri, 8 May 2026 11:39:48 -0700 Subject: [PATCH 1/4] crypto: Implement issued check for symcrypt, improve error handling --- openhcl/underhill_attestation/src/jwt.rs | 6 ++- support/crypto/src/x509/mod.rs | 2 +- support/crypto/src/x509/ossl.rs | 11 ++++- support/crypto/src/x509/symcrypt.rs | 61 +++++++++++++++++++++++- 4 files changed, 74 insertions(+), 6 deletions(-) diff --git a/openhcl/underhill_attestation/src/jwt.rs b/openhcl/underhill_attestation/src/jwt.rs index 4c51faa61c..8403c7e219 100644 --- a/openhcl/underhill_attestation/src/jwt.rs +++ b/openhcl/underhill_attestation/src/jwt.rs @@ -58,6 +58,8 @@ pub(crate) enum CertificateChainValidationError { VerifyChildSignatureWithParentPublicKey(#[source] crypto::x509::X509Error), #[error("cert chain validation failed -- signature mismatch")] CertChainSignatureMismatch, + #[error("failed to check if the certificate was issued by the issuer")] + CheckCertificateIssuedByIssuer(#[source] crypto::x509::X509Error), #[error("cert chain validation failed -- subject and issuer mismatch")] CertChainSubjectIssuerMismatch, } @@ -251,7 +253,9 @@ fn validate_cert_chain( Err(CertificateChainValidationError::CertChainSignatureMismatch)? } - let issued = parent.issued(child); + let issued = parent + .issued(child) + .map_err(CertificateChainValidationError::CheckCertificateIssuedByIssuer)?; if !issued { Err(CertificateChainValidationError::CertChainSubjectIssuerMismatch)? } diff --git a/support/crypto/src/x509/mod.rs b/support/crypto/src/x509/mod.rs index 659f9b358a..524b417a99 100644 --- a/support/crypto/src/x509/mod.rs +++ b/support/crypto/src/x509/mod.rs @@ -44,7 +44,7 @@ impl X509Certificate { } /// Check if this certificate (acting as issuer) issued `subject`. - pub fn issued(&self, subject: &X509Certificate) -> bool { + pub fn issued(&self, subject: &X509Certificate) -> Result { self.0.issued(&subject.0) } diff --git a/support/crypto/src/x509/ossl.rs b/support/crypto/src/x509/ossl.rs index 0d0d6b65e0..9479156050 100644 --- a/support/crypto/src/x509/ossl.rs +++ b/support/crypto/src/x509/ossl.rs @@ -33,9 +33,16 @@ impl X509CertificateInner { .map_err(|e| err(e, "verifying certificate signature")) } - pub fn issued(&self, subject: &X509CertificateInner) -> bool { + pub fn issued(&self, subject: &X509CertificateInner) -> Result { let result = self.0.issued(&subject.0); - result == openssl::x509::X509VerifyResult::OK + match result { + openssl::x509::X509VerifyResult::OK => Ok(true), + openssl::x509::X509VerifyResult::APPLICATION_VERIFICATION => Ok(false), + _ => Err(err( + openssl::error::ErrorStack::get(), + "checking if certificate was issued by issuer", + )), + } } pub fn to_der(&self) -> Result, X509Error> { diff --git a/support/crypto/src/x509/symcrypt.rs b/support/crypto/src/x509/symcrypt.rs index 79f73f1be0..df2d379b50 100644 --- a/support/crypto/src/x509/symcrypt.rs +++ b/support/crypto/src/x509/symcrypt.rs @@ -72,8 +72,65 @@ impl X509CertificateInner { Ok(true) } - pub fn issued(&self, subject: &X509CertificateInner) -> bool { - self.0.tbs_certificate().subject() == subject.0.tbs_certificate().issuer() + pub fn issued(&self, subject: &X509CertificateInner) -> Result { + use x509_cert::ext::pkix::AuthorityKeyIdentifier; + use x509_cert::ext::pkix::KeyUsage; + use x509_cert::ext::pkix::SubjectKeyIdentifier; + use x509_cert::ext::pkix::name::GeneralName; + + let issuer_tbs = self.0.tbs_certificate(); + let subject_tbs = subject.0.tbs_certificate(); + + // The subject's issuer name must match the issuer's subject name. + if subject_tbs.issuer() != issuer_tbs.subject() { + return Ok(false); + } + + // If this certificate has a KeyUsage extension, it must permit + // signing other certificates. + let ku = issuer_tbs + .get_extension::() + .map_err(|e| der_err(e, "parsing KeyUsage extension"))?; + if let Some((_crit, ku)) = ku + && !ku.key_cert_sign() + { + return Ok(false); + } + + // If the subject carries an AuthorityKeyIdentifier, validate its + // populated fields against this certificate (the candidate issuer). + let akid = subject_tbs + .get_extension::() + .map_err(|e| der_err(e, "parsing AuthorityKeyIdentifier extension"))?; + if let Some((_crit, akid)) = akid { + if let Some(akid_key_id) = &akid.key_identifier { + let skid = issuer_tbs + .get_extension::() + .map_err(|e| der_err(e, "parsing SubjectKeyIdentifier extension"))?; + if let Some((_crit, ski)) = skid { + if akid_key_id != &ski.0 { + return Ok(false); + } + } + } + if let Some(akid_serial) = &akid.authority_cert_serial_number { + if akid_serial != issuer_tbs.serial_number() { + return Ok(false); + } + } + if let Some(gens) = &akid.authority_cert_issuer + && let Some(dn) = gens.iter().find_map(|g| match g { + GeneralName::DirectoryName(n) => Some(n), + _ => None, + }) + { + if dn != issuer_tbs.issuer() { + return Ok(false); + } + } + } + + Ok(true) } pub fn to_der(&self) -> Result, X509Error> { From c3204f39a1586d38c2b56b6df862c15e37be6124 Mon Sep 17 00:00:00 2001 From: Steven Malis Date: Fri, 8 May 2026 11:53:23 -0700 Subject: [PATCH 2/4] feedback --- support/crypto/src/x509/symcrypt.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/support/crypto/src/x509/symcrypt.rs b/support/crypto/src/x509/symcrypt.rs index df2d379b50..d36a252ea4 100644 --- a/support/crypto/src/x509/symcrypt.rs +++ b/support/crypto/src/x509/symcrypt.rs @@ -107,10 +107,13 @@ impl X509CertificateInner { let skid = issuer_tbs .get_extension::() .map_err(|e| der_err(e, "parsing SubjectKeyIdentifier extension"))?; - if let Some((_crit, ski)) = skid { - if akid_key_id != &ski.0 { - return Ok(false); + match skid { + Some((_crit, ski)) => { + if akid_key_id != &ski.0 { + return Ok(false); + } } + None => return Ok(false), } } if let Some(akid_serial) = &akid.authority_cert_serial_number { @@ -124,7 +127,7 @@ impl X509CertificateInner { _ => None, }) { - if dn != issuer_tbs.issuer() { + if dn != issuer_tbs.subject() { return Ok(false); } } From ecbd12ae0ab15a985a4028e069c3a150c5b5ab09 Mon Sep 17 00:00:00 2001 From: Steven Malis Date: Fri, 8 May 2026 13:39:05 -0700 Subject: [PATCH 3/4] restore openssl with comment --- support/crypto/src/x509/ossl.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/support/crypto/src/x509/ossl.rs b/support/crypto/src/x509/ossl.rs index 9479156050..8bb672e280 100644 --- a/support/crypto/src/x509/ossl.rs +++ b/support/crypto/src/x509/ossl.rs @@ -34,15 +34,12 @@ impl X509CertificateInner { } pub fn issued(&self, subject: &X509CertificateInner) -> Result { - let result = self.0.issued(&subject.0); - match result { - openssl::x509::X509VerifyResult::OK => Ok(true), - openssl::x509::X509VerifyResult::APPLICATION_VERIFICATION => Ok(false), - _ => Err(err( - openssl::error::ErrorStack::get(), - "checking if certificate was issued by issuer", - )), - } + // `X509_check_issued` only performs deterministic comparisons on + // already-parsed data (name, AKID/SKID, serial, KeyUsage) and cannot + // fail with internal errors. Per the OpenSSL docs, every non-OK + // result is an `X509_V_ERR*` constant "indicating why the issuer + // does not match" — i.e., a legitimate `Ok(false)`. + Ok(self.0.issued(&subject.0) == openssl::x509::X509VerifyResult::OK) } pub fn to_der(&self) -> Result, X509Error> { From b0294674008c31089a9e9ae1d44516f2c5eea1cc Mon Sep 17 00:00:00 2001 From: Steven Malis Date: Fri, 8 May 2026 13:49:32 -0700 Subject: [PATCH 4/4] feedback --- support/crypto/src/x509/symcrypt.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/support/crypto/src/x509/symcrypt.rs b/support/crypto/src/x509/symcrypt.rs index d36a252ea4..74ef7096c7 100644 --- a/support/crypto/src/x509/symcrypt.rs +++ b/support/crypto/src/x509/symcrypt.rs @@ -121,13 +121,16 @@ impl X509CertificateInner { return Ok(false); } } - if let Some(gens) = &akid.authority_cert_issuer - && let Some(dn) = gens.iter().find_map(|g| match g { - GeneralName::DirectoryName(n) => Some(n), - _ => None, - }) - { - if dn != issuer_tbs.subject() { + if let Some(gens) = &akid.authority_cert_issuer { + let mut has_dn = false; + let has_matching_dn = gens.iter().any(|g| match g { + GeneralName::DirectoryName(dn) => { + has_dn = true; + dn == issuer_tbs.subject() + } + _ => false, + }); + if has_dn && !has_matching_dn { return Ok(false); } }