P2P: Feature-gate libp2p's mdns & remove wasm-timer

[PhilippGackstatter]

Description of change

This PR contains two Wasm-related changes in the stronghold-p2p crate.

1. Remove wasm-timer and add instant. This is required for proper Wasm support because libp2p itself has moved away from wasm-timer.
2. Feature-gate libp2p's mdns feature, which is not supported on Wasm targets.

Type of change

Choose a type of change, and delete any options that are not relevant.

• Bug fix (a non-breaking change which fixes an issue)
• Enhancement (a non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Fix

How the change has been tested

No new functionality was added, so only existing tests were run.

Change checklist

Add an x to the boxes that are relevant to your changes, and delete any items that are not.

• I have followed the contribution guidelines for this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[nothingismagick]

I can only hope that you are doing this in order to have a thin stronghold client in the browser, without any signing or other cryptographic powers. Otherwise, this is a clear violation of the underlying philosophy of never putting a stronghold in the browser. Can you please advise to your planned usage of this feature?

[elenaf9]

The test in behaviour.rs also needs to be changed, could you please include the below diff:

--- a/p2p/src/behaviour.rs
+++ b/p2p/src/behaviour.rs
@@ -971,9 +971,10 @@ mod test {
     use super::*;
     use crate::firewall::{PermissionValue, RequestPermissions, VariantPermission};
     use futures::{channel::mpsc, StreamExt};
+    #[cfg(feature = "mdns")]
+    use libp2p::mdns::{Mdns, MdnsConfig};
     use libp2p::{
         core::{identity, upgrade, PeerId, Transport},
-        mdns::{Mdns, MdnsConfig},
         noise::{Keypair as NoiseKeypair, NoiseConfig, X25519Spec},
         relay::{new_transport_and_behaviour, RelayConfig},
         swarm::{Swarm, SwarmBuilder, SwarmEvent},
@@ -1174,12 +1175,14 @@ mod test {
             .multiplex(YamuxConfig::default())
             .boxed();
 
+        #[cfg(feature = "mdns")]
         let mdns = Mdns::new(MdnsConfig::default())
             .await
             .expect("Failed to create mdns behaviour.");
         let (dummy_tx, _) = mpsc::channel(10);
         let behaviour = NetBehaviour::new(
             NetBehaviourConfig::default(),
+            #[cfg(feature = "mdns")]
             Some(mdns),
             Some(relay_behaviour),
             dummy_tx,

Looks good to me otherwise.

[PhilippGackstatter]

Can you please advise to your planned usage of this feature?

This PR re-enables compilation of stronghold-p2p for Wasm. This is required for the identity actor we're building, which is supposed to be able to run in node.js applications, as an example. We have not fully decided on whether an actor will eventually also run in a browser, but since it would need to authenticate to another actor before being able to do any cryptographic operations in that scenario, that does not have the same security implications as running a full stronghold in a browser. A similar argument goes for a "remote stronghold" (i.e. a thin client that connects to a stronghold via stronghold-p2p) that we may want to run in a browser.

We definitely do not plan to run full strongholds in browsers.

[PhilippGackstatter]

The test in behaviour.rs also needs to be changed

I've added mdns as a feature of libp2p in the dev-dependencies so feature-gating should not be required in tests then, right?

[elenaf9]

The test in behaviour.rs also needs to be changed

I've added mdns as a feature of libp2p in the dev-dependencies so feature-gating should not be required in tests then, right?

You can specify what features are active when running a test, so we also need conditional compilation within the test. Even if mdns is included as dev-dependency, running cargo test --no-default-features fails.

[nothingismagick]

We definitely do not plan to run full strongholds in browsers.

Understood. However if you make it feasible, someone will build it. Similar to gadget attacks, we also never shipped with functionality to read out a secret, you are potentially opening up venues for misuse and I think you would do well to make a MASSIVE disclaimer. Also, I am pretty sure this will trigger the requirement for an entire audit, since NEON bindings are not the same as WASM inclusion and the memory space of V8 + WASM means you have a plethora if new attack vectors such as electron and server-based applications to have reviewed.

[elenaf9]

Understood. However if you make it feasible, someone will build it. Similar to gadget attacks, we also never shipped with functionality to read out a secret, you are potentially opening up venues for misuse and I think you would do well to make a MASSIVE disclaimer. Also, I am pretty sure this will trigger the requirement for an entire audit, since NEON bindings are not the same as WASM inclusion and the memory space of V8 + WASM means you have a plethora if new attack vectors such as electron and server-based applications to have reviewed.

@nothingismagick maybe there is some misunderstanding here. This PR only targets the Stronghold-P2p crate (which does not have any Stronghold-related logic in it), not Stronghold itself.
Or am I missing something?

[PhilippGackstatter]

You can specify what features are active when running a test, so we also need conditional compilation within the test.

Got it, I feature-gated it.

[nothingismagick]

ok ok - sorry. you guys know my paranoia with anything surrounding WASM. sorry for the bruhaha :)

[elenaf9]

ok ok - sorry. you guys know my paranoia with anything surrounding WASM. sorry for the bruhaha :)

Your input is very much appreciated. Please continue the discussion in case you think that using the p2p crate in WASM could already be an issue.

[nothingismagick]

If you look at the description at https://noiseexplorer.com/patterns/XX/ - there is one comment that is the same in every stage:

"Assuming the corresponding private keys are secure, this authentication cannot be forged."

If you are using WASM in the browser, how do you guarantee that the private keys are secure?

[PhilippGackstatter]

Thanks for the input @nothingismagick, we will discuss this some more before going ahead with the PR.

[felsweg-iota]

thanks @nothingismagick for your input!

The more I think about it, the more I agree, that this might not be a good idea. There is the (still experimental) credentials api, that give access to an agent's password manager. However, the password manager can be easily compromised, thus storing access credentials to a running Stronghold instance in the background would break the security model.

I would like to add some thoughts on how an extension might interact with Stronghold without compromising security, and give the flexibility of a browser extension:

• Since it's a browser extension, user input must be allowed to be required. the integrating software, that most probably runs as a background service ( eg. daemon ) could issue one time tokens provided signed by a private key. The private key's hash could be shortly made visible during the creation inside the extension. A request is send to the background service, which in turn also requires confirmation by the user. A one time token is being issued for the resources requested. This token could only be used once, after that It would be invalid.
• Since browser would have access to hardware tokens that support user interaction (eg. solokeys or yubikey), the private key could be stored inside the hw token, requesting user interaction each time a certain action is required inside the background service. This type of interaction with a hw token could also be part of Stronghold itself. The communication with the background service would only be token based.

Having said that, there might be still a window when secrets can be intercepted when issuing requests from the extension. We surely have to discuss this.

[PhilippGackstatter]

Thanks again for the discussion here. This PR was intended to enable our identity actor to run on Wasm, but we have since replaced stronghold-p2p directly with libp2p, so this PR is not needed anymore, and I will close it.

Of course, the discussion around Wasm safety is still very important, regardless of this change, and unfortunately an ongoing topic as no solution has emerged yet. The discussion will thus be continued on other channels.