rework `dns_names` helper, remove alloc req. #178

cpu · 2023-09-14T14:24:22Z

Description

This branch reworks the EndEntityCert's dns_names helper. Principally it:

Simplifies the unit testing, and cleans up some misc nits.
Reworks dns_names and the corresponding subject_name::list_cert_dns_names fns to be infallible, this better matches how we're ignoring invalid DNS names when validating a subject name since ignore invalid value validating dns name list #69.
Removes the alloc requirement for the dns_names fn.
Reworks the name ref types to use as_str instead of a From impl
Removes the GeneralDnsNameRef type (with the potential to restore as future work c.f. Restore GeneralDnsNameRef, use in GeneralName::DnsName. #183)
Moves the subject_name::list_cert_dns_names free-standing fn to be associated with Cert and named valid_dns_names.

Resolves #46, replaces #50

cpu · 2023-09-14T14:25:23Z

@djc Interested in your thoughts on this branch. Mostly wanted to clear the path for #159 but then got a little bit carried away.

codecov · 2023-09-14T14:28:43Z

Codecov Report

Merging #178 (64fb1b7) into main (8d6a733) will increase coverage by 0.21%.
Report is 23 commits behind head on main.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   96.31%   96.53%   +0.21%     
==========================================
  Files          17       19       +2     
  Lines        4510     4496      -14     
==========================================
- Hits         4344     4340       -4     
+ Misses        166      156      -10

Files Changed	Coverage Δ
src/signed_data.rs	`100.00% <ø> (ø)`
src/subject_name/dns_name.rs	`88.66% <85.71%> (+1.79%)`	⬆️
src/cert.rs	`97.58% <100.00%> (+0.10%)`	⬆️
src/end_entity.rs	`100.00% <100.00%> (ø)`
src/subject_name/verify.rs	`91.89% <100.00%> (-0.21%)`	⬇️

... and 8 files with indirect coverage changes

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

djc

In general I feel that the GeneralDnsNameRef and WildcardDnsNameRef names are not that obvious and fairly verbose, so that is one reason that keeping them private made sense to me. Especially the notion of "General", I suppose? What does that mean?

src/subject_name/mod.rs

src/subject_name/dns_name.rs

src/subject_name/verify.rs

We switched to using `doc_auto_cfg` to automatically indicate in Rustdocs when an item requires a particular feature. This comment about the `dns_name::DnsName` re-export requiring alloc isn't necessary anymore.

We switched to `doc_auto_cfg` and don't need to manually annotate `cfg(feature ...)` annotations for docsrs purposes anymore.

This is what most consumers of the API are interested in, and avoids needing to export the `GeneralDnsNameRef` and `WildcardDnsNameRef` types.

We can express this test with the `expect_cert_dns_names` helper.

Prior to this commit the rustdoc comment on `EndEntityCert.dns_names` mentioned using `verify_is_valid_for_dns_name` and `verify_is_valid_for_at_least_one_dns_name`, but these functions don't exist anymore. This commit updates the comment to point to `EndEntityCert::verify_is_valid_for_subject_name`, and does so with a proper Rustdoc link so that future updates will be caught by `cargo doc` if we forget to fix this reference to match.

cpu · 2023-09-18T15:49:33Z

ci / Measure coverage (pull_request) Failing after 28s

This is time-rs/time#618

ci / Build+test (--all-features, nightly, ubuntu-20.04) (pull_request) Failing after 45s

I think this is a nightly bug? It seems like it doesn't like the . in the ASN.1 language specifier on the backtick code fence:

webpki/src/signed_data.rs

Lines 89 to 94 in adc4944

    
           /// ```ASN.1 
        
           /// Certificate (SEQUENCE) { 
        
           ///     tbsCertificate TBSCertificate, 
        
           ///     signatureAlgorithm AlgorithmIdentifier, 
        
           ///     signatureValue BIT STRING 
        
           /// }

Switching to asn1 fixes the compile error, but that's not the name Linguist/GitHub use for ASN.1 syntax highlighting.

ctz · 2023-09-18T16:06:09Z

I think this is a nightly bug?

I think we're just missing an end to the code block?

(the upstream issue that has changed the parser is rust-lang/rust#110800 in case we do need to report the regression though.)

cpu · 2023-09-18T16:16:19Z

I think we're just missing an end to the code block?

Ah, good catch. I fixed that in this branch, but it's still generating a warning.

I was also able to reproduce with a minimal example for an upstream issue:

/// This is a test:
/// ```ASN.1
/// foo
/// ```
fn main() {
    println!("Hello, world!");
}

(the upstream issue that has changed the parser is rust-lang/rust#110800 in case we do need to report the regression though.)

Thanks! I'll mention that in the issue.

djc · 2023-09-20T09:23:59Z

Feedback in cpu#1. I think these changes together mean that we can revert/simplify a bunch of the additions from #42, which would be nice?

cpu · 2023-09-20T13:24:20Z

I think these changes together mean that we can revert/simplify a bunch of the additions from #42, which would be nice?

@djc Thanks, the majority of these changes seem good to me, but I think we might want to consider leaving the GeneralDnsNameRef type.

I thought about removing it when I reworked this branch to iterate w/ &str but after revisiting the context from #42 while working on this branch I remembered that the purpose of this type was to enable something like briansmith/webpki#66 It seemed to me like that's still worthwhile to pursue, but since it's a little bit more involved (and crate-internal) I didn't want to do it for the 0.102 release (or at least, not until I've finished more of the Rustls release blocker work).

WDYT?

djc · 2023-09-20T13:40:42Z

That's fair. So what do you want to do with cpu#1? Merge it on top of this without making further changes? (I didn't finish reviewing all commits here after I got myself distracted from removing the use of .into().)

cpu · 2023-09-20T13:44:38Z

That's fair. So what do you want to do with cpu#1? Merge it on top of this without making further changes?

That sounds good to me. I can merge it in and then back out 27a1234.

djc · 2023-09-20T13:52:47Z

Hmm, yeah, I think that'll leave us with a bunch of warnings -- I'm not sure putting allow(dead_code) on a bunch of stuff here is a great alternative.

Maybe better to remove this code and create a follow-up issue to bring back the ideas from briansmith/webpki#66? Probably something I could pick up soon.

cpu · 2023-09-20T13:55:30Z

I'm not sure putting allow(dead_code) on a bunch of stuff here is a great alternative.

Good point.

Maybe better to remove this code and create a follow-up issue to bring back the ideas from briansmith/webpki#66? Probably something I could pick up soon.

That sounds good to me. 👍

cpu · 2023-09-20T14:13:55Z

Maybe better to remove this code and create a follow-up issue

#183

cpu · 2023-09-20T14:41:56Z

@ctz Could you give this branch an independent review since both myself and Djc have contributed code to it? 🙇

djc

As a further change, how do we feel about attaching list_cert_dns_names() to EndEntityCert, or, even better, Cert, as a method? Feels right to me. Maybe change the name to valid_dns_names()?

src/end_entity.rs

The purpose of the `dns_names` helper on an `EndEntityCert` is to provide users the opportunity to get information on the dNSName SAN values in a certificate for **non-validation** purposes. Checking that a certificate is valid for a particular name should always be done with `verify_is_valid_for_at_least_one_dns_name`. With that use-case in mind, we can make the `dns_names` helper easier for consumers to use by filtering out invalid general names, returning an `Iterator<Item = &'a str>` unconditionally, instead of a `Result`. This better matches the updated name validation semantics where we ignore `MalformedDnsIdentifier` errors to continue to try to find a valid name to validate against.

cpu · 2023-09-20T15:19:12Z

As a further change, how do we feel about attaching list_cert_dns_names() to EndEntityCert, or, even better, Cert, as a method? Feels right to me. Maybe change the name to valid_dns_names()?

Good idea 👍 I put it on Cert as valid_dns_names.

tests/integration.rs

src/end_entity.rs

With the update to the `dns_names` function in the previous commit we can now make `EndEntity.dns_names` work without requiring `alloc`.

Avoid combinator chaining, use explicit `match`.

The From impl feels a little unidiomatic because the DnsNameRef is not consumed. An AsRef impl would unnecessarily constrain the lifetime of the output value to `&self`, whereas it can live as long as `'a`.

The From impl feels a little unidiomatic because the WildcardDnsNameRef is not consumed. An AsRef impl would unnecessarily constrain the lifetime of the output value to `&self`, whereas it can live as long as `'a`.

The From impl feels a little unidiomatic because the GeneralDnsNameRef is not consumed. An AsRef impl would unnecessarily constrain the lifetime of the output value to `&self`, whereas it can live as long as `'a`.

This commit lifts the free-standing `list_cert_dns_names` helper from the `subject_name` module to be associated with a `Cert`. Doing so also requires making the `subject_name::NameIterator` and `subject_name::WildcardDnsNameRef` `pub(crate)` visible.

djc reviewed Sep 14, 2023

View reviewed changes

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

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

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

hawkw reviewed Sep 14, 2023

View reviewed changes

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

cpu marked this pull request as draft September 14, 2023 22:18

cpu added 10 commits September 18, 2023 10:11

subject_name: remove comment about required feature

4e2ef1b

We switched to using `doc_auto_cfg` to automatically indicate in Rustdocs when an item requires a particular feature. This comment about the `dns_name::DnsName` re-export requiring alloc isn't necessary anymore.

lib: remove unneeded manual docsrs cfg_attr

839cd93

We switched to `doc_auto_cfg` and don't need to manually annotate `cfg(feature ...)` annotations for docsrs purposes anymore.

tests: remove unnecessary pub visibility

0682cec

tests: rename data -> cert_der in expect_cert_dns_names

12cd0ea

end_entity: rework dns_names to return iter of &str

f5ace6d

This is what most consumers of the API are interested in, and avoids needing to export the `GeneralDnsNameRef` and `WildcardDnsNameRef` types.

tests: simplify expect_cert_dns_names

de8b2ec

tests: move no_subject_alt_names test above helper

8d60950

tests: rework no_subject_alt_names test

2350c2e

We can express this test with the `expect_cert_dns_names` helper.

tests: clean up unneeded bindings

22cebce

cpu force-pushed the cpu-rework-dns-names-iter branch from bf20589 to 9754177 Compare September 18, 2023 15:31

cpu marked this pull request as ready for review September 18, 2023 15:44

cpu changed the title ~~rework dns_names helper, expose more types, remove alloc req.~~ rework dns_names helper, remove alloc req. Sep 18, 2023

cpu mentioned this pull request Sep 18, 2023

Optionally support aws-lc-rs #158

Merged

djc mentioned this pull request Sep 20, 2023

Clean up name api #182

Closed

cpu mentioned this pull request Sep 20, 2023

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

Open

djc reviewed Sep 20, 2023

View reviewed changes

src/end_entity.rs Show resolved Hide resolved

djc approved these changes Sep 20, 2023

View reviewed changes

cpu force-pushed the cpu-rework-dns-names-iter branch 2 times, most recently from 5379817 to 49067e2 Compare September 20, 2023 15:18

ctz approved these changes Sep 20, 2023

View reviewed changes

tests/integration.rs Outdated Show resolved Hide resolved

src/end_entity.rs Show resolved Hide resolved

cpu and others added 11 commits September 20, 2023 11:25

lib: rm alloc req. for dns_names

807df74

With the update to the `dns_names` function in the previous commit we can now make `EndEntity.dns_names` work without requiring `alloc`.

subject_name: tidy up list_cert_dns_names

be4dba6

Avoid combinator chaining, use explicit `match`.

signed_data: fix code block missing close marker

4332b8c

Move InvalidDnsNameError definition down below usage

415c387

Move GeneralDnsNameRef definition to the top

7aa0f8d

Refactor faux-bool enum definition for AllowWildcards

d0522c0

Use as_str() method to reference DnsNameRef contents

b11327b

The From impl feels a little unidiomatic because the DnsNameRef is not consumed. An AsRef impl would unnecessarily constrain the lifetime of the output value to `&self`, whereas it can live as long as `'a`.

Use as_str() method to reference WildcardDnsNameRef contents

abf970c

The From impl feels a little unidiomatic because the WildcardDnsNameRef is not consumed. An AsRef impl would unnecessarily constrain the lifetime of the output value to `&self`, whereas it can live as long as `'a`.

Use as_str() method to reference GeneralDnsNameRef contents

f470c07

The From impl feels a little unidiomatic because the GeneralDnsNameRef is not consumed. An AsRef impl would unnecessarily constrain the lifetime of the output value to `&self`, whereas it can live as long as `'a`.

Remove API that is now unused

1e92abf

cpu force-pushed the cpu-rework-dns-names-iter branch from 49067e2 to 64fb1b7 Compare September 20, 2023 15:25

cpu added this pull request to the merge queue Sep 20, 2023

Merged via the queue into rustls:main with commit dc9c85c Sep 20, 2023
28 of 29 checks passed

cpu deleted the cpu-rework-dns-names-iter branch September 20, 2023 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework `dns_names` helper, remove alloc req. #178

rework `dns_names` helper, remove alloc req. #178

cpu commented Sep 14, 2023 •

edited

Loading

cpu commented Sep 14, 2023

codecov bot commented Sep 14, 2023 •

edited

Loading

djc left a comment

cpu commented Sep 18, 2023

ctz commented Sep 18, 2023 •

edited

Loading

cpu commented Sep 18, 2023

djc commented Sep 20, 2023

cpu commented Sep 20, 2023

djc commented Sep 20, 2023 •

edited

Loading

cpu commented Sep 20, 2023

djc commented Sep 20, 2023 •

edited

Loading

cpu commented Sep 20, 2023

cpu commented Sep 20, 2023

cpu commented Sep 20, 2023

djc left a comment

cpu commented Sep 20, 2023

rework dns_names helper, remove alloc req. #178

rework dns_names helper, remove alloc req. #178

Conversation

cpu commented Sep 14, 2023 • edited Loading

Description

cpu commented Sep 14, 2023

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

djc left a comment

Choose a reason for hiding this comment

cpu commented Sep 18, 2023

ctz commented Sep 18, 2023 • edited Loading

cpu commented Sep 18, 2023

djc commented Sep 20, 2023

cpu commented Sep 20, 2023

djc commented Sep 20, 2023 • edited Loading

cpu commented Sep 20, 2023

djc commented Sep 20, 2023 • edited Loading

cpu commented Sep 20, 2023

cpu commented Sep 20, 2023

cpu commented Sep 20, 2023

djc left a comment

Choose a reason for hiding this comment

cpu commented Sep 20, 2023

rework `dns_names` helper, remove alloc req. #178

rework `dns_names` helper, remove alloc req. #178

cpu commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

ctz commented Sep 18, 2023 •

edited

Loading

djc commented Sep 20, 2023 •

edited

Loading

djc commented Sep 20, 2023 •

edited

Loading