Skip to content

Commit

Permalink
rust/pv: Support Armonk in IBM signing key subject
Browse files Browse the repository at this point in the history 
New IBM signing keys will have Armonk as locality in the subject.
Ensure that CRLs with Poughkeepsie as issuer locality are still
discovered if they are signed with the signing keys private key.
Also, drop the check for issuer/subject comparison and only rely on
validity period and cryptographic signatures.

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
  • Loading branch information
@steffen-eiden
steffen-eiden committed Mar 22, 2024
1 parent f6c6f0c commit 1a3d0b7
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 70 deletions.
58 changes: 46 additions & 12 deletions rust/pv/src/verify.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
// Copyright IBM Corp. 2023

use core::slice;
use log::debug;
use log::{debug, trace};
use openssl::error::ErrorStack;
use openssl::stack::Stack;
use openssl::x509::store::X509Store;
use openssl::x509::{CrlStatus, X509Ref, X509StoreContext, X509};
use openssl::x509::{CrlStatus, X509NameRef, X509Ref, X509StoreContext, X509StoreContextRef, X509};
use openssl_extensions::crl::{StackableX509Crl, X509StoreContextExtension, X509StoreExtension};

#[cfg(not(test))]
Expand Down Expand Up @@ -86,8 +87,8 @@ impl HkdVerifier for CertVerifier {
if verified_crls.is_empty() {
bail_hkd_verify!(NoCrl);
}
for crl in &verified_crls {
match crl.get_by_cert(&hkd.to_owned()) {
for crl in verified_crls {
match crl.get_by_serial(hkd.serial_number()) {
CrlStatus::NotRevoked => (),
_ => bail_hkd_verify!(HdkRevoked),
}
Expand All @@ -98,21 +99,54 @@ impl HkdVerifier for CertVerifier {
}

impl CertVerifier {
fn quirk_crls(
ctx: &mut X509StoreContextRef,
subject: &X509NameRef,
) -> Result<Stack<StackableX509Crl>, ErrorStack> {
match ctx.crls(subject) {
Ok(ret) if !ret.is_empty() => return Ok(ret),
_ => (),
}

// Armonk/Poughkeepsie fixup
trace!("quirk_crls: Try Locality");
if let Some(locality_subject) = helper::armonk_locality_fixup(subject) {
match ctx.crls(&locality_subject) {
Ok(ret) if !ret.is_empty() => return Ok(ret),
_ => (),
}

// reorder
trace!("quirk_crls: Try Locality+Reorder");
if let Ok(locality_ordered_subject) = helper::reorder_x509_names(&locality_subject) {
match ctx.crls(&locality_ordered_subject) {
Ok(ret) if !ret.is_empty() => return Ok(ret),
_ => (),
}
}
}

// reorder unchanged loaciliy subject
trace!("quirk_crls: Try Reorder");
if let Ok(ordered_subject) = helper::reorder_x509_names(subject) {
match ctx.crls(&ordered_subject) {
Ok(ret) if !ret.is_empty() => return Ok(ret),
_ => (),
}
}
// nothing found, return empty stack
Stack::new()
}

///Download the CLRs that a HKD refers to.
pub fn hkd_crls(&self, hkd: &X509Ref) -> Result<Stack<StackableX509Crl>> {
let mut ctx = X509StoreContext::new()?;
// Unfortunately we cannot use a dedicated function here and have to use a closure (E0434)
// Otherwise, we cannot refer to self
// Search for local CRLs
let mut crls = ctx.init_opt(&self.store, None, None, |ctx| {
let subject = self.ibm_z_sign_key.subject_name();
match ctx.crls(subject) {
Ok(crls) => Ok(crls),
_ => {
// reorder the name and try again
let broken_subj = helper::reorder_x509_names(subject)?;
ctx.crls(&broken_subj).or_else(helper::stack_err_hlp)
}
}
Self::quirk_crls(ctx, subject)
})?;

if !self.offline {
Expand Down
75 changes: 27 additions & 48 deletions rust/pv/src/verify/helper.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use openssl::{
error::ErrorStack,
nid::Nid,
ssl::SslFiletype,
stack::{Stack, Stackable},
stack::Stack,
x509::{
store::{File, X509Lookup, X509StoreBuilder, X509StoreRef},
verify::{X509VerifyFlags, X509VerifyParam},
Expand All @@ -20,6 +20,7 @@ use openssl::{
},
};
use openssl_extensions::akid::{AkidCheckResult, AkidExtension};
use std::str::from_utf8;
use std::{cmp::Ordering, ffi::c_int, usize};

/// Minimum security level for the keys/certificates used to establish a chain of
Expand All @@ -34,7 +35,6 @@ const SECURITY_CHAIN_MAX_LEN: c_int = 2;
/// verifies that the HKD
/// * has enough security bits
/// * is inside its validity period
/// * issuer name is the subject name of the [`sign_key`]
/// * the Authority Key ID matches the Signing Key ID of the [`sign_key`]
pub fn verify_hkd_options(hkd: &X509Ref, sign_key: &X509Ref) -> Result<()> {
let hk_pkey = hkd.public_key()?;
Expand All @@ -48,9 +48,6 @@ pub fn verify_hkd_options(hkd: &X509Ref, sign_key: &X509Ref) -> Result<()> {
// verify that the hkd is still valid
check_validity_period(hkd.not_before(), hkd.not_after())?;

// check if hkd.issuer_name == issuer.subject
check_x509_name_equal(sign_key.subject_name(), hkd.issuer_name())?;

// verify that the AKID of the hkd matches the SKID of the issuer
if let Some(akid) = hkd.akid() {
if akid.check(sign_key) != AkidCheckResult::OK {
Expand All @@ -70,9 +67,6 @@ pub fn verify_crl(crl: &X509CrlRef, issuer: &X509Ref) -> Option<()> {
return None;
}
}

check_x509_name_equal(crl.issuer_name(), issuer.subject_name()).ok()?;

match crl.verify(issuer.public_key().ok()?.as_ref()).ok()? {
true => Some(()),
false => None,
Expand Down Expand Up @@ -191,7 +185,8 @@ pub fn extract_ibm_sign_key(certs: Vec<X509>) -> Result<(X509, Stack<X509>)> {
//Asn1StringRef::as_slice aka ASN1_STRING_get0_data gives a string without \0 delimiter
const IBM_Z_COMMON_NAME: &[u8; 43usize] = b"International Business Machines Corporation";
const IBM_Z_COUNTRY_NAME: &[u8; 2usize] = b"US";
const IBM_Z_LOCALITY_NAME: &[u8; 12usize] = b"Poughkeepsie";
const IBM_Z_LOCALITY_NAME_POUGHKEEPSIE: &[u8; 12usize] = b"Poughkeepsie";
const IBM_Z_LOCALITY_NAME_ARMONK: &[u8; 6usize] = b"Armonk";
const IBM_Z_ORGANIZATIONAL_UNIT_NAME_SUFFIX: &str = "Key Signing Service";
const IBM_Z_ORGANIZATION_NAME: &[u8; 43usize] = b"International Business Machines Corporation";
const IBM_Z_STATE: &[u8; 8usize] = b"New York";
Expand All @@ -210,7 +205,8 @@ fn is_ibm_signing_cert(cert: &X509) -> bool {
if subj.entries().count() != IMB_Z_ENTRY_COUNT
|| !name_data_eq(subj, Nid::COUNTRYNAME, IBM_Z_COUNTRY_NAME)
|| !name_data_eq(subj, Nid::STATEORPROVINCENAME, IBM_Z_STATE)
|| !name_data_eq(subj, Nid::LOCALITYNAME, IBM_Z_LOCALITY_NAME)
|| !(name_data_eq(subj, Nid::LOCALITYNAME, IBM_Z_LOCALITY_NAME_POUGHKEEPSIE)
|| name_data_eq(subj, Nid::LOCALITYNAME, IBM_Z_LOCALITY_NAME_ARMONK))
|| !name_data_eq(subj, Nid::ORGANIZATIONNAME, IBM_Z_ORGANIZATION_NAME)
|| !name_data_eq(subj, Nid::COMMONNAME, IBM_Z_COMMON_NAME)
{
Expand Down Expand Up @@ -354,24 +350,6 @@ fn check_validity_period(not_before: &Asn1TimeRef, not_after: &Asn1TimeRef) -> R
}
}

fn check_x509_name_equal(lhs: &X509NameRef, rhs: &X509NameRef) -> Result<()> {
if lhs.entries().count() != rhs.entries().count() {
bail_hkd_verify!(IssuerMismatch);
}

for l in lhs.entries() {
// search for the matching value in the rhs names
// found none? -> names are not equal
if !rhs
.entries()
.any(|r| l.data().as_slice() == r.data().as_slice())
{
bail_hkd_verify!(IssuerMismatch);
}
}
Ok(())
}

const NIDS_CORRECT_ORDER: [Nid; 6] = [
Nid::COUNTRYNAME,
Nid::ORGANIZATIONNAME,
Expand All @@ -394,13 +372,28 @@ pub fn reorder_x509_names(subject: &X509NameRef) -> std::result::Result<X509Name
Ok(correct_subj.build())
}

pub fn stack_err_hlp<T: Stackable>(
e: ErrorStack,
) -> std::result::Result<Stack<T>, openssl::error::ErrorStack> {
match e.errors().len() {
0 => Stack::<T>::new(),
_ => Err(e),
/**
* Workaround for potential locality mismatches between CRLs and Certs
* # Return
* fixed subject or none if locality was not Armonk or any OpenSSL error
*/
pub fn armonk_locality_fixup(subject: &X509NameRef) -> Option<X509Name> {
if !name_data_eq(subject, Nid::LOCALITYNAME, IBM_Z_LOCALITY_NAME_ARMONK) {
return None;
}

let mut ret = X509Name::builder().ok()?;
for entry in subject.entries() {
match entry.object().nid() {
nid @ Nid::LOCALITYNAME => ret
.append_entry_by_nid(nid, from_utf8(IBM_Z_LOCALITY_NAME_POUGHKEEPSIE).ok()?)
.ok()?,
_ => {
ret.append_entry(entry).ok()?;
}
}
}
Some(ret.build())
}

#[cfg(test)]
Expand Down Expand Up @@ -436,20 +429,6 @@ mod test {
));
}

#[test]
fn x509_name_equal() {
let sign_crt = load_gen_cert("ibm.crt");
let hkd = load_gen_cert("host.crt");
let other = load_gen_cert("inter_ca.crt");

assert!(super::check_x509_name_equal(sign_crt.subject_name(), hkd.issuer_name()).is_ok(),);

assert!(matches!(
super::check_x509_name_equal(other.subject_name(), hkd.subject_name()),
Err(Error::HkdVerify(IssuerMismatch))
));
}

#[test]
fn is_ibm_z_sign_key() {
let ibm_crt = load_gen_cert("ibm.crt");
Expand Down
44 changes: 34 additions & 10 deletions rust/pv/src/verify/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,15 @@ fn dist_points() {
assert_eq!(res, exp);
}

fn verify(offline: bool, ibm_crt: &'static str, ibm_crl: &'static str) {
fn verify(offline: bool, ibm_crt: &'static str, ibm_crl: &'static str, hkd: &'static str) {
let root_crt = get_cert_asset_path_string("root_ca.chained.crt");
let inter_crt = get_cert_asset_path_string("inter_ca.crt");
let inter_crl = get_cert_asset_path_string("inter_ca.crl");
let ibm_crt = get_cert_asset_path_string(ibm_crt);
let ibm_crl = get_cert_asset_path_string(ibm_crl);
let hkd_revoked = load_gen_cert("host_rev.crt");
let hkd_inv = load_gen_cert("host_invalid_signing_key.crt");
let hkd_exp = load_gen_cert("host_crt_expired.crt");
let hkd = load_gen_cert("host.crt");
let hkd = load_gen_cert(hkd);

let crls = &[ibm_crl, inter_crl];
let verifier = CertVerifier::new(
Expand All @@ -102,11 +101,6 @@ fn verify(offline: bool, ibm_crt: &'static str, ibm_crl: &'static str) {
Err(Error::HkdVerify(HdkRevoked))
));

assert!(matches!(
verifier.verify(&hkd_inv),
Err(Error::HkdVerify(IssuerMismatch))
));

assert!(matches!(
verifier.verify(&hkd_exp),
Err(Error::HkdVerify(AfterValidity))
Expand All @@ -115,10 +109,40 @@ fn verify(offline: bool, ibm_crt: &'static str, ibm_crl: &'static str) {

#[test]
fn verify_online() {
verify(false, "ibm.crt", "ibm.crl")
verify(false, "ibm.crt", "ibm.crl", "host.crt")
}

#[test]
fn verify_offline() {
verify(true, "ibm.crt", "ibm.crl")
verify(true, "ibm.crt", "ibm.crl", "host.crt")
}

#[test]
fn verify_armonk_crt_online() {
verify(false, "ibm_armonk.crt", "ibm.crl", "host.crt")
}

#[test]
fn verify_armonk_crt_offline() {
verify(true, "ibm_armonk.crt", "ibm.crl", "host.crt")
}

#[test]
fn verify_armonk_crl_online() {
verify(false, "ibm_armonk.crt", "ibm_armonk.crl", "host.crt")
}

#[test]
fn verify_armonk_crl_offline() {
verify(true, "ibm_armonk.crt", "ibm_armonk.crl", "host.crt")
}

#[test]
fn verify_armonk_hkd_online() {
verify(false, "ibm_armonk.crt", "ibm_armonk.crl", "host_armonk.crt")
}

#[test]
fn verify_armonk_hkd_offline() {
verify(true, "ibm_armonk.crt", "ibm_armonk.crl", "host_armonk.crt")
}

0 comments on commit 1a3d0b7

Please sign in to comment.