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

Panic if no trust anchors are found #125

Merged
merged 2 commits into from
Nov 28, 2020
Merged

Conversation

djc
Copy link
Member

@djc djc commented Nov 9, 2020

It looks like hyper-rustls wants to abstract over the differences between using the webpki-roots and rustls-native-certs backend, but this just bit me in an unobvious way that I think could be improved. I'm building a scratch container, meaning it only contains the statically linked binary and nothing else. One of my dependencies uses hyper-rustls as a dependency with the default features. As a result, (a) rustls-native-certs returns an empty RootCertStore without flagging that there might be an issue, (b) when hyper tries to connect through the Connector, it fails with a webpki::Error::UnknownIssuer. And, this doesn't happen when testing outside the containerized environment, but fails inside the container.

I feel like something should probably generate an error or panic if this happens, but I'm not sure which level of the stack is most appropriate. Since the return value of rustls_native_certs::load_native_certs() is technically correct, I feel like the problem here is in how hyper-rustls tries to abstract over the differences.

The solution implemented here is to have hyper-rustls panic if Connector::new() doesn't find any trust anchors; since it can already panic when it is unable to access a native cert store, this seems like an okay solution. Alternatively, maybe rustls_native_certs::load_native_certs() should return an error if it doesn't find anything? This is technically correct but not very useful behavior.

@djc djc changed the title Panic empty store Panic if no trust anchors are found Nov 9, 2020
@djc
Copy link
Member Author

djc commented Nov 9, 2020

After some further experiments, I believe the enforcement of mutually exclusive enabling of the features doesn't fit well with how Cargo features are intended to be used. If you want users to make an explicit choice, this should probably be done at the API level (for example, HttpsConnector could have with_webpki_roots() and with_native_roots() instead of new()). The compile error says that binary crates can use default-features to force one or the other, but in complex dependency graphs I don't think that's true: if a binary crate also depends on a library that depends on the default features for hyper-rustls, the binary crate will still get both webpki-roots and rustls-native-certs enabled, leading to an error.

(Let me know if you want some help with this crate, it turns out to be pretty central to my current work.)

@ctz ctz merged commit 0f4a108 into rustls:master Nov 28, 2020
@ctz
Copy link
Member

ctz commented Nov 28, 2020

Thanks!

If you want users to make an explicit choice, this should probably be done at the API level

Yeah, I'd prefer this. I'm quite happy for default-features for this crate to include both webpki-roots and rustls-native-certs as dependencies.

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.

2 participants