Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: wasm compilation errors #122

Closed
wants to merge 4 commits into from

Conversation

adenh93
Copy link

@adenh93 adenh93 commented Aug 6, 2024

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.

@ctz
Copy link
Member

ctz commented Aug 6, 2024

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"] }

@adenh93
Copy link
Author

adenh93 commented Aug 7, 2024

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

@@ -47,6 +47,8 @@ android_logger = { version = "0.13", optional = true } # Only used during testin

[target.'cfg(target_arch = "wasm32")'.dependencies]
webpki-roots = "0.26"
webpki = { package = "rustls-webpki", version = "0.102", default-features = false }
ring = { version = "0.17.7", features = ["wasm32_unknown_unknown_js"] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is correct. We just recently removed an issue where this crate unconditionally depended on ring regardless of rustls's set cryptography provider (#102).

May I ask what led to needing to enabling this feature in production builds?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @complexspaces ,

My apologies for the late response, I was inactive over the weekend.

If I remember, adding it as a conditional dev-dependency would appease clippy-ci, but not clippy-msrv-ci.

But you're right, it was a very poor choice to promote it to a production dependency 😓

@complexspaces
Copy link
Collaborator

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.

@djc
Copy link
Member

djc commented Aug 26, 2024

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?

@cpu
Copy link
Member

cpu commented Aug 26, 2024

I feel like we had a more specific discussion about this issue but cannot find it now

I think you're thinking of rustls/rustls#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.

@ctz
Copy link
Member

ctz commented Aug 30, 2024

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

@complexspaces
Copy link
Collaborator

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.

@djc
Copy link
Member

djc commented Sep 2, 2024

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

@cpu
Copy link
Member

cpu commented Sep 5, 2024

is there still anything from this PR that we should consider for merging?

I don't think so, it seems to me that #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 cpu closed this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants