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 undocumented panic in with_native_roots #228

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Remove undocumented panic in with_native_roots #228

merged 1 commit into from
Nov 15, 2023

Conversation

kayabaNerve
Copy link
Contributor

Breaking API change as required to implement this safety.

Somewhat resolves #187.

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.

Hi, thanks for the PR. I have a few initial comments.

At a meta-level I'm also wondering whether we should augment this API surface with something feature-gated on webpki-roots that allows trying to find native trust anchors, but automatically falling back to bundled webpki-roots trust anchors when it comes up empty. I suspect that might be more user friendly, and matches the approach offered in rustls-platform-verifier (see some discussion in rustls/rustls-platform-verifier#12).

The rustls-platform-verifier work was motivated by observation in the field that openssl-probe wasn't always reliable, and that's the dependency underpinning rustls-native-roots on non-MacOS unix systems. What platform are you seeing this panic on? There were similar requests for more information in #187 that nobody has provided to date.

examples/client.rs Outdated Show resolved Hide resolved
src/config.rs Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@kayabaNerve
Copy link
Contributor Author

but automatically falling back to bundled webpki-roots trust anchors when it comes up empty. I suspect that might be more user friendly, and matches the approach offered in rustls-platform-verifier (see some discussion in rustls/rustls-platform-verifier#12).

While I'd support this, I believe that should be an additional, recommended function. Not the replacement behavior for this function.

I observed this on a Linux system without ca-certificates installed. Specifically, a GH CI runner I uninstalled most packages from. Then, I also encountered it on a Debian slim Docker image.

I'm fine updating the documentation 👍

@kayabaNerve
Copy link
Contributor Author

Done

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/connector/builder.rs Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented Oct 31, 2023

Usually we would postpone builder API errors until the end of the builder chain, which this would subvert.

@kayabaNerve
Copy link
Contributor Author

While I'm not against standing in the way in convention with PR, I'd note that policy makes it more difficult to identify which step in the process failed as you get one Result for all steps, not one Result for each step which may fail. While the error itself may reasonably allowing discovery of the issue, the proposed path here is if with_native_roots returns None/error, callers fallback to with_webpki_roots. They'd need a way to introspect the final Result to check if it's due to this specific issue in order to safely execute the suggested fallback (unless they simply blindly try again with with_webpki_roots).

Since the suggested error to replace this with was std:io::Error, not hyper_rustls::config::BuildError, this seems infeasible.

Also, while I do agree the error would be more descriptive, I think None to signify there were no valid roots on the system is fine. Regardless, I'm happy to move it to an error, so long as the policy on where to put the error and the suggested mediation path is resolved.

@kayabaNerve
Copy link
Contributor Author

@ctz Resolved your comments :)

src/config.rs Outdated Show resolved Hide resolved
@kayabaNerve
Copy link
Contributor Author

Also, the lib.rs example is failing as its function doesn't return an error, so that does have to be an expect...

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.

Thank you.

I will circle back with the idea of making a new combined "try native then webpki-roots" method later on (unless you want to tackle that in a follow-up yourself).

@kayabaNerve
Copy link
Contributor Author

Personally, I think that's trivial enough it doesn't have to be in hyper-rustls, but it's hard to argue against libs adding extra helpers to go the extra mile in friendliness...

@kayabaNerve
Copy link
Contributor Author

Anything else needed for this to be merged?

@cpu
Copy link
Member

cpu commented Nov 15, 2023

Anything else needed for this to be merged?

Looks like there's a CI failure from another example in lib.rs that needs to expect. Could you fix that up @kayabaNerve? It would also be helpful if you could squash your commit history.

@kayabaNerve
Copy link
Contributor Author

kayabaNerve commented Nov 15, 2023

Oh, sorry. I have no idea how I missed that. Will correct that now.

Would love to get this included with whatever release updates to hyper 1.0 :)

@kayabaNerve
Copy link
Contributor Author

Pushed a new single commit.

@kayabaNerve
Copy link
Contributor Author

CI passed :D

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.

this looks good to me

@djc djc added this pull request to the merge queue Nov 15, 2023
Merged via the queue into rustls:main with commit 77154da Nov 15, 2023
10 checks passed
@cpu
Copy link
Member

cpu commented Nov 15, 2023

Thanks kayabaNerve.

@kayabaNerve
Copy link
Contributor Author

Happy to.

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.

panics "no CA certificates found"
4 participants