Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logic to verify and get signing key from chain #70

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions verifier/src/x509.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// TODO: Remove dead_code exception once this is connected up to the rest of the codebase
#![allow(dead_code)]
mod certs;
mod chain;
mod crl;
mod error;

Expand Down
115 changes: 92 additions & 23 deletions verifier/src/x509/certs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

//! Verifier(s) for [`CertificationData`](`mc-sgx-dcap-types::CertificationData`).

extern crate alloc;

use super::{Error, Result};
use alloc::vec::Vec;
use core::time::Duration;
use p256::ecdsa::signature::Verifier;
use p256::ecdsa::{Signature, VerifyingKey};
Expand All @@ -11,28 +14,36 @@ use x509_cert::Certificate as X509Certificate;

/// A certificate whose signature has not been verified.
#[derive(Debug, PartialEq, Eq)]
pub struct UnverifiedCertificate<'a> {
pub struct UnverifiedCertificate {
// In order to verify the signature, we need to access the original DER
// bytes
der_bytes: &'a [u8],
certificate: X509Certificate,
der_bytes: Vec<u8>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got rid of the lifetimes here, it prevented the PEM decoding in the code, since pem generates a vector, and since this logic inherently needs alloc behind the scenes it doesn't seem like we gain much trying to minimize vector usage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

pub(crate) certificate: X509Certificate,
// The signature and key are persisted here since they are fallible
// operations and it's more ergonomic to fail fast than fail later for a
// bad key or signature
signature: Signature,
pub(crate) key: VerifyingKey,
key: VerifyingKey,
}

/// A certificate whose signature has been verified.
#[derive(Debug, PartialEq, Eq)]
pub struct VerifiedCertificate<'a> {
_der_bytes: &'a [u8],
pub struct VerifiedCertificate {
_certificate: X509Certificate,
_signature: Signature,
_key: VerifyingKey,
key: VerifyingKey,
}

impl VerifiedCertificate {
pub(crate) fn public_key(&self) -> VerifyingKey {
self.key
}
}

impl<'a> UnverifiedCertificate<'a> {
impl UnverifiedCertificate {
pub fn verify_self_signed(&self, unix_time: Duration) -> Result<VerifiedCertificate> {
self.verify(&self.key, unix_time)
}

/// Verify the certificate signature and time are valid.
///
/// # Arguments
Expand All @@ -44,19 +55,13 @@ impl<'a> UnverifiedCertificate<'a> {
/// SystemTime::now().duration_since(UNIX_EPOCH)
/// ```
/// or equivalent
pub fn verify(
self,
key: &VerifyingKey,
unix_time: Duration,
) -> Result<VerifiedCertificate<'a>> {
pub fn verify(&self, key: &VerifyingKey, unix_time: Duration) -> Result<VerifiedCertificate> {
self.verify_time(unix_time)?;
self.verify_signature(key)?;

Ok(VerifiedCertificate {
_der_bytes: self.der_bytes,
_certificate: self.certificate,
_signature: self.signature,
_key: self.key,
_certificate: self.certificate.clone(),
key: self.key,
})
}

Expand Down Expand Up @@ -91,11 +96,21 @@ impl<'a> UnverifiedCertificate<'a> {
}
}

/// Convert a PEM-encoded certificate into an [`UnverifiedCertificate`].
impl TryFrom<&str> for UnverifiedCertificate {
type Error = Error;

fn try_from(pem: &str) -> ::core::result::Result<Self, Self::Error> {
let (_, der_bytes) = pem_rfc7468::decode_vec(pem.as_bytes())?;
Self::try_from(&der_bytes[..])
}
}

/// Convert a DER-encoded certificate into an [`UnverifiedCertificate`].
impl<'a> TryFrom<&'a [u8]> for UnverifiedCertificate<'a> {
impl TryFrom<&[u8]> for UnverifiedCertificate {
type Error = Error;

fn try_from(der_bytes: &'a [u8]) -> ::core::result::Result<Self, Self::Error> {
fn try_from(der_bytes: &[u8]) -> ::core::result::Result<Self, Self::Error> {
let certificate = X509Certificate::from_der(der_bytes)?;
let signature_bytes = certificate
.signature
Expand All @@ -113,7 +128,7 @@ impl<'a> TryFrom<&'a [u8]> for UnverifiedCertificate<'a> {
)
.map_err(|_| Error::KeyDecoding)?;
Ok(UnverifiedCertificate {
der_bytes,
der_bytes: der_bytes.to_vec(),
certificate,
signature,
key,
Expand All @@ -123,16 +138,35 @@ impl<'a> TryFrom<&'a [u8]> for UnverifiedCertificate<'a> {

#[cfg(test)]
mod test {
extern crate alloc;

use super::*;
use alloc::string::ToString;
use const_oid::ObjectIdentifier;
use yare::parameterized;

const LEAF_CERT: &str = include_str!("../../data/tests/leaf_cert.pem");
const PROCESSOR_CA: &str = include_str!("../../data/tests/processor_ca.pem");
const ROOT_CA: &str = include_str!("../../data/tests/root_ca.pem");

#[parameterized(
root = { ROOT_CA },
processor = { PROCESSOR_CA },
leaf = { LEAF_CERT },
)]
Comment on lines +150 to +154
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why parametrize these values if there's only one value for each param? Consider removing this usage if there isn't a good reason...

Copy link
Collaborator Author

@nick-mobilecoin nick-mobilecoin May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. Parameterized tests allow one to re-use the same testing behavior with different inputs(parameters). The number of parameters per test seems orthogonal to that use.

For this specific case I'll admit it doesn't gain much, other than if one wants to add another certificate to try it's one line vs 4 and doesn't require as much thought on the naming.
For example the explicit creation of dedicated tests would be something like:

    fn try_from_root_ca() {
        assert!(UnverifiedCertificate::try_from(ROOT_CA).is_ok());
    }
    
    fn try_from_processor_ca() {
        assert!(UnverifiedCertificate::try_from(PROCESSOR_CA).is_ok());
    }

    fn try_from_leaf_cert() {
        assert!(UnverifiedCertificate::try_from(LEAF_CERT).is_ok());
    }

fn try_from_pem(pem: &str) {
assert!(UnverifiedCertificate::try_from(pem).is_ok());
}

#[test]
fn try_from_bad_pem_errors() {
let pem = ROOT_CA.to_string();
let bad_pem = pem.replace("-----END CERTIFICATE-----", "");

assert!(matches!(
UnverifiedCertificate::try_from(bad_pem.as_str()),
Err(Error::PemDecoding(_))
));
}

#[parameterized(
root = { ROOT_CA },
processor = { PROCESSOR_CA },
Expand Down Expand Up @@ -381,4 +415,39 @@ mod test {
Err(Error::CertificateExpired)
);
}

#[test]
fn verify_self_signed_root_ca() {
let root_cert = UnverifiedCertificate::try_from(ROOT_CA)
.expect("Failed to decode certificate from PEM");

let unix_time = root_cert
.certificate
.tbs_certificate
.validity
.not_after
.to_unix_duration();

assert_eq!(root_cert.verify_self_signed(unix_time).is_ok(), true);
}

#[test]
fn verify_self_signed_root_ca_fails_when_expired() {
let root_cert = UnverifiedCertificate::try_from(ROOT_CA)
.expect("Failed to decode certificate from PEM");

let mut unix_time = root_cert
.certificate
.tbs_certificate
.validity
.not_after
.to_unix_duration();

unix_time += Duration::new(0, 1);

assert_eq!(
root_cert.verify_self_signed(unix_time),
Err(Error::CertificateExpired)
);
}
}
174 changes: 174 additions & 0 deletions verifier/src/x509/chain.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
// Copyright (c) 2023 The MobileCoin Foundation

//! Support for verifying certificate chains

extern crate alloc;

use super::certs::UnverifiedCertificate;
use super::Result;
use crate::x509::certs::VerifiedCertificate;
use alloc::vec::Vec;
use core::time::Duration;
use p256::ecdsa::VerifyingKey;

/// An X509 certificate chain. This is a valid path from the trust root to the
/// leaf certificate.
pub struct CertificateChain {
certificates: Vec<UnverifiedCertificate>,
}

impl CertificateChain {
/// Create a new certificate chain from a path from a trust root to the
/// leaf certificate.
///
/// A certificate chain without a valid path will result in errors for
/// functions like [`CertificateChain::signing_key`].
pub fn new(certificates: Vec<UnverifiedCertificate>) -> Self {
Self { certificates }
}

/// Returning the signing key from the leaf certificate
///
/// The chain will be verified against the `trust_root` and the `unix_time`.
pub fn signing_key(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be passed a collection of CRLs added a bullet item to #50 to keep track of this

&self,
trust_root: VerifiedCertificate,
unix_time: Duration,
) -> Result<VerifyingKey> {
Comment on lines +30 to +37
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be doing too much, but TBH I'm still on the fence on the "unverified***"->"verified***" logic I have for certs and CRLs.

let mut key = trust_root.public_key();
for cert in &self.certificates {
let verified_cert = cert.verify(&key, unix_time)?;
key = verified_cert.public_key();
}
Ok(key)
}
}

#[cfg(test)]
mod test {
Comment on lines +47 to +48
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is going to need some more robust testing, thinking of grabbing some of the test vectors from https://csrc.nist.gov/projects/pki-testing

use super::super::Error;
use super::*;

use x509_cert::der::Decode;
use x509_cert::Certificate as X509Certificate;
use yare::parameterized;

const LEAF_CERT: &str = include_str!("../../data/tests/leaf_cert.pem");
const PROCESSOR_CA: &str = include_str!("../../data/tests/processor_ca.pem");
const ROOT_CA: &str = include_str!("../../data/tests/root_ca.pem");

fn key_and_start_time(cert: &str) -> (VerifyingKey, Duration) {
let (_, der_bytes) = pem_rfc7468::decode_vec(cert.as_bytes()).expect("Failed decoding PEM");
let cert = X509Certificate::from_der(der_bytes.as_slice()).expect("Falied decoding DER");

// The leaf certificate should have the narrowest time range.
let unix_time = cert.tbs_certificate.validity.not_before.to_unix_duration();
let key = VerifyingKey::from_sec1_bytes(
cert.tbs_certificate
.subject_public_key_info
.subject_public_key
.as_bytes()
.expect("Failed decoding key"),
)
.expect("Failed decoding key");

(key, unix_time)
}

#[parameterized(
full_chain = { &[ROOT_CA, PROCESSOR_CA, LEAF_CERT] },
to_intermediate = { &[ROOT_CA, PROCESSOR_CA] },
only_root = { &[ROOT_CA] },
)]
fn signing_key_from_certificate_chain(pem_chain: &[&str]) {
let certs = pem_chain
.iter()
.map(|pem| UnverifiedCertificate::try_from(*pem).expect("Failed decoding pem"))
.collect::<Vec<_>>();

let end = pem_chain
.last()
.expect("Should be at least one certificate");
let (expected_key, unix_time) = key_and_start_time(end);

let chain = CertificateChain::new(certs);
let unverified_root =
UnverifiedCertificate::try_from(ROOT_CA).expect("Failed decoding pem");
let root = unverified_root
.verify_self_signed(unix_time)
.expect("Failed verifying root certificate");

let signing_key = chain
.signing_key(root, unix_time)
.expect("Failed getting signing key");
assert_eq!(signing_key, expected_key);
}

#[test]
fn signing_key_fails_when_outside_valid_time() {
let pem_chain = [ROOT_CA, PROCESSOR_CA, LEAF_CERT];
let certs = pem_chain
.iter()
.map(|pem| UnverifiedCertificate::try_from(*pem).expect("Failed decoding pem"))
.collect::<Vec<_>>();

let (_, mut unix_time) = key_and_start_time(LEAF_CERT);

unix_time -= Duration::from_nanos(1);

let chain = CertificateChain::new(certs);
let unverified_root =
UnverifiedCertificate::try_from(ROOT_CA).expect("Failed decoding pem");
let root = unverified_root
.verify_self_signed(unix_time)
.expect("Failed verifying root certificate");
assert_eq!(
chain.signing_key(root, unix_time),
Err(Error::CertificateNotYetValid)
);
}

#[test]
fn cert_chain_out_of_order_fails() {
let pem_chain = [ROOT_CA, LEAF_CERT, PROCESSOR_CA];
let certs = pem_chain
.iter()
.map(|pem| UnverifiedCertificate::try_from(*pem).expect("Failed decoding pem"))
.collect::<Vec<_>>();

let (_, unix_time) = key_and_start_time(LEAF_CERT);

let chain = CertificateChain::new(certs);
let unverified_root =
UnverifiedCertificate::try_from(ROOT_CA).expect("Failed decoding pem");
let root = unverified_root
.verify_self_signed(unix_time)
.expect("Failed verifying root certificate");
assert_eq!(
chain.signing_key(root, unix_time),
Err(Error::SignatureVerification)
);
}

#[test]
fn cert_chain_missing_intermediate_ca_fails() {
let pem_chain = [ROOT_CA, LEAF_CERT];
let certs = pem_chain
.iter()
.map(|pem| UnverifiedCertificate::try_from(*pem).expect("Failed decoding pem"))
.collect::<Vec<_>>();

let (_, unix_time) = key_and_start_time(LEAF_CERT);

let chain = CertificateChain::new(certs);
let unverified_root =
UnverifiedCertificate::try_from(ROOT_CA).expect("Failed decoding pem");
let root = unverified_root
.verify_self_signed(unix_time)
.expect("Failed verifying root certificate");
assert_eq!(
chain.signing_key(root, unix_time),
Err(Error::SignatureVerification)
);
}
}
Loading
Loading