Expose built path in public API #174

djc · 2023-09-10T09:44:31Z

Builds on top of #173. This shows that it is possible to expose the built path without doing substantial extra work (130 extra lines, but with fairly okay abstractions IMO).

Instead of storing the intermediate certificates in stack frames, this pre-allocates an array of fixed size MAX_CA_SUB_COUNT (on the stack) and stores the parsed certificates in there. The PathNode from #173 is then redefined to iterate over the PartialPath type containing this array, and path building functions are changed to yield the TrustAnchor.

codecov · 2023-09-10T09:46:16Z

Codecov Report

Merging #174 (5dfe82e) into main (a3a4b94) will increase coverage by 0.08%.
The diff coverage is 98.85%.

@@            Coverage Diff             @@
##             main     #174      +/-   ##
==========================================
+ Coverage   96.44%   96.53%   +0.08%     
==========================================
  Files          20       20              
  Lines        4478     4617     +139     
==========================================
+ Hits         4319     4457     +138     
- Misses        159      160       +1

Files	Coverage Δ
src/cert.rs	`97.60% <100.00%> (+0.02%)`	⬆️
src/crl/mod.rs	`97.42% <100.00%> (-0.08%)`	⬇️
src/crl/types.rs	`99.53% <100.00%> (+<0.01%)`	⬆️
src/end_entity.rs	`100.00% <100.00%> (ø)`
src/lib.rs	`100.00% <ø> (ø)`
src/verify_cert.rs	`97.31% <98.61%> (+0.58%)`	⬆️

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

src/verify_cert.rs

ctz · 2023-09-10T10:49:16Z

It's common for there to be several possible valid paths (eg if a CA is both cross-signed and also directly trusted, like Let's Encrypt in past years). So this API would be OK for consumers that want to (eg) display or log one of the valid paths, but not for those who want to apply further constraints to it (HPKP, DANE, etc.) if those constraints allow one possible chain but not others. Further constraints really have to be applied during path building so the search could continue to consider other paths.

djc · 2023-09-10T18:30:05Z

It's common for there to be several possible valid paths (eg if a CA is both cross-signed and also directly trusted, like Let's Encrypt in past years). So this API would be OK for consumers that want to (eg) display or log one of the valid paths, but not for those who want to apply further constraints to it (HPKP, DANE, etc.) if those constraints allow one possible chain but not others. Further constraints really have to be applied during path building so the search could continue to consider other paths.

Fair point! This PR (and the previous) were mostly focused on first creating a representation of the Path in away that can be presented in a public API. I've now added an extension point that allows callers to fail a particular complete path. Do you think that would be a good fit for the HPKP/DANE/etc use case? At least what I've seen from DANE it seems like it would allow callers to take care of that use case (though we may have to extend the public API of our Cert type some more).

(Would appreciate any suggestions on how to test verify_path().)

cpu

Very cool!

In terms of testing, I think my comment about exposing more public API surface will be helpful. After that, perhaps generate some certificate paths where one contains an additional EKU that you verify for in the callback, rejecting the candidates that do not have the required EKU throughout?

src/verify_cert.rs

src/crl/mod.rs

src/lib.rs

src/verify_cert.rs

tests/integration.rs

cpu · 2023-09-14T14:30:08Z

Should this be a blocker for #159 ? I'm not sure what timeline we're thinking for #159, if we want to cut the tag Soon this might be better for a follow-up. WDYT?

djc · 2023-09-14T14:35:32Z

I think so? I think there's no point in cutting a 0.102 release until rustls is ready for 0.22, so I suspect there's some time.

cpu · 2023-09-14T14:37:44Z

👍 Added to the list.

src/verify_cert.rs

src/lib.rs

cpu

This is great 👍

src/end_entity.rs

src/verify_cert.rs

I find this useful while developing (especially for more explorative development). We deny warnings in CI, so warnings still shouldn't make it to the main branch.

djc · 2023-10-05T12:56:45Z

@ctz did you want to take another look at this?

ctz

lgtm!

ctz · 2023-10-05T14:28:23Z

src/verify_cert.rs

+/// Path from end-entity certificate to trust anchor that's been verified.
+///
+/// See [`EndEntityCert::verify_for_usage()`] for more details on what verification entails.
+pub struct VerifiedPath<'p> {


do we want any derives on this? (i guess not, based on EndEntityCert and Cert not having any?)

We should maybe reconsider that, but given that EndEntityCert and Cert don't have any it seems mostly orthogonal to this PR.

djc · 2023-10-05T14:44:09Z

Published alpha.5.

cpu · 2023-10-05T14:47:34Z

@djc Did the test from #191 get dropped in a rebase?

djc requested review from cpu and ctz September 10, 2023 09:44

djc force-pushed the path-array branch from 8aa4b5e to 741d767 Compare September 10, 2023 09:58

djc commented Sep 10, 2023

View reviewed changes

src/verify_cert.rs Show resolved Hide resolved

djc force-pushed the path-array branch from 741d767 to a4ab7ef Compare September 10, 2023 16:16

djc force-pushed the path-array branch from f424202 to 41257ba Compare September 10, 2023 18:35

cpu reviewed Sep 11, 2023

View reviewed changes

ctz reviewed Sep 11, 2023

View reviewed changes

src/verify_cert.rs Outdated Show resolved Hide resolved

tests/integration.rs Outdated Show resolved Hide resolved

djc force-pushed the path-node branch 2 times, most recently from bc5a487 to 8c68276 Compare September 11, 2023 20:47

Base automatically changed from path-node to main September 11, 2023 20:57

wez mentioned this pull request Sep 12, 2023

DNS-Based Authentication of Named Entities (DANE) rustls/rustls#816

Open

cpu mentioned this pull request Sep 14, 2023

0.102 release planning #159

Closed

13 tasks

djc force-pushed the path-array branch from 41257ba to fb4a407 Compare September 17, 2023 09:04

djc commented Sep 17, 2023

View reviewed changes

src/verify_cert.rs Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

djc mentioned this pull request Sep 20, 2023

Let EndEntityCert deref to Cert #184

Merged

djc force-pushed the path-array branch 5 times, most recently from ff13776 to 6307b14 Compare September 21, 2023 13:59

cpu approved these changes Sep 21, 2023

View reviewed changes

djc force-pushed the path-array branch 2 times, most recently from de9386c to c7d165a Compare September 21, 2023 15:09

cpu mentioned this pull request Sep 27, 2023

Add support for accessing an EndEntityCert's spki #28

Closed

djc commented Sep 29, 2023

View reviewed changes

src/verify_cert.rs Outdated Show resolved Hide resolved

cpu mentioned this pull request Sep 29, 2023

Quick test for rejecting candidate paths #191

Merged

djc added 4 commits October 5, 2023 13:07

Allow warnings during development

c7f73d6

I find this useful while developing (especially for more explorative development). We deny warnings in CI, so warnings still shouldn't make it to the main branch.

Remove unused verification code

bdb5db3

Slightly extend verify_for_usage() documentation

40ab028

Use role() abstraction in CRL processing

845b834

djc force-pushed the path-array branch from 633c2e0 to 87a995d Compare October 5, 2023 12:39

djc added 5 commits October 5, 2023 14:50

Store intermediate certificates in an array

d1c5920

tests: pass trust anchors by reference

09ac88f

tests: pass EndEntityCert into verify_chain()

ba612fc

tests: pass intermediates by reference

b49f878

Yield trust anchor from path building

840e012

djc force-pushed the path-array branch 2 times, most recently from 4bfbcd2 to 58b4de2 Compare October 5, 2023 12:56

djc added 4 commits October 5, 2023 15:06

Yield path representation from verify_for_usage()

23526f8

Add path verification extension point

45d98fb

Add Cert::der() method to access raw certificate DER

7b41eba

Bump version to alpha.5

5dfe82e

djc force-pushed the path-array branch from 743e39e to 5dfe82e Compare October 5, 2023 13:09

ctz approved these changes Oct 5, 2023

View reviewed changes

djc added this pull request to the merge queue Oct 5, 2023

Merged via the queue into main with commit cbbf352 Oct 5, 2023
56 checks passed

djc deleted the path-array branch October 5, 2023 14:35

cpu mentioned this pull request Oct 5, 2023

verify_cert: add test for rejecting candidate path #197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose built path in public API #174

Expose built path in public API #174

djc commented Sep 10, 2023 •

edited

codecov bot commented Sep 10, 2023 •

edited

ctz commented Sep 10, 2023

djc commented Sep 10, 2023 •

edited

cpu left a comment

cpu commented Sep 14, 2023

djc commented Sep 14, 2023

cpu commented Sep 14, 2023

cpu left a comment

djc commented Oct 5, 2023

ctz left a comment

ctz Oct 5, 2023

djc Oct 5, 2023

djc commented Oct 5, 2023

cpu commented Oct 5, 2023

Expose built path in public API #174

Expose built path in public API #174

Conversation

djc commented Sep 10, 2023 • edited

codecov bot commented Sep 10, 2023 • edited

Codecov Report

ctz commented Sep 10, 2023

djc commented Sep 10, 2023 • edited

cpu left a comment

Choose a reason for hiding this comment

cpu commented Sep 14, 2023

djc commented Sep 14, 2023

cpu commented Sep 14, 2023

cpu left a comment

Choose a reason for hiding this comment

djc commented Oct 5, 2023

ctz left a comment

Choose a reason for hiding this comment

ctz Oct 5, 2023

Choose a reason for hiding this comment

djc Oct 5, 2023

Choose a reason for hiding this comment

djc commented Oct 5, 2023

cpu commented Oct 5, 2023

djc commented Sep 10, 2023 •

edited

codecov bot commented Sep 10, 2023 •

edited

djc commented Sep 10, 2023 •

edited