Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple domain components in issuer #301

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Multiple domain components in issuer #301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions rcgen/src/certificate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,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
Expand Down Expand Up @@ -670,7 +670,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
Expand All @@ -680,7 +680,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
Expand Down Expand Up @@ -869,7 +869,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())
Expand Down
2 changes: 1 addition & 1 deletion rcgen/src/crl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
145 changes: 120 additions & 25 deletions rcgen/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<DnType, DnValue>,
order: Vec<DnType>,
entries: Vec<(DnType, DnValue)>,
}

impl DistinguishedName {
Expand All @@ -309,20 +307,22 @@ 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
/// Removes all attributes with the specified DnType
///
/// Returns true when an actual removal happened, false
/// 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);
}
removed
let prev_len = self.entries.len();
self.entries.retain(|(dn_type, _dn_val)| dn_type != &ty);

prev_len != self.entries.len()
}
/// Inserts or updates an attribute that consists of type and name
///
Expand All @@ -331,20 +331,41 @@ 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<DnValue>) {
if !self.entries.contains_key(&ty) {
self.order.push(ty.clone());
self.entries.push((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(1), Some(&DnValue::PrintableString("Other Master Cert".try_into().unwrap())).as_ref());
/// ```
pub fn replace_or_push(&mut self, ty: DnType, s: impl Into<DnValue>) {
for (dn_type, dn_value) in self.entries.iter_mut() {
if *dn_type == ty {
*dn_value = s.into();
return;
}
}
self.entries.insert(ty, s.into());

self.push(ty, s)
}

/// Iterate over the entries
pub fn iter(&self) -> DistinguishedNameIterator<'_> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing this pub API item will break semver. I think you should try to orient your diff to avoid that, or to make a stronger case for why it's necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I don't understand why the previous implementation would require the DistinguishedNameIterator as own iterator implementation.

I'm implementing Iterator for DistinguishedName - so iter() is actually still available even though the explicit struct function was removed.
I would argue that the interface is actually not breaking - iter() is still available after the change.

@cpu Is there a better way to achieve this so that semver accepts the change as non-breaking?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@da-kami implementing Iterator for DistinguishedName does not make any method called iter() available -- Iterator::iter() does not exist. If you want to make it possible to represent multiple entries with the same DnType, I'd suggest changing the representation to hold a Vec<(DnType, DnValue)> and then build on top of that.

Copy link
Author

@da-kami da-kami Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djc @cpu fixed, ready for review again :)

DistinguishedNameIterator {
distinguished_name: self,
iter: self.order.iter(),
iter: self.entries.iter(),
}
}

Expand Down Expand Up @@ -397,17 +418,14 @@ impl DistinguishedName {
Iterator over [`DistinguishedName`] entries
*/
pub struct DistinguishedNameIterator<'a> {
distinguished_name: &'a DistinguishedName,
iter: std::slice::Iter<'a, DnType>,
iter: std::slice::Iter<'a, (DnType, DnValue)>,
}

impl<'a> Iterator for DistinguishedNameIterator<'a> {
type Item = (&'a DnType, &'a DnValue);

fn next(&mut self) -> Option<Self::Item> {
self.iter
.next()
.and_then(|ty| self.distinguished_name.entries.get(ty).map(|v| (ty, v)))
self.iter.next().map(|(key, value)| (key, value))
}
}

Expand Down Expand Up @@ -568,7 +586,7 @@ 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() {
writer.next().write_set(|writer| {
Expand Down Expand Up @@ -596,7 +614,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()),
}
});
});
Expand Down Expand Up @@ -776,6 +794,83 @@ mod tests {
}
}

#[test]
fn distinguished_name_remove_no_match() {
let mut dn = DistinguishedName::new();
// Domain Component (DC)
let dc_type = DnType::CustomDnType(vec![0, 9, 2342, 19200300, 100, 1, 25]);

dn.push(
DnType::CommonName,
DnValue::PrintableString("Master Cert".try_into().unwrap()),
);

let removed = dn.remove(dc_type.clone());
assert!(!removed);
assert_eq!(
dn.get(&DnType::CommonName).get(0),
Some(&DnValue::PrintableString("Master Cert".try_into().unwrap())).as_ref()
);
assert_eq!(dn.entries.len(), 1);
assert_eq!(dn.get(&dc_type).get(0), None);
}

#[test]
fn distinguished_name_remove_single_attribute() {
let mut dn = DistinguishedName::new();
// Domain Component (DC)
let dc_type = DnType::CustomDnType(vec![0, 9, 2342, 19200300, 100, 1, 25]);

dn.push(
DnType::CommonName,
DnValue::PrintableString("Master Cert".try_into().unwrap()),
);
dn.push(
dc_type.clone(),
DnValue::PrintableString("example".try_into().unwrap()),
);

let removed = dn.remove(dc_type.clone());
assert!(removed);
assert_eq!(
dn.get(&DnType::CommonName).get(0),
Some(&DnValue::PrintableString("Master Cert".try_into().unwrap())).as_ref()
);
assert_eq!(dn.entries.len(), 1);
assert_eq!(dn.get(&dc_type).get(0), None);
}

#[test]
fn distinguished_name_remove_multiple_attributes_of_same_type() {
let mut dn = DistinguishedName::new();
// Domain Component (DC)
let dc_type = DnType::CustomDnType(vec![0, 9, 2342, 19200300, 100, 1, 25]);

dn.push(
DnType::CommonName,
DnValue::PrintableString("Master Cert".try_into().unwrap()),
);
dn.push(
// Domain Component (DC)
dc_type.clone(),
DnValue::PrintableString("example".try_into().unwrap()),
);
dn.push(
dc_type.clone(),
DnValue::PrintableString("com".try_into().unwrap()),
);

let removed = dn.remove(dc_type.clone());
assert!(removed);
assert_eq!(
dn.get(&DnType::CommonName).get(0),
Some(&DnValue::PrintableString("Master Cert".try_into().unwrap())).as_ref()
);
assert_eq!(dn.entries.len(), 1);
assert_eq!(dn.get(&dc_type).get(0), None);
assert_eq!(dn.get(&dc_type).get(1), None);
}

#[cfg(feature = "x509-parser")]
mod test_ip_address_from_octets {
use super::super::ip_addr_from_octets;
Expand Down
70 changes: 65 additions & 5 deletions rcgen/tests/openssl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
#![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;
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 rcgen::{
BasicConstraints, Certificate, CertificateParams, DnType, DnValue, GeneralSubtree, IsCa,
KeyPair, NameConstraints,
};
use std::cell::RefCell;
use std::io::{Error, ErrorKind, Read, Result as ioResult, Write};
use std::rc::Rc;

mod util;

Expand Down Expand Up @@ -540,3 +538,65 @@ 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();
}

#[test]
#[cfg(feature = "x509-parser")]
fn test_parse_certificate_with_multiple_domain_components() {
use rcgen::Ia5String;
use std::str::FromStr;

/// 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-----
"#;

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("example").unwrap()),
&DnValue::Ia5String(Ia5String::from_str("com").unwrap()),
]
)
}
Loading