add a method to collect DNS names from a certificate #42

cpu · 2023-04-05T19:12:58Z

This branch adds an EndEntityCert::dns_names method, which returns a list of the DNS names provided in the subject alternative names extension of the certificate.

This branch is based on work done by @seanmonstar in briansmith/webpki#91, by @Geal in briansmith/webpki#103 and @hawkw in #6. The development train keeps chugging along in this branch :-)

In #6 @hawkw updated the changeset to track the main branch of the rustls/webpki repository. Since I wasn't able to push commits to the linkerd fork to continue develop in #6 I have addressed the feedback on that branch in this separate PR. The new changes are:

Updating list_cert_dns_names to return impl Iterator instead of a Vec based on @djc's feedback.
Fixing the stale comments about Eq, PartialEq, etc flagged by @samlh in a drive-by review. Brian Smith implemented Eq, PartialEq and Hash for the DNSName type in briansmith/webpki@96de094 but the comments referring to not implementing them slipped through.
Rebasing on main.
Applying cargo fmt, fixing cargo clippy findings, import drifts.
Removing alloc requirement on Debug impls with an allocation-free strategy for lowercasing.
Removing unnecessary use of a RefCell by changing the name iterator helper to accept an impl FnMut argument instead of dyn Fn.
Removing the owned WildcardDnsName type.
Clarifying the rationale for the GeneralDnsNameRef type, and why DnsNameRef can't contain wildcards while WildcardDnsNameRef may.
Implementing AsRef<str> for GeneralDnsNameRef.

Any bugs/errors are mine :-P

Closes #2
Replaces #6

Authored-by: Geoffroy Couprie geo.couprie@gmail.com
Co-authored-by: Sean McArthur sean@seanmonstar.com
Co-authored-by: Eliza Weisman eliza@buoyant.io
Co-authored-by: Daniel McCarney daniel@binaryparadox.net
Signed-off-by: Daniel McCarney daniel@binaryparadox.net

cpu · 2023-04-05T19:14:02Z

Oops, looks like I picked a stale base branch and need to resolve conflicts. Setting this as WIP to fix that up.

cpu · 2023-04-05T19:52:36Z

need to resolve conflicts. Setting this as WIP to fix that up.

All fixed up.

djc · 2023-04-05T20:23:16Z

Would be nice to squash this all into a single commit (with a whole bunch of co-author lines).

hawkw

Looks good to me, thanks for picking up this work!

I had one small suggestion that occurred to me while skimming this code.

src/subject_name/dns_name.rs

cpu · 2023-04-06T14:00:21Z

I had one small suggestion that occurred to me while skimming this code.

Thanks for the review!

Would be nice to squash this all into a single commit (with a whole bunch of co-author lines).

Done ☑️

codecov · 2023-04-06T14:01:13Z

Codecov Report

Merging #42 (9b66ec4) into main (6dd4a44) will decrease coverage by 0.40%.
The diff coverage is 82.27%.

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
- Coverage   94.17%   93.78%   -0.40%     
==========================================
  Files          14       14              
  Lines        2505     2573      +68     
==========================================
+ Hits         2359     2413      +54     
- Misses        146      160      +14

Impacted Files	Coverage Δ
src/subject_name/dns_name.rs	`87.14% <68.88%> (-3.28%)`	⬇️
src/end_entity.rs	`54.54% <100.00%> (+2.16%)`	⬆️
src/subject_name/verify.rs	`95.81% <100.00%> (+0.49%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/subject_name/dns_name.rs

src/subject_name/verify.rs

src/subject_name/dns_name.rs

src/subject_name/mod.rs

src/end_entity.rs

src/subject_name/dns_name.rs

This commit adds an `EndEntityCert::dns_names` method, which returns a list of the DNS names provided in the subject alternative names extension of the certificate. Authored-by: Geoffroy Couprie geo.couprie@gmail.com Co-authored-by: Sean McArthur sean@seanmonstar.com Co-authored-by: Eliza Weisman eliza@buoyant.io Co-authored-by: Daniel McCarney daniel@binaryparadox.net Signed-off-by: Daniel McCarney daniel@binaryparadox.net

Geal · 2023-04-21T20:24:05Z

I'm amazed to see this merged, it took a few detours, but it's there!! Thanks!

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream.

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency.

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency.

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <eliza@buoyant.io> Signed-off-by: Adam Shaw <adam.shaw@vipps.no>

cpu self-assigned this Apr 5, 2023

cpu marked this pull request as draft April 5, 2023 19:13

cpu marked this pull request as ready for review April 5, 2023 19:51

cpu mentioned this pull request Apr 5, 2023

add a method to collect the DNS names from a certificate #6

Closed

hawkw approved these changes Apr 5, 2023

View reviewed changes

src/subject_name/dns_name.rs Outdated Show resolved Hide resolved

src/subject_name/dns_name.rs Outdated Show resolved Hide resolved

djc reviewed Apr 6, 2023

View reviewed changes

src/subject_name/dns_name.rs Outdated Show resolved Hide resolved

src/subject_name/verify.rs Outdated Show resolved Hide resolved

src/subject_name/dns_name.rs Show resolved Hide resolved

cpu requested a review from djc April 12, 2023 14:59

moparisthebest reviewed Apr 18, 2023

View reviewed changes

src/subject_name/mod.rs Show resolved Hide resolved

djc reviewed Apr 18, 2023

View reviewed changes

src/end_entity.rs Show resolved Hide resolved

src/subject_name/dns_name.rs Outdated Show resolved Hide resolved

src/subject_name/dns_name.rs Outdated Show resolved Hide resolved

src/subject_name/dns_name.rs Outdated Show resolved Hide resolved

djc approved these changes Apr 18, 2023

View reviewed changes

This was referenced Apr 18, 2023

Breaking change: replace DnsNameAsRef<[u8]> w/ AsRef<str> #45

Closed

Refactor verify.rs list_cert_dns_names to avoid allocations. #46

Closed

djc merged commit bdb7874 into rustls:main Apr 20, 2023

cpu deleted the cpu-adopts-6-cert-dns-names branch April 20, 2023 21:13

moparisthebest mentioned this pull request Apr 22, 2023

Expose GeneralDnsNameRef #50

Closed

hawkw mentioned this pull request Aug 31, 2023

identity: Proxy fails to validate certificates with name constraints linkerd/linkerd2#9299

Closed

hawkw mentioned this pull request Sep 5, 2023

use rustls-webpki instead of linkerd/webpki linkerd/linkerd2-proxy#2465

Merged

hawkw mentioned this pull request Sep 11, 2023

proxy: v2.209.0 linkerd/linkerd2#11361

Merged

djc mentioned this pull request Sep 20, 2023

rework dns_names helper, remove alloc req. #178

Merged

cpu mentioned this pull request Sep 20, 2023

Restore GeneralDnsNameRef, use in GeneralName::DnsName. #183

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a method to collect DNS names from a certificate #42

add a method to collect DNS names from a certificate #42

cpu commented Apr 5, 2023 •

edited

Loading

cpu commented Apr 5, 2023

cpu commented Apr 5, 2023

djc commented Apr 5, 2023

hawkw left a comment

cpu commented Apr 6, 2023

codecov bot commented Apr 6, 2023 •

edited

Loading

Geal commented Apr 21, 2023

add a method to collect DNS names from a certificate #42

add a method to collect DNS names from a certificate #42

Conversation

cpu commented Apr 5, 2023 • edited Loading

cpu commented Apr 5, 2023

cpu commented Apr 5, 2023

djc commented Apr 5, 2023

hawkw left a comment

Choose a reason for hiding this comment

cpu commented Apr 6, 2023

codecov bot commented Apr 6, 2023 • edited Loading

Codecov Report

Geal commented Apr 21, 2023

cpu commented Apr 5, 2023 •

edited

Loading

codecov bot commented Apr 6, 2023 •

edited

Loading