Remove undocumented panic in with_native_roots

[kayabaNerve]

Breaking API change as required to implement this safety.

Somewhat resolves #187.

[cpu]

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.

[kayabaNerve]

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]

Done

[djc]

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

[kayabaNerve]

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]

@ctz Resolved your comments :)

[kayabaNerve]

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

[cpu]

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]

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]

Anything else needed for this to be merged?

[cpu]

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]

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]

Pushed a new single commit.

[kayabaNerve]

CI passed :D

[ctz]

this looks good to me

[cpu]

Thanks kayabaNerve.

[kayabaNerve]

Happy to.