rustls-platform-verifier icon indicating copy to clipboard operation
rustls-platform-verifier copied to clipboard

fix: wasm compilation errors

Open adenh93 opened this issue 1 year ago • 3 comments

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.

adenh93 avatar Aug 06 '24 07:08 adenh93

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 avatar Aug 06 '24 10:08 ctz

@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_*.

adenh93 avatar Aug 07 '24 01:08 adenh93

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.

complexspaces avatar Aug 11 '24 06:08 complexspaces

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.

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

djc avatar Aug 26 '24 11:08 djc

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.

cpu avatar Aug 26 '24 14:08 cpu

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.)

ctz avatar Aug 30 '24 14:08 ctz

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.

complexspaces avatar Sep 01 '24 06:09 complexspaces

So since #136 has been merged, is there still anything from this PR that we should consider for merging?

djc avatar Sep 02 '24 09:09 djc

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.

cpu avatar Sep 05 '24 15:09 cpu