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.

Sign up for GitHub

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

Jump to bottom

cert: relax enforcing positive serial numbers. #36

Merged
merged 6 commits into from
Mar 13, 2023
Merged

cert: relax enforcing positive serial numbers. #36

merged 6 commits into from
Mar 13, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Mar 13, 2023 •
edited
Loading

This branch extends the #10 PR submitted by @dequbed in the hopes of getting this work merged before the release tag is cut. Thanks and credit to them for the original work!

Original Description

Based on discussion in Discord: https://discord.com/channels/976380008299917365/1015156984007381033/1037367413357948928

This follows the crypt implementation of Go, as per the following commit: golang/go@a0ea93d

New Updates

  • The existing TODOs for unreadable UNIX epoch literals are addressed by adding human readable comments: d986ba0 (addresses review feedback from @cpu)
  • Conflicts with the main branch are resolved.
  • More justification from RFC 5280 is included in a comment: 71a699e (addresses review feedback from @ctz)
  • The max serial length check is restored: da7b6af (addresses review feedback from @ctz).
  • Thew new unit test is gated on the alloc feature to fix cargo test --no-default-features: f4e3ad3

cpu and others added 4 commits March 13, 2023 11:09
@cpu
chore: comment epoch literals w/ RFC 3339 vals.
d986ba0 
This commit removes some TODOs on `clippy::unreadable_literal` allows in
the integration tests by adding RFC 3339 values in comments for each of
the UNIX epoch literals.

In an ideal world we'd specify the dates _as_ RFC 3339 date strings and
parse them to a UNIX epoch, but doing so would require a new crate
dependency. This approach makes the literals more readable and requires
no new deps.
@dequbed @cpu
Allow negative serials in all certificates, not just trust anchors.
c97eab3 
This follows the crypt implementation of Go, as per the following commit: golang/go@a0ea93d

Signed-off-by: Nadja Reitzenstein <me@dequbed.space>
@dequbed @cpu
Add negative serial test for end certificates
9802622 
Signed-off-by: Nadja Reitzenstein <me@dequbed.space>
@cpu
cert: quote RFC 5280 RE: lax serial numbers.
71a699e
@cpu cpu mentioned this pull request Mar 13, 2023
Closed
cpu
cpu commented Mar 13, 2023
View reviewed changes
src/cert.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #36 (c6c93fb) into main (c675914) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
- Coverage   94.15%   94.14%   -0.02%     
==========================================
  Files          14       14              
  Lines        2533     2509      -24     
==========================================
- Hits         2385     2362      -23     
+ Misses        148      147       -1
Impacted Files Coverage Δ
src/der.rs 94.07% <ø> (-0.22%) ⬇️
src/cert.rs 96.72% <100.00%> (-0.18%) ⬇️
src/trust_anchor.rs 92.72% <100.00%> (-1.31%) ⬇️

... and 1 file with indirect coverage changes

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

djc
djc approved these changes Mar 13, 2023
View reviewed changes
src/cert.rs Outdated Show resolved Hide resolved
@cpu
Copy link
Member Author

cpu commented Mar 13, 2023

It looks like there's a test failure in read_ee_with_neg_serial to sort out specific to --no-default-features. I see the same failure running cargo test --no-default-features on the unmodified #10 branch. The test passes when not providing --no-default-features 🤔

@cpu
tests: gate read_ee_with_neg_serial on alloc feat.
f4e3ad3 
The `ALL_SIGALGS` slice of `SignatureAlgorithm`'s only contains the RSA
algorithms this test relies on when the `alloc` feature is enabled.
Therefore, like some other tests in `tests/integration.rs` we must gate
this particular test on the `alloc` feature.
@cpu
Copy link
Member Author

cpu commented Mar 13, 2023 •
edited
Loading

It looks like there's a test failure in read_ee_with_neg_serial to sort out specific to --no-default-features. I see the same failure running cargo test --no-default-features on the unmodified #10 branch. The test passes when not providing --no-default-features

This turned out to be pretty simple. The new test needs to be gated on the alloc feature. See f4e3ad3.

@ctz ctz merged commit f33f3ad into rustls:main Mar 13, 2023
@cpu cpu deleted the cpu-adopt-10-negative-serials branch March 13, 2023 18:51
@cpu cpu mentioned this pull request Mar 24, 2023
Closed
@cpu cpu mentioned this pull request Dec 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

@ctz ctz ctz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cpu @codecov-commenter @djc @ctz @dequbed