From 6f2b01f47f59c83dc7fe09699d3526069ea3f0ec Mon Sep 17 00:00:00 2001 From: Daniel Karzel Date: Thu, 21 Nov 2024 17:34:23 +1100 Subject: [PATCH 1/4] Store issuer items as vec and not map There can be multiple of the same entry, e.g. DC (domain component) entries in an issuer. Using a HashMap is too restrictive. --- rcgen/src/certificate.rs | 8 ++--- rcgen/src/crl.rs | 2 +- rcgen/src/lib.rs | 70 ++++++++++++++++++---------------------- 3 files changed, 37 insertions(+), 43 deletions(-) diff --git a/rcgen/src/certificate.rs b/rcgen/src/certificate.rs index 0c409048..9ca32a96 100644 --- a/rcgen/src/certificate.rs +++ b/rcgen/src/certificate.rs @@ -601,7 +601,7 @@ impl CertificateParams { let der = subject_key.sign_der(|writer| { // Write version writer.next().write_u8(0); - write_distinguished_name(writer.next(), distinguished_name); + write_distinguished_name(writer.next(), distinguished_name.clone()); serialize_public_key_der(subject_key, writer.next()); // According to the spec in RFC 2986, even if attributes are empty we need the empty attribute tag @@ -663,7 +663,7 @@ impl CertificateParams { // Write signature algorithm issuer.key_pair.alg.write_alg_ident(writer.next()); // Write issuer name - write_distinguished_name(writer.next(), &issuer.distinguished_name); + write_distinguished_name(writer.next(), issuer.distinguished_name.clone()); // Write validity writer.next().write_sequence(|writer| { // Not before @@ -673,7 +673,7 @@ impl CertificateParams { Ok::<(), Error>(()) })?; // Write subject - write_distinguished_name(writer.next(), &self.distinguished_name); + write_distinguished_name(writer.next(), self.distinguished_name.clone()); // Write subjectPublicKeyInfo serialize_public_key_der(pub_key, writer.next()); // write extensions @@ -856,7 +856,7 @@ fn write_general_subtrees(writer: DERWriter, tag: u64, general_subtrees: &[Gener GeneralSubtree::Rfc822Name(name) | GeneralSubtree::DnsName(name) => writer.write_ia5_string(name), GeneralSubtree::DirectoryName(name) => { - write_distinguished_name(writer, name) + write_distinguished_name(writer, name.clone()) }, GeneralSubtree::IpAddress(subnet) => { writer.write_bytes(&subnet.to_bytes()) diff --git a/rcgen/src/crl.rs b/rcgen/src/crl.rs index 19ff6bf0..3059b1d2 100644 --- a/rcgen/src/crl.rs +++ b/rcgen/src/crl.rs @@ -234,7 +234,7 @@ impl CertificateRevocationListParams { // Write issuer. // RFC 5280 §5.1.2.3: // The issuer field MUST contain a non-empty X.500 distinguished name (DN). - write_distinguished_name(writer.next(), &issuer.distinguished_name); + write_distinguished_name(writer.next(), issuer.distinguished_name.clone()); // Write thisUpdate date. // RFC 5280 §5.1.2.4: diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index e99540a7..9f282669 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -33,7 +33,6 @@ println!("{}", key_pair.serialize_pem()); #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] #![warn(unreachable_pub)] -use std::collections::HashMap; use std::fmt; use std::hash::Hash; use std::net::IpAddr; @@ -299,8 +298,7 @@ See also the RFC 5280 sections on the [issuer](https://tools.ietf.org/html/rfc52 and [subject](https://tools.ietf.org/html/rfc5280#section-4.1.2.6) fields. */ pub struct DistinguishedName { - entries: HashMap, - order: Vec, + entries: Vec<(DnType, DnValue)>, } impl DistinguishedName { @@ -309,8 +307,11 @@ impl DistinguishedName { Self::default() } /// Obtains the attribute value for the given attribute type - pub fn get(&self, ty: &DnType) -> Option<&DnValue> { - self.entries.get(ty) + pub fn get(&self, ty: &DnType) -> Vec<&DnValue> { + self.entries + .iter() + .filter_map(|(dn_type, dn_value)| if ty == dn_type { Some(dn_value) } else { None }) + .collect() } /// Removes the attribute with the specified DnType /// @@ -318,11 +319,20 @@ impl DistinguishedName { /// when no attribute with the specified DnType was /// found. pub fn remove(&mut self, ty: DnType) -> bool { - let removed = self.entries.remove(&ty).is_some(); - if removed { - self.order.retain(|ty_o| &ty != ty_o); + let mut remove_indices = vec![]; + for (index, (dn_type, _dn_val)) in self.entries.iter().enumerate() { + if dn_type == &ty { + remove_indices.push(index); + } + } + + let is_remove_indices = !remove_indices.is_empty(); + + for index in remove_indices { + self.entries.remove(index); } - removed + + is_remove_indices } /// Inserts or updates an attribute that consists of type and name /// @@ -331,21 +341,11 @@ impl DistinguishedName { /// let mut dn = DistinguishedName::new(); /// dn.push(DnType::OrganizationName, "Crab widgits SE"); /// dn.push(DnType::CommonName, DnValue::PrintableString("Master Cert".try_into().unwrap())); - /// assert_eq!(dn.get(&DnType::OrganizationName), Some(&DnValue::Utf8String("Crab widgits SE".to_string()))); - /// assert_eq!(dn.get(&DnType::CommonName), Some(&DnValue::PrintableString("Master Cert".try_into().unwrap()))); + /// assert_eq!(dn.get(&DnType::OrganizationName).get(0), Some(&DnValue::Utf8String("Crab widgits SE".to_string())).as_ref()); + /// assert_eq!(dn.get(&DnType::CommonName).get(0), Some(&DnValue::PrintableString("Master Cert".try_into().unwrap())).as_ref()); /// ``` pub fn push(&mut self, ty: DnType, s: impl Into) { - if !self.entries.contains_key(&ty) { - self.order.push(ty.clone()); - } - self.entries.insert(ty, s.into()); - } - /// Iterate over the entries - pub fn iter(&self) -> DistinguishedNameIterator<'_> { - DistinguishedNameIterator { - distinguished_name: self, - iter: self.order.iter(), - } + self.entries.push((ty, s.into())); } #[cfg(feature = "x509-parser")] @@ -393,21 +393,15 @@ impl DistinguishedName { } } -/** -Iterator over [`DistinguishedName`] entries -*/ -pub struct DistinguishedNameIterator<'a> { - distinguished_name: &'a DistinguishedName, - iter: std::slice::Iter<'a, DnType>, -} - -impl<'a> Iterator for DistinguishedNameIterator<'a> { - type Item = (&'a DnType, &'a DnValue); +impl Iterator for DistinguishedName { + type Item = (DnType, DnValue); fn next(&mut self) -> Option { - self.iter - .next() - .and_then(|ty| self.distinguished_name.entries.get(ty).map(|v| (ty, v))) + self.entries.pop() + } + + fn size_hint(&self) -> (usize, Option) { + self.entries.iter().size_hint() } } @@ -568,9 +562,9 @@ fn write_dt_utc_or_generalized(writer: DERWriter, dt: OffsetDateTime) { } } -fn write_distinguished_name(writer: DERWriter, dn: &DistinguishedName) { +fn write_distinguished_name(writer: DERWriter, dn: DistinguishedName) { writer.write_sequence(|writer| { - for (ty, content) in dn.iter() { + for (ty, content) in dn.into_iter() { writer.next().write_set(|writer| { writer.next().write_sequence(|writer| { writer.next().write_oid(&ty.to_oid()); @@ -596,7 +590,7 @@ fn write_distinguished_name(writer: DERWriter, dn: &DistinguishedName) { .write_tagged_implicit(TAG_UNIVERSALSTRING, |writer| { writer.write_bytes(s.as_bytes()) }), - DnValue::Utf8String(s) => writer.next().write_utf8_string(s), + DnValue::Utf8String(s) => writer.next().write_utf8_string(s.as_str()), } }); }); From 7f3368d31e884226133839a81a19bd65095e6e61 Mon Sep 17 00:00:00 2001 From: Daniel Karzel Date: Thu, 21 Nov 2024 18:34:56 +1100 Subject: [PATCH 2/4] Use VecDeque and pop from wrong when iterating The order was reversed after parsing a cert. --- rcgen/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 9f282669..209bfeb3 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -33,6 +33,7 @@ println!("{}", key_pair.serialize_pem()); #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] #![warn(unreachable_pub)] +use std::collections::VecDeque; use std::fmt; use std::hash::Hash; use std::net::IpAddr; @@ -298,7 +299,7 @@ See also the RFC 5280 sections on the [issuer](https://tools.ietf.org/html/rfc52 and [subject](https://tools.ietf.org/html/rfc5280#section-4.1.2.6) fields. */ pub struct DistinguishedName { - entries: Vec<(DnType, DnValue)>, + entries: VecDeque<(DnType, DnValue)>, } impl DistinguishedName { @@ -345,7 +346,7 @@ impl DistinguishedName { /// assert_eq!(dn.get(&DnType::CommonName).get(0), Some(&DnValue::PrintableString("Master Cert".try_into().unwrap())).as_ref()); /// ``` pub fn push(&mut self, ty: DnType, s: impl Into) { - self.entries.push((ty, s.into())); + self.entries.push_front((ty, s.into())); } #[cfg(feature = "x509-parser")] @@ -397,7 +398,7 @@ impl Iterator for DistinguishedName { type Item = (DnType, DnValue); fn next(&mut self) -> Option { - self.entries.pop() + self.entries.pop_back() } fn size_hint(&self) -> (usize, Option) { From 949736f135b69d543091e1c20bacec91cd4394f0 Mon Sep 17 00:00:00 2001 From: Daniel Karzel Date: Thu, 21 Nov 2024 19:13:30 +1100 Subject: [PATCH 3/4] Add replace or push functionality --- rcgen/src/lib.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/rcgen/src/lib.rs b/rcgen/src/lib.rs index 209bfeb3..d54d9910 100644 --- a/rcgen/src/lib.rs +++ b/rcgen/src/lib.rs @@ -349,6 +349,30 @@ impl DistinguishedName { self.entries.push_front((ty, s.into())); } + /// Replaces the *fist occurrence* of a type with a new value. + /// This is a convenience function to avoid duplicating values. + /// + /// If there are multiple occurrences of a type there is currently no way of changing the besides iterating over the types and values of an existing instance and creating a new instance. + /// + /// ``` + /// # use rcgen::{DistinguishedName, DnType, DnValue}; + /// let mut dn = DistinguishedName::new(); + /// dn.push(DnType::CommonName, DnValue::PrintableString("Master Cert".try_into().unwrap())); + /// assert_eq!(dn.get(&DnType::CommonName).get(0), Some(&DnValue::PrintableString("Master Cert".try_into().unwrap())).as_ref()); + /// dn.push(DnType::CommonName, DnValue::PrintableString("Other Master Cert".try_into().unwrap())); + /// assert_eq!(dn.get(&DnType::CommonName).get(0), Some(&DnValue::PrintableString("Other Master Cert".try_into().unwrap())).as_ref()); + /// ``` + pub fn replace_or_push(&mut self, ty: DnType, s: impl Into) { + for (dn_type, dn_value) in self.entries.iter_mut() { + if *dn_type == ty { + *dn_value = s.into(); + return; + } + } + + self.push(ty, s) + } + #[cfg(feature = "x509-parser")] fn from_name(name: &x509_parser::x509::X509Name) -> Result { use x509_parser::der_parser::asn1_rs::Tag; From 7192b84fc39f68b190b69c32d42dc37796eccdf8 Mon Sep 17 00:00:00 2001 From: Daniel Karzel Date: Thu, 21 Nov 2024 20:27:31 +1100 Subject: [PATCH 4/4] Fix tests and add test that proves multiple DC can be set --- rcgen/tests/generic.rs | 5 +-- rcgen/tests/openssl.rs | 71 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/rcgen/tests/generic.rs b/rcgen/tests/generic.rs index 73324a42..4e704243 100644 --- a/rcgen/tests/generic.rs +++ b/rcgen/tests/generic.rs @@ -374,11 +374,12 @@ mod test_parse_ia5string_subject { let params_from_cert = CertificateParams::from_ca_cert_der(cert_der).unwrap(); // We should find the expected distinguished name in the reconstituted params. - let expected_names = &[(&email_address_dn_type, &email_address_dn_value)]; + let expected_names = &[(email_address_dn_type, email_address_dn_value)]; let names = params_from_cert .distinguished_name - .iter() + .into_iter() .collect::>(); + assert_eq!(names, expected_names); } } diff --git a/rcgen/tests/openssl.rs b/rcgen/tests/openssl.rs index ae0b9bbf..c1042054 100644 --- a/rcgen/tests/openssl.rs +++ b/rcgen/tests/openssl.rs @@ -1,9 +1,5 @@ #![cfg(feature = "pem")] -use std::cell::RefCell; -use std::io::{Error, ErrorKind, Read, Result as ioResult, Write}; -use std::rc::Rc; - use openssl::asn1::{Asn1Integer, Asn1Time}; use openssl::bn::BigNum; use openssl::pkey::PKey; @@ -11,10 +7,14 @@ use openssl::ssl::{HandshakeError, SslAcceptor, SslConnector, SslMethod}; use openssl::stack::Stack; use openssl::x509::store::{X509Store, X509StoreBuilder}; use openssl::x509::{CrlStatus, X509Crl, X509Req, X509StoreContext, X509}; +use std::cell::RefCell; +use std::io::{Error, ErrorKind, Read, Result as ioResult, Write}; +use std::rc::Rc; +use std::str::FromStr; use rcgen::{ - BasicConstraints, Certificate, CertificateParams, DnType, DnValue, GeneralSubtree, IsCa, - KeyPair, NameConstraints, + BasicConstraints, Certificate, CertificateParams, DnType, DnValue, GeneralSubtree, Ia5String, + IsCa, KeyPair, NameConstraints, }; mod util; @@ -540,3 +540,62 @@ fn test_openssl_pkcs1_and_sec1_keys() { let pkcs8_ec_key_der = PrivateKeyDer::try_from(ec_key.private_key_to_pkcs8().unwrap()).unwrap(); KeyPair::try_from(&pkcs8_ec_key_der).unwrap(); } + +/// Command used to generate: +/// `openssl req -x509 -newkey rsa:4096 -nodes -out mycert.pem -keyout mykey.pem -days 365 -subj "/C=US/ST=California/L=San Francisco/O=Example Company/OU=IT Department/CN=www.example.com/DC=example/DC=com"` +/// Contains two distinct "DC" entries. +const CERT_WITH_MULTI_DC: &str = r#"-----BEGIN CERTIFICATE----- +MIIGSzCCBDOgAwIBAgIUECjoFzATY6rTCtu7HKjBtfXnB/owDQYJKoZIhvcNAQEL +BQAwgbQxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQH +DA1TYW4gRnJhbmNpc2NvMRgwFgYDVQQKDA9FeGFtcGxlIENvbXBhbnkxFjAUBgNV +BAsMDUlUIERlcGFydG1lbnQxGDAWBgNVBAMMD3d3dy5leGFtcGxlLmNvbTEXMBUG +CgmSJomT8ixkARkWB2V4YW1wbGUxEzARBgoJkiaJk/IsZAEZFgNjb20wHhcNMjQx +MTIxMDkxNTE2WhcNMjUxMTIxMDkxNTE2WjCBtDELMAkGA1UEBhMCVVMxEzARBgNV +BAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xGDAWBgNVBAoM +D0V4YW1wbGUgQ29tcGFueTEWMBQGA1UECwwNSVQgRGVwYXJ0bWVudDEYMBYGA1UE +AwwPd3d3LmV4YW1wbGUuY29tMRcwFQYKCZImiZPyLGQBGRYHZXhhbXBsZTETMBEG +CgmSJomT8ixkARkWA2NvbTCCAiIwDQYJKoZIhvcNAQEBBQADggIPADCCAgoCggIB +ANla4cBCTS+6JdEw6kVQHskanjtHbw7F25TZ2tZWC1f/UJuQnpF/JJqADdV6R3ta +xjcGj2ubJnKS1npcdiVN6A95jYggbQqjfZV+Z0cxjL8L4dQ+UPDsNyP8W0+S6UnK ++W813DG/QGXxEFrT8nZIfhyD4qZEtOSFGgp/ZA2f687Svx1+SKiutHeRovEf/OTb +fK4NHhewa1IxiV7shYNy7hhJmDqcsRIhVfuiWn4TU++qB6JTiPATYmzFRALli7B6 +g5m8KhvWcdAssgb2+bNpbs3fTcytrqwiNnNYtZ5a7DV0WWH4+wfor7KlomPMviPg +jiFwWWKW/N5dQ+f9vpo7SDOT9Jl26BWj0vJYTceLgkOGwYMXsg7pbWmPH4sL+GNv +WpRG7fDmual98y4DFwD8vHp4Mvax2OWKxfxe6xPqdn7or7D3ZSSyBu//ZlhQ6yMd +F6tLTl2/5VcWdJy0W+FDEnZIHnPm3zyCiplEP4bxY2Blpdnqf5Cx80mz8YSQhddn +gVNrM7iaNnIvRLjFS88w4KMOKbYSPbxEt2eWO4ggVcn1akcifDFTpyInRKQxQkXa +SXH/iu2dm7kuyGwSwrIW1l41vUkT+Lsm/9TFQ3a+UWWzut4oux9oGmcuUP5EiUZb +rWw2GIP2DaluKsZNUh8QIWVccBmX6AaKw3+K0r/tFqShAgMBAAGjUzBRMB0GA1Ud +DgQWBBTru/FFL1lBGB6d1a1xe3Tn3wV/RzAfBgNVHSMEGDAWgBTru/FFL1lBGB6d +1a1xe3Tn3wV/RzAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4ICAQCY +dKu+CYxHb7drJqDhfMXUq2ogZiYI0fYyPEf+rpNpM5A8C0PyG9Um7WZwKAfp38IE +a/McBidxI7TuNq9ojruIyY79LCThz64Z1Nc5rb3sthcygZLd1t98Zh8vaG07kk7s +n2/BMLgHPvm47cUJ1VaQpLwx2tSBaFB+Osroq0ZXMqyO6s7Gyk+hrI+l6b+gqryA +b8kHzbeslxPK6QkDz9Kt+qPkZVRgfKgyqyd0YGoe1LaAwctMdrTPZRzkFRDLYDls +JK/PFi027oljJJzFZ07k9c8WJBeM3xiIHFlxIJ5XehVpLLFEhxX1ypnvku7GeINq +I9356ueSmMPn1BIsLonTOYR3k1hue+giO5AiD6J3yl8OhJStouG3FOZbB5dDRae+ +9bdhU4npsmKTmBX/CDUFYJl4yqavEGfvw40p77gaqIOShEBB54ASKDaSyuLSeYbi +3TQsa+JyWmJ5iNmqVsAy8YfioKveNmyl023hRTjtqJgKQY1UzY6M0bnHa0IlgZq/ +l4A7hDDsvi3rDFiqvKg/WTEZd5G87E9hwIcHF/bJPc+0+MjelRoxFTSty2bpbniR +p3mmtsYxi+XCHdwUwRLhbBrdu93z5Iy3AWIb7vGeTKznnnDweJzYpfHCXuWZdr/d +z6cbmudPzN1l99Op5eH9i1JikA+DQ8BQv1OgkNBw2A== +-----END CERTIFICATE----- +"#; + +#[test] +#[cfg(feature = "x509-parser")] +fn test_parse_certificate_with_multiple_domain_components() { + let param = CertificateParams::from_ca_cert_pem(CERT_WITH_MULTI_DC).unwrap(); + + let domain_component_values = param.distinguished_name.get(&DnType::CustomDnType(vec![ + 0, 9, 2342, 19200300, 100, 1, 25, + ])); + + assert_eq!( + domain_component_values, + vec![ + &DnValue::Ia5String(Ia5String::from_str("com").unwrap()), + &DnValue::Ia5String(Ia5String::from_str("example").unwrap()), + ] + ) +}