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

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

Closed
wants to merge 5 commits into from

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Sep 23, 2022

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 and @Geal in briansmith/webpki#103. I've simply
updated this change to track the main branch of the rustls/webpki
repository.

Closes #2

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

@codecov-commenter
Copy link

Codecov Report

Merging #6 (01e2796) into main (dcfe0b4) will increase coverage by 2.25%.
The diff coverage is 85.39%.

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   74.60%   76.86%   +2.25%     
==========================================
  Files          19       19              
  Lines        1788     1962     +174     
==========================================
+ Hits         1334     1508     +174     
  Misses        454      454              
Impacted Files Coverage Δ
src/name/dns_name.rs 65.72% <43.47%> (-3.77%) ⬇️
src/end_entity.rs 45.45% <100.00%> (+7.35%) ⬆️
src/name/verify.rs 27.22% <100.00%> (+23.08%) ⬆️
tests/integration.rs 100.00% <100.00%> (ø)

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

/// `WildcardDnsName` stores a copy of the input it was constructed from in a `String`
/// and so it is only available when the `alloc` default feature is enabled.
///
/// `Eq`, `PartialEq`, etc. are not implemented because name comparison
Copy link

Choose a reason for hiding this comment

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

Drive-by: This comment doesn't match the derive below?

#[cfg(feature = "alloc")]
pub fn list_cert_dns_names<'names>(
cert: &'names crate::EndEntityCert<'names>,
) -> Result<Vec<GeneralDnsNameRef<'names>>, Error> {
Copy link
Member

Choose a reason for hiding this comment

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

How hard would it be to rewire this to return an impl Iterator instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it certainly could be rewritten to return impl Iterator --- I wanted to start by coping the original diff from upstream exactly, but would be happy to revise.

@cpu
Copy link
Member

cpu commented Mar 3, 2023

@hawkw 👋 gentle poke for this branch. Do you think you'll have a chance to address the earlier feedback? It looks like a useful change 👍

@hawkw
Copy link
Contributor Author

hawkw commented Mar 3, 2023

@hawkw wave gentle poke for this branch. Do you think you'll have a chance to address the earlier feedback? It looks like a useful change +1

sure, i'm happy to pick this back up!

@cpu
Copy link
Member

cpu commented Apr 5, 2023

I'd like to see this work land so I've adopted the branch to try and address the last bits of feedback. If any of the original authors want to pick this back up again I'm happy to step back to a reviewer role. In the meantime I made a second PR because I couldn't push commits to the linkerd fork for this branch. PTAL at #42

I'm going to close this PR to consolidate discussion on the new one but as mentioned, also happy to come back to this branch if Eliza or someone else has cycles to iterate. Thanks for getting it so close to being done!

@cpu cpu closed this Apr 5, 2023
@cpu
Copy link
Member

cpu commented Apr 21, 2023

Just wanted to mention in case folks are watching this PR - we landed the feature in #42.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a means to collect SANs from a certificate
6 participants