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

Gracefully handle invalid native root certificates #283

Merged
merged 1 commit into from
May 18, 2023

Conversation

CBenoit
Copy link
Contributor

@CBenoit CBenoit commented May 17, 2023

Before this patch, the rustls::RootCertStore::add method was used to add all the root certificates found by rustls_native_certs crate. This is a problem when an ancient or invalid certificate is present in the native root store. rustls documentation says the following:

This is suitable for a small set of root certificates that
are expected to parse successfully. For large collections of
roots (for example from a system store) it is expected that
some of them might not be valid according to the rules rustls
implements. As long as a relatively limited number of certificates
are affected, this should not be a cause for concern. Use
RootCertStore::add_parsable_certificates in order to add as many
valid roots as possible and to understand how many certificates have
been diagnosed as malformed.

With this patch, RootCertStore::add_parsable_certificates is used instead for maximal compability with system store.

Parse the given DER-encoded certificates and add all that can be
parsed in a best-effort fashion.

This is because large collections of root certificates often include
ancient or syntactically invalid certificates.

@CBenoit CBenoit force-pushed the fix-error-on-bad-root-cert branch 3 times, most recently from 2d61819 to 885104c Compare May 17, 2023 15:35
Before this patch, the `rustls::RootCertStore::add` method was used
to add all the root certificates found by `rustls_native_certs` crate.
This is a problem when an ancient or invalid certificate is present
in the native root store. `rustls` documentation says the following:

> This is suitable for a small set of root certificates that
> are expected to parse successfully. For large collections of
> roots (for example from a system store) it is expected that
> some of them might not be valid according to the rules `rustls`
> implements. As long as a relatively limited number of certificates
> are affected, this should not be a cause for concern. Use
> `RootCertStore::add_parsable_certificates` in order to add as many
> valid roots as possible and to understand how many certificates have
> been diagnosed as malformed.

With this patch, `RootCertStore::add_parsable_certificates` is used
instead for maximal compability with system store.

> Parse the given DER-encoded certificates and add all that can be
> parsed in a best-effort fashion.
>
> This is because large collections of root certificates often include
> ancient or syntactically invalid certificates.
@CBenoit CBenoit force-pushed the fix-error-on-bad-root-cert branch from 885104c to c3e09c4 Compare May 17, 2023 15:42
@CBenoit
Copy link
Contributor Author

CBenoit commented May 17, 2023

Demonstration in my project:
image

Copy link
Member

@daniel-abramov daniel-abramov left a comment

Choose a reason for hiding this comment

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

Sounds reasonable, thanks!

@daniel-abramov daniel-abramov merged commit a6c2d13 into snapview:master May 18, 2023
5 checks passed
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

2 participants