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

Extract PathNode from Cert #173

Merged
merged 12 commits into from
Sep 11, 2023
Merged

Extract PathNode from Cert #173

merged 12 commits into from
Sep 11, 2023

Conversation

djc
Copy link
Member

@djc djc commented Sep 8, 2023

Previously, a Cert type (which mostly represents a certificate) also doubled as a linked list node via the (somewhat cryptically named) ee_or_ca field. Instead, use a linked list node type (here called PathNode to evoke the somewhat well-known notion of path building) that contains the Cert.

Cert construction becomes a lot more obvious and simple because it is no longer obliged to deal with the linked list, and we can draw a clean distinction between types for dealing the with the linked list path and types that just need the certificate data from a Cert.

After spending some time with the path-building code, I wanted to try and improve cohesion so tacked on a bunch of commits that try to colocate related code.

@djc djc requested review from cpu and ctz September 8, 2023 19:01
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #173 (8c68276) into main (7ff3664) will increase coverage by 0.05%.
Report is 1 commits behind head on main.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
+ Coverage   96.64%   96.70%   +0.05%     
==========================================
  Files          16       17       +1     
  Lines        4532     4518      -14     
==========================================
- Hits         4380     4369      -11     
+ Misses        152      149       -3     
Files Changed Coverage Δ
src/crl/mod.rs 97.42% <97.42%> (ø)
src/verify_cert.rs 97.79% <97.60%> (+0.11%) ⬆️
src/crl/types.rs 99.53% <99.55%> (ø)
src/cert.rs 97.47% <100.00%> (+0.60%) ⬆️
src/end_entity.rs 100.00% <100.00%> (ø)
src/subject_name/verify.rs 96.27% <100.00%> (-0.14%) ⬇️
src/trust_anchor.rs 98.48% <100.00%> (ø)

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

cpu
cpu previously approved these changes Sep 8, 2023
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Cool, this worked out nicely 👍

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Really appreciate this clean-up 🙏 Thanks!

src/verify_cert.rs Show resolved Hide resolved
src/crl/mod.rs Outdated Show resolved Hide resolved
Merged via the queue into main with commit 58ab3c0 Sep 11, 2023
46 checks passed
@djc djc deleted the path-node branch September 11, 2023 20:57
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.

3 participants