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.

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

Simplify NameIterator #311

Merged
merged 4 commits into from
Jan 20, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Only check DirectoryName as part of name constraints
  • Loading branch information
djc committed Jan 20, 2025
commit b73e2c45dff44d12f14f3a214a08af32b6f56ffc
2 changes: 1 addition & 1 deletion src/cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl<'a> Cert<'a> {
///
/// [EndEntityCert::verify_is_valid_for_subject_name]: crate::EndEntityCert::verify_is_valid_for_subject_name
pub fn valid_dns_names(&self) -> impl Iterator<Item = &str> {
NameIterator::new(Some(self.subject), self.subject_alt_name).filter_map(|result| {
NameIterator::new(self.subject_alt_name).filter_map(|result| {
let presented_id = match result.ok()? {
GeneralName::DnsName(presented) => presented,
_ => return None,
Expand Down
4 changes: 2 additions & 2 deletions src/subject_name/dns_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::error::{Error, InvalidNameContext};

pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Result<(), Error> {
let dns_name = untrusted::Input::from(reference.as_ref().as_bytes());
let result = NameIterator::new(Some(cert.subject), cert.subject_alt_name).find_map(|result| {
let result = NameIterator::new(cert.subject_alt_name).find_map(|result| {
let name = match result {
Ok(name) => name,
Err(err) => return Some(Err(err)),
Expand Down Expand Up @@ -58,7 +58,7 @@ pub(crate) fn verify_dns_names(reference: &DnsName<'_>, cert: &Cert<'_>) -> Resu
{
Err(Error::CertNotValidForName(InvalidNameContext {
expected: ServerName::DnsName(reference.to_owned()),
presented: NameIterator::new(Some(cert.subject), cert.subject_alt_name)
presented: NameIterator::new(cert.subject_alt_name)
.filter_map(|result| Some(format!("{:?}", result.ok()?)))
.collect(),
}))
Expand Down
4 changes: 2 additions & 2 deletions src/subject_name/ip_address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub(crate) fn verify_ip_address_names(reference: &IpAddr, cert: &Cert<'_>) -> Re
IpAddr::V6(ip) => untrusted::Input::from(ip.as_ref()),
};

let result = NameIterator::new(None, cert.subject_alt_name).find_map(|result| {
let result = NameIterator::new(cert.subject_alt_name).find_map(|result| {
let name = match result {
Ok(name) => name,
Err(err) => return Some(Err(err)),
Expand Down Expand Up @@ -58,7 +58,7 @@ pub(crate) fn verify_ip_address_names(reference: &IpAddr, cert: &Cert<'_>) -> Re
{
Err(Error::CertNotValidForName(InvalidNameContext {
expected: ServerName::from(*reference),
presented: NameIterator::new(None, cert.subject_alt_name)
presented: NameIterator::new(cert.subject_alt_name)
.filter_map(|result| Some(format!("{:?}", result.ok()?)))
.collect(),
}))
Expand Down
52 changes: 25 additions & 27 deletions src/subject_name/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,20 +53,30 @@ pub(crate) fn check_name_constraints(
let excluded_subtrees = parse_subtrees(constraints, der::Tag::ContextSpecificConstructed1)?;

for path in path.iter() {
let result = NameIterator::new(Some(path.cert.subject), path.cert.subject_alt_name)
.find_map(|result| {
let name = match result {
Ok(name) => name,
Err(err) => return Some(Err(err)),
};
let result = NameIterator::new(path.cert.subject_alt_name).find_map(|result| {
let name = match result {
Ok(name) => name,
Err(err) => return Some(Err(err)),
};

check_presented_id_conforms_to_constraints(
name,
permitted_subtrees,
excluded_subtrees,
budget,
)
});

if let Some(Err(err)) = result {
return Err(err);
}

check_presented_id_conforms_to_constraints(
name,
permitted_subtrees,
excluded_subtrees,
budget,
)
});
let result = check_presented_id_conforms_to_constraints(
GeneralName::DirectoryName,
permitted_subtrees,
excluded_subtrees,
budget,
);

if let Some(Err(err)) = result {
return Err(err);
Expand Down Expand Up @@ -203,19 +213,12 @@ enum Subtrees {

pub(crate) struct NameIterator<'a> {
subject_alt_name: Option<untrusted::Reader<'a>>,
subject_directory_name: Option<untrusted::Input<'a>>,
}

impl<'a> NameIterator<'a> {
pub(crate) fn new(
subject: Option<untrusted::Input<'a>>,
subject_alt_name: Option<untrusted::Input<'a>>,
) -> Self {
NameIterator {
pub(crate) fn new(subject_alt_name: Option<untrusted::Input<'a>>) -> Self {
Self {
subject_alt_name: subject_alt_name.map(untrusted::Reader::new),

// If `subject` is present, we always consider it as a `DirectoryName`.
subject_directory_name: subject,
}
}
}
Expand All @@ -240,17 +243,12 @@ impl<'a> Iterator for NameIterator<'a> {

// Make sure we don't yield any items after this error.
self.subject_alt_name = None;
self.subject_directory_name = None;
return Some(Err(err));
} else {
self.subject_alt_name = None;
}
}

if self.subject_directory_name.take().is_some() {
return Some(Ok(GeneralName::DirectoryName));
}

None
}
}
Expand Down
4 changes: 0 additions & 4 deletions tests/generate.py
Original file line number Diff line number Diff line change
Expand Up @@ -296,10 +296,6 @@ def generate_tls_server_cert_test(
presented_names_str += f'"IpAddress({san.value})"'

ip_addr_sans = all(isinstance(san, x509.IPAddress) for san in (sans or []))
if expected_error is None and not (ip_addr_sans and subject_common_name is None):
if presented_names_str:
presented_names_str += ", "
presented_names_str += '"DirectoryName"'

print(
"""
Expand Down
18 changes: 8 additions & 10 deletions tests/tls_server_certs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ fn no_name_constraints() {
ca,
&["dns.example.com"],
&["subject.example.com"],
&["DnsName(\"dns.example.com\")", "DirectoryName"]
&["DnsName(\"dns.example.com\")"]
),
Ok(())
);
Expand All @@ -91,8 +91,7 @@ fn additional_dns_labels() {
&["subject.example.com"],
&[
"DnsName(\"host1.example.com\")",
"DnsName(\"host2.example.com\")",
"DirectoryName"
"DnsName(\"host2.example.com\")"
]
),
Ok(())
Expand All @@ -114,7 +113,7 @@ fn allow_subject_common_name() {
let ee = include_bytes!("tls_server_certs/allow_subject_common_name.ee.der");
let ca = include_bytes!("tls_server_certs/allow_subject_common_name.ca.der");
assert_eq!(
check_cert(ee, ca, &[], &["allowed.example.com"], &["DirectoryName"]),
check_cert(ee, ca, &[], &["allowed.example.com"], &[]),
Ok(())
);
}
Expand All @@ -129,7 +128,7 @@ fn allow_dns_san() {
ca,
&["allowed.example.com"],
&[],
&["DnsName(\"allowed.example.com\")", "DirectoryName"]
&["DnsName(\"allowed.example.com\")"]
),
Ok(())
);
Expand All @@ -145,7 +144,7 @@ fn allow_dns_san_and_subject_common_name() {
ca,
&["allowed-san.example.com"],
&["allowed-cn.example.com"],
&["DnsName(\"allowed-san.example.com\")", "DirectoryName"]
&["DnsName(\"allowed-san.example.com\")"]
),
Ok(())
);
Expand Down Expand Up @@ -207,7 +206,7 @@ fn we_ignore_constraints_on_names_that_do_not_appear_in_cert() {
ca,
&["notexample.com"],
&["example.com"],
&["DnsName(\"notexample.com\")", "DirectoryName"]
&["DnsName(\"notexample.com\")"]
),
Ok(())
);
Expand All @@ -223,7 +222,7 @@ fn wildcard_san_accepted_if_in_subtree() {
ca,
&["bob.example.com", "jane.example.com"],
&["example.com", "uh.oh.example.com"],
&["DnsName(\"*.example.com\")", "DirectoryName"]
&["DnsName(\"*.example.com\")"]
),
Ok(())
);
Expand Down Expand Up @@ -399,8 +398,7 @@ fn invalid_dns_name_matching() {
&[],
&[
"DnsName(\"{invalid}.example.com\")",
"DnsName(\"dns.example.com\")",
"DirectoryName"
"DnsName(\"dns.example.com\")"
]
),
Ok(())
Expand Down