rustls-platform-verifier
rustls-platform-verifier copied to clipboard
fix: wasm compilation errors
Thanks for the awesome library!
This PR aims to resolve compilation errors when targeting wasm32, which were caused by breaking changes to the RootCertStore interface that needed to be migrated in the conditionally compiled code.
Additionally, I've added a couple Clippy build steps to the CI workflow to try to prevent the code targeting WASM from becoming stale over time as rustls continues to evolve.
Looks like this will need enabling the js feature on getrandom to work? Maybe something like
[target.'cfg(target_arch = "wasm32")'.dev-dependencies]
getrandom = { version = "*", features = ["js"] }
@ctz Doh! I probably should've run cargo clippy-ci locally! 😅 🤦
In the end, I just had to specify the wasm32_unknown_unknown_js feature for the ring dev-dependency when targeting wasm32_*.
I took another look at this PR this evening. These are the minimum changes I made to get cargo clippy-ci --target wasm32-unknown-unknown passing on my system using the latest main (not counting lock file updates):
diff --git a/rustls-platform-verifier/Cargo.toml b/rustls-platform-verifier/Cargo.toml
index eb7c128..304108b 100644
--- a/rustls-platform-verifier/Cargo.toml
+++ b/rustls-platform-verifier/Cargo.toml
@@ -46,8 +46,13 @@ webpki = { package = "rustls-webpki", version = "0.102", default-features = fals
android_logger = { version = "0.13", optional = true } # Only used during testing.
[target.'cfg(target_arch = "wasm32")'.dependencies]
+webpki = { package = "rustls-webpki", version = "0.102", default-features = false }
+rustls-pki-types = { version = "1", features = ["web"] }
webpki-roots = "0.26"
+[target.'cfg(target_arch = "wasm32")'.dev-dependencies]
+ring = { version = "0.17.7", features = ["wasm32_unknown_unknown_js"] }
+
# BSD targets require webpki-roots for the real-world verification tests.
[target.'cfg(target_os = "freebsd")'.dev-dependencies]
webpki-roots = "0.26"
diff --git a/rustls-platform-verifier/src/verification/others.rs b/rustls-platform-verifier/src/verification/others.rs
index 29dc19d..9cd4e8a 100644
--- a/rustls-platform-verifier/src/verification/others.rs
+++ b/rustls-platform-verifier/src/verification/others.rs
@@ -154,14 +154,8 @@ impl Verifier {
#[cfg(target_arch = "wasm32")]
{
- root_store.add_trust_anchors(webpki_roots::TLS_SERVER_ROOTS.iter().map(|root| {
- rustls::OwnedTrustAnchor::from_subject_spki_name_constraints(
- root.subject,
- root.spki,
- root.name_constraints,
- )
- }));
- };
+ root_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned());
+ }
WebPkiServerVerifier::builder_with_provider(
root_store.into(),
One open question I have is if this library is the right place to set the web target on rustls-pki-types, or if that should be a flag in rustls itself. I ask this because usually library code should not enable js or web features in crate because it assumes wasm32-unknown-unknown is being used in the browser, which may not be the case. @cpu @djc do either of you have thoughts on this? I don't know if rustls has gotten any WASM questions before.
One open question I have is if this library is the right place to set the
webtarget onrustls-pki-types, or if that should be a flag inrustlsitself. I ask this because usually library code should not enablejsorwebfeatures in crate because it assumeswasm32-unknown-unknownis being used in the browser, which may not be the case. @cpu @djc do either of you have thoughts on this? I don't know ifrustlshas gotten any WASM questions before.
Some discussion linked below, I feel like we had a more specific discussion about this issue but cannot find it now -- maybe on Discord or in the pki-types repo?
- https://github.com/rustls/rustls/issues/808
- https://github.com/rustls/rustls/pull/1713
- https://github.com/rustls/pki-types/pull/32
I feel like we had a more specific discussion about this issue but cannot find it now
I think you're thinking of https://github.com/rustls/rustls/pull/1921, which did propose adding a rustls level web feature that itself activated the pki-types feature.
I'm not sure we reached a conclusion there other than to suggest we needed a more holistic approach to WASM support that considered both WASI and browsers and assoc. test coverage.
I don't think any crate in the middle of the dependency tree should set the web feature, even conditionally. Whether WASM is targetted at a browser or non-browser environment is something only known by the top-most crate.
(I think it's actually a significant design error that this is not just part of the target triple.)
I think it's actually a significant design error that this is not just part of the target triple
We agree on that point! wasm32-unknown-unknown was a bad target to chose for Rust-on-the-web broadly.
So since #136 has been merged, is there still anything from this PR that we should consider for merging?
is there still anything from this PR that we should consider for merging?
I don't think so, it seems to me that https://github.com/rustls/rustls-platform-verifier/pull/136 picked up everything we want. I think based on the above comments we're happy to test wasm32-wasip1 in CI in place of wasm32-unknown-unknown. Feel free to re-open if I've misjudged.
Thanks for the PR adhen93! Apologies it sat for so long.