From 3b39f657cb2a694e8fc289958ca723af7667c755 Mon Sep 17 00:00:00 2001 From: Alex Cameron Date: Sat, 28 Oct 2023 12:44:18 +1100 Subject: [PATCH] validation: add Rust-side extension validation helpers (#9781) * validation: add Rust-side extension validation helpers * rust: Add unit tests for criticality and basic extension policy flow * rust: Remove validators, these can be in a separate PR * rust: Fix comment * rust: Collapse criticality unit tests * rust: Test case where maybe present validator detects incorrect criticality * rust: Remove unused `PolicyError` variants * rust: Add unit test exercising formatting of `DuplicateExtensionsError` * rust: Remove the need for printing `DuplicateExtensionsError` --- .../src/policy/extension.rs | 400 ++++++++++++++++++ .../src/policy/mod.rs | 6 + src/rust/cryptography-x509/src/extensions.rs | 2 +- 3 files changed, 407 insertions(+), 1 deletion(-) create mode 100644 src/rust/cryptography-x509-validation/src/policy/extension.rs diff --git a/src/rust/cryptography-x509-validation/src/policy/extension.rs b/src/rust/cryptography-x509-validation/src/policy/extension.rs new file mode 100644 index 000000000000..06d88c4e3ad7 --- /dev/null +++ b/src/rust/cryptography-x509-validation/src/policy/extension.rs @@ -0,0 +1,400 @@ +// This file is dual licensed under the terms of the Apache License, Version +// 2.0, and the BSD License. See the LICENSE file in the root of this repository +// for complete details. + +use asn1::ObjectIdentifier; +use cryptography_x509::{ + certificate::Certificate, + extensions::{Extension, Extensions}, +}; + +use crate::ops::CryptoOps; + +use super::{Policy, PolicyError}; + +// TODO: Remove `dead_code` attributes once we start using these helpers. + +/// Represents different criticality states for an extension. +#[allow(dead_code)] +pub(crate) enum Criticality { + /// The extension MUST be marked as critical. + Critical, + /// The extension MAY be marked as critical. + Agnostic, + /// The extension MUST NOT be marked as critical. + NonCritical, +} + +#[allow(dead_code)] +impl Criticality { + pub(crate) fn permits(&self, critical: bool) -> bool { + match (self, critical) { + (Criticality::Critical, true) => true, + (Criticality::Critical, false) => false, + (Criticality::Agnostic, _) => true, + (Criticality::NonCritical, true) => false, + (Criticality::NonCritical, false) => true, + } + } +} + +#[allow(dead_code)] +type PresentExtensionValidatorCallback = + fn(&Policy<'_, B>, &Certificate<'_>, &Extension<'_>) -> Result<(), PolicyError>; + +#[allow(dead_code)] +type MaybeExtensionValidatorCallback = + fn(&Policy<'_, B>, &Certificate<'_>, Option<&Extension<'_>>) -> Result<(), PolicyError>; + +/// Represents different validation states for an extension. +#[allow(dead_code)] +pub(crate) enum ExtensionValidator { + /// The extension MUST NOT be present. + NotPresent, + /// The extension MUST be present. + Present { + /// The extension's criticality. + criticality: Criticality, + /// An optional validator over the extension's inner contents, with + /// the surrounding `Policy` as context. + validator: Option>, + }, + /// The extension MAY be present; the interior validator is + /// always called if supplied, including if the extension is not present. + MaybePresent { + criticality: Criticality, + validator: Option>, + }, +} + +/// A "policy" for validating a specific X.509v3 extension, identified by +/// its OID. +#[allow(dead_code)] +pub(crate) struct ExtensionPolicy { + pub(crate) oid: asn1::ObjectIdentifier, + pub(crate) validator: ExtensionValidator, +} + +#[allow(dead_code)] +impl ExtensionPolicy { + pub(crate) fn not_present(oid: ObjectIdentifier) -> Self { + Self { + oid, + validator: ExtensionValidator::NotPresent, + } + } + + pub(crate) fn present( + oid: ObjectIdentifier, + criticality: Criticality, + validator: Option>, + ) -> Self { + Self { + oid, + validator: ExtensionValidator::Present { + criticality, + validator, + }, + } + } + + pub(crate) fn maybe_present( + oid: ObjectIdentifier, + criticality: Criticality, + validator: Option>, + ) -> Self { + Self { + oid, + validator: ExtensionValidator::MaybePresent { + criticality, + validator, + }, + } + } + + pub(crate) fn permits( + &self, + policy: &Policy<'_, B>, + cert: &Certificate<'_>, + extensions: &Extensions<'_>, + ) -> Result<(), PolicyError> { + match (&self.validator, extensions.get_extension(&self.oid)) { + // Extension MUST NOT be present and isn't; OK. + (ExtensionValidator::NotPresent, None) => Ok(()), + // Extension MUST NOT be present but is; NOT OK. + (ExtensionValidator::NotPresent, Some(_)) => Err(PolicyError::Other( + "EE certificate contains prohibited extension", + )), + // Extension MUST be present but is not; NOT OK. + (ExtensionValidator::Present { .. }, None) => Err(PolicyError::Other( + "EE certificate is missing required extension", + )), + // Extension MUST be present and is; check it. + ( + ExtensionValidator::Present { + criticality, + validator, + }, + Some(extn), + ) => { + if !criticality.permits(extn.critical) { + return Err(PolicyError::Other( + "EE certificate extension has incorrect criticality", + )); + } + + // If a custom validator is supplied, apply it. + validator.map_or(Ok(()), |v| v(policy, cert, &extn)) + } + // Extension MAY be present. + ( + ExtensionValidator::MaybePresent { + criticality, + validator, + }, + extn, + ) => { + // If the extension is present, apply our criticality check. + if extn + .as_ref() + .map_or(false, |extn| !criticality.permits(extn.critical)) + { + return Err(PolicyError::Other( + "EE certificate extension has incorrect criticality", + )); + } + + // If a custom validator is supplied, apply it. + validator.map_or(Ok(()), |v| v(policy, cert, extn.as_ref())) + } + } + } +} + +#[cfg(test)] +mod tests { + use super::{Criticality, ExtensionPolicy}; + use crate::ops::tests::{cert, v1_cert_pem, NullOps}; + use crate::ops::CryptoOps; + use crate::policy::{Policy, PolicyError}; + use asn1::{ObjectIdentifier, SimpleAsn1Writable}; + use cryptography_x509::certificate::Certificate; + use cryptography_x509::extensions::{BasicConstraints, Extension, Extensions}; + use cryptography_x509::oid::BASIC_CONSTRAINTS_OID; + + #[test] + fn test_criticality_variants() { + let criticality = Criticality::Critical; + assert!(criticality.permits(true)); + assert!(!criticality.permits(false)); + + let criticality = Criticality::Agnostic; + assert!(criticality.permits(true)); + assert!(criticality.permits(false)); + + let criticality = Criticality::NonCritical; + assert!(!criticality.permits(true)); + assert!(criticality.permits(false)); + } + + fn epoch() -> asn1::DateTime { + asn1::DateTime::new(1970, 1, 1, 0, 0, 0).unwrap() + } + + fn create_encoded_extensions( + oid: ObjectIdentifier, + critical: bool, + ext: &T, + ) -> Vec { + let ext_value = asn1::write_single(&ext).unwrap(); + let exts = vec![Extension { + extn_id: oid, + critical, + extn_value: &ext_value, + }]; + let der_exts = asn1::write_single(&asn1::SequenceOfWriter::new(exts)).unwrap(); + der_exts + } + + fn create_empty_encoded_extensions() -> Vec { + let exts: Vec> = vec![]; + let der_exts = asn1::write_single(&asn1::SequenceOfWriter::new(exts)).unwrap(); + der_exts + } + + fn present_extension_validator( + _policy: &Policy<'_, B>, + _cert: &Certificate<'_>, + _ext: &Extension<'_>, + ) -> Result<(), PolicyError> { + Ok(()) + } + + #[test] + fn test_extension_policy_present() { + // The certificate doesn't get used for this validator, so the certificate we use isn't important. + let cert_pem = v1_cert_pem(); + let cert = cert(&cert_pem); + let ops = NullOps {}; + let policy = Policy::new(ops, None, epoch()); + + // Test a policy that stipulates that a given extension MUST be present. + let extension_policy = ExtensionPolicy::present( + BASIC_CONSTRAINTS_OID, + Criticality::Critical, + Some(present_extension_validator), + ); + + // Check the case where the extension is present. + let bc = BasicConstraints { + ca: true, + path_length: Some(3), + }; + let der_exts = create_encoded_extensions(BASIC_CONSTRAINTS_OID, true, &bc); + let raw_exts = asn1::parse_single(&der_exts).unwrap(); + let exts = Extensions::from_raw_extensions(Some(&raw_exts)) + .ok() + .unwrap(); + assert!(extension_policy.permits(&policy, &cert, &exts).is_ok()); + + // Check the case where the extension isn't present. + let der_exts: Vec = create_empty_encoded_extensions(); + let raw_exts = asn1::parse_single(&der_exts).unwrap(); + let exts = Extensions::from_raw_extensions(Some(&raw_exts)) + .ok() + .unwrap(); + assert!(extension_policy.permits(&policy, &cert, &exts).is_err()); + } + + fn maybe_extension_validator( + _policy: &Policy<'_, B>, + _cert: &Certificate<'_>, + _ext: Option<&Extension<'_>>, + ) -> Result<(), PolicyError> { + Ok(()) + } + + #[test] + fn test_extension_policy_maybe() { + // The certificate doesn't get used for this validator, so the certificate we use isn't important. + let cert_pem = v1_cert_pem(); + let cert = cert(&cert_pem); + let ops = NullOps {}; + let policy = Policy::new(ops, None, epoch()); + + // Test a policy that stipulates that a given extension CAN be present. + let extension_policy = ExtensionPolicy::maybe_present( + BASIC_CONSTRAINTS_OID, + Criticality::Critical, + Some(maybe_extension_validator), + ); + + // Check the case where the extension is present. + let bc = BasicConstraints { + ca: false, + path_length: Some(3), + }; + let der_exts = create_encoded_extensions(BASIC_CONSTRAINTS_OID, true, &bc); + let raw_exts = asn1::parse_single(&der_exts).unwrap(); + let exts = Extensions::from_raw_extensions(Some(&raw_exts)) + .ok() + .unwrap(); + assert!(extension_policy.permits(&policy, &cert, &exts).is_ok()); + + // Check the case where the extension isn't present. + let der_exts: Vec = create_empty_encoded_extensions(); + let raw_exts = asn1::parse_single(&der_exts).unwrap(); + let exts = Extensions::from_raw_extensions(Some(&raw_exts)) + .ok() + .unwrap(); + assert!(extension_policy.permits(&policy, &cert, &exts).is_ok()); + } + + #[test] + fn test_extension_policy_not_present() { + // The certificate doesn't get used for this validator, so the certificate we use isn't important. + let cert_pem = v1_cert_pem(); + let cert = cert(&cert_pem); + let ops = NullOps {}; + let policy = Policy::new(ops, None, epoch()); + + // Test a policy that stipulates that a given extension MUST NOT be present. + let extension_policy = ExtensionPolicy::not_present(BASIC_CONSTRAINTS_OID); + + // Check the case where the extension is present. + let bc = BasicConstraints { + ca: false, + path_length: Some(3), + }; + let der_exts = create_encoded_extensions(BASIC_CONSTRAINTS_OID, true, &bc); + let raw_exts = asn1::parse_single(&der_exts).unwrap(); + let exts = Extensions::from_raw_extensions(Some(&raw_exts)) + .ok() + .unwrap(); + assert!(extension_policy.permits(&policy, &cert, &exts).is_err()); + + // Check the case where the extension isn't present. + let der_exts: Vec = create_empty_encoded_extensions(); + let raw_exts = asn1::parse_single(&der_exts).unwrap(); + let exts = Extensions::from_raw_extensions(Some(&raw_exts)) + .ok() + .unwrap(); + assert!(extension_policy.permits(&policy, &cert, &exts).is_ok()); + } + + #[test] + fn test_extension_policy_present_incorrect_criticality() { + // The certificate doesn't get used for this validator, so the certificate we use isn't important. + let cert_pem = v1_cert_pem(); + let cert = cert(&cert_pem); + let ops = NullOps {}; + let policy = Policy::new(ops, None, epoch()); + + // Test a present policy that stipulates that a given extension MUST be critical. + let extension_policy = ExtensionPolicy::present( + BASIC_CONSTRAINTS_OID, + Criticality::Critical, + Some(present_extension_validator), + ); + + // Mark the extension as non-critical despite our policy stipulating that it must be critical. + let bc = BasicConstraints { + ca: true, + path_length: Some(3), + }; + let der_exts = create_encoded_extensions(BASIC_CONSTRAINTS_OID, false, &bc); + let raw_exts = asn1::parse_single(&der_exts).unwrap(); + let exts = Extensions::from_raw_extensions(Some(&raw_exts)) + .ok() + .unwrap(); + assert!(extension_policy.permits(&policy, &cert, &exts).is_err()); + } + + #[test] + fn test_extension_policy_maybe_present_incorrect_criticality() { + // The certificate doesn't get used for this validator, so the certificate we use isn't important. + let cert_pem = v1_cert_pem(); + let cert = cert(&cert_pem); + let ops = NullOps {}; + let policy = Policy::new(ops, None, epoch()); + + // Test a maybe present policy that stipulates that a given extension MUST be critical. + let extension_policy = ExtensionPolicy::maybe_present( + BASIC_CONSTRAINTS_OID, + Criticality::Critical, + Some(maybe_extension_validator), + ); + + // Mark the extension as non-critical despite our policy stipulating that it must be critical. + let bc = BasicConstraints { + ca: true, + path_length: Some(3), + }; + let der_exts = create_encoded_extensions(BASIC_CONSTRAINTS_OID, false, &bc); + let raw_exts = asn1::parse_single(&der_exts).unwrap(); + let exts = Extensions::from_raw_extensions(Some(&raw_exts)) + .ok() + .unwrap(); + assert!(extension_policy.permits(&policy, &cert, &exts).is_err()); + } +} diff --git a/src/rust/cryptography-x509-validation/src/policy/mod.rs b/src/rust/cryptography-x509-validation/src/policy/mod.rs index b9bc437901b3..725020c6a2b6 100644 --- a/src/rust/cryptography-x509-validation/src/policy/mod.rs +++ b/src/rust/cryptography-x509-validation/src/policy/mod.rs @@ -2,6 +2,8 @@ // 2.0, and the BSD License. See the LICENSE file in the root of this repository // for complete details. +mod extension; + use std::collections::HashSet; use asn1::ObjectIdentifier; @@ -111,6 +113,10 @@ const RFC5280_CRITICAL_CA_EXTENSIONS: &[asn1::ObjectIdentifier] = const RFC5280_CRITICAL_EE_EXTENSIONS: &[asn1::ObjectIdentifier] = &[BASIC_CONSTRAINTS_OID, SUBJECT_ALTERNATIVE_NAME_OID]; +pub enum PolicyError { + Other(&'static str), +} + /// Represents a logical certificate "subject," i.e. a principal matching /// one of the names listed in a certificate's `subjectAltNames` extension. pub enum Subject<'a> { diff --git a/src/rust/cryptography-x509/src/extensions.rs b/src/rust/cryptography-x509/src/extensions.rs index f4deb7c8451f..fd7a3aaa0a3a 100644 --- a/src/rust/cryptography-x509/src/extensions.rs +++ b/src/rust/cryptography-x509/src/extensions.rs @@ -295,7 +295,7 @@ impl KeyUsage<'_> { mod tests { use crate::oid::{AUTHORITY_KEY_IDENTIFIER_OID, BASIC_CONSTRAINTS_OID}; - use super::{BasicConstraints, Extension, Extensions, KeyUsage}; + use super::{BasicConstraints, DuplicateExtensionsError, Extension, Extensions, KeyUsage}; #[test] fn test_get_extension() {