From 007a2d66e94e50b419551650fefa0354776856fa Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sat, 9 Sep 2023 21:49:14 -0400 Subject: [PATCH 1/4] tests: add x509-parser test for custom exts Previously there was no test coverage for custom extensions in certificates or CSRs. This commit adds a simple example of encoding a custom extension, and then demonstrating that it can be parsed with `x509-parser`, both in a serialized certificate and in a CSR. There's no support in webpki, openssl-rs or botan-rs for handling custom extensions so no test coverage for those libraries is possible at this time. --- tests/generic.rs | 68 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/generic.rs b/tests/generic.rs index 647eb7df..ad817a73 100644 --- a/tests/generic.rs +++ b/tests/generic.rs @@ -84,6 +84,74 @@ mod test_convert_x509_subject_alternative_name { } } +#[cfg(feature = "x509-parser")] +mod test_x509_custom_ext { + use crate::util; + use crate::Certificate; + + use rcgen::CustomExtension; + use x509_parser::oid_registry::asn1_rs; + use x509_parser::prelude::{ + FromDer, ParsedCriAttribute, X509Certificate, X509CertificationRequest, + }; + + #[test] + fn custom_ext() { + // Create an imaginary critical custom extension for testing. + let test_oid = asn1_rs::Oid::from(&[2, 5, 29, 999999]).unwrap(); + let test_ext = yasna::construct_der(|writer| { + writer.write_utf8_string("🦀 greetz to ferris 🦀"); + }); + let mut custom_ext = CustomExtension::from_oid_content( + test_oid.iter().unwrap().collect::>().as_slice(), + 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(); + params.custom_extensions = vec![custom_ext]; + let test_cert = Certificate::from_params(params).unwrap(); + let test_cert_der = test_cert.serialize_der().unwrap(); + let (_, x509_test_cert) = X509Certificate::from_der(&test_cert_der).unwrap(); + + // We should be able to find the extension by OID, with expected criticality and value. + let favorite_drink_ext = x509_test_cert + .get_extension_unique(&test_oid) + .expect("invalid extensions") + .expect("missing custom extension"); + assert_eq!(favorite_drink_ext.critical, true); + assert_eq!(favorite_drink_ext.value, test_ext); + + // Generate a CSR with the custom extension, parse it with x509-parser. + let test_cert_csr_der = test_cert.serialize_request_der().unwrap(); + let (_, x509_csr) = X509CertificationRequest::from_der(&test_cert_csr_der).unwrap(); + + // We should find that the CSR contains requested extensions. + // Note: we can't use `x509_csr.requested_extensions()` here because it maps the raw extension + // request extensions to their parsed form, and of course x509-parser doesn't parse our custom extension. + let exts = x509_csr + .certification_request_info + .iter_attributes() + .find_map(|attr| { + if let ParsedCriAttribute::ExtensionRequest(requested) = &attr.parsed_attribute() { + Some(requested.extensions.iter().collect::>()) + } else { + None + } + }) + .expect("missing requested extensions"); + + // We should find the custom extension with expected criticality and value. + let custom_ext = exts + .iter() + .find(|ext| ext.oid == test_oid) + .expect("missing requested custom extension"); + assert_eq!(custom_ext.critical, true); + assert_eq!(custom_ext.value, test_ext); + } +} + #[cfg(feature = "x509-parser")] mod test_x509_parser_crl { use crate::util; From 0604d2cb1908ae7ca50b8bf42cfebd48afc54dd5 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sat, 9 Sep 2023 20:32:28 -0400 Subject: [PATCH 2/4] lib: write key usage ext w/ helper. --- src/lib.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 7742e1c6..f3445e3b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -999,11 +999,7 @@ impl CertificateParams { // Write standard key usage if !self.key_usages.is_empty() { - writer.next().write_sequence(|writer| { - let oid = ObjectIdentifier::from_slice(OID_KEY_USAGE); - writer.next().write_oid(&oid); - writer.next().write_bool(true); - + write_x509_extension(writer.next(), OID_KEY_USAGE, true, |writer| { let mut bits: u16 = 0; for entry in self.key_usages.iter() { @@ -1032,12 +1028,7 @@ impl CertificateParams { // Finally take only the bytes != 0 let bits = &bits[..nb]; - let der = yasna::construct_der(|writer| { - writer.write_bitvec_bytes(&bits, msb as usize) - }); - - // Write them - writer.next().write_bytes(&der); + writer.write_bitvec_bytes(&bits, msb as usize) }); } From 401e95045ae8ed98a948b054e9e80ddfedf84cdd Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sat, 9 Sep 2023 21:52:30 -0400 Subject: [PATCH 3/4] lib: write custom exts w/ helper --- src/lib.rs | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f3445e3b..cb95916b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -922,17 +922,12 @@ impl CertificateParams { // Write custom extensions for ext in custom_extensions { - writer.next().write_sequence(|writer| { - let oid = ObjectIdentifier::from_slice(&ext.oid); - writer.next().write_oid(&oid); - // If the extension is critical, we should signal this. - // It's false by default so we don't need to write anything - // if the extension is not critical. - if ext.critical { - writer.next().write_bool(true); - } - writer.next().write_bytes(&ext.content); - }); + write_x509_extension( + writer.next(), + &ext.oid, + ext.critical, + |writer| writer.write_der(ext.content()), + ); } }); }); @@ -1148,16 +1143,8 @@ impl CertificateParams { // Write the custom extensions for ext in &self.custom_extensions { - writer.next().write_sequence(|writer| { - let oid = ObjectIdentifier::from_slice(&ext.oid); - writer.next().write_oid(&oid); - // If the extension is critical, we should signal this. - // It's false by default so we don't need to write anything - // if the extension is not critical. - if ext.critical { - writer.next().write_bool(true); - } - writer.next().write_bytes(&ext.content); + write_x509_extension(writer.next(), &ext.oid, ext.critical, |writer| { + writer.write_der(ext.content()) }); } }); From 646e8fcdffff8224307ac262225bdc6f77dae75a Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Sun, 10 Sep 2023 11:44:44 -0400 Subject: [PATCH 4/4] lib: fix custom exts for CSR w/o SANs. Previously when writing CSR DER from `CertificateParams` that specified custom extensions, but did not specify any SANs, the serialization code would skip over writing the PKCS9 extension request attribute. This commit updates the serialization logic to ensure the attribute is written when either SANs are provided, or custom extensions are present. Prior to this update, the modified `test_x509_custom_ext` test fails, reproducing the problem reported in the issue tracker: ``` 'test_x509_custom_ext::custom_ext' panicked at 'missing requested extensions' ``` With the update, it passes again. --- src/lib.rs | 2 +- tests/generic.rs | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index cb95916b..ceaad18d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -911,7 +911,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 !subject_alt_names.is_empty() { + 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); diff --git a/tests/generic.rs b/tests/generic.rs index ad817a73..2b158c7c 100644 --- a/tests/generic.rs +++ b/tests/generic.rs @@ -111,6 +111,9 @@ mod test_x509_custom_ext { // Generate a certificate with the custom extension, parse it with x509-parser. let mut params = util::default_params(); params.custom_extensions = vec![custom_ext]; + // Ensure the custom exts. being omitted into a CSR doesn't require SAN ext being present. + // See https://github.com/rustls/rcgen/issues/122 + params.subject_alt_names = Vec::default(); let test_cert = Certificate::from_params(params).unwrap(); let test_cert_der = test_cert.serialize_der().unwrap(); let (_, x509_test_cert) = X509Certificate::from_der(&test_cert_der).unwrap();