From 8c29e1298aac72f56bdabeacf2be9e7b2da9d20e Mon Sep 17 00:00:00 2001 From: Dmitry Baryshkov Date: Tue, 18 Apr 2023 03:54:56 +0300 Subject: [PATCH] x509-cert: builder updates (#1001) - Per RFC5280, DigitalSignature 'is asserted when the subject public key is used for verifying digital signatures, other than signatures on certificates (bit 5) and CRLs (bit 6)'. Using CA keys to sign random data would definitely be a bad practice and should be avoided. Thus remove the DigitalSignature keyUsage from these certificates. - RSA PSS implements DynSignatureAlgorithmIdentifier only for the SigningKey, not for the verifying key. To allow using CertificateBuilder with RSA PSS keys require DynSignatureAlgorithmIdentifier implementation on S rather than on S::VerifyingKey. - Signer (unlike SignerMut) is not expected to be mutable. Don't require mutability of the Signer argument. - ECDSA keys can not be used for keyEncipherment. Make this keyUsage bit optional. - Follow the rules from RFC 5280 Section 4.1.2.1 to set the certificate's version depending on the presence of the extensions and identifiers. - Remove unused conversion when building RDN fields. Signed-off-by: Dmitry Baryshkov --- x509-cert/Cargo.toml | 2 +- x509-cert/src/builder.rs | 64 +++++++++++++++-------------- x509-cert/test-support/src/zlint.rs | 4 +- x509-cert/tests/builder.rs | 58 ++++++++------------------ 4 files changed, 55 insertions(+), 73 deletions(-) diff --git a/x509-cert/Cargo.toml b/x509-cert/Cargo.toml index 8d74686d6..f216f6211 100644 --- a/x509-cert/Cargo.toml +++ b/x509-cert/Cargo.toml @@ -22,7 +22,7 @@ spki = { version = "0.7.1", features = ["alloc"] } # optional dependencies arbitrary = { version = "1.3", features = ["derive"], optional = true } sha1 = { version = "0.10.0", optional = true } -signature = { version = "2.1.0", features = ["digest"], optional = true } +signature = { version = "2.1.0", optional = true } [dev-dependencies] hex-literal = "0.4" diff --git a/x509-cert/src/builder.rs b/x509-cert/src/builder.rs index 8174678d7..10b050785 100644 --- a/x509-cert/src/builder.rs +++ b/x509-cert/src/builder.rs @@ -36,6 +36,9 @@ pub enum Error { Signature(signature::Error), } +#[cfg(feature = "std")] +impl std::error::Error for Error {} + impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { @@ -87,6 +90,8 @@ pub enum Profile { issuer: Name, /// should the key agreement flag of KeyUsage be enabled enable_key_agreement: bool, + /// should the key encipherment flag of KeyUsage be enabled + enable_key_encipherment: bool, }, #[cfg(feature = "hazmat")] /// Opt-out of the default extensions @@ -161,20 +166,18 @@ impl Profile { // Build Key Usage extension match self { Profile::Root | Profile::SubCA { .. } => { - extensions.push( - KeyUsage( - KeyUsages::DigitalSignature | KeyUsages::KeyCertSign | KeyUsages::CRLSign, - ) - .to_extension(tbs)?, - ); + extensions + .push(KeyUsage(KeyUsages::KeyCertSign | KeyUsages::CRLSign).to_extension(tbs)?); } Profile::Leaf { enable_key_agreement, + enable_key_encipherment, .. } => { - let mut key_usage = KeyUsages::DigitalSignature - | KeyUsages::NonRepudiation - | KeyUsages::KeyEncipherment; + let mut key_usage = KeyUsages::DigitalSignature | KeyUsages::NonRepudiation; + if *enable_key_encipherment { + key_usage |= KeyUsages::KeyEncipherment; + } if *enable_key_agreement { key_usage |= KeyUsages::KeyAgreement; } @@ -194,7 +197,6 @@ impl Profile { /// ``` /// use der::Decode; /// use x509_cert::spki::SubjectPublicKeyInfoOwned; -/// use x509_cert::certificate::Version; /// use x509_cert::builder::{CertificateBuilder, Profile}; /// use x509_cert::name::Name; /// use x509_cert::serial_number::SerialNumber; @@ -223,35 +225,32 @@ impl Profile { /// let mut signer = rsa_signer(); /// let mut builder = CertificateBuilder::new( /// profile, -/// Version::V3, /// serial_number, /// validity, /// subject, /// pub_key, -/// &mut signer, +/// &signer, /// ) /// .expect("Create certificate"); /// ``` pub struct CertificateBuilder<'s, S> { tbs: TbsCertificate, - signer: &'s mut S, + signer: &'s S, } impl<'s, S> CertificateBuilder<'s, S> where - S: Keypair, + S: Keypair + DynSignatureAlgorithmIdentifier, S::VerifyingKey: EncodePublicKey, - S::VerifyingKey: DynSignatureAlgorithmIdentifier, { /// Creates a new certificate builder pub fn new( profile: Profile, - version: Version, serial_number: SerialNumber, mut validity: Validity, subject: Name, subject_public_key_info: SubjectPublicKeyInfoOwned, - signer: &'s mut S, + signer: &'s S, ) -> Result where S: Signer, @@ -261,14 +260,14 @@ where .to_public_key_der()? .decode_msg::()?; - let signature_alg = verifying_key.signature_algorithm_identifier()?; + let signature_alg = signer.signature_algorithm_identifier()?; let issuer = profile.get_issuer(&subject); validity.not_before.rfc5280_adjust_utc_time()?; validity.not_after.rfc5280_adjust_utc_time()?; let mut tbs = TbsCertificate { - version, + version: Version::V3, serial_number, signature: signature_alg, issuer, @@ -286,15 +285,13 @@ where subject_unique_id: None, }; - if tbs.version == Version::V3 { - let extensions = profile.build_extensions( - tbs.subject_public_key_info.owned_to_ref(), - signer_pub.owned_to_ref(), - &tbs, - )?; - if !extensions.is_empty() { - tbs.extensions = Some(extensions); - } + let extensions = profile.build_extensions( + tbs.subject_public_key_info.owned_to_ref(), + signer_pub.owned_to_ref(), + &tbs, + )?; + if !extensions.is_empty() { + tbs.extensions = Some(extensions); } Ok(Self { tbs, signer }) @@ -317,17 +314,24 @@ where } /// Run the certificate through the signer and build the end certificate. - pub fn build(&mut self) -> Result + pub fn build(mut self) -> Result where S: Signer, Signature: SignatureEncoding, { + if self.tbs.extensions.is_none() { + if self.tbs.issuer_unique_id.is_some() || self.tbs.subject_unique_id.is_some() { + self.tbs.version = Version::V2; + } else { + self.tbs.version = Version::V1; + } + } let signature = self.signer.try_sign(&self.tbs.to_der()?)?; let signature = BitString::from_bytes(signature.to_bytes().as_ref())?; let cert = Certificate { tbs_certificate: self.tbs.clone(), - signature_algorithm: self.tbs.signature.clone(), + signature_algorithm: self.tbs.signature, signature, }; diff --git a/x509-cert/test-support/src/zlint.rs b/x509-cert/test-support/src/zlint.rs index f13c1dced..dd72deadc 100644 --- a/x509-cert/test-support/src/zlint.rs +++ b/x509-cert/test-support/src/zlint.rs @@ -11,7 +11,7 @@ use std::{ }; use tempfile::tempdir; -#[derive(Debug, Copy, Clone, PartialEq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum Status { NotApplicable, NotEffective, @@ -29,7 +29,7 @@ impl Status { } } -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct LintStatus { pub status: Status, pub details: Option, diff --git a/x509-cert/tests/builder.rs b/x509-cert/tests/builder.rs index 0a2622f8d..4dd730fba 100644 --- a/x509-cert/tests/builder.rs +++ b/x509-cert/tests/builder.rs @@ -9,7 +9,6 @@ use spki::SubjectPublicKeyInfoOwned; use std::{str::FromStr, time::Duration}; use x509_cert::{ builder::{CertificateBuilder, Profile}, - certificate::Version, name::Name, serial_number::SerialNumber, time::Validity, @@ -31,17 +30,10 @@ fn root_ca_certificate() { let pub_key = SubjectPublicKeyInfoOwned::try_from(RSA_2048_DER_EXAMPLE).expect("get rsa pub key"); - let mut signer = rsa_signer(); - let mut builder = CertificateBuilder::new( - profile, - Version::V3, - serial_number, - validity, - subject, - pub_key, - &mut signer, - ) - .expect("Create certificate"); + let signer = rsa_signer(); + let builder = + CertificateBuilder::new(profile, serial_number, validity, subject, pub_key, &signer) + .expect("Create certificate"); let certificate = builder.build().unwrap(); @@ -57,33 +49,26 @@ fn sub_ca_certificate() { let serial_number = SerialNumber::from(42u32); let validity = Validity::from_now(Duration::new(5, 0)).unwrap(); - let issuer = Name::from_str("CN=World domination corporation,O=World domination Inc,C=US") - .unwrap() - .to_der() - .unwrap(); - let issuer = Name::from_der(&issuer).unwrap(); + let issuer = + Name::from_str("CN=World domination corporation,O=World domination Inc,C=US").unwrap(); let profile = Profile::SubCA { issuer, path_len_constraint: Some(0), }; - let subject = Name::from_str("CN=World domination task force,O=World domination Inc,C=US") - .unwrap() - .to_der() - .unwrap(); - let subject = Name::from_der(&subject).unwrap(); + let subject = + Name::from_str("CN=World domination task force,O=World domination Inc,C=US").unwrap(); let pub_key = SubjectPublicKeyInfoOwned::try_from(RSA_2048_DER_EXAMPLE).expect("get rsa pub key"); - let mut signer = ecdsa_signer(); - let mut builder = CertificateBuilder::new::>( + let signer = ecdsa_signer(); + let builder = CertificateBuilder::new::>( profile, - Version::V3, serial_number, validity, subject, pub_key, - &mut signer, + &signer, ) .expect("Create certificate"); @@ -108,33 +93,26 @@ fn leaf_certificate() { let serial_number = SerialNumber::from(42u32); let validity = Validity::from_now(Duration::new(5, 0)).unwrap(); - let issuer = Name::from_str("CN=World domination corporation,O=World domination Inc,C=US") - .unwrap() - .to_der() - .unwrap(); - let issuer = Name::from_der(&issuer).unwrap(); + let issuer = + Name::from_str("CN=World domination corporation,O=World domination Inc,C=US").unwrap(); let profile = Profile::Leaf { issuer, enable_key_agreement: false, + enable_key_encipherment: false, }; - let subject = Name::from_str("CN=service.domination.world") - .unwrap() - .to_der() - .unwrap(); - let subject = Name::from_der(&subject).unwrap(); + let subject = Name::from_str("CN=service.domination.world").unwrap(); let pub_key = SubjectPublicKeyInfoOwned::try_from(RSA_2048_DER_EXAMPLE).expect("get rsa pub key"); - let mut signer = ecdsa_signer(); - let mut builder = CertificateBuilder::new::>( + let signer = ecdsa_signer(); + let builder = CertificateBuilder::new::>( profile, - Version::V3, serial_number, validity, subject, pub_key, - &mut signer, + &signer, ) .expect("Create certificate");