Remove subject common name parsing by hawkw · Pull Request #169 · rustls/webpki

hawkw · 2023-09-07T17:06:10Z

Currently, rustls-webpki fails to parse common names names from certificates where the subject common name is not a UTF8String, or a SEQUENCE containing an empty SET. This is incorrect, as some spec-compliant encoders may emit PrintableString rather than UTF8String, and may represent an empty subject common name as an empty SEQUENCE, rather than a SEQUENCE of empty SET. This results in issues while iterating over the subject alternate names of such a certificate, as described in #167.

Rather than changing the common name parsing to support such cases, the simplest solution to these issues is to remove common name parsing entirely. As explained by @cpu in #167 (comment):

For the purpose of certificate validation we only rely on SANs. The common name handling code is only used for verifying name constraints, and for the name iteration code that you found the bug with. For the former, if we assume the webpki profile the subj. CN must be present as a SAN, so validating the SANs vs the constraints seems sufficient (?). For the latter, it seems potentially confusing to return a name from the CN that doesn't get used for validation (though again, the webpki profile would require that it matches a SAN value).

Therefore, this branch does the following:

Introduce tests for iterating over the DNS names in certificates where the CN is a PrintableString (a53c3ee) and where the CN is an empty SEQUENCE (c075316),
Remove the existing tests for name constraints involving CNs which are not also present in the SANs, since the webpki profile requires the CN to also be a SAN (7d31075)
Remove the common name parsing from rustls-webpki entirely (ac1740c and 429a316)

Fixes #167

codecov · 2023-09-07T17:19:48Z

Codecov Report

Merging #169 (a863348) into main (932ad95) will increase coverage by 0.00%.
The diff coverage is 97.60%.

@@           Coverage Diff           @@
##             main     #169   +/-   ##
=======================================
  Coverage   96.70%   96.71%           
=======================================
  Files          15       16    +1     
  Lines        4549     4532   -17     
=======================================
- Hits         4399     4383   -16     
+ Misses        150      149    -1

Files Changed	Coverage Δ
src/der.rs	`99.57% <ø> (ø)`
src/subject_name/verify.rs	`96.41% <93.75%> (+0.07%)`	⬆️
src/end_entity.rs	`100.00% <100.00%> (ø)`
src/test_utils.rs	`100.00% <100.00%> (ø)`
src/verify_cert.rs	`97.86% <100.00%> (-0.28%)`	⬇️

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

📢 Have feedback on the report? Share it here.

src/subject_name/verify.rs

cpu

Here's some initial feedback. Thanks again for running with this 🎉 It will be really nice to put another nail in the subject common name coffin.

tests/integration.rs

src/der.rs

src/subject_name/verify.rs

@cpu

As suggested by @cpu in this comment thread: rustls#169 (comment)

djc

Some thoughts on how to make the commit history a bit cleaner...

src/verify_cert.rs

tests/integration.rs

src/subject_name/verify.rs

tests/misc/empty_sequence_common_name.der.txt

src/der.rs

djc

Yup, this is great! I'd probably have kept the extraction/move of test utils in a separate commit, but that doesn't matter as much.

BTW, the content of the changes looks great, some nice simplification here.

This commit adds a pair of tests reproducing issue rustls#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included.

@ctz

As suggested by @ctz in this comment: rustls#167 (comment).

This commit removes parsing of the subject common name field from `NameIterator`, since `rustls-webpki` does not actually verify subject common names except when enforcing name constraints. This fixes issues with common names in formats that `rustls-webpki` doesn't currently support, by removing this code entirely. Fixes rustls-webpki/webpki#167

hawkw · 2023-09-07T21:17:47Z

I'd probably have kept the extraction/move of test utils in a separate commit, but that doesn't matter as much.

Ah, yeah, probably should have pulled that out, my bad! Oh well --- not sure if it can be easily changed now.

Also, I had to force-push again after noticing a clippy lint on an earlier commit, so if you don't mind, it looks like CI will need to be re-approved.

cpu

Nice work!

cpu · 2023-09-07T21:47:41Z

I'm going to leave this unmerged overnight to give ctz a chance to do a review in case he's interested. If it isn't merged or reviewed by the time I log back on east-coast morning time tomorrow. I'll go ahead and merge.

hawkw · 2023-09-07T22:33:22Z

@cpu Sounds good to me! We would love to be able to get a release with this fixed, of course. But, since it looks you all are in the middle of getting v0.102.0 ready , we can definitely take a Git dependency in the meantime.

Thanks so much for all your guidance on figuring out the issue and how to fix it, I really appreciate it!

ctz

Looks great, thanks!

djc · 2023-09-08T07:28:49Z

@hawkw if you want to do a backport to 0.101.x we'd be happy to review and merge that, but other than that, taking on a rustls alpha release might make sense? Will require some other changes though (but would be great to have early feedback on those anyway). Backport probably won't be completely trivial, this code changed a bunch since 0.101.

cpu · 2023-09-08T12:28:02Z

I'm working on a 0.101.x release branch, I can take a look at folding this in.

cpu · 2023-09-08T15:46:23Z

I'm working on a 0.101.x release branch, I can take a look at folding this in.

It wasn't too bad to backport 👍 #170

hawkw · 2023-09-08T15:59:32Z

I'm working on a 0.101.x release branch, I can take a look at folding this in.

It wasn't too bad to backport 👍 #170

Thank you, that's wonderful! I would've been fine with a Git dep on the alpha, so thank you for going out of your way to do that!

hawkw force-pushed the eliza/rm-common-name-validation branch from 429a316 to 8aa5b09 Compare September 7, 2023 17:11

hawkw mentioned this pull request Sep 7, 2023

rustls-webpki returns an error when parsing DNS names from a certificate generated using cfssl #167

Closed

hawkw commented Sep 7, 2023

View reviewed changes

src/subject_name/verify.rs Show resolved Hide resolved

cpu reviewed Sep 7, 2023

View reviewed changes

tests/integration.rs Outdated Show resolved Hide resolved

tests/integration.rs Outdated Show resolved Hide resolved

src/der.rs Outdated Show resolved Hide resolved

src/subject_name/verify.rs Show resolved Hide resolved

hawkw added a commit to hawkw/rustls-webpki that referenced this pull request Sep 7, 2023

Move tests to end_entity.rs, factor out test utils

5194bd8

As suggested by @cpu in this comment thread: rustls#169 (comment)

hawkw force-pushed the eliza/rm-common-name-validation branch from dd6e00e to 5194bd8 Compare September 7, 2023 18:07

djc reviewed Sep 7, 2023

View reviewed changes

hawkw force-pushed the eliza/rm-common-name-validation branch from 5194bd8 to 9cf25e7 Compare September 7, 2023 21:07

hawkw requested review from cpu and djc September 7, 2023 21:08

djc approved these changes Sep 7, 2023

View reviewed changes

hawkw added 3 commits September 7, 2023 14:15

Remove tests for common name handling

53c2039

As suggested by @ctz in this comment: rustls#167 (comment).

hawkw force-pushed the eliza/rm-common-name-validation branch from 9cf25e7 to a863348 Compare September 7, 2023 21:16

cpu approved these changes Sep 7, 2023

View reviewed changes

ctz approved these changes Sep 8, 2023

View reviewed changes

ctz added this pull request to the merge queue Sep 8, 2023

Merged via the queue into rustls:main with commit dbd7a01 Sep 8, 2023

cpu mentioned this pull request Sep 8, 2023

v0.101.5 preparation #170

Merged

hawkw mentioned this pull request Sep 8, 2023

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

Merged

Comments

Conversation

hawkw commented Sep 7, 2023

Uh oh!

codecov bot commented Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw commented Sep 7, 2023

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

cpu commented Sep 7, 2023

Uh oh!

hawkw commented Sep 7, 2023

Uh oh!

ctz left a comment

Choose a reason for hiding this comment

Uh oh!

djc commented Sep 8, 2023

Uh oh!

cpu commented Sep 8, 2023

Uh oh!

cpu commented Sep 8, 2023

Uh oh!

hawkw commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 7, 2023 •

edited

Loading