From 7fe09b660b371b6e242b673400be948764fb39b5 Mon Sep 17 00:00:00 2001 From: Lukas Velikov Date: Mon, 11 Nov 2024 21:49:13 -0500 Subject: [PATCH 1/9] Reorganize CSR roundtrip unit tests --- rcgen/tests/generic.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/rcgen/tests/generic.rs b/rcgen/tests/generic.rs index d9eef933..39910d74 100644 --- a/rcgen/tests/generic.rs +++ b/rcgen/tests/generic.rs @@ -364,16 +364,9 @@ mod test_csr { #[test] fn test_csr_roundtrip() { - let key_pair = KeyPair::generate().unwrap(); - // We should be able to serialize a CSR, and then parse the CSR. - let csr = CertificateParams::default() - .serialize_request(&key_pair) - .unwrap(); - let csrp = CertificateSigningRequestParams::from_der(csr.der()).unwrap(); - - // Ensure algorithms match. - assert_eq!(key_pair.algorithm(), csrp.public_key.algorithm()); + let params = CertificateParams::default(); + generate_and_test_parsed_csr(¶ms); } #[test] @@ -395,7 +388,15 @@ mod test_csr { // KeyUsagePurpose::EncipherOnly, KeyUsagePurpose::DecipherOnly, ]; + generate_and_test_parsed_csr(¶ms); + } + + fn generate_and_test_parsed_csr(params: &CertificateParams) { + // Generate a key pair for the CSR + let key_pair = KeyPair::generate().unwrap(); + // Serialize the CSR into DER from the given parameters let csr = params.serialize_request(&key_pair).unwrap(); + // Parse the CSR we just serialized let csrp = CertificateSigningRequestParams::from_der(csr.der()).unwrap(); // Ensure algorithms match. From 5bfe41bf55d9afe8e71059fecae247afcf2217bb Mon Sep 17 00:00:00 2001 From: lvkv Date: Tue, 12 Nov 2024 22:35:08 -0500 Subject: [PATCH 2/9] Implement PartialEq, Eq for CertificateParams --- rcgen/src/certificate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcgen/src/certificate.rs b/rcgen/src/certificate.rs index 68ec0f67..8e0eb3bd 100644 --- a/rcgen/src/certificate.rs +++ b/rcgen/src/certificate.rs @@ -63,7 +63,7 @@ impl From for CertificateDer<'static> { /// Parameters used for certificate generation #[allow(missing_docs)] #[non_exhaustive] -#[derive(Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] pub struct CertificateParams { pub not_before: OffsetDateTime, pub not_after: OffsetDateTime, From 644701358a5cea0102daec2878c96f3b4955046f Mon Sep 17 00:00:00 2001 From: lvkv Date: Tue, 12 Nov 2024 22:59:51 -0500 Subject: [PATCH 3/9] Add validation logic to CSR roundtrip unit tests --- rcgen/tests/generic.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rcgen/tests/generic.rs b/rcgen/tests/generic.rs index 39910d74..1fd2d2bb 100644 --- a/rcgen/tests/generic.rs +++ b/rcgen/tests/generic.rs @@ -401,7 +401,7 @@ mod test_csr { // Ensure algorithms match. assert_eq!(key_pair.algorithm(), csrp.public_key.algorithm()); - // Ensure key usages match. - assert_eq!(csrp.params.key_usages, params.key_usages); + // Assert that our parsed parameters match our initial parameters + assert_eq!(*params, csrp.params); } } From 5d243d318afb5e8bc5284c00387ee30449408dd5 Mon Sep 17 00:00:00 2001 From: Lukas Velikov Date: Mon, 11 Nov 2024 21:55:03 -0500 Subject: [PATCH 4/9] Add more CSR roundtrip unit tests --- rcgen/tests/generic.rs | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/rcgen/tests/generic.rs b/rcgen/tests/generic.rs index 1fd2d2bb..a04864b9 100644 --- a/rcgen/tests/generic.rs +++ b/rcgen/tests/generic.rs @@ -360,7 +360,10 @@ mod test_parse_other_name_alt_name { #[cfg(feature = "x509-parser")] mod test_csr { - use rcgen::{CertificateParams, CertificateSigningRequestParams, KeyPair, KeyUsagePurpose}; + use rcgen::{ + CertificateParams, CertificateSigningRequestParams, ExtendedKeyUsagePurpose, KeyPair, + KeyUsagePurpose, + }; #[test] fn test_csr_roundtrip() { @@ -370,10 +373,7 @@ mod test_csr { } #[test] - fn test_nontrivial_csr_roundtrip() { - let key_pair = KeyPair::generate().unwrap(); - - // We should be able to serialize a CSR, and then parse the CSR. + fn test_csr_with_key_usages_roundtrip() { let mut params = CertificateParams::default(); params.key_usages = vec![ KeyUsagePurpose::DigitalSignature, @@ -391,6 +391,24 @@ mod test_csr { generate_and_test_parsed_csr(¶ms); } + #[test] + fn test_csr_with_extended_key_usages_roundtrip() { + let mut params = CertificateParams::default(); + params.extended_key_usages = vec![ + ExtendedKeyUsagePurpose::ServerAuth, + ExtendedKeyUsagePurpose::ClientAuth, + ]; + generate_and_test_parsed_csr(¶ms); + } + + #[test] + fn test_csr_with_key_usgaes_and_extended_key_usages_roundtrip() { + let mut params = CertificateParams::default(); + params.key_usages = vec![KeyUsagePurpose::DigitalSignature]; + params.extended_key_usages = vec![ExtendedKeyUsagePurpose::ClientAuth]; + generate_and_test_parsed_csr(¶ms); + } + fn generate_and_test_parsed_csr(params: &CertificateParams) { // Generate a key pair for the CSR let key_pair = KeyPair::generate().unwrap(); From e947f64387d215c0b5329debe58aa0b47b249bc2 Mon Sep 17 00:00:00 2001 From: Lukas Velikov Date: Sat, 9 Nov 2024 15:18:12 -0500 Subject: [PATCH 5/9] Fix: Don't write SANs in CSRs if none are present --- rcgen/src/certificate.rs | 4 ++++ rcgen/tests/generic.rs | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/rcgen/src/certificate.rs b/rcgen/src/certificate.rs index 8e0eb3bd..4b8239df 100644 --- a/rcgen/src/certificate.rs +++ b/rcgen/src/certificate.rs @@ -461,6 +461,10 @@ impl CertificateParams { } fn write_subject_alt_names(&self, writer: DERWriter) { + if self.subject_alt_names.is_empty() { + return; + } + write_x509_extension(writer, oid::SUBJECT_ALT_NAME, false, |writer| { writer.write_sequence(|writer| { for san in self.subject_alt_names.iter() { diff --git a/rcgen/tests/generic.rs b/rcgen/tests/generic.rs index a04864b9..3f804493 100644 --- a/rcgen/tests/generic.rs +++ b/rcgen/tests/generic.rs @@ -358,6 +358,25 @@ mod test_parse_other_name_alt_name { } } +#[cfg(feature = "x509-parser")] +mod test_csr_extension_request { + use rcgen::{CertificateParams, ExtendedKeyUsagePurpose, KeyPair, KeyUsagePurpose}; + use x509_parser::prelude::{FromDer, ParsedExtension, X509CertificationRequest}; + + #[test] + fn dont_write_sans_extension_if_no_sans_are_present() { + let mut params = CertificateParams::default(); + params.key_usages.push(KeyUsagePurpose::DigitalSignature); + let key_pair = KeyPair::generate().unwrap(); + let csr = params.serialize_request(&key_pair).unwrap(); + let (_, parsed_csr) = X509CertificationRequest::from_der(csr.der()).unwrap(); + assert!(!parsed_csr + .requested_extensions() + .unwrap() + .any(|ext| matches!(ext, ParsedExtension::SubjectAlternativeName(_)))); + } +} + #[cfg(feature = "x509-parser")] mod test_csr { use rcgen::{ From f00ada8fc45792308e6e9459213d92c214b549eb Mon Sep 17 00:00:00 2001 From: Lukas Velikov Date: Sat, 9 Nov 2024 15:49:29 -0500 Subject: [PATCH 6/9] Fix: Don't forget to write EKUs in CSRs --- rcgen/src/certificate.rs | 11 +++++++---- rcgen/tests/generic.rs | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/rcgen/src/certificate.rs b/rcgen/src/certificate.rs index 4b8239df..9fa49364 100644 --- a/rcgen/src/certificate.rs +++ b/rcgen/src/certificate.rs @@ -546,6 +546,12 @@ impl CertificateParams { return Err(Error::UnsupportedInCsr); } + // Whether or not to write an extension request attribute + let write_extension_request = !key_usages.is_empty() + || !subject_alt_names.is_empty() + || !extended_key_usages.is_empty() + || !custom_extensions.is_empty(); + let der = subject_key.sign_der(|writer| { // Write version writer.next().write_u8(0); @@ -556,10 +562,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| { - if !key_usages.is_empty() - || !subject_alt_names.is_empty() - || !custom_extensions.is_empty() - { + if write_extension_request { writer.write_sequence(|writer| { let oid = ObjectIdentifier::from_slice(oid::PKCS_9_AT_EXTENSION_REQUEST); writer.next().write_oid(&oid); diff --git a/rcgen/tests/generic.rs b/rcgen/tests/generic.rs index 3f804493..3c336888 100644 --- a/rcgen/tests/generic.rs +++ b/rcgen/tests/generic.rs @@ -375,6 +375,25 @@ mod test_csr_extension_request { .unwrap() .any(|ext| matches!(ext, ParsedExtension::SubjectAlternativeName(_)))); } + + #[test] + fn write_extension_request_if_ekus_are_present() { + let mut params = CertificateParams::default(); + params + .extended_key_usages + .push(ExtendedKeyUsagePurpose::ClientAuth); + let key_pair = KeyPair::generate().unwrap(); + let csr = params.serialize_request(&key_pair).unwrap(); + let (_, parsed_csr) = X509CertificationRequest::from_der(csr.der()).unwrap(); + let requested_extensions = parsed_csr + .requested_extensions() + .unwrap() + .collect::>(); + assert!(matches!( + requested_extensions.first().unwrap(), + ParsedExtension::ExtendedKeyUsage(_) + )); + } } #[cfg(feature = "x509-parser")] From b11c9cab07f7fa74261cc923308d1eb75ce9c5a1 Mon Sep 17 00:00:00 2001 From: Lukas Velikov Date: Sat, 9 Nov 2024 15:53:59 -0500 Subject: [PATCH 7/9] Fix: Write CSR attributes as an implicit set --- rcgen/src/certificate.rs | 54 ++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/rcgen/src/certificate.rs b/rcgen/src/certificate.rs index 9fa49364..1b5912c0 100644 --- a/rcgen/src/certificate.rs +++ b/rcgen/src/certificate.rs @@ -561,33 +561,39 @@ impl CertificateParams { serialize_public_key_der(subject_key, writer.next()); // 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| { - if write_extension_request { - 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_tagged_implicit(Tag::context(0), |writer| { + // RFC 2986 specifies that attributes are a SET OF Attribute + writer.write_set_of(|writer| { + if write_extension_request { writer.next().write_sequence(|writer| { - // Write key_usage - self.write_key_usage(writer.next()); - // Write subject_alt_names - self.write_subject_alt_names(writer.next()); - self.write_extended_key_usage(writer.next()); - - // Write custom extensions - for ext in custom_extensions { - write_x509_extension( - writer.next(), - &ext.oid, - ext.critical, - |writer| writer.write_der(ext.content()), - ); - } + 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| { + // Write key_usage + self.write_key_usage(writer.next()); + // Write subject_alt_names + self.write_subject_alt_names(writer.next()); + self.write_extended_key_usage(writer.next()); + + // Write custom extensions + for ext in custom_extensions { + write_x509_extension( + writer.next(), + &ext.oid, + ext.critical, + |writer| writer.write_der(ext.content()), + ); + } + }); + }); }); - }); + } }); - } - }); + }); Ok(()) })?; From 8f97ee6bbd6b11dbe2d8d09548a7175b37b0e84d Mon Sep 17 00:00:00 2001 From: Lukas Velikov Date: Sat, 9 Nov 2024 15:57:50 -0500 Subject: [PATCH 8/9] Add CertificateParams::write_extension_request_attribute for cleanup --- rcgen/src/certificate.rs | 52 +++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/rcgen/src/certificate.rs b/rcgen/src/certificate.rs index 1b5912c0..caf6d710 100644 --- a/rcgen/src/certificate.rs +++ b/rcgen/src/certificate.rs @@ -427,6 +427,33 @@ impl CertificateParams { Ok(result) } + /// Write a CSR extension request attribute as defined in [RFC 2985]. + /// + /// [RFC 2985]: + fn write_extension_request_attribute(&self, writer: DERWriter) { + writer.write_sequence(|writer| { + writer.next().write_oid(&ObjectIdentifier::from_slice( + oid::PKCS_9_AT_EXTENSION_REQUEST, + )); + writer.next().write_set(|writer| { + writer.next().write_sequence(|writer| { + // Write key_usage + self.write_key_usage(writer.next()); + // Write subject_alt_names + self.write_subject_alt_names(writer.next()); + self.write_extended_key_usage(writer.next()); + + // Write custom extensions + for ext in &self.custom_extensions { + write_x509_extension(writer.next(), &ext.oid, ext.critical, |writer| { + writer.write_der(ext.content()) + }); + } + }); + }); + }); + } + /// Write a certificate's KeyUsage as defined in RFC 5280. fn write_key_usage(&self, writer: DERWriter) { // RFC 5280 defines 9 key usages, which we detail in our key usage enum @@ -567,30 +594,7 @@ impl CertificateParams { // RFC 2986 specifies that attributes are a SET OF Attribute writer.write_set_of(|writer| { if write_extension_request { - writer.next().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| { - // Write key_usage - self.write_key_usage(writer.next()); - // Write subject_alt_names - self.write_subject_alt_names(writer.next()); - self.write_extended_key_usage(writer.next()); - - // Write custom extensions - for ext in custom_extensions { - write_x509_extension( - writer.next(), - &ext.oid, - ext.critical, - |writer| writer.write_der(ext.content()), - ); - } - }); - }); - }); + self.write_extension_request_attribute(writer.next()); } }); }); From 2ae10baced660b04875f3179c61ae4ecc7346fec Mon Sep 17 00:00:00 2001 From: Lukas Velikov Date: Sat, 9 Nov 2024 16:02:48 -0500 Subject: [PATCH 9/9] Add PKCS#10 attributes to CSR serializer --- rcgen/src/certificate.rs | 49 +++++++++++++++++++++++++++++++--- rcgen/src/lib.rs | 4 +-- rcgen/tests/generic.rs | 57 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 5 deletions(-) diff --git a/rcgen/src/certificate.rs b/rcgen/src/certificate.rs index caf6d710..0c409048 100644 --- a/rcgen/src/certificate.rs +++ b/rcgen/src/certificate.rs @@ -533,6 +533,25 @@ impl CertificateParams { pub fn serialize_request( &self, subject_key: &KeyPair, + ) -> Result { + self.serialize_request_with_attributes(subject_key, Vec::new()) + } + + /// Generate and serialize a certificate signing request (CSR) with custom PKCS #10 attributes. + /// as defined in [RFC 2986]. + /// + /// The constructed CSR will contain attributes based on the certificate parameters, + /// and include the subject public key information from `subject_key`. Additionally, + /// the CSR will be self-signed using the subject key. + /// + /// Note that subsequent invocations of `serialize_request_with_attributes()` will not produce the exact + /// same output. + /// + /// [RFC 2986]: + pub fn serialize_request_with_attributes( + &self, + subject_key: &KeyPair, + attrs: Vec, ) -> Result { // No .. pattern, we use this to ensure every field is used #[deny(unused)] @@ -582,11 +601,9 @@ impl CertificateParams { let der = subject_key.sign_der(|writer| { // Write version writer.next().write_u8(0); - // Write subject name write_distinguished_name(writer.next(), distinguished_name); - // Write subjectPublicKeyInfo serialize_public_key_der(subject_key, writer.next()); - // Write extensions + // According to the spec in RFC 2986, even if attributes are empty we need the empty attribute tag writer .next() @@ -596,6 +613,13 @@ impl CertificateParams { if write_extension_request { self.write_extension_request_attribute(writer.next()); } + + for Attribute { oid, values } in attrs { + writer.next().write_sequence(|writer| { + writer.next().write_oid(&ObjectIdentifier::from_slice(&oid)); + writer.next().write_der(&values); + }); + } }); }); @@ -846,6 +870,25 @@ fn write_general_subtrees(writer: DERWriter, tag: u64, general_subtrees: &[Gener }); } +/// A PKCS #10 CSR attribute, as defined in [RFC 5280] and constrained +/// by [RFC 2986]. +/// +/// [RFC 5280]: +/// [RFC 2986]: +#[derive(Debug, PartialEq, Eq, Hash, Clone)] +pub struct Attribute { + /// `AttributeType` of the `Attribute`, defined as an `OBJECT IDENTIFIER`. + pub oid: &'static [u64], + /// DER-encoded values of the `Attribute`, defined by [RFC 2986] as: + /// + /// ```text + /// SET SIZE(1..MAX) OF ATTRIBUTE.&Type({IOSet}{@type}) + /// ``` + /// + /// [RFC 2986]: https://datatracker.ietf.org/doc/html/rfc2986#section-4 + pub values: Vec, +} + /// 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)] diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 6aea3f2d..4f8fb639 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -49,8 +49,8 @@ use yasna::DERWriter; use yasna::Tag; pub use certificate::{ - date_time_ymd, BasicConstraints, Certificate, CertificateParams, CidrSubnet, CustomExtension, - DnType, ExtendedKeyUsagePurpose, GeneralSubtree, IsCa, NameConstraints, + date_time_ymd, Attribute, BasicConstraints, Certificate, CertificateParams, CidrSubnet, + CustomExtension, DnType, ExtendedKeyUsagePurpose, GeneralSubtree, IsCa, NameConstraints, }; pub use crl::{ CertificateRevocationList, CertificateRevocationListParams, CrlDistributionPoint, diff --git a/rcgen/tests/generic.rs b/rcgen/tests/generic.rs index 3c336888..73324a42 100644 --- a/rcgen/tests/generic.rs +++ b/rcgen/tests/generic.rs @@ -135,6 +135,63 @@ mod test_x509_custom_ext { } } +#[cfg(feature = "x509-parser")] +mod test_csr_custom_attributes { + use rcgen::{Attribute, CertificateParams, KeyPair}; + use x509_parser::{ + der_parser::Oid, + prelude::{FromDer, X509CertificationRequest}, + }; + + /// Test serializing a CSR with custom attributes. + /// This test case uses `challengePassword` from [RFC 2985], a simple + /// ATTRIBUTE that contains a single UTF8String. + /// + /// [RFC 2985]: + #[test] + fn test_csr_custom_attributes() { + // OID for challengePassword + const CHALLENGE_PWD_OID: &[u64] = &[1, 2, 840, 113549, 1, 9, 7]; + + // Attribute values for challengePassword + let challenge_pwd_values = yasna::try_construct_der::<_, ()>(|writer| { + // Reminder: CSR attribute values are contained in a SET + writer.write_set(|writer| { + // Challenge passwords only have one value, a UTF8String + writer + .next() + .write_utf8_string("nobody uses challenge passwords anymore"); + Ok(()) + }) + }) + .unwrap(); + + // Challenge password attribute + let challenge_password_attribute = Attribute { + oid: CHALLENGE_PWD_OID, + values: challenge_pwd_values.clone(), + }; + + // Serialize a DER-encoded CSR + let params = CertificateParams::default(); + let key_pair = KeyPair::generate().unwrap(); + let csr = params + .serialize_request_with_attributes(&key_pair, vec![challenge_password_attribute]) + .unwrap(); + + // Parse the CSR + let (_, x509_csr) = X509CertificationRequest::from_der(csr.der()).unwrap(); + let parsed_attribute_value = x509_csr + .certification_request_info + .attributes_map() + .unwrap() + .get(&Oid::from(CHALLENGE_PWD_OID).unwrap()) + .unwrap() + .value; + assert_eq!(parsed_attribute_value, challenge_pwd_values); + } +} + #[cfg(feature = "x509-parser")] mod test_x509_parser_crl { use crate::util;