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

Remove TrustAnchorWithDn type #1459

Merged
merged 5 commits into from
Sep 11, 2023
Merged

Remove TrustAnchorWithDn type #1459

merged 5 commits into from
Sep 11, 2023

Conversation

djc
Copy link
Member

@djc djc commented Sep 11, 2023

TBD: do we still need/want the RootCertStore type after this, or can we make do with a Vec<TrustAnchor<'static>>? I think we'll still need a place to wrap webpki::extract_trust_anchor(), and this eliminates the annoying allocations required to convert a Vec<TrustAnchorWithDn<'_>> to a &[TrustAnchor<'_>] so keeping it probably makes sense?

@djc djc requested review from cpu and ctz September 11, 2023 11:34
/// use x509_parser::prelude::FromDer;
/// println!("{}", x509_parser::x509::X509Name::from_der(dn.as_ref())?.1);
/// ```
pub fn in_sequence(bytes: &[u8]) -> Self {
Copy link
Member Author

Choose a reason for hiding this comment

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

Any name suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

I can think of lots of other names, but no convincing arguments why they'd be better than this :)

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a perfectly cromulent name as-is 👍

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #1459 (81eaf41) into main (384dd1c) will decrease coverage by 0.01%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1459      +/-   ##
==========================================
- Coverage   96.50%   96.49%   -0.01%     
==========================================
  Files          71       71              
  Lines       15148    15105      -43     
==========================================
- Hits        14619    14576      -43     
  Misses        529      529              
Files Changed Coverage Δ
rustls/src/webpki/mod.rs 71.42% <ø> (ø)
rustls/src/msgs/handshake.rs 98.12% <100.00%> (+<0.01%) ⬆️
rustls/src/msgs/handshake_test.rs 99.59% <100.00%> (+<0.01%) ⬆️
rustls/src/verifybench.rs 100.00% <100.00%> (ø)
rustls/src/webpki/anchors.rs 90.24% <100.00%> (-4.36%) ⬇️
rustls/src/webpki/verify.rs 99.71% <100.00%> (-0.02%) ⬇️

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

Copy link
Member

@ctz ctz left a comment

Choose a reason for hiding this comment

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

TBD: do we still need/want the RootCertStore type after this, or can we make do with a Vec<TrustAnchor<'static>>?

I feel like the two remaining methods get quite a bit of use and are generally useful -- so I would like to see them kept. I guess they could become free functions that mutate a Vec<TrustAnchor>? The other option would be an extension trait on Vec<TrustAnchor>, which I generally find to be pretty annoying from an API discoverability point of view.

Given the item in RootCertStore is and always has been pub, people can move into/out of this type pretty freely to use these functions if desired?

/// use x509_parser::prelude::FromDer;
/// println!("{}", x509_parser::x509::X509Name::from_der(dn.as_ref())?.1);
/// ```
pub fn in_sequence(bytes: &[u8]) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

I can think of lots of other names, but no convincing arguments why they'd be better than this :)

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.

LGTM

/// use x509_parser::prelude::FromDer;
/// println!("{}", x509_parser::x509::X509Name::from_der(dn.as_ref())?.1);
/// ```
pub fn in_sequence(bytes: &[u8]) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a perfectly cromulent name as-is 👍

@djc djc added this pull request to the merge queue Sep 11, 2023
Merged via the queue into main with commit 53e9e77 Sep 11, 2023
39 of 40 checks passed
@djc djc deleted the rm-with-dn branch September 11, 2023 13:45
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.

None yet

3 participants