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

docs: recommend add_parseable_certificates. #62

Closed
wants to merge 1 commit into from

Conversation

cpu
Copy link
Member

@cpu cpu commented Mar 24, 2023

Unfortunately the system root store may contain legacy, malformed certificates. Calling RootCertStore.add with these certificates (as previously recommended in the example usage) will error.

In this commit an example using the more lenient add_parseable_certificates is offered ahead of the add example.

See rustls/rustls#1248, rustls/rustls#1249 and rustls/rustls#1246 for more information.

Unfortunately the system root store may contain legacy, malformed
certificates. Calling `RootCertStore.add` with these certificates (as
previously recommended in the example usage) will error.

In this commit an example using the more lenient
`add_parseable_certificates` is offered ahead of the `add` example.
//! let root_certs = &rustls_native_certs::load_native_certs()
//! .expect("could not load platform certs")
//! .iter()
//! .map(|c| c.0.clone())
Copy link
Member Author

Choose a reason for hiding this comment

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

This clone is my clumsy way to avoid moving the Vec<u8>. I think probably we want to take a ref, but I wanted to put this draft up for feedback before logging off.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, well add_parseable_certificates takes Vec<u8> so having Vec<&u8> won't be helpful. The clone might be best after all.

//! .map(|c| c.0.clone())
//! .collect::<Vec<Vec<u8>>>();
//!
//! roots.add_parsable_certificates(root_certs);
Copy link
Member Author

Choose a reason for hiding this comment

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

There might be an argument to be made that we should do something with the return from add_parseable_certificates to make it more obvious that this function returns useful information about how many roots were added vs ignored.

I've left it out to keep the example succinct. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've left it out to keep the example succinct. WDYT?

I've come around to this being important to include but I'm not sure what to do with the results. Put them in unused but well named _ prefixed vars? Print them? Other?

@cpu
Copy link
Member Author

cpu commented Mar 27, 2023

I think with the plan to revert the webpki serial length strictness change this PR isn't as useful. Presumably we'll keep treating invalid roots from system root stores in a relaxed manner for compat. and this add example can stand as-is.

@cpu cpu closed this Mar 27, 2023
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

1 participant