rustls: optionally use WebPKI roots to avoid panicking on Android & iOS

[ewilken]

Motivation

Using kube-rs on iOS, I came across a panic in the underlying hyper-rustls root cert initialization trying to access the native roots, which doesn't work on Android or iOS, according to this open issue.

Solution

My solution was to add a check to fall back to the WebPKI roots when on Android or iOS, which fixes the issue for me, but could make sense upstream, too.

Please let me know if this change makes sense to you or whether I'm missing anything. And thanks a lot for all the work you've done on the kube-rs ecosystem! 😊

[clux]

Hey, thanks for this!

I do think this, or some variant of this makes sense, and am happy to include a variant of this. The few bits i am sceptical of atm are:

• default inclusion - as this would be a new dep for everyone, even though we use native roots elsewhere
• magic inference - it's probably better to let users choose this rather than bake in this (particularly as it gives the impression we test iOS/android which we dont)

There are two alternatives I think;

1. make an explicit feature for it so people can choose this option if they want to in general
2. try to do some [target.cfg...] magic in Cargo.toml

I think 1. makes the most sense, but it might be hairier to make a feature for this? At the very least reqwest has a quite hairy feature selection setup to get all the things exposed.

[codecov]

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 75.3%. Comparing base (bb9a44e) to head (f5c33fe).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kube-client/src/client/tls.rs	0.0%	3 Missing ⚠️
kube-client/src/client/auth/oauth.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1323     +/-   ##
=======================================
- Coverage   75.3%   75.3%   -0.0%     
=======================================
  Files         82      82             
  Lines       7328    7331      +3     
=======================================
  Hits        5515    5515             
- Misses      1813    1816      +3     

Files with missing lines	Coverage Δ
kube-client/src/client/auth/oidc.rs	56.1% <100.0%> (-0.3%)	⬇️
kube-client/src/client/auth/oauth.rs	0.0% <0.0%> (ø)
kube-client/src/client/tls.rs	79.6% <0.0%> (-0.9%)	⬇️

[ewilken]

Makes sense. Happy to try out either one of the two scenarios!

But is the failing CI check about webpki-roots bringing in an MPL-2.0 license a show-stopper?

[clux]

MPL-2.0

It shouldn't be a problem. Particularly if it's an opt-in feature, as then it's up to users whether they want the stricter license the transitive dependency comes with.

[clux]

Given we want opt-in for many reasons, i think if we add a feature for it in kube-client/Cargo.toml:

webpki-roots = ["hyper-rustls/webpki-roots"]

plus re-export it from kube/Cargo.toml (i.e. feature in kube crate needs to enable kube-client/webpki-roots).

then we should be good.

we would need a licenses.clarify entry in deny.toml (see docs) about this license being excluded because it is an optional feature.

[ewilken]

Gotcha. Do you think the feature should gate the current check to use WebPKI roots only on Android and iOS, or just alternate between native and WebPKI roots on every platform by setting the feature flag? My gut feeling is the latter is probably the cleaner thing to do and less confusing for an API consumer.

[ewilken]

Sorry it went quiet here for so long. Trying to give this another go now. cargo-deny seems to be happy even without a licenses.clarify, but please let me know if I got anything wrong on that end.

[clux]

I think this generally looks good. I had also forgotten about this. Two minor suggestions.