Add support for rustls-platform-verifier #2286
Conversation
|
CI failure seems legit, looks like a type no longer implements the needed |
djc
force-pushed
the
rustls-platform-verifier
branch
3 times, most recently
from
4a982ef to
ad5e037
Compare
| #[cfg(feature = "rustls-tls-platform-verifier")] | ||
| let verifier = Arc::new(rustls_platform_verifier::Verifier::new()); | ||
| #[cfg(not(feature = "rustls-tls-platform-verifier"))] | ||
| let verifier = |
There was a problem hiding this comment.
The WebPkiServerVerifierBuilder::build() step yields Err if no roots have been passed in, meaning that a whole bunch of tests now panic when testing under rustls-tls-manual-roots (without specifying any roots). I've just disabled these tests when testing with this particular feature, which seems like the easiest approach?
Alternatively, we could have slightly more complex setup here which would avoid the error at builder time, but failing earlier actually seems desirable to me.
djc
force-pushed
the
rustls-platform-verifier
branch
from
ad5e037 to
afb4788
Compare
|
@seanmonstar friendly ping, want to take another look? (Let me know if you'd prefer fewer reminders.) |
| @@ -1,4 +1,4 @@ | |||
| #![cfg(not(target_arch = "wasm32"))] | |||
| #![cfg(not(any(target_arch = "wasm32", feature = "rustls-tls-manual-roots")))] | |||
There was a problem hiding this comment.
Hm, I guess I haven't fully internalized the difference. Looking at the rest of this test file, it seems to just make a "normal" client. Why would that fail?
Is it that if only manual roots are enabled, but none are added, that's an error?
And, what if other rustls features are enabled too? It should work, then?
There was a problem hiding this comment.
Hm, I guess I haven't fully internalized the difference. Looking at the rest of this test file, it seems to just make a "normal" client. Why would that fail?
Is it that if only manual roots are enabled, but none are added, that's an error?
Yes, that's the problem.
And, what if other rustls features are enabled too? It should work, then?
Because we can't use the rustls-platform-verifier in conjunction with otherwise-supplied roots on many platforms (only on non-Apple Unix, see rustls/rustls-platform-verifier#58), this PR chooses to skip gathering roots on all platforms in favor of exclusively using the platform verifier (which comes down to using rustls-native certs on non-Apple Unix).
There was a problem hiding this comment.
This seems to disrupt the additivity of the features. If anywhere in the dependency tree enables this feature, everywhere else that was depending on detecting native roots will now start erroring. That doesn't sound like a great experience for users.
There was a problem hiding this comment.
Yes, as discussed previously in #2159. Do you want to block this addition on rustls/rustls-platform-verifier#58, which would enable making it additive?
There was a problem hiding this comment.
Thinking from the point of view of users, I believe that is something they would expect to work. So yea, that seems like a requirement.
seanmonstar
added
the
B-upstream
Fixes #2159. Consider that issue for more context.