From e5bb52440854c652ff3ca185a622705e80c92976 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 09:54:35 -0500 Subject: [PATCH 01/29] error: alpha-sort Error variants --- rcgen/src/error.rs | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/rcgen/src/error.rs b/rcgen/src/error.rs index acffe1be..0bce273b 100644 --- a/rcgen/src/error.rs +++ b/rcgen/src/error.rs @@ -4,45 +4,45 @@ use std::fmt; #[non_exhaustive] /// The error type of the rcgen crate pub enum Error { + /// The provided certificate's signature algorithm + /// is incompatible with the given key pair + CertificateKeyPairMismatch, /// The given certificate couldn't be parsed CouldNotParseCertificate, /// The given certificate signing request couldn't be parsed CouldNotParseCertificationRequest, /// The given key pair couldn't be parsed CouldNotParseKeyPair, + /// Invalid certificate revocation list (CRL) next update. + InvalidCrlNextUpdate, + /// An IP address was provided as a byte array, but the byte array was an invalid length. + InvalidIpAddressOctetLength(usize), #[cfg(feature = "x509-parser")] /// Invalid subject alternative name type InvalidNameType, - /// An IP address was provided as a byte array, but the byte array was an invalid length. - InvalidIpAddressOctetLength(usize), + /// CRL issuer specifies Key Usages that don't include cRLSign. + IssuerNotCrlSigner, /// There is no support for generating /// keys for the given algorithm KeyGenerationUnavailable, - #[cfg(feature = "x509-parser")] - /// Unsupported extension requested in CSR - UnsupportedExtension, - /// The requested signature algorithm is not supported - UnsupportedSignatureAlgorithm, - /// Unspecified `ring` error - RingUnspecified, - /// The `ring` library rejected the key upon loading - RingKeyRejected(String), - /// The provided certificate's signature algorithm - /// is incompatible with the given key pair - CertificateKeyPairMismatch, - /// Time conversion related errors - Time, #[cfg(feature = "pem")] /// Error from the pem crate PemError(String), /// Error generated by a remote key operation RemoteKeyError, + /// The `ring` library rejected the key upon loading + RingKeyRejected(String), + /// Unspecified `ring` error + RingUnspecified, + /// Time conversion related errors + Time, + #[cfg(feature = "x509-parser")] + /// Unsupported extension requested in CSR + UnsupportedExtension, /// Unsupported field when generating a CSR UnsupportedInCsr, - /// Invalid certificate revocation list (CRL) next update. - InvalidCrlNextUpdate, - /// CRL issuer specifies Key Usages that don't include cRLSign. - IssuerNotCrlSigner, + /// The requested signature algorithm is not supported + UnsupportedSignatureAlgorithm, } impl fmt::Display for Error { From e8f27214cb72b090f1fb4445f88378dc60ff0a30 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Sep 2023 13:01:21 -0400 Subject: [PATCH 02/29] ext: introduce module for X.509 extension handling This commit creates a new crate-internal module, `ext`, for managing X.509 extensions. In this commit we wire up emitting extensions managed by this module, but do not yet convert any existing extensions to the new arrangement. This will begin in subsequent commits. This adds a dedicated `Extensions` struct and `Extension` trait that handle: * tracking extensions maintaining insertion order. * ensuring the invariant that we never add more than one instance of the same extension OID. * writing the DER encoded SEQUENCE of extensions. * writing each DER encoded extension SEQUENCE - including the OID, criticality, and value. The `Extension` trait allows common operations across all extensions like: * getting the ext OID. * getting the criticality (using a new `Criticality` enum). * getting the raw DER value. --- rcgen/src/crl.rs | 37 +++++++ rcgen/src/error.rs | 5 + rcgen/src/ext.rs | 251 +++++++++++++++++++++++++++++++++++++++++++++ rcgen/src/lib.rs | 44 +++++++- 4 files changed, 334 insertions(+), 3 deletions(-) create mode 100644 rcgen/src/ext.rs diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index 75a89bcd..9be9e86b 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -4,6 +4,7 @@ use time::OffsetDateTime; use yasna::DERWriter; use yasna::Tag; +use crate::ext::Extensions; use crate::oid::*; #[cfg(feature = "pem")] use crate::ENCODE_CONFIG; @@ -251,6 +252,13 @@ impl CertificateRevocationListParams { // extensions in all CRLs issued. writer.next().write_tagged(Tag::context(0), |writer| { writer.write_sequence(|writer| { + // TODO: have the Extensions type write the outer sequence and each + // contained extension once we've ported each of the below + // extensions to self.extensions(). + for ext in self.extensions(Some(ca)).iter() { + Extensions::write_extension(writer, ext); + } + // Write authority key identifier. write_x509_authority_key_identifier(writer.next(), ca); @@ -276,6 +284,19 @@ impl CertificateRevocationListParams { Ok(()) }) } + /// Returns the X.509 extensions that the [CertificateRevocationListParams] describe. + /// + /// If an issuer [Certificate] is provided, additional extensions specific to the issuer will + /// be included (e.g. the authority key identifier). + fn extensions(&self, _issuer: Option<&Certificate>) -> Extensions { + let exts = Extensions::default(); + + // TODO: AKI. + // TODO: CRL number. + // TODO: issuing distribution point. + + exts + } } /// A certificate revocation list (CRL) issuing distribution point, to be included in a CRL's @@ -364,6 +385,13 @@ impl RevokedCertParams { let has_invalidity_date = self.invalidity_date.is_some(); if has_reason_code || has_invalidity_date { writer.next().write_sequence(|writer| { + // TODO: have the Extensions type write the outer sequence and each + // contained extension once we've ported each of the below + // extensions to self.extensions(). + for ext in self.extensions().iter() { + Extensions::write_extension(writer, ext); + } + // Write reason code if present. self.reason_code.map(|reason_code| { write_x509_extension(writer.next(), OID_CRL_REASONS, false, |writer| { @@ -386,4 +414,13 @@ impl RevokedCertParams { } }) } + /// Returns the X.509 extensions that the [RevokedCertParams] describe. + fn extensions(&self) -> Extensions { + let exts = Extensions::default(); + + // TODO: reason code. + // TODO: invalidity date. + + exts + } } diff --git a/rcgen/src/error.rs b/rcgen/src/error.rs index 0bce273b..0328cf2d 100644 --- a/rcgen/src/error.rs +++ b/rcgen/src/error.rs @@ -13,6 +13,8 @@ pub enum Error { CouldNotParseCertificationRequest, /// The given key pair couldn't be parsed CouldNotParseKeyPair, + /// Duplicate extension OID + DuplicateExtension(String), /// Invalid certificate revocation list (CRL) next update. InvalidCrlNextUpdate, /// An IP address was provided as a byte array, but the byte array was an invalid length. @@ -91,6 +93,9 @@ impl fmt::Display for Error { f, "CRL issuer must specify no key usage, or key usage including cRLSign" )?, + DuplicateExtension(oid) => { + write!(f, "Extension with OID {oid} present multiple times")? + }, }; Ok(()) } diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs new file mode 100644 index 00000000..6788f176 --- /dev/null +++ b/rcgen/src/ext.rs @@ -0,0 +1,251 @@ +use std::collections::HashSet; +use std::fmt::Debug; + +use yasna::models::ObjectIdentifier; +use yasna::{DERWriter, DERWriterSeq}; + +use crate::{CertificateParams, Error}; + +/// The criticality of an extension. +/// +/// This controls how a certificate-using system should handle an unrecognized or un-parsable +/// extension. +/// +/// See [RFC 5280 Section 4.2] for more information. +/// +/// [RFC 5280 Section 4.2]: +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub(crate) enum Criticality { + /// The extension MUST be recognized and parsed correctly. + /// + /// A certificate-using system MUST reject the certificate if it encounters a critical + /// extension it does not recognize or a critical extension that contains information that it + /// cannot process. + Critical, + + /// The extension MAY be ignored if it is not recognized or parsed correctly. + /// + /// A non-critical extension MAY be ignored if it is not recognized, but MUST be + /// processed if it is recognized + NonCritical, +} + +/// A trait describing an X.509 Extension. +/// +/// All extensions have an OID, an indicator of whether they are critical or not, and can be +/// encoded to a DER value for inclusion in an X.509 certificate extension SEQUENCE. +pub(crate) trait Extension: Debug { + /// Return the OID of the extension. + fn oid(&self) -> ObjectIdentifier; + + /// Return the criticality of the extension. + fn criticality(&self) -> Criticality; + + /// Write the extension's value to the DER writer. + fn write_value(&self, writer: DERWriter); +} + +/// A collection of X.509 extensions. +/// +/// Preserves the order that extensions were added and maintains the invariant that +/// there are no duplicate extension OIDs. +#[derive(Debug, Default)] +pub(crate) struct Extensions { + exts: Vec>, + oids: HashSet, +} + +impl Extensions { + /// Construct a set of extensions from an iterator of extensions. + /// + /// # Errors + /// + /// Returns [Error::DuplicateExtension] if any of the extensions have the same OID. + pub(crate) fn new( + extensions: impl IntoIterator>, + ) -> Result { + let mut result = Self::default(); + result.add_extensions(extensions)?; + Ok(result) + } + + /// Add an extension to the collection. + /// + /// # Errors + /// + /// Returns [Error::DuplicateExtension] if the extension's OID is already present in the collection. + pub(crate) fn add_extension(&mut self, extension: Box) -> Result<(), Error> { + if self.oids.get(&extension.oid()).is_some() { + return Err(Error::DuplicateExtension(extension.oid().to_string())); + } + + self.oids.insert(extension.oid()); + self.exts.push(extension); + Ok(()) + } + + pub(crate) fn add_extensions( + &mut self, + extensions: impl IntoIterator>, + ) -> Result<(), Error> { + for ext in extensions { + self.add_extension(ext)? + } + Ok(()) + } + + /// Write the SEQUENCE of extensions to the DER writer. + /// + /// This will return without writing anything if there are no extensions in the collection. + pub(crate) fn write_der(&self, writer: DERWriter) { + debug_assert_eq!(self.exts.len(), self.oids.len()); + + // Avoid writing an empty extensions sequence. + if self.exts.is_empty() { + return; + } + + // Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension + writer.write_sequence(|writer| { + for extension in &self.exts { + Self::write_extension(writer, extension); + } + }) + } + + /// TODO(@cpu): Remove once `Extensions::write_der` is being used. + pub(crate) fn iter(&self) -> impl Iterator> { + self.exts.iter() + } + + /// TODO(@cpu): Reduce visibility once `Extensions::write_der` is being used. + pub(crate) fn write_extension(writer: &mut DERWriterSeq, extension: &Box) { + // Extension ::= SEQUENCE { + // extnID OBJECT IDENTIFIER, + // critical BOOLEAN DEFAULT FALSE, + // extnValue OCTET STRING + // -- contains the DER encoding of an ASN.1 value + // -- corresponding to the extension type identified + // -- by extnID + // } + writer.next().write_sequence(|writer| { + writer.next().write_oid(&extension.oid()); + writer + .next() + .write_bool(matches!(extension.criticality(), Criticality::Critical)); + writer.next().write_bytes(&yasna::construct_der(|writer| { + extension.write_value(writer) + })); + }); + } +} + +#[cfg(test)] +mod extensions_tests { + use crate::oid; + + use super::Criticality::*; + use super::*; + + #[test] + fn test_no_duplicates() { + let oid = ObjectIdentifier::from_slice(oid::OID_SUBJECT_ALT_NAME); + let ext = Box::new(DummyExt { + oid: oid.clone(), + critical: NonCritical, + der: Vec::default(), + }); + + // It should be an error to add two extensions with the same OID. + let mut exts = Extensions::default(); + exts.add_extension(ext.clone()).unwrap(); + assert_eq!( + exts.add_extension(ext.clone()), + Err(Error::DuplicateExtension(oid.to_string())), + ); + + // Or to construct an extensions set from an iterator containing two extensions with the + // same OID. + assert_eq!( + Extensions::new(vec![ + ext.clone() as Box, + ext.clone() as Box + ]) + .unwrap_err(), + Error::DuplicateExtension(oid.to_string()), + ); + } + + #[test] + fn test_write_der() { + use yasna::construct_der; + + // Construct three dummy extensions. + let ext_a = Box::new(DummyExt { + oid: ObjectIdentifier::from_slice(&[1, 3, 6, 1, 4, 3]), + critical: Critical, + der: b"a".to_vec(), + }); + + let ext_b = Box::new(DummyExt { + oid: ObjectIdentifier::from_slice(&[1, 3, 6, 1, 4, 2]), + critical: NonCritical, + der: b"b".to_vec(), + }); + + let ext_c = Box::new(DummyExt { + oid: ObjectIdentifier::from_slice(&[1, 3, 6, 1, 4, 1]), + critical: Critical, + der: b"c".to_vec(), + }); + + // Items of note: + // - We expect the extensions to be written in the order they were added. + // - The ext_b criticality is elided because it is non-critical - it would be a mis-encoding + // to write a value for a FALSE BOOLEAN in DER. + // - Each extension DER value should have been written unmodified, with no extra tags + // or length bytes. + let expected_der = vec![ + 0x30, 0x2D, // exts SEQUENCE + 0x30, 0xD, // ext_a SEQUENCE + 0x6, 0x5, 0x2B, 0x6, 0x1, 0x4, 0x3, 0x1, 0x1, // ext_a OID + 0xFF, // ext_A CRITICAL = true + 0x4, 0x1, 0x61, // ext_A OCTET SEQUENCE "A" (0x61) + 0x30, 0xD, // ext_b SEQUENCE + 0x6, 0x5, 0x2B, 0x6, 0x1, 0x4, 0x2, 0x1, 0x1, 0x0, // ext_b OID + // ext_b criticality elided + 0x4, 0x1, 0x62, // ext_b OCTET SEQUENCE "B" (0x62) + 0x30, 0xD, // ext_b SEQUENCE + 0x6, 0x5, 0x2B, 0x6, 0x1, 0x4, 0x1, 0x1, 0x1, // ext_c OID + 0xFF, // ext_b CRITICAL = true + 0x4, 0x1, 0x63, // ext_c OCTET SEQUENCE "C" (0x63) + ]; + + // Building the extensions and encoding to DER should result in the expected DER. + let test_exts: Vec> = vec![ext_a.clone(), ext_b.clone(), ext_c.clone()]; + let exts = Extensions::new(test_exts).unwrap(); + assert_eq!(construct_der(|writer| exts.write_der(writer)), expected_der); + } + + /// Mock extension for testing. + #[derive(Debug, Clone)] + struct DummyExt { + oid: ObjectIdentifier, + critical: Criticality, + der: Vec, + } + + impl Extension for DummyExt { + fn oid(&self) -> ObjectIdentifier { + self.oid.clone() + } + + fn criticality(&self) -> Criticality { + self.critical + } + + fn write_value(&self, writer: DERWriter) { + writer.write_der(&self.der); + } + } +} diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 666dae47..f5c831f5 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -57,6 +57,7 @@ pub use crate::crl::{ }; pub use crate::csr::{CertificateSigningRequest, PublicKey}; pub use crate::error::Error; +use crate::ext::Extensions; use crate::key_pair::PublicKeyData; pub use crate::key_pair::{KeyPair, RemoteKeyPair}; use crate::oid::*; @@ -114,6 +115,7 @@ pub fn generate_simple_self_signed( mod crl; mod csr; mod error; +mod ext; mod key_pair; mod oid; mod sign_algo; @@ -893,12 +895,20 @@ impl CertificateParams { // Write extensions // According to the spec in RFC 2986, even if attributes are empty we need the empty attribute tag writer.next().write_tagged(Tag::context(0), |writer| { + let extensions = self.extensions(None)?; if !subject_alt_names.is_empty() || !custom_extensions.is_empty() { writer.write_sequence(|writer| { let oid = ObjectIdentifier::from_slice(OID_PKCS_9_AT_EXTENSION_REQUEST); writer.next().write_oid(&oid); writer.next().write_set(|writer| { writer.next().write_sequence(|writer| { + // TODO: have the Extensions type write the outer sequence and each + // contained extension once we've ported each of the below + // extensions to self.extensions(). + for ext in extensions.iter() { + Extensions::write_extension(writer, ext); + } + // Write subject_alt_names self.write_subject_alt_names(writer.next()); @@ -915,9 +925,9 @@ impl CertificateParams { }); }); } - }); - }); - Ok(()) + Ok(()) + }) + }) } fn write_cert( &self, @@ -956,6 +966,7 @@ impl CertificateParams { // Write subjectPublicKeyInfo pub_key.serialize_public_key_der(writer.next()); // write extensions + let extensions = self.extensions(Some(ca))?; let should_write_exts = self.use_authority_key_identifier_extension || !self.subject_alt_names.is_empty() || !self.extended_key_usages.is_empty() @@ -966,6 +977,13 @@ impl CertificateParams { if should_write_exts { writer.next().write_tagged(Tag::context(3), |writer| { writer.write_sequence(|writer| { + // TODO: have the Extensions type write the outer sequence and each + // contained extension once we've ported each of the below + // extensions to self.extensions(). + for ext in extensions.iter() { + Extensions::write_extension(writer, ext); + } + if self.use_authority_key_identifier_extension { write_x509_authority_key_identifier(writer.next(), ca) } @@ -1175,6 +1193,26 @@ impl CertificateParams { }) }) } + /// Returns the X.509 extensions that the [CertificateParams] describe, or an [Error] + /// if the described extensions are invalid. + /// + /// If an issuer [Certificate] is provided, additional extensions specific to the issuer will + /// be included (e.g. the authority key identifier). + fn extensions(&self, _issuer: Option<&Certificate>) -> Result { + let exts = Extensions::default(); + + // TODO: AKI. + // TODO: SAN. + // TODO: KU. + // TODO: EKU. + // TODO: name constraints. + // TODO: crl distribution points. + // TODO: basic constraints. + // TODO: subject key identifier. + // TODO: custom extensions + + Ok(exts) + } } /// Whether the certificate is allowed to sign other certificates From 1948f6a33ba28add3863efa082e006bc578f93df Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 12:57:21 -0500 Subject: [PATCH 03/29] ext: implement authority key identifier. This commit lifts the authority key identifier extension into the `ext` module. --- rcgen/src/crl.rs | 19 +++++++++---------- rcgen/src/ext.rs | 47 +++++++++++++++++++++++++++++++++++++++++++++-- rcgen/src/lib.rs | 35 ++++++----------------------------- 3 files changed, 60 insertions(+), 41 deletions(-) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index 9be9e86b..972629a5 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -8,10 +8,7 @@ use crate::ext::Extensions; use crate::oid::*; #[cfg(feature = "pem")] use crate::ENCODE_CONFIG; -use crate::{ - write_distinguished_name, write_dt_utc_or_generalized, write_x509_authority_key_identifier, - write_x509_extension, -}; +use crate::{ext, write_distinguished_name, write_dt_utc_or_generalized, write_x509_extension}; use crate::{Certificate, Error, KeyIdMethod, KeyUsagePurpose, SerialNumber, SignatureAlgorithm}; /// A certificate revocation list (CRL) @@ -259,9 +256,6 @@ impl CertificateRevocationListParams { Extensions::write_extension(writer, ext); } - // Write authority key identifier. - write_x509_authority_key_identifier(writer.next(), ca); - // Write CRL number. write_x509_extension(writer.next(), OID_CRL_NUMBER, false, |writer| { writer.write_bigint_bytes(self.crl_number.as_ref(), true); @@ -288,10 +282,15 @@ impl CertificateRevocationListParams { /// /// If an issuer [Certificate] is provided, additional extensions specific to the issuer will /// be included (e.g. the authority key identifier). - fn extensions(&self, _issuer: Option<&Certificate>) -> Extensions { - let exts = Extensions::default(); + fn extensions(&self, issuer: Option<&Certificate>) -> Extensions { + let mut exts = Extensions::default(); + + if let Some(issuer) = issuer { + // Safety: `exts` is empty at this point - there can be no duplicate AKI ext OID. + exts.add_extension(Box::new(ext::AuthorityKeyIdentifier::from(issuer))) + .unwrap(); + } - // TODO: AKI. // TODO: CRL number. // TODO: issuing distribution point. diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 6788f176..7fa16346 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -2,9 +2,9 @@ use std::collections::HashSet; use std::fmt::Debug; use yasna::models::ObjectIdentifier; -use yasna::{DERWriter, DERWriterSeq}; +use yasna::{DERWriter, DERWriterSeq, Tag}; -use crate::{CertificateParams, Error}; +use crate::{oid, Certificate, Error}; /// The criticality of an extension. /// @@ -140,6 +140,49 @@ impl Extensions { } } +/// An X.509v3 authority key identifier extension according to +/// [RFC 5280 4.2.1.1](https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.1). +#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord)] +pub(crate) struct AuthorityKeyIdentifier { + key_identifier: Vec, +} + +impl Extension for AuthorityKeyIdentifier { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_AUTHORITY_KEY_IDENTIFIER) + } + + fn criticality(&self) -> Criticality { + // Conforming CAs MUST mark this extension as non-critical. + Criticality::NonCritical + } + + fn write_value(&self, writer: DERWriter) { + /* + AuthorityKeyIdentifier ::= SEQUENCE { + keyIdentifier [0] KeyIdentifier OPTIONAL, + authorityCertIssuer [1] GeneralNames OPTIONAL, + authorityCertSerialNumber [2] CertificateSerialNumber OPTIONAL } + KeyIdentifier ::= OCTET STRING + */ + writer.write_sequence(|writer| { + writer + .next() + .write_tagged_implicit(Tag::context(0), |writer| { + writer.write_bytes(&self.key_identifier) + }) + }); + } +} + +impl From<&Certificate> for AuthorityKeyIdentifier { + fn from(cert: &Certificate) -> Self { + Self { + key_identifier: cert.get_key_identifier(), + } + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index f5c831f5..44a8ea12 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -984,9 +984,6 @@ impl CertificateParams { Extensions::write_extension(writer, ext); } - if self.use_authority_key_identifier_extension { - write_x509_authority_key_identifier(writer.next(), ca) - } // Write subject_alt_names if !self.subject_alt_names.is_empty() { self.write_subject_alt_names(writer.next()); @@ -1198,10 +1195,13 @@ impl CertificateParams { /// /// If an issuer [Certificate] is provided, additional extensions specific to the issuer will /// be included (e.g. the authority key identifier). - fn extensions(&self, _issuer: Option<&Certificate>) -> Result { - let exts = Extensions::default(); + fn extensions(&self, issuer: Option<&Certificate>) -> Result { + let mut exts = Extensions::default(); + + if let Some(issuer) = issuer { + exts.add_extension(Box::new(ext::AuthorityKeyIdentifier::from(issuer)))?; + } - // TODO: AKI. // TODO: SAN. // TODO: KU. // TODO: EKU. @@ -1633,29 +1633,6 @@ fn write_x509_extension( }) } -/// Serializes an X.509v3 authority key identifier extension according to RFC 5280. -fn write_x509_authority_key_identifier(writer: DERWriter, ca: &Certificate) { - // Write Authority Key Identifier - // RFC 5280 states: - // 'The keyIdentifier field of the authorityKeyIdentifier extension MUST - // be included in all certificates generated by conforming CAs to - // facilitate certification path construction. There is one exception; - // where a CA distributes its public key in the form of a "self-signed" - // certificate, the authority key identifier MAY be omitted.' - // In addition, for CRLs: - // 'Conforming CRL issuers MUST use the key identifier method, and MUST - // include this extension in all CRLs issued.' - write_x509_extension(writer, OID_AUTHORITY_KEY_IDENTIFIER, false, |writer| { - writer.write_sequence(|writer| { - writer - .next() - .write_tagged_implicit(Tag::context(0), |writer| { - writer.write_bytes(ca.get_key_identifier().as_ref()) - }) - }); - }); -} - #[cfg(feature = "zeroize")] impl zeroize::Zeroize for KeyPair { fn zeroize(&mut self) { From 6a3359d39e1aacdcb22f28a00ecd2dc90f7031c9 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 13:35:43 -0500 Subject: [PATCH 04/29] ext: implement subject alternative name. This commit lifts the subject alternative name extension into the `ext` module. It additionally ensures we never write an empty SAN extension, if the `CertificateParams` contain an empty vec of SAN names. For the time being SAN extensions are always written as non-criticial, but the required plumbing to handle the RFC5280 guidance on SAN ext criticality is added for follow-up adjustment. --- rcgen/src/csr.rs | 14 ++--- rcgen/src/ext.rs | 140 ++++++++++++++++++++++++++++++++++++++++++++++- rcgen/src/lib.rs | 74 +++++++------------------ 3 files changed, 163 insertions(+), 65 deletions(-) diff --git a/rcgen/src/csr.rs b/rcgen/src/csr.rs index e0d9eb6f..160210b0 100644 --- a/rcgen/src/csr.rs +++ b/rcgen/src/csr.rs @@ -1,10 +1,10 @@ #[cfg(feature = "x509-parser")] -use crate::{DistinguishedName, SanType}; +use crate::DistinguishedName; #[cfg(feature = "pem")] use pem::Pem; use std::hash::Hash; -use crate::{Certificate, CertificateParams, Error, PublicKeyData, SignatureAlgorithm}; +use crate::{ext, Certificate, CertificateParams, Error, PublicKeyData, SignatureAlgorithm}; /// A public key, extracted from a CSR #[derive(Debug, PartialEq, Eq, Hash)] @@ -68,14 +68,8 @@ impl CertificateSigningRequest { if let Some(extensions) = csr.requested_extensions() { for ext in extensions { - match ext { - x509_parser::extensions::ParsedExtension::SubjectAlternativeName(san) => { - for name in &san.general_names { - params - .subject_alt_names - .push(SanType::try_from_general(name)?); - } - }, + match ext::SubjectAlternativeName::from_parsed(&mut params, ext) { + Ok(_) => (), _ => return Err(Error::UnsupportedExtension), } } diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 7fa16346..4416450f 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -1,10 +1,11 @@ use std::collections::HashSet; use std::fmt::Debug; +use std::net::IpAddr; use yasna::models::ObjectIdentifier; use yasna::{DERWriter, DERWriterSeq, Tag}; -use crate::{oid, Certificate, Error}; +use crate::{oid, Certificate, CertificateParams, Error, SanType}; /// The criticality of an extension. /// @@ -183,6 +184,82 @@ impl From<&Certificate> for AuthorityKeyIdentifier { } } +/// An X.509v3 subject alternative name extension according to +/// [RFC 5280 4.2.1.6](https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.6). +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct SubjectAlternativeName { + criticality: Criticality, + names: Vec, +} + +impl SubjectAlternativeName { + pub(crate) fn from_params(params: &CertificateParams) -> Option { + match params.subject_alt_names.is_empty() { + true => None, + false => Some(Self { + // TODO(XXX): For now we mark the SAN extension as non-critical, matching the pre-existing + // handling, however per 5280 this extension's criticality is determined based + // on whether or not the subject contains an empty sequence. + criticality: Criticality::NonCritical, + names: params.subject_alt_names.clone(), + }), + } + } + + #[cfg(feature = "x509-parser")] + pub(crate) fn from_parsed( + params: &mut CertificateParams, + ext: &x509_parser::extensions::ParsedExtension, + ) -> Result { + Ok(match ext { + x509_parser::extensions::ParsedExtension::SubjectAlternativeName(san) => { + for name in &san.general_names { + params + .subject_alt_names + .push(SanType::try_from_general(name)?); + } + true + }, + _ => false, + }) + } + + fn write_name(writer: DERWriter, san: &SanType) { + writer.write_tagged_implicit(Tag::context(san.tag()), |writer| match san { + SanType::Rfc822Name(name) | SanType::DnsName(name) | SanType::URI(name) => { + writer.write_ia5_string(&name) + }, + SanType::IpAddress(IpAddr::V4(addr)) => writer.write_bytes(&addr.octets()), + SanType::IpAddress(IpAddr::V6(addr)) => writer.write_bytes(&addr.octets()), + }) + } +} + +impl Extension for SubjectAlternativeName { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_SUBJECT_ALT_NAME) + } + + fn criticality(&self) -> Criticality { + // this extension's criticality is determined based on whether or not the subject contains + // an empty sequence. If it does, the SAN MUST be critical. If it has a non-empty subject + // distinguished name, the SAN SHOULD be non-critical. + self.criticality + } + + fn write_value(&self, writer: DERWriter) { + /* + SubjectAltName ::= GeneralNames + GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName + */ + writer.write_sequence(|writer| { + self.names + .iter() + .for_each(|san| Self::write_name(writer.next(), san)); + }); + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; @@ -292,3 +369,64 @@ mod extensions_tests { } } } + +#[cfg(test)] +mod san_ext_tests { + #[cfg(feature = "x509-parser")] + use x509_parser::prelude::FromDer; + + use super::*; + use crate::CertificateParams; + + #[test] + fn test_from_params() { + let domain_a = "test.example.com".to_string(); + let domain_b = "example.com".to_string(); + let ip = IpAddr::try_from([127, 0, 0, 1]).unwrap(); + let mut params = CertificateParams::new(vec![domain_a.clone(), domain_b.clone()]); + params + .subject_alt_names + .push(SanType::IpAddress(ip.clone())); + + let ext = SubjectAlternativeName::from_params(¶ms).unwrap(); + let expected_names = vec![ + SanType::DnsName(domain_a), + SanType::DnsName(domain_b), + SanType::IpAddress(ip), + ]; + assert_eq!(ext.names, expected_names); + } + + #[test] + fn test_from_empty_san() { + assert!(SubjectAlternativeName::from_params(&CertificateParams::default()).is_none()); + } + + #[test] + #[cfg(feature = "x509-parser")] + fn test_from_parsed() { + let domain_a = "test.example.com".to_string(); + let domain_b = "example.com".to_string(); + let ip = IpAddr::try_from([127, 0, 0, 1]).unwrap(); + let mut params = CertificateParams::new(vec![domain_a.clone(), domain_b.clone()]); + params + .subject_alt_names + .push(SanType::IpAddress(ip.clone())); + + let der = yasna::construct_der(|writer| { + SubjectAlternativeName::from_params(¶ms) + .unwrap() + .write_value(writer) + }); + + let parsed_ext = x509_parser::extensions::ParsedExtension::SubjectAlternativeName( + x509_parser::extensions::SubjectAlternativeName::from_der(&der) + .unwrap() + .1, + ); + + let mut recovered_params = CertificateParams::default(); + SubjectAlternativeName::from_parsed(&mut recovered_params, &parsed_ext).unwrap(); + assert_eq!(recovered_params.subject_alt_names, params.subject_alt_names); + } +} diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 44a8ea12..5a7204b2 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -607,7 +607,6 @@ impl CertificateParams { let dn = DistinguishedName::from_name(&x509.tbs_certificate.subject)?; let is_ca = Self::convert_x509_is_ca(&x509)?; let validity = x509.validity(); - let subject_alt_names = Self::convert_x509_subject_alternative_name(&x509)?; let key_usages = Self::convert_x509_key_usages(&x509)?; let extended_key_usages = Self::convert_x509_extended_key_usages(&x509)?; let name_constraints = Self::convert_x509_name_constraints(&x509)?; @@ -623,10 +622,10 @@ impl CertificateParams { }) .unwrap_or(KeyIdMethod::Sha256); - Ok(CertificateParams { + let mut params = CertificateParams { alg, is_ca, - subject_alt_names, + subject_alt_names: Vec::default(), key_usages, extended_key_usages, name_constraints, @@ -637,7 +636,20 @@ impl CertificateParams { not_before: validity.not_before.to_datetime(), not_after: validity.not_after.to_datetime(), ..Default::default() - }) + }; + + if let Some(san_ext) = x509 + .iter_extensions() + .find_map(|ext| match ext.parsed_extension() { + ext @ x509_parser::extensions::ParsedExtension::SubjectAlternativeName(_) => { + Some(ext) + }, + _ => None, + }) { + ext::SubjectAlternativeName::from_parsed(&mut params, san_ext)?; + } + + Ok(params) } #[cfg(feature = "x509-parser")] fn convert_x509_is_ca( @@ -670,25 +682,6 @@ impl CertificateParams { Ok(is_ca) } #[cfg(feature = "x509-parser")] - fn convert_x509_subject_alternative_name( - x509: &x509_parser::certificate::X509Certificate<'_>, - ) -> Result, Error> { - let sans = x509 - .subject_alternative_name() - .or(Err(Error::CouldNotParseCertificate))? - .map(|ext| &ext.value.general_names); - - if let Some(sans) = sans { - let mut subject_alt_names = Vec::with_capacity(sans.len()); - for san in sans { - subject_alt_names.push(SanType::try_from_general(san)?); - } - Ok(subject_alt_names) - } else { - Ok(Vec::new()) - } - } - #[cfg(feature = "x509-parser")] fn convert_x509_key_usages( x509: &x509_parser::certificate::X509Certificate<'_>, ) -> Result, Error> { @@ -826,28 +819,6 @@ impl CertificateParams { } Ok(result) } - fn write_subject_alt_names(&self, writer: DERWriter) { - write_x509_extension(writer, OID_SUBJECT_ALT_NAME, false, |writer| { - writer.write_sequence(|writer| { - for san in self.subject_alt_names.iter() { - writer.next().write_tagged_implicit( - Tag::context(san.tag()), - |writer| match san { - SanType::Rfc822Name(name) - | SanType::DnsName(name) - | SanType::URI(name) => writer.write_ia5_string(name), - SanType::IpAddress(IpAddr::V4(addr)) => { - writer.write_bytes(&addr.octets()) - }, - SanType::IpAddress(IpAddr::V6(addr)) => { - writer.write_bytes(&addr.octets()) - }, - }, - ); - } - }); - }); - } fn write_request(&self, pub_key: &K, writer: DERWriter) -> Result<(), Error> { // No .. pattern, we use this to ensure every field is used #[deny(unused)] @@ -909,9 +880,6 @@ impl CertificateParams { Extensions::write_extension(writer, ext); } - // Write subject_alt_names - self.write_subject_alt_names(writer.next()); - // Write custom extensions for ext in custom_extensions { write_x509_extension( @@ -984,11 +952,6 @@ impl CertificateParams { Extensions::write_extension(writer, ext); } - // Write subject_alt_names - if !self.subject_alt_names.is_empty() { - self.write_subject_alt_names(writer.next()); - } - // Write standard key usage if !self.key_usages.is_empty() { write_x509_extension(writer.next(), OID_KEY_USAGE, true, |writer| { @@ -1202,7 +1165,10 @@ impl CertificateParams { exts.add_extension(Box::new(ext::AuthorityKeyIdentifier::from(issuer)))?; } - // TODO: SAN. + if let Some(san_ext) = ext::SubjectAlternativeName::from_params(&self) { + exts.add_extension(Box::new(san_ext))?; + } + // TODO: KU. // TODO: EKU. // TODO: name constraints. From 063482e051ef5a7d148963c8fdb918aba487ecf4 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 14:26:01 -0500 Subject: [PATCH 05/29] ext: implement key usage This commit lifts the key usage extension into the `ext` module. --- rcgen/src/ext.rs | 163 ++++++++++++++++++++++++++++++++++++++++++++++- rcgen/src/lib.rs | 113 +++++++------------------------- 2 files changed, 186 insertions(+), 90 deletions(-) diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 4416450f..ef1f59d8 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -5,7 +5,7 @@ use std::net::IpAddr; use yasna::models::ObjectIdentifier; use yasna::{DERWriter, DERWriterSeq, Tag}; -use crate::{oid, Certificate, CertificateParams, Error, SanType}; +use crate::{oid, Certificate, CertificateParams, Error, KeyUsagePurpose, SanType}; /// The criticality of an extension. /// @@ -260,6 +260,117 @@ impl Extension for SubjectAlternativeName { } } +/// An X.509v3 key usage extension according to +/// [RFC 5280 4.2.1.3](https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.3). +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct KeyUsage { + usages: Vec, +} + +impl KeyUsage { + pub(crate) fn from_params(params: &CertificateParams) -> Option { + match params.key_usages.is_empty() { + true => None, + false => Some(Self { + usages: params.key_usages.clone(), + }), + } + } + + #[cfg(feature = "x509-parser")] + pub(crate) fn from_parsed( + params: &mut CertificateParams, + ext: &x509_parser::extensions::ParsedExtension, + ) -> Result { + match ext { + x509_parser::extensions::ParsedExtension::KeyUsage(ku) => { + let mut usages = Vec::new(); + if ku.digital_signature() { + usages.push(KeyUsagePurpose::DigitalSignature); + } + // Note: previous editions of X.509 called ContentCommitment "Non repudiation" + if ku.non_repudiation() { + usages.push(KeyUsagePurpose::ContentCommitment); + } + if ku.key_encipherment() { + usages.push(KeyUsagePurpose::KeyEncipherment); + } + if ku.key_cert_sign() { + usages.push(KeyUsagePurpose::KeyCertSign); + } + if ku.crl_sign() { + usages.push(KeyUsagePurpose::CrlSign); + } + if ku.encipher_only() { + usages.push(KeyUsagePurpose::EncipherOnly); + } + if ku.decipher_only() { + usages.push(KeyUsagePurpose::DecipherOnly); + } + params.key_usages = usages; + Ok(true) + }, + _ => Ok(false), + } + } +} + +impl Extension for KeyUsage { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_KEY_USAGE) + } + + fn criticality(&self) -> Criticality { + // When present, conforming CAs SHOULD mark this extension as critical. + Criticality::Critical + } + + fn write_value(&self, writer: DERWriter) { + use KeyUsagePurpose::*; + + /* + KeyUsage ::= BIT STRING { + digitalSignature (0), + nonRepudiation (1), -- recent editions of X.509 have + -- renamed this bit to contentCommitment + keyEncipherment (2), + dataEncipherment (3), + keyAgreement (4), + keyCertSign (5), + cRLSign (6), + encipherOnly (7), + decipherOnly (8) } + */ + let mut bits: u16 = 0; + + for entry in &self.usages { + // Map the index to a value + let index = match entry { + DigitalSignature => 0, + ContentCommitment => 1, + KeyEncipherment => 2, + DataEncipherment => 3, + KeyAgreement => 4, + KeyCertSign => 5, + CrlSign => 6, + EncipherOnly => 7, + DecipherOnly => 8, + }; + + bits |= 1 << index; + } + + // Compute the 1-based most significant bit + let msb = 16 - bits.leading_zeros(); + let nb = if msb <= 8 { 1 } else { 2 }; + let bits = bits.reverse_bits().to_be_bytes(); + + // Finally take only the bytes != 0 + let bits = &bits[..nb]; + writer.write_bitvec_bytes(&bits, msb as usize) + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; @@ -430,3 +541,53 @@ mod san_ext_tests { assert_eq!(recovered_params.subject_alt_names, params.subject_alt_names); } } + +#[cfg(test)] +mod ku_ext_tests { + #[cfg(feature = "x509-parser")] + use x509_parser::prelude::FromDer; + + use super::*; + use crate::CertificateParams; + + #[test] + fn test_from_params() { + let mut params = CertificateParams::default(); + params.key_usages = vec![ + KeyUsagePurpose::DigitalSignature, + KeyUsagePurpose::ContentCommitment, + KeyUsagePurpose::KeyEncipherment, + KeyUsagePurpose::DataEncipherment, + KeyUsagePurpose::KeyAgreement, + KeyUsagePurpose::KeyCertSign, + KeyUsagePurpose::CrlSign, + KeyUsagePurpose::EncipherOnly, + KeyUsagePurpose::DecipherOnly, + ]; + + let ext = KeyUsage::from_params(¶ms).unwrap(); + assert_eq!(ext.usages, params.key_usages); + } + + #[test] + #[cfg(feature = "x509-parser")] + fn test_from_parsed() { + let mut params = CertificateParams::default(); + params.key_usages = vec![ + KeyUsagePurpose::ContentCommitment, + KeyUsagePurpose::KeyEncipherment, + ]; + + let der = yasna::construct_der(|writer| { + KeyUsage::from_params(¶ms).unwrap().write_value(writer) + }); + + let parsed_ext = x509_parser::extensions::ParsedExtension::KeyUsage( + x509_parser::extensions::KeyUsage::from_der(&der).unwrap().1, + ); + + let mut recovered_params = CertificateParams::default(); + KeyUsage::from_parsed(&mut recovered_params, &parsed_ext).unwrap(); + assert_eq!(recovered_params.key_usages, params.key_usages); + } +} diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 5a7204b2..883b9583 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -45,6 +45,7 @@ use std::net::IpAddr; use std::net::{Ipv4Addr, Ipv6Addr}; use std::str::FromStr; use time::{Date, Month, OffsetDateTime, PrimitiveDateTime, Time}; +use x509_parser::prelude::ParsedExtension; use yasna::models::ObjectIdentifier; use yasna::models::{GeneralizedTime, UTCTime}; use yasna::tags::{TAG_BMPSTRING, TAG_TELETEXSTRING, TAG_UNIVERSALSTRING}; @@ -607,7 +608,6 @@ impl CertificateParams { let dn = DistinguishedName::from_name(&x509.tbs_certificate.subject)?; let is_ca = Self::convert_x509_is_ca(&x509)?; let validity = x509.validity(); - let key_usages = Self::convert_x509_key_usages(&x509)?; let extended_key_usages = Self::convert_x509_extended_key_usages(&x509)?; let name_constraints = Self::convert_x509_name_constraints(&x509)?; let serial_number = Some(x509.serial.to_bytes_be().into()); @@ -615,7 +615,7 @@ impl CertificateParams { let key_identifier_method = x509 .iter_extensions() .find_map(|ext| match ext.parsed_extension() { - x509_parser::extensions::ParsedExtension::SubjectKeyIdentifier(key_id) => { + ParsedExtension::SubjectKeyIdentifier(key_id) => { Some(KeyIdMethod::PreSpecified(key_id.0.into())) }, _ => None, @@ -625,8 +625,6 @@ impl CertificateParams { let mut params = CertificateParams { alg, is_ca, - subject_alt_names: Vec::default(), - key_usages, extended_key_usages, name_constraints, serial_number, @@ -638,17 +636,27 @@ impl CertificateParams { ..Default::default() }; - if let Some(san_ext) = x509 - .iter_extensions() - .find_map(|ext| match ext.parsed_extension() { - ext @ x509_parser::extensions::ParsedExtension::SubjectAlternativeName(_) => { - Some(ext) - }, - _ => None, - }) { + macro_rules! find_parsed_extension { + ($iter:expr, $variant:pat) => { + $iter + .iter_extensions() + .find_map(|ext| match ext.parsed_extension() { + ext @ $variant => Some(ext), + _ => None, + }) + }; + } + + if let Some(san_ext) = + find_parsed_extension!(x509, ParsedExtension::SubjectAlternativeName(_)) + { ext::SubjectAlternativeName::from_parsed(&mut params, san_ext)?; } + if let Some(ku_ext) = find_parsed_extension!(x509, ParsedExtension::KeyUsage(_)) { + ext::KeyUsage::from_parsed(&mut params, ku_ext)?; + } + Ok(params) } #[cfg(feature = "x509-parser")] @@ -682,47 +690,6 @@ impl CertificateParams { Ok(is_ca) } #[cfg(feature = "x509-parser")] - fn convert_x509_key_usages( - x509: &x509_parser::certificate::X509Certificate<'_>, - ) -> Result, Error> { - let key_usage = x509 - .key_usage() - .or(Err(Error::CouldNotParseCertificate))? - .map(|ext| ext.value); - - let mut key_usages = Vec::new(); - if let Some(key_usage) = key_usage { - if key_usage.digital_signature() { - key_usages.push(KeyUsagePurpose::DigitalSignature); - } - if key_usage.non_repudiation() { - key_usages.push(KeyUsagePurpose::ContentCommitment); - } - if key_usage.key_encipherment() { - key_usages.push(KeyUsagePurpose::KeyEncipherment); - } - if key_usage.data_encipherment() { - key_usages.push(KeyUsagePurpose::DataEncipherment); - } - if key_usage.key_agreement() { - key_usages.push(KeyUsagePurpose::KeyAgreement); - } - if key_usage.key_cert_sign() { - key_usages.push(KeyUsagePurpose::KeyCertSign); - } - if key_usage.crl_sign() { - key_usages.push(KeyUsagePurpose::CrlSign); - } - if key_usage.encipher_only() { - key_usages.push(KeyUsagePurpose::EncipherOnly); - } - if key_usage.decipher_only() { - key_usages.push(KeyUsagePurpose::DecipherOnly); - } - } - Ok(key_usages) - } - #[cfg(feature = "x509-parser")] fn convert_x509_extended_key_usages( x509: &x509_parser::certificate::X509Certificate<'_>, ) -> Result, Error> { @@ -952,41 +919,6 @@ impl CertificateParams { Extensions::write_extension(writer, ext); } - // Write standard key usage - if !self.key_usages.is_empty() { - write_x509_extension(writer.next(), OID_KEY_USAGE, true, |writer| { - let mut bits: u16 = 0; - - for entry in self.key_usages.iter() { - // Map the index to a value - let index = match entry { - KeyUsagePurpose::DigitalSignature => 0, - KeyUsagePurpose::ContentCommitment => 1, - KeyUsagePurpose::KeyEncipherment => 2, - KeyUsagePurpose::DataEncipherment => 3, - KeyUsagePurpose::KeyAgreement => 4, - KeyUsagePurpose::KeyCertSign => 5, - KeyUsagePurpose::CrlSign => 6, - KeyUsagePurpose::EncipherOnly => 7, - KeyUsagePurpose::DecipherOnly => 8, - }; - - bits |= 1 << index; - } - - // Compute the 1-based most significant bit - let msb = 16 - bits.leading_zeros(); - let nb = if msb <= 8 { 1 } else { 2 }; - - let bits = bits.reverse_bits().to_be_bytes(); - - // Finally take only the bytes != 0 - let bits = &bits[..nb]; - - writer.write_bitvec_bytes(&bits, msb as usize) - }); - } - // Write extended key usage if !self.extended_key_usages.is_empty() { write_x509_extension( @@ -1169,7 +1101,10 @@ impl CertificateParams { exts.add_extension(Box::new(san_ext))?; } - // TODO: KU. + if let Some(ku_ext) = ext::KeyUsage::from_params(&self) { + exts.add_extension(Box::new(ku_ext))?; + } + // TODO: EKU. // TODO: name constraints. // TODO: crl distribution points. From fecc1edda924a661e67d3ebfb9951da0f2d8a8ac Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 14:55:14 -0500 Subject: [PATCH 06/29] wip: extended key usage (and some CSR fixes) TODO: Split out the non-eku related bits. This commit lifts the extended key usage extension into the `ext` module. --- rcgen/src/csr.rs | 19 ++++++- rcgen/src/ext.rs | 144 ++++++++++++++++++++++++++++++++++++++++++++++- rcgen/src/lib.rs | 68 ++++------------------ 3 files changed, 169 insertions(+), 62 deletions(-) diff --git a/rcgen/src/csr.rs b/rcgen/src/csr.rs index 160210b0..50ed304e 100644 --- a/rcgen/src/csr.rs +++ b/rcgen/src/csr.rs @@ -69,15 +69,28 @@ impl CertificateSigningRequest { if let Some(extensions) = csr.requested_extensions() { for ext in extensions { match ext::SubjectAlternativeName::from_parsed(&mut params, ext) { - Ok(_) => (), - _ => return Err(Error::UnsupportedExtension), + Ok(true) => continue, // SAN extension handled. + Err(err) => return Err(err), + _ => {}, // Not a SAN. } + match ext::KeyUsage::from_parsed(&mut params, ext) { + Ok(true) => continue, // KU extension handled. + Err(err) => return Err(err), + _ => {}, // Not a KU. + } + match ext::ExtendedKeyUsage::from_parsed(&mut params, ext) { + Ok(true) => continue, // EKU extension handled. + Err(err) => return Err(err), + _ => {}, // Not an EKU. + } + + // If we get here, we've encountered an unknown and unhandled extension. + return Err(Error::UnsupportedExtension); } } // Not yet handled: // * is_ca - // * extended_key_usages // * name_constraints // and any other extensions. diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index ef1f59d8..9fee58b1 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -5,7 +5,9 @@ use std::net::IpAddr; use yasna::models::ObjectIdentifier; use yasna::{DERWriter, DERWriterSeq, Tag}; -use crate::{oid, Certificate, CertificateParams, Error, KeyUsagePurpose, SanType}; +use crate::{ + oid, Certificate, CertificateParams, Error, ExtendedKeyUsagePurpose, KeyUsagePurpose, SanType, +}; /// The criticality of an extension. /// @@ -371,6 +373,91 @@ impl Extension for KeyUsage { } } +/// An X.509v3 extended key usage extension according to +/// [RFC 5280 4.2.1.12](https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.12). +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct ExtendedKeyUsage { + usages: Vec, + // This extension MAY, at the option of the certificate issuer, be + // either critical or non-critical. + critical: Criticality, +} + +impl ExtendedKeyUsage { + pub(crate) fn from_params(params: &CertificateParams) -> Option { + match params.extended_key_usages.is_empty() { + true => None, + false => Some(Self { + usages: params.extended_key_usages.clone(), + // TODO(xxx): Consider making EKU criticality configurable through params. + critical: Criticality::NonCritical, + }), + } + } + + #[cfg(feature = "x509-parser")] + pub(crate) fn from_parsed( + params: &mut CertificateParams, + ext: &x509_parser::extensions::ParsedExtension, + ) -> Result { + match ext { + x509_parser::extensions::ParsedExtension::ExtendedKeyUsage(eku) => { + use ExtendedKeyUsagePurpose::*; + + let mut usages = Vec::new(); + if eku.any { + usages.push(Any); + } + if eku.server_auth { + usages.push(ServerAuth); + } + if eku.client_auth { + usages.push(ClientAuth); + } + if eku.code_signing { + usages.push(CodeSigning); + } + if eku.email_protection { + usages.push(EmailProtection); + } + if eku.time_stamping { + usages.push(TimeStamping); + } + if eku.ocsp_signing { + usages.push(OcspSigning); + } + params.extended_key_usages = usages; + Ok(true) + }, + _ => Ok(false), + } + } +} + +impl Extension for ExtendedKeyUsage { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_EXT_KEY_USAGE) + } + + fn criticality(&self) -> Criticality { + self.critical + } + + fn write_value(&self, writer: DERWriter) { + /* + ExtKeyUsageSyntax ::= SEQUENCE SIZE (1..MAX) OF KeyPurposeId + KeyPurposeId ::= OBJECT IDENTIFIER + */ + writer.write_sequence(|writer| { + for usage in self.usages.iter() { + writer + .next() + .write_oid(&ObjectIdentifier::from_slice(usage.oid())); + } + }); + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; @@ -591,3 +678,58 @@ mod ku_ext_tests { assert_eq!(recovered_params.key_usages, params.key_usages); } } + +#[cfg(test)] +mod eku_ext_tests { + #[cfg(feature = "x509-parser")] + use x509_parser::prelude::FromDer; + + use super::*; + use crate::CertificateParams; + + #[test] + fn test_from_params() { + let mut params = CertificateParams::default(); + params.extended_key_usages = vec![ + ExtendedKeyUsagePurpose::Any, + ExtendedKeyUsagePurpose::ServerAuth, + ExtendedKeyUsagePurpose::ClientAuth, + ExtendedKeyUsagePurpose::CodeSigning, + ExtendedKeyUsagePurpose::EmailProtection, + ExtendedKeyUsagePurpose::TimeStamping, + ExtendedKeyUsagePurpose::OcspSigning, + ]; + + let ext = ExtendedKeyUsage::from_params(¶ms).unwrap(); + assert_eq!(ext.usages, params.extended_key_usages); + } + + #[test] + #[cfg(feature = "x509-parser")] + fn test_from_parsed() { + let mut params = CertificateParams::default(); + params.extended_key_usages = vec![ + ExtendedKeyUsagePurpose::CodeSigning, + ExtendedKeyUsagePurpose::EmailProtection, + ]; + + let der = yasna::construct_der(|writer| { + ExtendedKeyUsage::from_params(¶ms) + .unwrap() + .write_value(writer) + }); + + let parsed_ext = x509_parser::extensions::ParsedExtension::ExtendedKeyUsage( + x509_parser::extensions::ExtendedKeyUsage::from_der(&der) + .unwrap() + .1, + ); + + let mut recovered_params = CertificateParams::default(); + ExtendedKeyUsage::from_parsed(&mut recovered_params, &parsed_ext).unwrap(); + assert_eq!( + recovered_params.extended_key_usages, + params.extended_key_usages + ); + } +} diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 883b9583..e30d0e0c 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -608,7 +608,6 @@ impl CertificateParams { let dn = DistinguishedName::from_name(&x509.tbs_certificate.subject)?; let is_ca = Self::convert_x509_is_ca(&x509)?; let validity = x509.validity(); - let extended_key_usages = Self::convert_x509_extended_key_usages(&x509)?; let name_constraints = Self::convert_x509_name_constraints(&x509)?; let serial_number = Some(x509.serial.to_bytes_be().into()); @@ -625,7 +624,6 @@ impl CertificateParams { let mut params = CertificateParams { alg, is_ca, - extended_key_usages, name_constraints, serial_number, key_identifier_method, @@ -657,6 +655,10 @@ impl CertificateParams { ext::KeyUsage::from_parsed(&mut params, ku_ext)?; } + if let Some(eku_ext) = find_parsed_extension!(x509, ParsedExtension::ExtendedKeyUsage(_)) { + ext::ExtendedKeyUsage::from_parsed(&mut params, eku_ext)?; + } + Ok(params) } #[cfg(feature = "x509-parser")] @@ -690,41 +692,6 @@ impl CertificateParams { Ok(is_ca) } #[cfg(feature = "x509-parser")] - fn convert_x509_extended_key_usages( - x509: &x509_parser::certificate::X509Certificate<'_>, - ) -> Result, Error> { - let extended_key_usage = x509 - .extended_key_usage() - .or(Err(Error::CouldNotParseCertificate))? - .map(|ext| ext.value); - - let mut extended_key_usages = Vec::new(); - if let Some(extended_key_usage) = extended_key_usage { - if extended_key_usage.any { - extended_key_usages.push(ExtendedKeyUsagePurpose::Any); - } - if extended_key_usage.server_auth { - extended_key_usages.push(ExtendedKeyUsagePurpose::ServerAuth); - } - if extended_key_usage.client_auth { - extended_key_usages.push(ExtendedKeyUsagePurpose::ClientAuth); - } - if extended_key_usage.code_signing { - extended_key_usages.push(ExtendedKeyUsagePurpose::CodeSigning); - } - if extended_key_usage.email_protection { - extended_key_usages.push(ExtendedKeyUsagePurpose::EmailProtection); - } - if extended_key_usage.time_stamping { - extended_key_usages.push(ExtendedKeyUsagePurpose::TimeStamping); - } - if extended_key_usage.ocsp_signing { - extended_key_usages.push(ExtendedKeyUsagePurpose::OcspSigning); - } - } - Ok(extended_key_usages) - } - #[cfg(feature = "x509-parser")] fn convert_x509_name_constraints( x509: &x509_parser::certificate::X509Certificate<'_>, ) -> Result, Error> { @@ -797,8 +764,8 @@ impl CertificateParams { subject_alt_names, distinguished_name, is_ca, - key_usages, - extended_key_usages, + key_usages: _, + extended_key_usages: _, name_constraints, crl_distribution_points, custom_extensions, @@ -815,8 +782,6 @@ impl CertificateParams { let _ = (alg, key_pair, not_before, not_after, key_identifier_method); if serial_number.is_some() || *is_ca != IsCa::NoCa - || !key_usages.is_empty() - || !extended_key_usages.is_empty() || name_constraints.is_some() || !crl_distribution_points.is_empty() || *use_authority_key_identifier_extension @@ -919,22 +884,6 @@ impl CertificateParams { Extensions::write_extension(writer, ext); } - // Write extended key usage - if !self.extended_key_usages.is_empty() { - write_x509_extension( - writer.next(), - OID_EXT_KEY_USAGE, - false, - |writer| { - writer.write_sequence(|writer| { - for usage in self.extended_key_usages.iter() { - let oid = ObjectIdentifier::from_slice(usage.oid()); - writer.next().write_oid(&oid); - } - }); - }, - ); - } if let Some(name_constraints) = &self.name_constraints { // If both trees are empty, the extension must be omitted. if !name_constraints.is_empty() { @@ -1105,7 +1054,10 @@ impl CertificateParams { exts.add_extension(Box::new(ku_ext))?; } - // TODO: EKU. + if let Some(eku_ext) = ext::ExtendedKeyUsage::from_params(&self) { + exts.add_extension(Box::new(eku_ext))?; + } + // TODO: name constraints. // TODO: crl distribution points. // TODO: basic constraints. From 33c197774d85256958c73d0593dd45537c42923d Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 14:56:53 -0500 Subject: [PATCH 07/29] wip: non x509-feature fixes --- rcgen/src/csr.rs | 4 ++-- rcgen/src/lib.rs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/rcgen/src/csr.rs b/rcgen/src/csr.rs index 50ed304e..c9ce66d1 100644 --- a/rcgen/src/csr.rs +++ b/rcgen/src/csr.rs @@ -1,10 +1,10 @@ #[cfg(feature = "x509-parser")] -use crate::DistinguishedName; +use crate::{ext, DistinguishedName}; #[cfg(feature = "pem")] use pem::Pem; use std::hash::Hash; -use crate::{ext, Certificate, CertificateParams, Error, PublicKeyData, SignatureAlgorithm}; +use crate::{Certificate, CertificateParams, Error, PublicKeyData, SignatureAlgorithm}; /// A public key, extracted from a CSR #[derive(Debug, PartialEq, Eq, Hash)] diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index e30d0e0c..a2ccd3d0 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -45,6 +45,7 @@ use std::net::IpAddr; use std::net::{Ipv4Addr, Ipv6Addr}; use std::str::FromStr; use time::{Date, Month, OffsetDateTime, PrimitiveDateTime, Time}; +#[cfg(feature = "x509-parser")] use x509_parser::prelude::ParsedExtension; use yasna::models::ObjectIdentifier; use yasna::models::{GeneralizedTime, UTCTime}; From 559dc0916d173052771b8ee12a0ff1da1ea9d198 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 15:20:26 -0500 Subject: [PATCH 08/29] ext: implement name constraints This commit lifts the name constraints extension into the `ext` module. --- rcgen/src/csr.rs | 6 +- rcgen/src/error.rs | 5 ++ rcgen/src/ext.rs | 181 ++++++++++++++++++++++++++++++++++++++++++++- rcgen/src/lib.rs | 148 +++++++----------------------------- 4 files changed, 218 insertions(+), 122 deletions(-) diff --git a/rcgen/src/csr.rs b/rcgen/src/csr.rs index c9ce66d1..ceb15482 100644 --- a/rcgen/src/csr.rs +++ b/rcgen/src/csr.rs @@ -83,6 +83,11 @@ impl CertificateSigningRequest { Err(err) => return Err(err), _ => {}, // Not an EKU. } + match ext::NameConstraints::from_parsed(&mut params, ext) { + Ok(true) => continue, // NC extension handled. + Err(err) => return Err(err), + _ => {}, // Not an NC. + } // If we get here, we've encountered an unknown and unhandled extension. return Err(Error::UnsupportedExtension); @@ -91,7 +96,6 @@ impl CertificateSigningRequest { // Not yet handled: // * is_ca - // * name_constraints // and any other extensions. Ok(Self { diff --git a/rcgen/src/error.rs b/rcgen/src/error.rs index 0328cf2d..4f1788e0 100644 --- a/rcgen/src/error.rs +++ b/rcgen/src/error.rs @@ -41,6 +41,9 @@ pub enum Error { #[cfg(feature = "x509-parser")] /// Unsupported extension requested in CSR UnsupportedExtension, + /// Unsupported general name type in CSR + #[cfg(feature = "x509-parser")] + UnsupportedGeneralName, /// Unsupported field when generating a CSR UnsupportedInCsr, /// The requested signature algorithm is not supported @@ -96,6 +99,8 @@ impl fmt::Display for Error { DuplicateExtension(oid) => { write!(f, "Extension with OID {oid} present multiple times")? }, + #[cfg(feature = "x509-parser")] + UnsupportedGeneralName => write!(f, "Unsupported general name in CSR",)?, }; Ok(()) } diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 9fee58b1..cbf638fe 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -6,7 +6,8 @@ use yasna::models::ObjectIdentifier; use yasna::{DERWriter, DERWriterSeq, Tag}; use crate::{ - oid, Certificate, CertificateParams, Error, ExtendedKeyUsagePurpose, KeyUsagePurpose, SanType, + oid, write_distinguished_name, Certificate, CertificateParams, Error, ExtendedKeyUsagePurpose, + GeneralSubtree, KeyUsagePurpose, SanType, }; /// The criticality of an extension. @@ -458,6 +459,122 @@ impl Extension for ExtendedKeyUsage { } } +/// An X.509v3 name constraints extension according to +/// [RFC 5280 4.2.1.10](https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.10). +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct NameConstraints { + permitted_subtrees: Vec, + excluded_subtrees: Vec, +} + +impl NameConstraints { + pub(crate) fn from_params(params: &CertificateParams) -> Option { + match ¶ms.name_constraints { + Some(nc) if nc.permitted_subtrees.is_empty() && nc.excluded_subtrees.is_empty() => { + return None; // Avoid writing an empty name constraints extension. + }, + Some(nc) => Some(Self { + permitted_subtrees: nc.permitted_subtrees.clone(), + excluded_subtrees: nc.excluded_subtrees.clone(), + }), + _ => None, + } + } + + #[cfg(feature = "x509-parser")] + pub(crate) fn from_parsed( + params: &mut CertificateParams, + ext: &x509_parser::extensions::ParsedExtension, + ) -> Result { + Ok(match ext { + x509_parser::extensions::ParsedExtension::NameConstraints(ncs) => { + let mut permitted_subtrees = Vec::default(); + if let Some(ncs_permitted) = &ncs.permitted_subtrees { + permitted_subtrees = ncs_permitted + .iter() + .map(GeneralSubtree::from_x509_general_subtree) + .collect::, _>>()?; + } + let mut excluded_subtrees = Vec::default(); + if let Some(ncs_excluded) = &ncs.excluded_subtrees { + excluded_subtrees = ncs_excluded + .iter() + .map(GeneralSubtree::from_x509_general_subtree) + .collect::, _>>()?; + } + if !permitted_subtrees.is_empty() || !excluded_subtrees.is_empty() { + params.name_constraints = Some(crate::NameConstraints { + permitted_subtrees, + excluded_subtrees, + }); + } + true + }, + _ => false, + }) + } + + fn write_general_subtrees(writer: DERWriter, tag: u64, general_subtrees: &[GeneralSubtree]) { + /* + GeneralSubtrees ::= SEQUENCE SIZE (1..MAX) OF GeneralSubtree + GeneralSubtree ::= SEQUENCE { + base GeneralName, + minimum [0] BaseDistance DEFAULT 0, + maximum [1] BaseDistance OPTIONAL } + BaseDistance ::= INTEGER (0..MAX) + */ + writer.write_tagged_implicit(Tag::context(tag), |writer| { + writer.write_sequence(|writer| { + for subtree in general_subtrees.iter() { + writer.next().write_sequence(|writer| { + writer.next().write_tagged_implicit( + Tag::context(subtree.tag()), + |writer| match subtree { + GeneralSubtree::Rfc822Name(name) + | GeneralSubtree::DnsName(name) => writer.write_ia5_string(name), + GeneralSubtree::DirectoryName(name) => { + write_distinguished_name(writer, name) + }, + GeneralSubtree::IpAddress(subnet) => { + writer.write_bytes(&subnet.to_bytes()) + }, + }, + ); + // minimum must be 0 (the default) and maximum must be absent + }); + } + }); + }); + } +} + +impl Extension for NameConstraints { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_NAME_CONSTRAINTS) + } + + fn criticality(&self) -> Criticality { + // Conforming CAs MUST mark this extension as critical + Criticality::Critical + } + + fn write_value(&self, writer: DERWriter) { + /* + NameConstraints ::= SEQUENCE { + permittedSubtrees [0] GeneralSubtrees OPTIONAL, + excludedSubtrees [1] GeneralSubtrees OPTIONAL } + */ + writer.write_sequence(|writer| { + if !self.permitted_subtrees.is_empty() { + Self::write_general_subtrees(writer.next(), 0, &self.permitted_subtrees); + } + if !self.excluded_subtrees.is_empty() { + Self::write_general_subtrees(writer.next(), 1, &self.excluded_subtrees); + } + }); + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; @@ -733,3 +850,65 @@ mod eku_ext_tests { ); } } + +#[cfg(test)] +mod name_constraints_tests { + #[cfg(feature = "x509-parser")] + use x509_parser::prelude::FromDer; + + use super::*; + use crate::CertificateParams; + + #[test] + fn test_from_params() { + let constraints = crate::NameConstraints { + permitted_subtrees: vec![GeneralSubtree::DnsName("com".into())], + excluded_subtrees: vec![GeneralSubtree::DnsName("org".into())], + }; + let mut params = CertificateParams::default(); + params.name_constraints = Some(constraints.clone()); + + let ext = NameConstraints::from_params(¶ms).unwrap(); + assert_eq!(ext.permitted_subtrees, constraints.permitted_subtrees); + assert_eq!(ext.excluded_subtrees, constraints.excluded_subtrees); + } + + #[test] + #[cfg(feature = "x509-parser")] + fn test_from_parsed() { + let mut params = CertificateParams::default(); + let constraints = crate::NameConstraints { + permitted_subtrees: vec![GeneralSubtree::DnsName("com".into())], + excluded_subtrees: Vec::default(), + }; + params.name_constraints = Some(constraints.clone()); + + let der = yasna::construct_der(|writer| { + NameConstraints::from_params(¶ms) + .unwrap() + .write_value(writer) + }); + + let parsed_ext = x509_parser::extensions::ParsedExtension::NameConstraints( + x509_parser::extensions::NameConstraints::from_der(&der) + .unwrap() + .1, + ); + + let mut recovered_params = CertificateParams::default(); + NameConstraints::from_parsed(&mut recovered_params, &parsed_ext).unwrap(); + assert!(recovered_params.name_constraints.is_some()); + assert_eq!( + recovered_params + .name_constraints + .as_ref() + .unwrap() + .permitted_subtrees, + constraints.permitted_subtrees, + ); + assert_eq!( + recovered_params.name_constraints.unwrap().excluded_subtrees, + constraints.excluded_subtrees, + ); + } +} diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index a2ccd3d0..3d18395f 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -222,6 +222,24 @@ impl GeneralSubtree { GeneralSubtree::IpAddress(_addr) => TAG_IP_ADDRESS, } } + + #[cfg(feature = "x509-parser")] + pub(crate) fn from_x509_general_subtree( + general_subtree: &x509_parser::extensions::GeneralSubtree, + ) -> Result { + use x509_parser::extensions::GeneralName as X509GeneralName; + match &general_subtree.base { + X509GeneralName::RFC822Name(name) => Ok(GeneralSubtree::Rfc822Name(name.to_string())), + X509GeneralName::DNSName(name) => Ok(GeneralSubtree::DnsName(name.to_string())), + X509GeneralName::DirectoryName(name) => Ok(GeneralSubtree::DirectoryName( + DistinguishedName::from_name(name)?, + )), + // TODO(XXX): Consider how to handle the &[u8] that x509_parser provides. + // It would need to be mapped into the rcgen CidrSubnet type. + // GeneralName::IPAddress(addr) => GeneralSubtree::IpAddress(...) + _ => Err(Error::UnsupportedGeneralName), + } + } } #[derive(Debug, PartialEq, Eq, Hash, Clone)] @@ -609,7 +627,6 @@ impl CertificateParams { let dn = DistinguishedName::from_name(&x509.tbs_certificate.subject)?; let is_ca = Self::convert_x509_is_ca(&x509)?; let validity = x509.validity(); - let name_constraints = Self::convert_x509_name_constraints(&x509)?; let serial_number = Some(x509.serial.to_bytes_be().into()); let key_identifier_method = x509 @@ -625,7 +642,6 @@ impl CertificateParams { let mut params = CertificateParams { alg, is_ca, - name_constraints, serial_number, key_identifier_method, distinguished_name: dn, @@ -660,6 +676,12 @@ impl CertificateParams { ext::ExtendedKeyUsage::from_parsed(&mut params, eku_ext)?; } + if let Some(name_constraints) = + find_parsed_extension!(x509, ParsedExtension::NameConstraints(_)) + { + ext::NameConstraints::from_parsed(&mut params, name_constraints)?; + } + Ok(params) } #[cfg(feature = "x509-parser")] @@ -692,68 +714,6 @@ impl CertificateParams { Ok(is_ca) } - #[cfg(feature = "x509-parser")] - fn convert_x509_name_constraints( - x509: &x509_parser::certificate::X509Certificate<'_>, - ) -> Result, Error> { - let constraints = x509 - .name_constraints() - .or(Err(Error::CouldNotParseCertificate))? - .map(|ext| ext.value); - - if let Some(constraints) = constraints { - let permitted_subtrees = if let Some(permitted) = &constraints.permitted_subtrees { - Self::convert_x509_general_subtrees(&permitted)? - } else { - Vec::new() - }; - - let excluded_subtrees = if let Some(excluded) = &constraints.excluded_subtrees { - Self::convert_x509_general_subtrees(&excluded)? - } else { - Vec::new() - }; - - let name_constraints = NameConstraints { - permitted_subtrees, - excluded_subtrees, - }; - - Ok(Some(name_constraints)) - } else { - Ok(None) - } - } - #[cfg(feature = "x509-parser")] - fn convert_x509_general_subtrees( - subtrees: &[x509_parser::extensions::GeneralSubtree<'_>], - ) -> Result, Error> { - use x509_parser::extensions::GeneralName; - - let mut result = Vec::new(); - for subtree in subtrees { - let subtree = match &subtree.base { - GeneralName::RFC822Name(s) => GeneralSubtree::Rfc822Name(s.to_string()), - GeneralName::DNSName(s) => GeneralSubtree::DnsName(s.to_string()), - GeneralName::DirectoryName(n) => { - GeneralSubtree::DirectoryName(DistinguishedName::from_name(&n)?) - }, - GeneralName::IPAddress(bytes) if bytes.len() == 8 => { - let addr: [u8; 4] = bytes[..4].try_into().unwrap(); - let mask: [u8; 4] = bytes[4..].try_into().unwrap(); - GeneralSubtree::IpAddress(CidrSubnet::V4(addr, mask)) - }, - GeneralName::IPAddress(bytes) if bytes.len() == 32 => { - let addr: [u8; 16] = bytes[..16].try_into().unwrap(); - let mask: [u8; 16] = bytes[16..].try_into().unwrap(); - GeneralSubtree::IpAddress(CidrSubnet::V6(addr, mask)) - }, - _ => continue, - }; - result.push(subtree); - } - Ok(result) - } fn write_request(&self, pub_key: &K, writer: DERWriter) -> Result<(), Error> { // No .. pattern, we use this to ensure every field is used #[deny(unused)] @@ -885,34 +845,6 @@ impl CertificateParams { Extensions::write_extension(writer, ext); } - if let Some(name_constraints) = &self.name_constraints { - // If both trees are empty, the extension must be omitted. - if !name_constraints.is_empty() { - write_x509_extension( - writer.next(), - OID_NAME_CONSTRAINTS, - true, - |writer| { - writer.write_sequence(|writer| { - if !name_constraints.permitted_subtrees.is_empty() { - write_general_subtrees( - writer.next(), - 0, - &name_constraints.permitted_subtrees, - ); - } - if !name_constraints.excluded_subtrees.is_empty() { - write_general_subtrees( - writer.next(), - 1, - &name_constraints.excluded_subtrees, - ); - } - }); - }, - ); - } - } if !self.crl_distribution_points.is_empty() { write_x509_extension( writer.next(), @@ -1059,7 +991,10 @@ impl CertificateParams { exts.add_extension(Box::new(eku_ext))?; } - // TODO: name constraints. + if let Some(name_constraints_ext) = ext::NameConstraints::from_params(&self) { + exts.add_extension(Box::new(name_constraints_ext))?; + } + // TODO: crl distribution points. // TODO: basic constraints. // TODO: subject key identifier. @@ -1339,33 +1274,6 @@ fn write_distinguished_name(writer: DERWriter, dn: &DistinguishedName) { }); } -fn write_general_subtrees(writer: DERWriter, tag: u64, general_subtrees: &[GeneralSubtree]) { - writer.write_tagged_implicit(Tag::context(tag), |writer| { - writer.write_sequence(|writer| { - for subtree in general_subtrees.iter() { - writer.next().write_sequence(|writer| { - writer - .next() - .write_tagged_implicit( - Tag::context(subtree.tag()), - |writer| match subtree { - GeneralSubtree::Rfc822Name(name) - | GeneralSubtree::DnsName(name) => writer.write_ia5_string(name), - GeneralSubtree::DirectoryName(name) => { - write_distinguished_name(writer, name) - }, - GeneralSubtree::IpAddress(subnet) => { - writer.write_bytes(&subnet.to_bytes()) - }, - }, - ); - // minimum must be 0 (the default) and maximum must be absent - }); - } - }); - }); -} - impl Certificate { /// Generates a new certificate from the given parameters. /// From 56e3c3c19d360544fc618596dd8fd500c0ea6f8c Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 15:29:14 -0500 Subject: [PATCH 09/29] wip: fixup with name constraints --- rcgen/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 3d18395f..ac0fbdea 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -743,7 +743,6 @@ impl CertificateParams { let _ = (alg, key_pair, not_before, not_after, key_identifier_method); if serial_number.is_some() || *is_ca != IsCa::NoCa - || name_constraints.is_some() || !crl_distribution_points.is_empty() || *use_authority_key_identifier_extension { From e5bcd11dcd9f137e512086e4e6ca4265891c0201 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 15:29:36 -0500 Subject: [PATCH 10/29] wip: and again... --- rcgen/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index ac0fbdea..bd29ecef 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -727,7 +727,7 @@ impl CertificateParams { is_ca, key_usages: _, extended_key_usages: _, - name_constraints, + name_constraints: _, crl_distribution_points, custom_extensions, key_pair, From 5ab3d947af72fd48dbd20091f24bb98c75503534 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 15:36:05 -0500 Subject: [PATCH 11/29] ext: implement CRL distribution points This commit lifts the CRL distribution points extension into the `ext` module. --- rcgen/src/csr.rs | 5 ++ rcgen/src/error.rs | 5 ++ rcgen/src/ext.rs | 133 +++++++++++++++++++++++++++++++++++++++++++++ rcgen/src/lib.rs | 28 ++++------ 4 files changed, 154 insertions(+), 17 deletions(-) diff --git a/rcgen/src/csr.rs b/rcgen/src/csr.rs index ceb15482..71a18a3e 100644 --- a/rcgen/src/csr.rs +++ b/rcgen/src/csr.rs @@ -88,6 +88,11 @@ impl CertificateSigningRequest { Err(err) => return Err(err), _ => {}, // Not an NC. } + match ext::CrlDistributionPoints::from_parsed(&mut params, ext) { + Ok(true) => continue, // CDP extension handled. + Err(err) => return Err(err), + _ => {}, // Not a CDP. + } // If we get here, we've encountered an unknown and unhandled extension. return Err(Error::UnsupportedExtension); diff --git a/rcgen/src/error.rs b/rcgen/src/error.rs index 4f1788e0..9f029f92 100644 --- a/rcgen/src/error.rs +++ b/rcgen/src/error.rs @@ -38,6 +38,9 @@ pub enum Error { RingUnspecified, /// Time conversion related errors Time, + /// Unsupported CRL distribution point extension in CSR + #[cfg(feature = "x509-parser")] + UnsupportedCrlDistributionPoint, #[cfg(feature = "x509-parser")] /// Unsupported extension requested in CSR UnsupportedExtension, @@ -101,6 +104,8 @@ impl fmt::Display for Error { }, #[cfg(feature = "x509-parser")] UnsupportedGeneralName => write!(f, "Unsupported general name in CSR",)?, + #[cfg(feature = "x509-parser")] + UnsupportedCrlDistributionPoint => write!(f, "Unsupported CRL distribution point in CSR",)?, }; Ok(()) } diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index cbf638fe..0f9aefe0 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -575,6 +575,87 @@ impl Extension for NameConstraints { } } +/// An X.509v3 CRL distribution points extension according to +/// [RFC 5280 4.2.1.13](https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.13). +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct CrlDistributionPoints { + distribution_points: Vec, +} + +impl CrlDistributionPoints { + pub(crate) fn from_params(params: &CertificateParams) -> Option { + match params.crl_distribution_points.is_empty() { + true => return None, // Avoid writing an empty CRL distribution points extension. + false => Some(Self { + distribution_points: params.crl_distribution_points.clone(), + }), + } + } + + #[cfg(feature = "x509-parser")] + pub(crate) fn from_parsed( + params: &mut CertificateParams, + ext: &x509_parser::extensions::ParsedExtension, + ) -> Result { + Ok(match ext { + x509_parser::extensions::ParsedExtension::CRLDistributionPoints(crl_dps) => { + let dps = crl_dps + .points + .iter() + .map(|dp| { + // Rcgen does not support CRL DPs with specific reasons, or an indirect issuer. + if dp.reasons.is_some() || dp.crl_issuer.is_some() { + return Err(Error::UnsupportedCrlDistributionPoint); + } + let general_names = match &dp.distribution_point { + Some(x509_parser::extensions::DistributionPointName::FullName( + general_names, + )) => Ok(general_names), + // Rcgen does not support CRL DPs missing a distribution point, + // or that specific a distribution point with a name relative + // to an issuer. + _ => Err(Error::UnsupportedCrlDistributionPoint), + }?; + let uris = general_names + .iter() + .map(|general_name| match general_name { + x509_parser::extensions::GeneralName::URI(uri) => { + Ok(uri.to_string()) + }, + // Rcgen does not support CRL DP general names other than URI. + _ => Err(Error::UnsupportedGeneralName), + }) + .collect::, _>>()?; + Ok(crate::CrlDistributionPoint { uris }) + }) + .collect::, _>>()?; + params.crl_distribution_points = dps; + true + }, + _ => false, + }) + } +} + +impl Extension for CrlDistributionPoints { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_CRL_DISTRIBUTION_POINTS) + } + + fn criticality(&self) -> Criticality { + // The extension SHOULD be non-critical + Criticality::NonCritical + } + + fn write_value(&self, writer: DERWriter) { + writer.write_sequence(|writer| { + for distribution_point in &self.distribution_points { + distribution_point.write_der(writer.next()); + } + }) + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; @@ -912,3 +993,55 @@ mod name_constraints_tests { ); } } + +#[cfg(test)] +mod crl_dps_test { + #[cfg(feature = "x509-parser")] + use x509_parser::prelude::FromDer; + + use super::*; + use crate::CertificateParams; + + #[test] + fn test_from_params() { + let crl_dps = vec![ + crate::CrlDistributionPoint { + uris: vec!["http://example.com".into()], + }, + crate::CrlDistributionPoint { + uris: vec!["http://example.org".into(), "ldap://example.com".into()], + }, + ]; + let mut params = CertificateParams::default(); + params.crl_distribution_points = crl_dps.clone(); + + let ext = CrlDistributionPoints::from_params(¶ms).unwrap(); + assert_eq!(ext.distribution_points, crl_dps); + } + + #[test] + #[cfg(feature = "x509-parser")] + fn test_from_parsed() { + let mut params = CertificateParams::default(); + let crl_dps = vec![crate::CrlDistributionPoint { + uris: vec!["http://example.com".into()], + }]; + params.crl_distribution_points = crl_dps.clone(); + + let der = yasna::construct_der(|writer| { + CrlDistributionPoints::from_params(¶ms) + .unwrap() + .write_value(writer) + }); + + let parsed_ext = x509_parser::extensions::ParsedExtension::CRLDistributionPoints( + x509_parser::extensions::CRLDistributionPoints::from_der(&der) + .unwrap() + .1, + ); + + let mut recovered_params = CertificateParams::default(); + CrlDistributionPoints::from_parsed(&mut recovered_params, &parsed_ext).unwrap(); + assert_eq!(recovered_params.crl_distribution_points, crl_dps,); + } +} diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index bd29ecef..6eb497ed 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -682,6 +682,12 @@ impl CertificateParams { ext::NameConstraints::from_parsed(&mut params, name_constraints)?; } + if let Some(crl_dps) = + find_parsed_extension!(x509, ParsedExtension::CrlDistributionPoints(_)) + { + ext::CrlDistributionPoints::from_parsed(&mut params, crl_dps)?; + } + Ok(params) } #[cfg(feature = "x509-parser")] @@ -728,7 +734,7 @@ impl CertificateParams { key_usages: _, extended_key_usages: _, name_constraints: _, - crl_distribution_points, + crl_distribution_points: _, custom_extensions, key_pair, use_authority_key_identifier_extension, @@ -743,7 +749,6 @@ impl CertificateParams { let _ = (alg, key_pair, not_before, not_after, key_identifier_method); if serial_number.is_some() || *is_ca != IsCa::NoCa - || !crl_distribution_points.is_empty() || *use_authority_key_identifier_extension { return Err(Error::UnsupportedInCsr); @@ -844,20 +849,6 @@ impl CertificateParams { Extensions::write_extension(writer, ext); } - if !self.crl_distribution_points.is_empty() { - write_x509_extension( - writer.next(), - OID_CRL_DISTRIBUTION_POINTS, - false, - |writer| { - writer.write_sequence(|writer| { - for distribution_point in &self.crl_distribution_points { - distribution_point.write_der(writer.next()); - } - }) - }, - ); - } match self.is_ca { IsCa::Ca(ref constraint) => { // Write subject_key_identifier @@ -994,7 +985,10 @@ impl CertificateParams { exts.add_extension(Box::new(name_constraints_ext))?; } - // TODO: crl distribution points. + if let Some(crl_dps) = ext::CrlDistributionPoints::from_params(&self) { + exts.add_extension(Box::new(crl_dps))?; + } + // TODO: basic constraints. // TODO: subject key identifier. // TODO: custom extensions From 28413686466b7994fe10c3dc54a5056218cb109d Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 15:40:39 -0500 Subject: [PATCH 12/29] wip: fixup with crldps --- rcgen/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 6eb497ed..cc426ce6 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -683,7 +683,7 @@ impl CertificateParams { } if let Some(crl_dps) = - find_parsed_extension!(x509, ParsedExtension::CrlDistributionPoints(_)) + find_parsed_extension!(x509, ParsedExtension::CRLDistributionPoints(_)) { ext::CrlDistributionPoints::from_parsed(&mut params, crl_dps)?; } From 6b9e715e19d49d4ece53fb827b28c7f56cdb1eec Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 16:13:14 -0500 Subject: [PATCH 13/29] wip: add a TODO --- rcgen/src/csr.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rcgen/src/csr.rs b/rcgen/src/csr.rs index 71a18a3e..9e47ae7a 100644 --- a/rcgen/src/csr.rs +++ b/rcgen/src/csr.rs @@ -45,6 +45,7 @@ impl CertificateSigningRequest { /// /// Currently, this only supports the `Subject Alternative Name` extension. /// On encountering other extensions, this function will return an error. + // TODO(@cpu): update this doc comment. #[cfg(feature = "x509-parser")] pub fn from_der(csr: &[u8]) -> Result { use x509_parser::prelude::FromDer; From a447dc66f1e7adb05b84eee8c2f5485672b37a3c Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 16:24:22 -0500 Subject: [PATCH 14/29] ext: implement subject key ID, specifying SKI This commit lifts the subject key identifier extension into the `ext` module. Diverging from the existing code we now adhere to the RFC 5280 advice and always emit the SKI extension when generating a certificate. Previously this was only done if the basic constraints specified `IsCa::Ca` or `IsCa::ExplicitNoCa`, but not when using `IsCa::NoCa`. --- rcgen/src/csr.rs | 5 +++ rcgen/src/ext.rs | 105 +++++++++++++++++++++++++++++++++++++++++++++++ rcgen/src/lib.rs | 55 +++++++++---------------- 3 files changed, 130 insertions(+), 35 deletions(-) diff --git a/rcgen/src/csr.rs b/rcgen/src/csr.rs index 9e47ae7a..b0da62f4 100644 --- a/rcgen/src/csr.rs +++ b/rcgen/src/csr.rs @@ -94,6 +94,11 @@ impl CertificateSigningRequest { Err(err) => return Err(err), _ => {}, // Not a CDP. } + match ext::SubjectKeyIdentifier::from_parsed(&mut params, ext) { + Ok(true) => continue, // SKI extension handled. + Err(err) => return Err(err), + _ => {}, // Not a SKI. + } // If we get here, we've encountered an unknown and unhandled extension. return Err(Error::UnsupportedExtension); diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 0f9aefe0..2c6496d0 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -5,6 +5,7 @@ use std::net::IpAddr; use yasna::models::ObjectIdentifier; use yasna::{DERWriter, DERWriterSeq, Tag}; +use crate::key_pair::PublicKeyData; use crate::{ oid, write_distinguished_name, Certificate, CertificateParams, Error, ExtendedKeyUsagePurpose, GeneralSubtree, KeyUsagePurpose, SanType, @@ -656,6 +657,52 @@ impl Extension for CrlDistributionPoints { } } +/// An X.509v3 subject key identifier extension according to +/// [RFC 5280 4.2.1.2](https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.2). +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct SubjectKeyIdentifier { + key_identifier: Vec, +} + +impl SubjectKeyIdentifier { + pub(crate) fn from_params(params: &CertificateParams, pub_key: &K) -> Self { + Self { + key_identifier: params.key_identifier(pub_key), + } + } + + #[cfg(feature = "x509-parser")] + pub(crate) fn from_parsed( + params: &mut CertificateParams, + ext: &x509_parser::extensions::ParsedExtension, + ) -> Result { + Ok(match ext { + x509_parser::extensions::ParsedExtension::SubjectKeyIdentifier(ski) => { + params.key_identifier_method = crate::KeyIdMethod::PreSpecified(ski.0.to_vec()); + true + }, + _ => false, + }) + } +} + +impl Extension for SubjectKeyIdentifier { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_SUBJECT_KEY_IDENTIFIER) + } + + fn criticality(&self) -> Criticality { + // Conforming CAs MUST mark this extension as non-critical. + Criticality::NonCritical + } + + fn write_value(&self, writer: DERWriter) { + // SubjectKeyIdentifier ::= KeyIdentifier + // KeyIdentifier ::= OCTET STRING + writer.write_bytes(&self.key_identifier) + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; @@ -1045,3 +1092,61 @@ mod crl_dps_test { assert_eq!(recovered_params.crl_distribution_points, crl_dps,); } } + +#[cfg(test)] +mod ski_ext_tests { + #[cfg(feature = "x509-parser")] + use x509_parser::prelude::FromDer; + + use super::*; + use crate::{CertificateParams, KeyIdMethod, KeyPair}; + + #[test] + fn test_from_params() { + let ski = vec![1, 2, 3, 4]; + let mut params = CertificateParams::default(); + params.key_identifier_method = KeyIdMethod::PreSpecified(ski.clone()); + + let keypair = KeyPair::generate(&crate::PKCS_ECDSA_P256_SHA256).unwrap(); + let ext = SubjectKeyIdentifier::from_params(¶ms, &keypair); + assert_eq!(ext.key_identifier, ski); + + let keypair = KeyPair::generate(&crate::PKCS_ECDSA_P256_SHA256).unwrap(); + let mut params = CertificateParams::default(); + params.key_pair = Some(keypair); + + let keypair = params.key_pair.as_ref().unwrap(); + let ext = SubjectKeyIdentifier::from_params(¶ms, keypair); + assert_ne!(ext.key_identifier, ski); + assert_eq!(ext.key_identifier.len(), 20); // SHA-256 digest truncated to SHA-1 length + } + + #[test] + #[cfg(feature = "x509-parser")] + fn test_from_parsed() { + let ski = vec![1, 2, 3, 4]; + let keypair = KeyPair::generate(&crate::PKCS_ECDSA_P256_SHA256).unwrap(); + + let mut params = CertificateParams::default(); + params.key_pair = Some(keypair); + params.key_identifier_method = KeyIdMethod::PreSpecified(ski.clone()); + + let keypair = params.key_pair.as_ref().unwrap(); + let der = yasna::construct_der(|writer| { + SubjectKeyIdentifier::from_params(¶ms, keypair).write_value(writer) + }); + + let parsed_ext = x509_parser::extensions::ParsedExtension::SubjectKeyIdentifier( + x509_parser::extensions::KeyIdentifier::from_der(&der) + .unwrap() + .1, + ); + + let mut recovered_params = CertificateParams::default(); + SubjectKeyIdentifier::from_parsed(&mut recovered_params, &parsed_ext).unwrap(); + assert_eq!( + recovered_params.key_identifier_method, + KeyIdMethod::PreSpecified(ski) + ); + } +} diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index cc426ce6..fcff2c11 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -629,21 +629,10 @@ impl CertificateParams { let validity = x509.validity(); let serial_number = Some(x509.serial.to_bytes_be().into()); - let key_identifier_method = x509 - .iter_extensions() - .find_map(|ext| match ext.parsed_extension() { - ParsedExtension::SubjectKeyIdentifier(key_id) => { - Some(KeyIdMethod::PreSpecified(key_id.0.into())) - }, - _ => None, - }) - .unwrap_or(KeyIdMethod::Sha256); - let mut params = CertificateParams { alg, is_ca, serial_number, - key_identifier_method, distinguished_name: dn, key_pair: Some(key_pair), not_before: validity.not_before.to_datetime(), @@ -688,6 +677,10 @@ impl CertificateParams { ext::CrlDistributionPoints::from_parsed(&mut params, crl_dps)?; } + if let Some(ski) = find_parsed_extension!(x509, ParsedExtension::SubjectKeyIdentifier(_)) { + ext::SubjectKeyIdentifier::from_parsed(&mut params, ski)?; + } + Ok(params) } #[cfg(feature = "x509-parser")] @@ -763,7 +756,7 @@ impl CertificateParams { // Write extensions // According to the spec in RFC 2986, even if attributes are empty we need the empty attribute tag writer.next().write_tagged(Tag::context(0), |writer| { - let extensions = self.extensions(None)?; + let extensions = self.extensions(pub_key, None)?; if !subject_alt_names.is_empty() || !custom_extensions.is_empty() { writer.write_sequence(|writer| { let oid = ObjectIdentifier::from_slice(OID_PKCS_9_AT_EXTENSION_REQUEST); @@ -831,7 +824,7 @@ impl CertificateParams { // Write subjectPublicKeyInfo pub_key.serialize_public_key_der(writer.next()); // write extensions - let extensions = self.extensions(Some(ca))?; + let extensions = self.extensions(pub_key, Some(ca))?; let should_write_exts = self.use_authority_key_identifier_extension || !self.subject_alt_names.is_empty() || !self.extended_key_usages.is_empty() @@ -851,16 +844,6 @@ impl CertificateParams { match self.is_ca { IsCa::Ca(ref constraint) => { - // Write subject_key_identifier - write_x509_extension( - writer.next(), - OID_SUBJECT_KEY_IDENTIFIER, - false, - |writer| { - let key_identifier = self.key_identifier(pub_key); - writer.write_bytes(key_identifier.as_ref()); - }, - ); // Write basic_constraints write_x509_extension( writer.next(), @@ -880,16 +863,6 @@ impl CertificateParams { ); }, IsCa::ExplicitNoCa => { - // Write subject_key_identifier - write_x509_extension( - writer.next(), - OID_SUBJECT_KEY_IDENTIFIER, - false, - |writer| { - let key_identifier = self.key_identifier(pub_key); - writer.write_bytes(key_identifier.as_ref()); - }, - ); // Write basic_constraints write_x509_extension( writer.next(), @@ -960,9 +933,16 @@ impl CertificateParams { /// Returns the X.509 extensions that the [CertificateParams] describe, or an [Error] /// if the described extensions are invalid. /// + /// The returned extensions will include a subject sublic key identifier extension for the + /// provided [PublicKeyData]. + /// /// If an issuer [Certificate] is provided, additional extensions specific to the issuer will /// be included (e.g. the authority key identifier). - fn extensions(&self, issuer: Option<&Certificate>) -> Result { + fn extensions( + &self, + pub_key: &K, + issuer: Option<&Certificate>, + ) -> Result { let mut exts = Extensions::default(); if let Some(issuer) = issuer { @@ -989,8 +969,13 @@ impl CertificateParams { exts.add_extension(Box::new(crl_dps))?; } + // RFC 5280 describes emitting a SKI as a MUST for CA certs and a SHOULD for EE, + // so we emit it unconditionally for all certs. + exts.add_extension(Box::new(ext::SubjectKeyIdentifier::from_params( + &self, pub_key, + )))?; + // TODO: basic constraints. - // TODO: subject key identifier. // TODO: custom extensions Ok(exts) From d310765533eeb4ee91dfd106647b98e5be58a183 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 16:50:40 -0500 Subject: [PATCH 15/29] ext: implement basic constraints This commit lifts the basic constraints extension into the `ext` module. --- rcgen/src/csr.rs | 8 +- rcgen/src/error.rs | 8 ++ rcgen/src/ext.rs | 195 ++++++++++++++++++++++++++++++++++++++++++++- rcgen/src/lib.rs | 77 ++---------------- 4 files changed, 216 insertions(+), 72 deletions(-) diff --git a/rcgen/src/csr.rs b/rcgen/src/csr.rs index b0da62f4..a6d08956 100644 --- a/rcgen/src/csr.rs +++ b/rcgen/src/csr.rs @@ -99,6 +99,11 @@ impl CertificateSigningRequest { Err(err) => return Err(err), _ => {}, // Not a SKI. } + match ext::BasicConstraints::from_parsed(&mut params, ext) { + Ok(true) => continue, // BC extension handled. + Err(err) => return Err(err), + _ => {}, // Not a BC. + } // If we get here, we've encountered an unknown and unhandled extension. return Err(Error::UnsupportedExtension); @@ -106,8 +111,7 @@ impl CertificateSigningRequest { } // Not yet handled: - // * is_ca - // and any other extensions. + // any other extensions. Ok(Self { params, diff --git a/rcgen/src/error.rs b/rcgen/src/error.rs index 9f029f92..8d985ac1 100644 --- a/rcgen/src/error.rs +++ b/rcgen/src/error.rs @@ -38,6 +38,9 @@ pub enum Error { RingUnspecified, /// Time conversion related errors Time, + /// Unsupported basic constraints extension path length in CSR + #[cfg(feature = "x509-parser")] + UnsupportedBasicConstraintsPathLen, /// Unsupported CRL distribution point extension in CSR #[cfg(feature = "x509-parser")] UnsupportedCrlDistributionPoint, @@ -106,6 +109,11 @@ impl fmt::Display for Error { UnsupportedGeneralName => write!(f, "Unsupported general name in CSR",)?, #[cfg(feature = "x509-parser")] UnsupportedCrlDistributionPoint => write!(f, "Unsupported CRL distribution point in CSR",)?, + #[cfg(feature = "x509-parser")] + UnsupportedBasicConstraintsPathLen => write!( + f, + "Unsupported basic constraints extension path length constraint in CSR" + )?, }; Ok(()) } diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 2c6496d0..b55a325b 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -8,7 +8,7 @@ use yasna::{DERWriter, DERWriterSeq, Tag}; use crate::key_pair::PublicKeyData; use crate::{ oid, write_distinguished_name, Certificate, CertificateParams, Error, ExtendedKeyUsagePurpose, - GeneralSubtree, KeyUsagePurpose, SanType, + GeneralSubtree, IsCa, KeyUsagePurpose, SanType, }; /// The criticality of an extension. @@ -703,6 +703,82 @@ impl Extension for SubjectKeyIdentifier { } } +/// An X.509v3 basic constraints extension according to +/// [RFC 5280 4.2.1.9](https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.9). +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct BasicConstraints { + is_ca: IsCa, +} + +impl BasicConstraints { + pub(crate) fn from_params(params: &CertificateParams) -> Option { + // For NoCa we don't emit the extension, it is implied not a CA. + // Use ExplicitNoCa when you want the false cA ext emitted. + match params.is_ca { + IsCa::NoCa => None, + _ => Some(Self { + is_ca: params.is_ca.clone(), + }), + } + } + + #[cfg(feature = "x509-parser")] + pub(crate) fn from_parsed( + params: &mut CertificateParams, + ext: &x509_parser::extensions::ParsedExtension, + ) -> Result { + Ok(match ext { + x509_parser::extensions::ParsedExtension::BasicConstraints(bc) => { + match (bc.ca, bc.path_len_constraint) { + (true, Some(len)) => { + params.is_ca = IsCa::Ca(crate::BasicConstraints::Constrained( + u8::try_from(len) + .map_err(|_| Error::UnsupportedBasicConstraintsPathLen)?, + )); + }, + (true, None) => { + params.is_ca = IsCa::Ca(crate::BasicConstraints::Unconstrained); + }, + (false, _) => { + params.is_ca = IsCa::ExplicitNoCa; + }, + } + true + }, + _ => false, + }) + } +} + +impl Extension for BasicConstraints { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_BASIC_CONSTRAINTS) + } + + fn criticality(&self) -> Criticality { + // Conforming CAs MUST include this extension in all CA certificates + // that contain public keys used to validate digital signatures on + // certificates and MUST mark the extension as critical in such + // certificates + Criticality::Critical + } + + fn write_value(&self, writer: DERWriter) { + /* + BasicConstraints ::= SEQUENCE { + cA BOOLEAN DEFAULT FALSE, + pathLenConstraint INTEGER (0..MAX) OPTIONAL } + */ + writer.write_sequence(|writer| { + writer.next().write_bool(matches!(self.is_ca, IsCa::Ca(_))); + if let IsCa::Ca(crate::BasicConstraints::Constrained(path_len_constraint)) = self.is_ca + { + writer.next().write_u8(path_len_constraint); + } + }); + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; @@ -1150,3 +1226,120 @@ mod ski_ext_tests { ); } } + +#[cfg(test)] +mod bc_ext_tests { + #[cfg(feature = "x509-parser")] + use x509_parser::prelude::FromDer; + + use super::*; + use crate::CertificateParams; + + #[test] + fn test_from_params() { + // NoCA + let params = CertificateParams::default(); + let ext = BasicConstraints::from_params(¶ms); + assert!(ext.is_none()); // No ext for NoCA. + + // Explicit NoCA + let mut params = CertificateParams::default(); + params.is_ca = IsCa::ExplicitNoCa; + let ext = BasicConstraints::from_params(¶ms); + assert_eq!(ext.unwrap().is_ca, IsCa::ExplicitNoCa); + + // CA unconstrained + let mut params = CertificateParams::default(); + params.is_ca = IsCa::Ca(crate::BasicConstraints::Unconstrained); + let ext = BasicConstraints::from_params(¶ms); + assert_eq!( + ext.unwrap().is_ca, + IsCa::Ca(crate::BasicConstraints::Unconstrained) + ); + + // CA constrained + let mut params = CertificateParams::default(); + params.is_ca = IsCa::Ca(crate::BasicConstraints::Constrained(1)); + let ext = BasicConstraints::from_params(¶ms); + assert_eq!( + ext.unwrap().is_ca, + IsCa::Ca(crate::BasicConstraints::Constrained(1)) + ); + } + + #[test] + #[cfg(feature = "x509-parser")] + fn test_from_parsed_explicit_no_ca() { + let mut params = CertificateParams::default(); + params.is_ca = IsCa::ExplicitNoCa; + + let der = yasna::construct_der(|writer| { + BasicConstraints::from_params(¶ms) + .unwrap() + .write_value(writer) + }); + + let parsed_ext = x509_parser::extensions::ParsedExtension::BasicConstraints( + x509_parser::extensions::BasicConstraints::from_der(&der) + .unwrap() + .1, + ); + + let mut recovered_params = CertificateParams::default(); + BasicConstraints::from_parsed(&mut recovered_params, &parsed_ext).unwrap(); + assert_eq!(recovered_params.is_ca, IsCa::ExplicitNoCa); + } + + #[test] + #[cfg(feature = "x509-parser")] + fn test_from_parsed_unconstrained() { + let mut params = CertificateParams::default(); + params.is_ca = IsCa::Ca(crate::BasicConstraints::Unconstrained); + + let der = yasna::construct_der(|writer| { + BasicConstraints::from_params(¶ms) + .unwrap() + .write_value(writer) + }); + + let parsed_ext = x509_parser::extensions::ParsedExtension::BasicConstraints( + x509_parser::extensions::BasicConstraints::from_der(&der) + .unwrap() + .1, + ); + + let mut recovered_params = CertificateParams::default(); + BasicConstraints::from_parsed(&mut recovered_params, &parsed_ext).unwrap(); + assert_eq!( + recovered_params.is_ca, + IsCa::Ca(crate::BasicConstraints::Unconstrained) + ); + } + + #[test] + #[cfg(feature = "x509-parser")] + fn test_from_parsed_constrained() { + let mut params = CertificateParams::default(); + let path_len = 5; + params.is_ca = IsCa::Ca(crate::BasicConstraints::Constrained(path_len)); + + let der = yasna::construct_der(|writer| { + BasicConstraints::from_params(¶ms) + .unwrap() + .write_value(writer) + }); + + let parsed_ext = x509_parser::extensions::ParsedExtension::BasicConstraints( + x509_parser::extensions::BasicConstraints::from_der(&der) + .unwrap() + .1, + ); + + let mut recovered_params = CertificateParams::default(); + BasicConstraints::from_parsed(&mut recovered_params, &parsed_ext).unwrap(); + assert_eq!( + recovered_params.is_ca, + IsCa::Ca(crate::BasicConstraints::Constrained(path_len)) + ); + } +} diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index fcff2c11..eeda2bf0 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -625,13 +625,11 @@ impl CertificateParams { let alg = SignatureAlgorithm::from_oid(&alg_oid.collect::>())?; let dn = DistinguishedName::from_name(&x509.tbs_certificate.subject)?; - let is_ca = Self::convert_x509_is_ca(&x509)?; let validity = x509.validity(); let serial_number = Some(x509.serial.to_bytes_be().into()); let mut params = CertificateParams { alg, - is_ca, serial_number, distinguished_name: dn, key_pair: Some(key_pair), @@ -681,37 +679,11 @@ impl CertificateParams { ext::SubjectKeyIdentifier::from_parsed(&mut params, ski)?; } - Ok(params) - } - #[cfg(feature = "x509-parser")] - fn convert_x509_is_ca( - x509: &x509_parser::certificate::X509Certificate<'_>, - ) -> Result { - use x509_parser::extensions::BasicConstraints as B; - - let basic_constraints = x509 - .basic_constraints() - .or(Err(Error::CouldNotParseCertificate))? - .map(|ext| ext.value); - - let is_ca = match basic_constraints { - Some(B { - ca: true, - path_len_constraint: Some(n), - }) if *n <= u8::MAX as u32 => IsCa::Ca(BasicConstraints::Constrained(*n as u8)), - Some(B { - ca: true, - path_len_constraint: Some(_), - }) => return Err(Error::CouldNotParseCertificate), - Some(B { - ca: true, - path_len_constraint: None, - }) => IsCa::Ca(BasicConstraints::Unconstrained), - Some(B { ca: false, .. }) => IsCa::ExplicitNoCa, - None => IsCa::NoCa, - }; + if let Some(bc) = find_parsed_extension!(x509, ParsedExtension::BasicConstraints(_)) { + ext::BasicConstraints::from_parsed(&mut params, bc)?; + } - Ok(is_ca) + Ok(params) } fn write_request(&self, pub_key: &K, writer: DERWriter) -> Result<(), Error> { // No .. pattern, we use this to ensure every field is used @@ -842,42 +814,6 @@ impl CertificateParams { Extensions::write_extension(writer, ext); } - match self.is_ca { - IsCa::Ca(ref constraint) => { - // Write basic_constraints - write_x509_extension( - writer.next(), - OID_BASIC_CONSTRAINTS, - true, - |writer| { - writer.write_sequence(|writer| { - writer.next().write_bool(true); // cA flag - if let BasicConstraints::Constrained( - path_len_constraint, - ) = constraint - { - writer.next().write_u8(*path_len_constraint); - } - }); - }, - ); - }, - IsCa::ExplicitNoCa => { - // Write basic_constraints - write_x509_extension( - writer.next(), - OID_BASIC_CONSTRAINTS, - true, - |writer| { - writer.write_sequence(|writer| { - writer.next().write_bool(false); // cA flag - }); - }, - ); - }, - IsCa::NoCa => {}, - } - // Write the custom extensions for ext in &self.custom_extensions { write_x509_extension(writer.next(), &ext.oid, ext.critical, |writer| { @@ -975,7 +911,10 @@ impl CertificateParams { &self, pub_key, )))?; - // TODO: basic constraints. + if let Some(bc_ext) = ext::BasicConstraints::from_params(&self) { + exts.add_extension(Box::new(bc_ext))?; + } + // TODO: custom extensions Ok(exts) From 9e9caf650200e0b970066a6ede4325b2504c64e4 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 17:37:29 -0500 Subject: [PATCH 16/29] ext: implement custom extensions This commit lifts the custom extension handling into the `ext` module. --- rcgen/src/csr.rs | 90 +++++++++++++----------- rcgen/src/error.rs | 6 ++ rcgen/src/ext.rs | 138 ++++++++++++++++++++++++++++++++++++- rcgen/src/lib.rs | 151 +++++++++++------------------------------ rcgen/tests/generic.rs | 6 +- 5 files changed, 233 insertions(+), 158 deletions(-) diff --git a/rcgen/src/csr.rs b/rcgen/src/csr.rs index a6d08956..2ba32ae0 100644 --- a/rcgen/src/csr.rs +++ b/rcgen/src/csr.rs @@ -67,52 +67,60 @@ impl CertificateSigningRequest { params.distinguished_name = DistinguishedName::from_name(&info.subject)?; let raw = info.subject_pki.subject_public_key.data.to_vec(); - if let Some(extensions) = csr.requested_extensions() { - for ext in extensions { - match ext::SubjectAlternativeName::from_parsed(&mut params, ext) { - Ok(true) => continue, // SAN extension handled. - Err(err) => return Err(err), - _ => {}, // Not a SAN. - } - match ext::KeyUsage::from_parsed(&mut params, ext) { - Ok(true) => continue, // KU extension handled. - Err(err) => return Err(err), - _ => {}, // Not a KU. - } - match ext::ExtendedKeyUsage::from_parsed(&mut params, ext) { - Ok(true) => continue, // EKU extension handled. - Err(err) => return Err(err), - _ => {}, // Not an EKU. - } - match ext::NameConstraints::from_parsed(&mut params, ext) { - Ok(true) => continue, // NC extension handled. - Err(err) => return Err(err), - _ => {}, // Not an NC. - } - match ext::CrlDistributionPoints::from_parsed(&mut params, ext) { - Ok(true) => continue, // CDP extension handled. - Err(err) => return Err(err), - _ => {}, // Not a CDP. - } - match ext::SubjectKeyIdentifier::from_parsed(&mut params, ext) { - Ok(true) => continue, // SKI extension handled. - Err(err) => return Err(err), - _ => {}, // Not a SKI. - } - match ext::BasicConstraints::from_parsed(&mut params, ext) { - Ok(true) => continue, // BC extension handled. - Err(err) => return Err(err), - _ => {}, // Not a BC. + // Pull out the extension requests attributes from the CSR. + // Note: we avoid using csr.requested_extensions() here because it maps to the parsed + // extension value and we want the raw extension value to handle unknown extensions + // ourselves. + let requested_exts = csr + .certification_request_info + .iter_attributes() + .filter_map(|attr| { + if let x509_parser::prelude::ParsedCriAttribute::ExtensionRequest(requested) = + &attr.parsed_attribute() + { + Some(requested.extensions.iter().collect::>()) + } else { + None } + }) + .flatten() + .collect::>(); - // If we get here, we've encountered an unknown and unhandled extension. - return Err(Error::UnsupportedExtension); + for ext in requested_exts { + use x509_parser::extensions::ParsedExtension; + + let supported = match ext.parsed_extension() { + ext @ ParsedExtension::SubjectAlternativeName(_) => { + ext::SubjectAlternativeName::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::KeyUsage(_) => ext::KeyUsage::from_parsed(&mut params, ext)?, + ext @ ParsedExtension::ExtendedKeyUsage(_) => { + ext::ExtendedKeyUsage::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::NameConstraints(_) => { + ext::NameConstraints::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::CRLDistributionPoints(_) => { + ext::CrlDistributionPoints::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::SubjectKeyIdentifier(_) => { + ext::SubjectKeyIdentifier::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::BasicConstraints(_) => { + ext::BasicConstraints::from_parsed(&mut params, ext)? + }, + ParsedExtension::AuthorityKeyIdentifier(_) => { + true // We always handle emitting this ourselves - don't copy it as a custom extension. + }, + _ => false, + }; + if !supported { + params + .custom_extensions + .push(ext::CustomExtension::from_parsed(ext)?); } } - // Not yet handled: - // any other extensions. - Ok(Self { params, public_key: PublicKey { alg, raw }, diff --git a/rcgen/src/error.rs b/rcgen/src/error.rs index 8d985ac1..9535b530 100644 --- a/rcgen/src/error.rs +++ b/rcgen/src/error.rs @@ -15,6 +15,8 @@ pub enum Error { CouldNotParseKeyPair, /// Duplicate extension OID DuplicateExtension(String), + /// Invalid ACME identifier extension digest length. + InvalidAcmeIdentifierLength, /// Invalid certificate revocation list (CRL) next update. InvalidCrlNextUpdate, /// An IP address was provided as a byte array, but the byte array was an invalid length. @@ -114,6 +116,10 @@ impl fmt::Display for Error { f, "Unsupported basic constraints extension path length constraint in CSR" )?, + InvalidAcmeIdentifierLength => write!( + f, + "Invalid ACME identifier extension digest length. Must be 32 bytes.", + )?, }; Ok(()) } diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index b55a325b..3bf29acb 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -6,6 +6,7 @@ use yasna::models::ObjectIdentifier; use yasna::{DERWriter, DERWriterSeq, Tag}; use crate::key_pair::PublicKeyData; +use crate::oid::OID_PE_ACME; use crate::{ oid, write_distinguished_name, Certificate, CertificateParams, Error, ExtendedKeyUsagePurpose, GeneralSubtree, IsCa, KeyUsagePurpose, SanType, @@ -20,7 +21,7 @@ use crate::{ /// /// [RFC 5280 Section 4.2]: #[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub(crate) enum Criticality { +pub enum Criticality { /// The extension MUST be recognized and parsed correctly. /// /// A certificate-using system MUST reject the certificate if it encounters a critical @@ -35,6 +36,141 @@ pub(crate) enum Criticality { NonCritical, } +/// A custom extension of a certificate, as specified in +/// [RFC 5280](https://tools.ietf.org/html/rfc5280#section-4.2) +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct CustomExtension { + /// OID identifying the extension. + /// + /// Only one extension with a given OID may appear within a certificate. + pub oid: ObjectIdentifier, + + /// Criticality of the extension. + /// + /// See [Criticality] for more information. + pub criticality: Criticality, + + /// The raw DER encoded value of the extension. + /// + /// This should not contain the OID, criticality, OCTET STRING, or the outer extension SEQUENCE + /// of the extension itself: it should only be the DER encoded bytes that will be found + /// within the extensions' OCTET STRING value. + pub der_value: Vec, +} + +impl CustomExtension { + /// Create a new custom extension with the specified content + pub fn from_oid_content(oid: &[u64], criticality: Criticality, der_value: Vec) -> Self { + Self { + oid: ObjectIdentifier::from_slice(oid), + criticality, + der_value, + } + } + + /// Obtains the OID components of the extensions, as u64 pieces + pub fn oid_components(&self) -> impl Iterator + '_ { + self.oid.components().iter().copied() + } + + #[cfg(feature = "x509-parser")] + pub(crate) fn from_parsed( + parsed: &x509_parser::prelude::X509Extension<'_>, + ) -> Result { + Ok(CustomExtension { + oid: ObjectIdentifier::from_slice( + &parsed + .oid + .iter() + .ok_or(Error::UnsupportedExtension)? + .collect::>(), + ), + criticality: if parsed.critical { + Criticality::Critical + } else { + Criticality::NonCritical + }, + der_value: parsed.value.to_vec(), + }) + } +} + +impl Extension for CustomExtension { + fn oid(&self) -> ObjectIdentifier { + self.oid.clone() + } + + fn criticality(&self) -> Criticality { + self.criticality + } + + fn write_value(&self, writer: DERWriter) { + writer.write_der(&self.der_value) + } +} + +/// An ACME TLS-ALPN-01 challenge response certificate extension. +/// +/// See [RFC 8737 Section 3] for more information. +/// +/// [RFC 8737 Section 3]: +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AcmeIdentifier { + // The SHA256 digest of the RFC 8555 key authorization for a TLS-ALPN-01 challenge + // issued by the CA. + key_auth_digest: [u8; 32], +} + +impl AcmeIdentifier { + /// Construct an ACME TLS-ALPN-01 challenge response certificate extension. + /// + /// `key_auth_digest` should be the SHA-256 digest of the key authorization for the + /// TLS-ALPN-01 challenge issued by the CA. + /// + /// If you have a `Vec` or `&[u8]` use `try_from` and handle the potential error + /// if the input length is not 32 bytes. + pub fn new(key_auth_digest: [u8; 32]) -> Self { + Self { + key_auth_digest: key_auth_digest, + } + } +} + +impl TryFrom<&[u8]> for AcmeIdentifier { + type Error = Error; + + fn try_from(key_auth_digest: &[u8]) -> Result { + // All TLS-ALPN-01 challenge response digests are 32 bytes long, + // matching the output of the SHA256 digest algorithm. + if key_auth_digest.len() != 32 { + return Err(Error::InvalidAcmeIdentifierLength); + } + + let mut sha_digest = [0u8; 32]; + sha_digest.copy_from_slice(&key_auth_digest); + Ok(Self { + key_auth_digest: sha_digest, + }) + } +} + +impl Extension for AcmeIdentifier { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(OID_PE_ACME) + } + + fn criticality(&self) -> Criticality { + // The acmeIdentifier extension MUST be critical so that the certificate isn't inadvertently + // used by non-ACME software. + Criticality::Critical + } + + fn write_value(&self, writer: DERWriter) { + // Authorization ::= OCTET STRING (SIZE (32)) + writer.write_bytes(&self.key_auth_digest); + } +} + /// A trait describing an X.509 Extension. /// /// All extensions have an OID, an indicator of whether they are critical or not, and can be diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index eeda2bf0..21006ae4 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -66,6 +66,8 @@ use crate::oid::*; pub use crate::sign_algo::algo::*; pub use crate::sign_algo::SignatureAlgorithm; +pub use crate::ext::{AcmeIdentifier, Criticality, CustomExtension}; + /// Type-alias for the old name of [`Error`]. #[deprecated( note = "Renamed to `Error`. We recommend to refer to it by fully-qualifying the crate: `rcgen::Error`." @@ -638,49 +640,37 @@ impl CertificateParams { ..Default::default() }; - macro_rules! find_parsed_extension { - ($iter:expr, $variant:pat) => { - $iter - .iter_extensions() - .find_map(|ext| match ext.parsed_extension() { - ext @ $variant => Some(ext), - _ => None, - }) + for ext in x509.iter_extensions() { + let handled = match ext.parsed_extension() { + ext @ ParsedExtension::SubjectAlternativeName(_) => { + ext::SubjectAlternativeName::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::KeyUsage(_) => ext::KeyUsage::from_parsed(&mut params, ext)?, + ext @ ParsedExtension::ExtendedKeyUsage(_) => { + ext::ExtendedKeyUsage::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::NameConstraints(_) => { + ext::NameConstraints::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::CRLDistributionPoints(_) => { + ext::CrlDistributionPoints::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::SubjectKeyIdentifier(_) => { + ext::SubjectKeyIdentifier::from_parsed(&mut params, ext)? + }, + ext @ ParsedExtension::BasicConstraints(_) => { + ext::BasicConstraints::from_parsed(&mut params, ext)? + }, + ParsedExtension::AuthorityKeyIdentifier(_) => { + true // We always handle emitting this ourselves - don't copy it as a custom extension. + }, + _ => false, }; - } - - if let Some(san_ext) = - find_parsed_extension!(x509, ParsedExtension::SubjectAlternativeName(_)) - { - ext::SubjectAlternativeName::from_parsed(&mut params, san_ext)?; - } - - if let Some(ku_ext) = find_parsed_extension!(x509, ParsedExtension::KeyUsage(_)) { - ext::KeyUsage::from_parsed(&mut params, ku_ext)?; - } - - if let Some(eku_ext) = find_parsed_extension!(x509, ParsedExtension::ExtendedKeyUsage(_)) { - ext::ExtendedKeyUsage::from_parsed(&mut params, eku_ext)?; - } - - if let Some(name_constraints) = - find_parsed_extension!(x509, ParsedExtension::NameConstraints(_)) - { - ext::NameConstraints::from_parsed(&mut params, name_constraints)?; - } - - if let Some(crl_dps) = - find_parsed_extension!(x509, ParsedExtension::CRLDistributionPoints(_)) - { - ext::CrlDistributionPoints::from_parsed(&mut params, crl_dps)?; - } - - if let Some(ski) = find_parsed_extension!(x509, ParsedExtension::SubjectKeyIdentifier(_)) { - ext::SubjectKeyIdentifier::from_parsed(&mut params, ski)?; - } - - if let Some(bc) = find_parsed_extension!(x509, ParsedExtension::BasicConstraints(_)) { - ext::BasicConstraints::from_parsed(&mut params, bc)?; + if !handled { + params + .custom_extensions + .push(CustomExtension::from_parsed(ext)?); + } } Ok(params) @@ -741,16 +731,6 @@ impl CertificateParams { for ext in extensions.iter() { Extensions::write_extension(writer, ext); } - - // Write custom extensions - for ext in custom_extensions { - write_x509_extension( - writer.next(), - &ext.oid, - ext.critical, - |writer| writer.write_der(ext.content()), - ); - } }); }); }); @@ -813,13 +793,6 @@ impl CertificateParams { for ext in extensions.iter() { Extensions::write_extension(writer, ext); } - - // Write the custom extensions - for ext in &self.custom_extensions { - write_x509_extension(writer.next(), &ext.oid, ext.critical, |writer| { - writer.write_der(ext.content()) - }); - } }); }); } @@ -915,7 +888,12 @@ impl CertificateParams { exts.add_extension(Box::new(bc_ext))?; } - // TODO: custom extensions + exts.add_extensions( + self.custom_extensions + .clone() + .into_iter() + .map(|x| Box::new(x) as Box), + )?; Ok(exts) } @@ -1039,59 +1017,6 @@ impl ExtendedKeyUsagePurpose { } } -/// A custom extension of a certificate, as specified in -/// [RFC 5280](https://tools.ietf.org/html/rfc5280#section-4.2) -#[derive(Debug, PartialEq, Eq, Hash, Clone)] -pub struct CustomExtension { - oid: Vec, - critical: bool, - - /// The content must be DER-encoded - content: Vec, -} - -impl CustomExtension { - /// Creates a new acmeIdentifier extension for ACME TLS-ALPN-01 - /// as specified in [RFC 8737](https://tools.ietf.org/html/rfc8737#section-3) - /// - /// Panics if the passed `sha_digest` parameter doesn't hold 32 bytes (256 bits). - pub fn new_acme_identifier(sha_digest: &[u8]) -> Self { - assert_eq!(sha_digest.len(), 32, "wrong size of sha_digest"); - let content = yasna::construct_der(|writer| { - writer.write_bytes(sha_digest); - }); - Self { - oid: OID_PE_ACME.to_owned(), - critical: true, - content, - } - } - /// Create a new custom extension with the specified content - pub fn from_oid_content(oid: &[u64], content: Vec) -> Self { - Self { - oid: oid.to_owned(), - critical: false, - content, - } - } - /// Sets the criticality flag of the extension. - pub fn set_criticality(&mut self, criticality: bool) { - self.critical = criticality; - } - /// Obtains the criticality flag of the extension. - pub fn criticality(&self) -> bool { - self.critical - } - /// Obtains the content of the extension. - pub fn content(&self) -> &[u8] { - &self.content - } - /// Obtains the OID components of the extensions, as u64 pieces - pub fn oid_components(&self) -> impl Iterator + '_ { - self.oid.iter().copied() - } -} - /// Method to generate key identifiers from public keys. /// /// This allows choice over methods to generate key identifiers diff --git a/rcgen/tests/generic.rs b/rcgen/tests/generic.rs index 582c6210..166a6153 100644 --- a/rcgen/tests/generic.rs +++ b/rcgen/tests/generic.rs @@ -93,7 +93,7 @@ mod test_convert_x509_subject_alternative_name { mod test_x509_custom_ext { use crate::util; - use rcgen::{Certificate, CustomExtension}; + use rcgen::{Certificate, Criticality, CustomExtension}; use x509_parser::oid_registry::asn1_rs; use x509_parser::prelude::{ FromDer, ParsedCriAttribute, X509Certificate, X509CertificationRequest, @@ -106,11 +106,11 @@ mod test_x509_custom_ext { let test_ext = yasna::construct_der(|writer| { writer.write_utf8_string("🦀 greetz to ferris 🦀"); }); - let mut custom_ext = CustomExtension::from_oid_content( + let custom_ext = CustomExtension::from_oid_content( test_oid.iter().unwrap().collect::>().as_slice(), + Criticality::Critical, test_ext.clone(), ); - custom_ext.set_criticality(true); // Generate a certificate with the custom extension, parse it with x509-parser. let mut params = util::default_params(); From 0369073e49aa6abee1b4e36b1539eb80789d9514 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 17:48:21 -0500 Subject: [PATCH 17/29] ext: use Extensions to write DER as needed --- rcgen/src/ext.rs | 42 ++++++++++++++++++++++++++++++++++-------- rcgen/src/lib.rs | 44 +++++--------------------------------------- 2 files changed, 39 insertions(+), 47 deletions(-) diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 3bf29acb..e7c7fba6 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -6,7 +6,7 @@ use yasna::models::ObjectIdentifier; use yasna::{DERWriter, DERWriterSeq, Tag}; use crate::key_pair::PublicKeyData; -use crate::oid::OID_PE_ACME; +use crate::oid::{OID_PE_ACME, OID_PKCS_9_AT_EXTENSION_REQUEST}; use crate::{ oid, write_distinguished_name, Certificate, CertificateParams, Error, ExtendedKeyUsagePurpose, GeneralSubtree, IsCa, KeyUsagePurpose, SanType, @@ -235,17 +235,44 @@ impl Extensions { Ok(()) } - /// Write the SEQUENCE of extensions to the DER writer. + /// Write the SEQUENCE of extensions to the DER writer, wrapped in the context tag for + /// an optional X.509 V3 certificate extensions field. /// - /// This will return without writing anything if there are no extensions in the collection. - pub(crate) fn write_der(&self, writer: DERWriter) { - debug_assert_eq!(self.exts.len(), self.oids.len()); + /// Nothing will be written to the writer if there were no extensions. + pub(crate) fn write_exts_der(&self, writer: DERWriter) { + // Avoid writing an empty tagged extensions sequence. + if self.exts.is_empty() { + return; + } + + // extensions [3] Extensions OPTIONAL + writer.write_tagged(Tag::context(3), |writer| self.write_der(writer)); + } - // Avoid writing an empty extensions sequence. + /// Write the SEQUENCE of extensions to the DER writer, wrapped in the PKCS 9 attribute + /// extension request OID and set for a CSR. + /// + /// Nothing will be written to the writer if there were no extensions. + pub(crate) fn write_csr_der(&self, writer: DERWriter) { + // Avoid writing an empty attribute requests sequence. if self.exts.is_empty() { return; } + writer.write_sequence(|writer| { + writer.next().write_oid(&ObjectIdentifier::from_slice( + OID_PKCS_9_AT_EXTENSION_REQUEST, + )); + writer.next().write_set(|writer| { + self.write_der(writer.next()); + }); + }); + } + + /// Write the SEQUENCE of extensions to the DER writer. + fn write_der(&self, writer: DERWriter) { + debug_assert_eq!(self.exts.len(), self.oids.len()); + // Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension writer.write_sequence(|writer| { for extension in &self.exts { @@ -254,12 +281,11 @@ impl Extensions { }) } - /// TODO(@cpu): Remove once `Extensions::write_der` is being used. pub(crate) fn iter(&self) -> impl Iterator> { self.exts.iter() } - /// TODO(@cpu): Reduce visibility once `Extensions::write_der` is being used. + /// Write a single extension SEQUENCE to the DER writer. pub(crate) fn write_extension(writer: &mut DERWriterSeq, extension: &Box) { // Extension ::= SEQUENCE { // extnID OBJECT IDENTIFIER, diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 21006ae4..482e57a4 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -683,14 +683,14 @@ impl CertificateParams { not_before, not_after, serial_number, - subject_alt_names, + subject_alt_names: _, distinguished_name, is_ca, key_usages: _, extended_key_usages: _, name_constraints: _, crl_distribution_points: _, - custom_extensions, + custom_extensions: _, key_pair, use_authority_key_identifier_extension, key_identifier_method, @@ -718,23 +718,7 @@ impl CertificateParams { // Write extensions // According to the spec in RFC 2986, even if attributes are empty we need the empty attribute tag writer.next().write_tagged(Tag::context(0), |writer| { - let extensions = self.extensions(pub_key, None)?; - if !subject_alt_names.is_empty() || !custom_extensions.is_empty() { - writer.write_sequence(|writer| { - let oid = ObjectIdentifier::from_slice(OID_PKCS_9_AT_EXTENSION_REQUEST); - writer.next().write_oid(&oid); - writer.next().write_set(|writer| { - writer.next().write_sequence(|writer| { - // TODO: have the Extensions type write the outer sequence and each - // contained extension once we've ported each of the below - // extensions to self.extensions(). - for ext in extensions.iter() { - Extensions::write_extension(writer, ext); - } - }); - }); - }); - } + self.extensions(pub_key, None)?.write_csr_der(writer); Ok(()) }) }) @@ -776,26 +760,8 @@ impl CertificateParams { // Write subjectPublicKeyInfo pub_key.serialize_public_key_der(writer.next()); // write extensions - let extensions = self.extensions(pub_key, Some(ca))?; - let should_write_exts = self.use_authority_key_identifier_extension - || !self.subject_alt_names.is_empty() - || !self.extended_key_usages.is_empty() - || self.name_constraints.iter().any(|c| !c.is_empty()) - || matches!(self.is_ca, IsCa::ExplicitNoCa) - || matches!(self.is_ca, IsCa::Ca(_)) - || !self.custom_extensions.is_empty(); - if should_write_exts { - writer.next().write_tagged(Tag::context(3), |writer| { - writer.write_sequence(|writer| { - // TODO: have the Extensions type write the outer sequence and each - // contained extension once we've ported each of the below - // extensions to self.extensions(). - for ext in extensions.iter() { - Extensions::write_extension(writer, ext); - } - }); - }); - } + self.extensions(pub_key, Some(ca))? + .write_exts_der(writer.next()); Ok(()) }) } From 2d0b890e843ba37717e7a4958d200c58e86eb69f Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 17:52:09 -0500 Subject: [PATCH 18/29] wip: fixup with last commit --- rcgen/src/lib.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 482e57a4..9760094e 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -918,12 +918,6 @@ pub struct NameConstraints { pub excluded_subtrees: Vec, } -impl NameConstraints { - fn is_empty(&self) -> bool { - self.permitted_subtrees.is_empty() && self.excluded_subtrees.is_empty() - } -} - /// One of the purposes contained in the [key usage](https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.3) extension #[derive(Debug, PartialEq, Eq, Hash, Clone)] pub enum KeyUsagePurpose { From dbc3d36449cb55f854ccd2b3820ada3c64f59392 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 17:54:56 -0500 Subject: [PATCH 19/29] ext: implement crl number extension This commit lifts the CRL number extension handling into the `ext` module. --- rcgen/src/crl.rs | 13 ++++++------- rcgen/src/ext.rs | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index 972629a5..48d57ad9 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -247,20 +247,16 @@ impl CertificateRevocationListParams { // Conforming CRL issuers are REQUIRED to include the authority key // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) // extensions in all CRLs issued. + let exts = self.extensions(Some(ca)); writer.next().write_tagged(Tag::context(0), |writer| { writer.write_sequence(|writer| { // TODO: have the Extensions type write the outer sequence and each // contained extension once we've ported each of the below // extensions to self.extensions(). - for ext in self.extensions(Some(ca)).iter() { + for ext in exts.iter() { Extensions::write_extension(writer, ext); } - // Write CRL number. - write_x509_extension(writer.next(), OID_CRL_NUMBER, false, |writer| { - writer.write_bigint_bytes(self.crl_number.as_ref(), true); - }); - // Write issuing distribution point (if present). if let Some(issuing_distribution_point) = &self.issuing_distribution_point { write_x509_extension( @@ -291,7 +287,10 @@ impl CertificateRevocationListParams { .unwrap(); } - // TODO: CRL number. + // Safety: there can be no duplicate CRL number ext OID by this point. + exts.add_extension(Box::new(ext::CrlNumber::from_params(&self))) + .unwrap(); + // TODO: issuing distribution point. exts diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index e7c7fba6..33853f5a 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -8,8 +8,8 @@ use yasna::{DERWriter, DERWriterSeq, Tag}; use crate::key_pair::PublicKeyData; use crate::oid::{OID_PE_ACME, OID_PKCS_9_AT_EXTENSION_REQUEST}; use crate::{ - oid, write_distinguished_name, Certificate, CertificateParams, Error, ExtendedKeyUsagePurpose, - GeneralSubtree, IsCa, KeyUsagePurpose, SanType, + oid, write_distinguished_name, Certificate, CertificateParams, CertificateRevocationListParams, + Error, ExtendedKeyUsagePurpose, GeneralSubtree, IsCa, KeyUsagePurpose, SanType, SerialNumber, }; /// The criticality of an extension. @@ -941,6 +941,38 @@ impl Extension for BasicConstraints { } } +/// An X.509v3 CRL number extension according to +/// [RFC 5280 5.2.3](https://www.rfc-editor.org/rfc/rfc5280#section-5.2.3) +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct CrlNumber { + number: SerialNumber, +} + +impl CrlNumber { + pub(crate) fn from_params(params: &CertificateRevocationListParams) -> Self { + Self { + number: params.crl_number.clone(), + } + } +} + +impl Extension for CrlNumber { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_CRL_NUMBER) + } + + fn criticality(&self) -> Criticality { + // CRL issuers conforming to this profile MUST include this extension in all + // CRLs and MUST mark this extension as non-critical. + Criticality::NonCritical + } + + fn write_value(&self, writer: DERWriter) { + // CRLNumber ::= INTEGER (0..MAX) + writer.write_bigint_bytes(self.number.as_ref(), true); + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; From f0548fa9621788edae77952a078b6a241018224f Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:01:01 -0500 Subject: [PATCH 20/29] ext: implement issuing distribution point extension This commit lifts the CRL issuing distribution point extension handling into the `ext` module. --- rcgen/src/crl.rs | 8 ++++++-- rcgen/src/ext.rs | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index 48d57ad9..88563d28 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -291,7 +291,10 @@ impl CertificateRevocationListParams { exts.add_extension(Box::new(ext::CrlNumber::from_params(&self))) .unwrap(); - // TODO: issuing distribution point. + if let Some(idp_ext) = ext::IssuingDistributionPoint::from_params(&self) { + // Safety: there can be no duplicate IDP ext OID by this point. + exts.add_extension(Box::new(idp_ext)).unwrap(); + } exts } @@ -299,6 +302,7 @@ impl CertificateRevocationListParams { /// A certificate revocation list (CRL) issuing distribution point, to be included in a CRL's /// [issuing distribution point extension](https://datatracker.ietf.org/doc/html/rfc5280#section-5.2.5). +#[derive(Debug, Clone, Eq, PartialEq)] pub struct CrlIssuingDistributionPoint { /// The CRL's distribution point, containing a sequence of URIs the CRL can be retrieved from. pub distribution_point: CrlDistributionPoint, @@ -308,7 +312,7 @@ pub struct CrlIssuingDistributionPoint { } impl CrlIssuingDistributionPoint { - fn write_der(&self, writer: DERWriter) { + pub(crate) fn write_der(&self, writer: DERWriter) { // IssuingDistributionPoint SEQUENCE writer.write_sequence(|writer| { // distributionPoint [0] DistributionPointName OPTIONAL diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 33853f5a..5ffdd1d8 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -9,7 +9,8 @@ use crate::key_pair::PublicKeyData; use crate::oid::{OID_PE_ACME, OID_PKCS_9_AT_EXTENSION_REQUEST}; use crate::{ oid, write_distinguished_name, Certificate, CertificateParams, CertificateRevocationListParams, - Error, ExtendedKeyUsagePurpose, GeneralSubtree, IsCa, KeyUsagePurpose, SanType, SerialNumber, + CrlIssuingDistributionPoint, Error, ExtendedKeyUsagePurpose, GeneralSubtree, IsCa, + KeyUsagePurpose, SanType, SerialNumber, }; /// The criticality of an extension. @@ -973,6 +974,38 @@ impl Extension for CrlNumber { } } +/// An X.509v3 issuing distribution point extension according to +/// [RFC 5280 5.2.5](https://www.rfc-editor.org/rfc/rfc5280#section-5.2.5) +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct IssuingDistributionPoint { + point: CrlIssuingDistributionPoint, +} + +impl IssuingDistributionPoint { + pub(crate) fn from_params(params: &CertificateRevocationListParams) -> Option { + match ¶ms.issuing_distribution_point { + Some(idp) => Some(Self { point: idp.clone() }), + None => None, + } + } +} + +impl Extension for IssuingDistributionPoint { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_CRL_ISSUING_DISTRIBUTION_POINT) + } + + fn criticality(&self) -> Criticality { + // Although the extension is critical, conforming implementations are not required to support this + // extension. + Criticality::Critical + } + + fn write_value(&self, writer: DERWriter) { + self.point.write_der(writer); + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; From 0c96153f81d5188d966cbdf47082fe49b57aa480 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:02:12 -0500 Subject: [PATCH 21/29] crl: unconditionally emit AKI --- rcgen/src/crl.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index 88563d28..e073782a 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -243,11 +243,7 @@ impl CertificateRevocationListParams { // RFC 5280 §5.1.2.7: // This field may only appear if the version is 2 (Section 5.1.2.1). If // present, this field is a sequence of one or more CRL extensions. - // RFC 5280 §5.2: - // Conforming CRL issuers are REQUIRED to include the authority key - // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) - // extensions in all CRLs issued. - let exts = self.extensions(Some(ca)); + let exts = self.extensions(ca); writer.next().write_tagged(Tag::context(0), |writer| { writer.write_sequence(|writer| { // TODO: have the Extensions type write the outer sequence and each @@ -278,14 +274,16 @@ impl CertificateRevocationListParams { /// /// If an issuer [Certificate] is provided, additional extensions specific to the issuer will /// be included (e.g. the authority key identifier). - fn extensions(&self, issuer: Option<&Certificate>) -> Extensions { + fn extensions(&self, issuer: &Certificate) -> Extensions { let mut exts = Extensions::default(); - if let Some(issuer) = issuer { - // Safety: `exts` is empty at this point - there can be no duplicate AKI ext OID. - exts.add_extension(Box::new(ext::AuthorityKeyIdentifier::from(issuer))) - .unwrap(); - } + // RFC 5280 §5.2: + // Conforming CRL issuers are REQUIRED to include the authority key + // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) + // extensions in all CRLs issued. + // Safety: `exts` is empty at this point - there can be no duplicate AKI ext OID. + exts.add_extension(Box::new(ext::AuthorityKeyIdentifier::from(issuer))) + .unwrap(); // Safety: there can be no duplicate CRL number ext OID by this point. exts.add_extension(Box::new(ext::CrlNumber::from_params(&self))) From a50d9769ebfd096ffe35dec3715c80d24fc7b93e Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:02:45 -0500 Subject: [PATCH 22/29] wip: fixup with CRL IDP ext --- rcgen/src/crl.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index e073782a..0e6a5c2c 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -252,18 +252,6 @@ impl CertificateRevocationListParams { for ext in exts.iter() { Extensions::write_extension(writer, ext); } - - // Write issuing distribution point (if present). - if let Some(issuing_distribution_point) = &self.issuing_distribution_point { - write_x509_extension( - writer.next(), - OID_CRL_ISSUING_DISTRIBUTION_POINT, - true, - |writer| { - issuing_distribution_point.write_der(writer); - }, - ); - } }); }); From 5d9559496b354db99d5c9d5bd1bc25b4efd34a15 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:05:26 -0500 Subject: [PATCH 23/29] crl: write DER with Extensions --- rcgen/src/crl.rs | 12 +----------- rcgen/src/ext.rs | 10 ++++++++++ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index 0e6a5c2c..86e47da5 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -243,17 +243,7 @@ impl CertificateRevocationListParams { // RFC 5280 §5.1.2.7: // This field may only appear if the version is 2 (Section 5.1.2.1). If // present, this field is a sequence of one or more CRL extensions. - let exts = self.extensions(ca); - writer.next().write_tagged(Tag::context(0), |writer| { - writer.write_sequence(|writer| { - // TODO: have the Extensions type write the outer sequence and each - // contained extension once we've ported each of the below - // extensions to self.extensions(). - for ext in exts.iter() { - Extensions::write_extension(writer, ext); - } - }); - }); + self.extensions(ca).write_crl_der(writer.next()); Ok(()) }) diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 5ffdd1d8..d3b989eb 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -270,6 +270,16 @@ impl Extensions { }); } + pub(crate) fn write_crl_der(&self, writer: DERWriter) { + // Avoid writing an empty tagged extensions sequence. + if self.exts.is_empty() { + return; + } + + // crlExtensions [0] Extensions OPTIONAL + writer.write_tagged(Tag::context(0), |writer| self.write_der(writer)); + } + /// Write the SEQUENCE of extensions to the DER writer. fn write_der(&self, writer: DERWriter) { debug_assert_eq!(self.exts.len(), self.oids.len()); From 199d6046d0751fb9992944743c4ffeb4aca2da52 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:09:52 -0500 Subject: [PATCH 24/29] ext: implement reason code extension This commit lifts the CRL entry reason code extension handling into the `ext` module. --- rcgen/src/crl.rs | 15 ++++++-------- rcgen/src/ext.rs | 51 +++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index 86e47da5..f5ad1c4c 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -370,13 +370,6 @@ impl RevokedCertParams { Extensions::write_extension(writer, ext); } - // Write reason code if present. - self.reason_code.map(|reason_code| { - write_x509_extension(writer.next(), OID_CRL_REASONS, false, |writer| { - writer.write_enum(reason_code as i64); - }); - }); - // Write invalidity date if present. self.invalidity_date.map(|invalidity_date| { write_x509_extension( @@ -394,9 +387,13 @@ impl RevokedCertParams { } /// Returns the X.509 extensions that the [RevokedCertParams] describe. fn extensions(&self) -> Extensions { - let exts = Extensions::default(); + let mut exts = Extensions::default(); + + if let Some(reason_code_ext) = ext::ReasonCode::from_params(&self) { + // Safety: there can be no duplicate reason code ext OID by this point. + exts.add_extension(Box::new(reason_code_ext)).unwrap(); + } - // TODO: reason code. // TODO: invalidity date. exts diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index d3b989eb..17fc7638 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -8,9 +8,9 @@ use yasna::{DERWriter, DERWriterSeq, Tag}; use crate::key_pair::PublicKeyData; use crate::oid::{OID_PE_ACME, OID_PKCS_9_AT_EXTENSION_REQUEST}; use crate::{ - oid, write_distinguished_name, Certificate, CertificateParams, CertificateRevocationListParams, - CrlIssuingDistributionPoint, Error, ExtendedKeyUsagePurpose, GeneralSubtree, IsCa, - KeyUsagePurpose, SanType, SerialNumber, + crl, oid, write_distinguished_name, Certificate, CertificateParams, + CertificateRevocationListParams, CrlIssuingDistributionPoint, Error, ExtendedKeyUsagePurpose, + GeneralSubtree, IsCa, KeyUsagePurpose, RevokedCertParams, SanType, SerialNumber, }; /// The criticality of an extension. @@ -1016,6 +1016,51 @@ impl Extension for IssuingDistributionPoint { } } +/// An X.509v3 reason code extension according to +/// [RFC 5280 5.3.1](https://www.rfc-editor.org/rfc/rfc5280#section-5.3.1). +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct ReasonCode { + reason: crl::RevocationReason, +} + +impl ReasonCode { + pub(crate) fn from_params(params: &RevokedCertParams) -> Option { + match ¶ms.reason_code { + Some(reason) => Some(Self { reason: *reason }), + None => None, + } + } +} + +impl Extension for ReasonCode { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_CRL_REASONS) + } + + fn criticality(&self) -> Criticality { + // The reasonCode is a non-critical CRL entry extension + Criticality::NonCritical + } + + fn write_value(&self, writer: DERWriter) { + /* + CRLReason ::= ENUMERATED { + unspecified (0), + keyCompromise (1), + cACompromise (2), + affiliationChanged (3), + superseded (4), + cessationOfOperation (5), + certificateHold (6), + -- value 7 is not used + removeFromCRL (8), + privilegeWithdrawn (9), + aACompromise (10) } + */ + writer.write_enum(self.reason as i64); + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; From 0287f546aa8080f94916ad20aedbfc276cd16bf4 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:13:17 -0500 Subject: [PATCH 25/29] ext: implement invalidity date extension This commit lifts the CRL entry invalidity date extension into the `ext` module. There are no longer any references to the lib.rs `write_x509_extension` helper, so it is also removed. --- rcgen/src/crl.rs | 20 +++++--------------- rcgen/src/ext.rs | 40 +++++++++++++++++++++++++++++++++++++--- rcgen/src/lib.rs | 28 ---------------------------- 3 files changed, 42 insertions(+), 46 deletions(-) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index f5ad1c4c..c820c2bc 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -5,10 +5,9 @@ use yasna::DERWriter; use yasna::Tag; use crate::ext::Extensions; -use crate::oid::*; #[cfg(feature = "pem")] use crate::ENCODE_CONFIG; -use crate::{ext, write_distinguished_name, write_dt_utc_or_generalized, write_x509_extension}; +use crate::{ext, write_distinguished_name, write_dt_utc_or_generalized}; use crate::{Certificate, Error, KeyIdMethod, KeyUsagePurpose, SerialNumber, SignatureAlgorithm}; /// A certificate revocation list (CRL) @@ -369,18 +368,6 @@ impl RevokedCertParams { for ext in self.extensions().iter() { Extensions::write_extension(writer, ext); } - - // Write invalidity date if present. - self.invalidity_date.map(|invalidity_date| { - write_x509_extension( - writer.next(), - OID_CRL_INVALIDITY_DATE, - false, - |writer| { - write_dt_utc_or_generalized(writer, invalidity_date); - }, - ) - }); }); } }) @@ -394,7 +381,10 @@ impl RevokedCertParams { exts.add_extension(Box::new(reason_code_ext)).unwrap(); } - // TODO: invalidity date. + if let Some(invalidity_date_ext) = ext::InvalidityDate::from_params(&self) { + // Safety: there can be no duplicate invalidity date ext OID by this point. + exts.add_extension(Box::new(invalidity_date_ext)).unwrap(); + } exts } diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 17fc7638..7005a02d 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -1,6 +1,7 @@ use std::collections::HashSet; use std::fmt::Debug; use std::net::IpAddr; +use time::OffsetDateTime; use yasna::models::ObjectIdentifier; use yasna::{DERWriter, DERWriterSeq, Tag}; @@ -8,9 +9,10 @@ use yasna::{DERWriter, DERWriterSeq, Tag}; use crate::key_pair::PublicKeyData; use crate::oid::{OID_PE_ACME, OID_PKCS_9_AT_EXTENSION_REQUEST}; use crate::{ - crl, oid, write_distinguished_name, Certificate, CertificateParams, - CertificateRevocationListParams, CrlIssuingDistributionPoint, Error, ExtendedKeyUsagePurpose, - GeneralSubtree, IsCa, KeyUsagePurpose, RevokedCertParams, SanType, SerialNumber, + crl, oid, write_distinguished_name, write_dt_utc_or_generalized, Certificate, + CertificateParams, CertificateRevocationListParams, CrlIssuingDistributionPoint, Error, + ExtendedKeyUsagePurpose, GeneralSubtree, IsCa, KeyUsagePurpose, RevokedCertParams, SanType, + SerialNumber, }; /// The criticality of an extension. @@ -1061,6 +1063,38 @@ impl Extension for ReasonCode { } } +/// An X.509v3 invalidity date extension according to +/// [RFC 5280 5.3.2](https://www.rfc-editor.org/rfc/rfc5280#section-5.3.2). +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) struct InvalidityDate { + date: OffsetDateTime, +} + +impl InvalidityDate { + pub(crate) fn from_params(params: &RevokedCertParams) -> Option { + match ¶ms.invalidity_date { + Some(date) => Some(Self { date: date.clone() }), + None => None, + } + } +} + +impl Extension for InvalidityDate { + fn oid(&self) -> ObjectIdentifier { + ObjectIdentifier::from_slice(oid::OID_CRL_INVALIDITY_DATE) + } + + fn criticality(&self) -> Criticality { + // The invalidity date is a non-critical CRL entry extension + Criticality::NonCritical + } + + fn write_value(&self, writer: DERWriter) { + // InvalidityDate ::= GeneralizedTime + write_dt_utc_or_generalized(writer, self.date); + } +} + #[cfg(test)] mod extensions_tests { use crate::oid; diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 9760094e..91cd4ab8 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -1169,34 +1169,6 @@ impl Certificate { } } -/// Serializes an X.509v3 extension according to RFC 5280 -fn write_x509_extension( - writer: DERWriter, - extension_oid: &[u64], - is_critical: bool, - value_serializer: impl FnOnce(DERWriter), -) { - // Extension specification: - // Extension ::= SEQUENCE { - // extnID OBJECT IDENTIFIER, - // critical BOOLEAN DEFAULT FALSE, - // extnValue OCTET STRING - // -- contains the DER encoding of an ASN.1 value - // -- corresponding to the extension type identified - // -- by extnID - // } - - writer.write_sequence(|writer| { - let oid = ObjectIdentifier::from_slice(extension_oid); - writer.next().write_oid(&oid); - if is_critical { - writer.next().write_bool(true); - } - let bytes = yasna::construct_der(value_serializer); - writer.next().write_bytes(&bytes); - }) -} - #[cfg(feature = "zeroize")] impl zeroize::Zeroize for KeyPair { fn zeroize(&mut self) { From 006bf28c5d0f26b410e63d6c56e0f750b2283a7f Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:16:03 -0500 Subject: [PATCH 26/29] crl: use Extensions to write DER Now that all of the CRL entry extensions have been migrated to `Extensions` we can let that type write the `SEQUENCE` and extension values. There are no longer any callers to `Extensions.iter()` so we remove that fn. --- rcgen/src/crl.rs | 14 +------------- rcgen/src/ext.rs | 16 +++++++++------- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index c820c2bc..e06f45b6 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -357,19 +357,7 @@ impl RevokedCertParams { // optional for conforming CRL issuers and applications. However, CRL // issuers SHOULD include reason codes (Section 5.3.1) and invalidity // dates (Section 5.3.2) whenever this information is available. - let has_reason_code = - matches!(self.reason_code, Some(reason) if reason != RevocationReason::Unspecified); - let has_invalidity_date = self.invalidity_date.is_some(); - if has_reason_code || has_invalidity_date { - writer.next().write_sequence(|writer| { - // TODO: have the Extensions type write the outer sequence and each - // contained extension once we've ported each of the below - // extensions to self.extensions(). - for ext in self.extensions().iter() { - Extensions::write_extension(writer, ext); - } - }); - } + self.extensions().write_der(writer.next()); }) } /// Returns the X.509 extensions that the [RevokedCertParams] describe. diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 7005a02d..5358b274 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -282,10 +282,16 @@ impl Extensions { writer.write_tagged(Tag::context(0), |writer| self.write_der(writer)); } - /// Write the SEQUENCE of extensions to the DER writer. - fn write_der(&self, writer: DERWriter) { + /// Write the SEQUENCE of extensions to the DER writer as a raw SEQUENCE. + /// Usually you will want to use `write_exts_der`, `write_csr_der` or `write_crl_der` instead. + pub(crate) fn write_der(&self, writer: DERWriter) { debug_assert_eq!(self.exts.len(), self.oids.len()); + // Avoid writing an empty extension SEQUENCE. + if self.exts.is_empty() { + return; + } + // Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension writer.write_sequence(|writer| { for extension in &self.exts { @@ -294,12 +300,8 @@ impl Extensions { }) } - pub(crate) fn iter(&self) -> impl Iterator> { - self.exts.iter() - } - /// Write a single extension SEQUENCE to the DER writer. - pub(crate) fn write_extension(writer: &mut DERWriterSeq, extension: &Box) { + fn write_extension(writer: &mut DERWriterSeq, extension: &Box) { // Extension ::= SEQUENCE { // extnID OBJECT IDENTIFIER, // critical BOOLEAN DEFAULT FALSE, From 98c020fe5ce091741a88f35577bdb5c2c263ea2c Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:17:50 -0500 Subject: [PATCH 27/29] wip: tidy up write_request --- rcgen/src/lib.rs | 33 ++------------------------------- 1 file changed, 2 insertions(+), 31 deletions(-) diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 91cd4ab8..b0542a9c 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -676,43 +676,14 @@ impl CertificateParams { Ok(params) } fn write_request(&self, pub_key: &K, writer: DERWriter) -> Result<(), Error> { - // No .. pattern, we use this to ensure every field is used - #[deny(unused)] - let Self { - alg, - not_before, - not_after, - serial_number, - subject_alt_names: _, - distinguished_name, - is_ca, - key_usages: _, - extended_key_usages: _, - name_constraints: _, - crl_distribution_points: _, - custom_extensions: _, - key_pair, - use_authority_key_identifier_extension, - key_identifier_method, - } = self; - // - alg and key_pair will be used by the caller - // - not_before and not_after cannot be put in a CSR - // - There might be a use case for specifying the key identifier - // in the CSR, but in the current API it can't be distinguished - // from the defaults so this is left for a later version if - // needed. - let _ = (alg, key_pair, not_before, not_after, key_identifier_method); - if serial_number.is_some() - || *is_ca != IsCa::NoCa - || *use_authority_key_identifier_extension - { + if self.serial_number.is_some() || self.use_authority_key_identifier_extension { return Err(Error::UnsupportedInCsr); } writer.write_sequence(|writer| { // Write version writer.next().write_u8(0); // Write issuer - write_distinguished_name(writer.next(), &distinguished_name); + write_distinguished_name(writer.next(), &self.distinguished_name); // Write subjectPublicKeyInfo pub_key.serialize_public_key_der(writer.next()); // Write extensions From 973271cc9c2e2d8127a08af3c650c432081c25e5 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:21:42 -0500 Subject: [PATCH 28/29] ext: fix type leak in custom extension --- rcgen/src/ext.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 5358b274..92893ffd 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -46,7 +46,7 @@ pub struct CustomExtension { /// OID identifying the extension. /// /// Only one extension with a given OID may appear within a certificate. - pub oid: ObjectIdentifier, + pub oid: Vec, /// Criticality of the extension. /// @@ -65,7 +65,7 @@ impl CustomExtension { /// Create a new custom extension with the specified content pub fn from_oid_content(oid: &[u64], criticality: Criticality, der_value: Vec) -> Self { Self { - oid: ObjectIdentifier::from_slice(oid), + oid: oid.to_vec(), criticality, der_value, } @@ -73,7 +73,7 @@ impl CustomExtension { /// Obtains the OID components of the extensions, as u64 pieces pub fn oid_components(&self) -> impl Iterator + '_ { - self.oid.components().iter().copied() + self.oid.iter().copied() } #[cfg(feature = "x509-parser")] @@ -81,13 +81,11 @@ impl CustomExtension { parsed: &x509_parser::prelude::X509Extension<'_>, ) -> Result { Ok(CustomExtension { - oid: ObjectIdentifier::from_slice( - &parsed - .oid - .iter() - .ok_or(Error::UnsupportedExtension)? - .collect::>(), - ), + oid: parsed + .oid + .iter() + .ok_or(Error::UnsupportedExtension)? + .collect::>(), criticality: if parsed.critical { Criticality::Critical } else { @@ -100,7 +98,7 @@ impl CustomExtension { impl Extension for CustomExtension { fn oid(&self) -> ObjectIdentifier { - self.oid.clone() + ObjectIdentifier::from_slice(&self.oid) } fn criticality(&self) -> Criticality { From 0225a26b6d8c046ff9a428c6ab4b4064080b0d7b Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Dec 2023 18:22:27 -0500 Subject: [PATCH 29/29] ext: mark new constructor as test-only --- rcgen/src/ext.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rcgen/src/ext.rs b/rcgen/src/ext.rs index 92893ffd..ddbf0f84 100644 --- a/rcgen/src/ext.rs +++ b/rcgen/src/ext.rs @@ -203,6 +203,7 @@ impl Extensions { /// # Errors /// /// Returns [Error::DuplicateExtension] if any of the extensions have the same OID. + #[cfg(test)] pub(crate) fn new( extensions: impl IntoIterator>, ) -> Result {