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

CIP-0045? | Decentralized WebRTC dApp-Wallet Communication #395

Merged
merged 21 commits into from
Jan 4, 2024

Conversation

fabianbormann
Copy link
Contributor

@fabianbormann fabianbormann commented Dec 1, 2022

This CIP aims to propose a decentralized communication method between dApps and wallets based on WebTorrent trackers and WebRTC.


(rendered CIP document in branch)

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@fabianbormann we have to shorten the GitHub title a bit (and remove a potentially offending special character) because of the limited space for the title we get in certain contexts.

@SebastienGllmt @KtorZ @crptmppt I wanted to check with you before also recommending that the code in /demo be moved elsewhere. There are some accountability issues with having this code in this particular repository... so unless there are any objections I'm going to recommend this be moved into another repo controlled by the author (if not there already) & linked to from the CIP document.

I also believe the spirit of the CIP process is for each proposal to focus on a standard that could have many implementations: so to have specific code here for a particular implementation creates a false impression that the implementation is the standard. 🧐

CIP-????/README.md Outdated Show resolved Hide resolved
@rphair rphair changed the title CIP-???? | Decentralized WebRTC dApp/Wallet Communication Using WebTorrent Tracker for Peer Discovery CIP-???? | Decentralized WebRTC dApp-Wallet Communication Dec 1, 2022
CIP-????/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

I would look forward to another more technical review but I can say for my own part that this has all the basics of a good design. It's relevant to Cardano community needs and the Path to Active is robust and detailed.

@fabianbormann FYI this is going to be introduced in "Triage" at the biweekly CIP meeting on Discord in case you are there to say a few words about it (cc @KtorZ). In any case I would recommend that this be promoted to a candidate CIP for more active consideration.

@KtorZ
Copy link
Member

KtorZ commented Dec 6, 2022

As discussed in today's meeting: the proposal looks well articulated, with a PoC and a clear motivation. It would be interesting to get some wallet providers involved in the discussion.

In the meantime, we'll assign a number to that proposal and mark it for editors' review after some more reviews from the community have happened.

@hazae41
Copy link

hazae41 commented Dec 12, 2022

Maybe it can also be based on libp2p pubsub

@fabianbormann
Copy link
Contributor Author

fabianbormann commented Dec 12, 2022

@hazae41 good idea. It would be great to support different methods to push the decentralization. Meanwhile I reimplemented bugout into a typescript lib that supports react (webpack 5) without eject (called Meerkat) so that the next step would be a reference implementation of the api injection in boostwallet (@jimcase is currently working on this). But I will look into libp2p pubsub in parallel and if it makes sense, add it as an alternative peer discovery method.

CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
@KtorZ KtorZ changed the title CIP-???? | Decentralized WebRTC dApp-Wallet Communication CIP-0045? | Decentralized WebRTC dApp-Wallet Communication Jan 17, 2023
CIP-????/README.md Outdated Show resolved Hide resolved
@KtorZ KtorZ added the Category: Wallets Proposals belonging to the 'Wallets' category. label Mar 18, 2023
@jehrhardt
Copy link

@fabianbormann I think the CIP is not really correct on the used term TURN server.

A TURN server is required to do hole punshing. Let's say your mobile phone is connected via 5g and your wallet is on your laptop connected via WIFI. Then your wallet cannot be reached by your phone directly over the internet. That's why both will connect to a TURN server first to open a connection to the internet, which will then be used for p2p traffic.
Actually WebRTC without TURN server would only work within the same network or with devices open to public internet.

What is meant in this CIP is a Signaling server, which you replace with a WebTorrent tracker. It is used to exchange the connection information and is independent of using a TURN server for hole punshing.

While this CIP is about establishing a connection via WebRTC, I wonder where the RPC protocol that is used between dapp and wallet is specified. I guess, it will be derived from CIP-30. But maybe it is worth to make it another CIP?

@hazae41 I thought about libp2p as well, when I read the title. But in libp2p you still need a way how wallet and dapp can establish a p2p connection. So a WebTorrent tracker could be used as well.

@OlofBlomqvist
Copy link

@fabianbormann I think the CIP is not really correct on the used term TURN server.

A TURN server is required to do hole punshing. Let's say your mobile phone is connected via 5g and your wallet is on your laptop connected via WIFI. Then your wallet cannot be reached by your phone directly over the internet. That's why both will connect to a TURN server first to open a connection to the internet, which will then be used for p2p traffic. Actually WebRTC without TURN server would only work within the same network or with devices open to public internet.

What is meant in this CIP is a Signaling server, which you replace with a WebTorrent tracker. It is used to exchange the connection information and is independent of using a TURN server for hole punshing.

While this CIP is about establishing a connection via WebRTC, I wonder where the RPC protocol that is used between dapp and wallet is specified. I guess, it will be derived from CIP-30. But maybe it is worth to make it another CIP?

@hazae41 I thought about libp2p as well, when I read the title. But in libp2p you still need a way how wallet and dapp can establish a p2p connection. So a WebTorrent tracker could be used as well.

To be a bit more precice, STUN servers are used for NAT hole-punching where ICE is not enough and TURN is used as last resort proxy relays between peers that cannot otherwise establish direct peer-to-peer connections.

The Bugout library mentioned uses Google's public STUN servers (via webtorrent->peer-connect) by default but also requires TURN servers in order to work for all cases.

Afaict this CIP does as you said only propose replacing custom signaling servers with webtorrent trackers, but we'd still rely on both STUN and TURN to exist or some peers will not be able to connect with each other.

Looking at the POC implementation for the CIP, it does indeed look like it uses webtorrent (thru meerkat).

If so it sounds a bit misleading to say in the CIP that WebTorrent trackers are used instead of STUN/TURN.. unless im missing something?

@fabianbormann
Copy link
Contributor Author

Hey @OlofBlomqvist and @jehrhardt, thanks for coming up with your feedback. We are 98% done with the implementation of the p2p wallet connector (thru meerkat as @OlofBlomqvist mentioned). We tested wifi+cellular, wifi+wifi & cellular+cellular (browser<->browser) across different countries and all of our tests worked to inject the cip30 api with webrtc and webtorrent trackers from the wallet into the dapp.

If I understood the bugout explanation correctly, then they use webtorrent trackers for the peer discovery instead of a single server which is the STUN or TURN (I thought) but if I understood it incorrectly (and if I used an incorrect wording) let's just have a quick sync meeting and iterate over the markdown file of this CIP and use the proper terms and words.

I guess the overall goal to have a lightweight wallet <-> dapp communication for mobile wallets + dapps should not be affected by that discussion as we already see that it works in our different setups.

@jehrhardt
Copy link

Hey @fabianbormann, I totally agree that it does not affect the overall goal. Just the wording in the CIP is misleading.

Your wifi+cellular test works, because meerkat is using Google's public TURN under the hood to establish the connection after using WebTorrent for the discovery. Otherwise it would technically not be possible to establish a connection between devices in both networks.

@fabianbormann
Copy link
Contributor Author

@jehrhardt & @OlofBlomqvist would be great to have a meeting with you to go a little bit deeper. Could you drop me a mail to [email protected] so that we can align on a date+time ? 😊 Or text me on Discord (abundzugamer#4587)

@fabianbormann
Copy link
Contributor Author

@jehrhardt it took a while, but I updated the CIP as discussed. Thank you very much for the productive conversation, and for your valuable suggestions! 😊

@Crypto2099
Copy link
Collaborator

Saw a message from Eternl about providing support for this standard so I decided to find it and learn more about it. Seems very interesting and a lot of exciting potential use cases. Primary amongst them being the ability to connect to a desktop or kiosk session from your mobile phone that has your wallet details.

Based on this "multi-device" bridging, I wonder if it would be good for an extension/new authority to CIP-13 here using the web+cardano: standard so that wallets integrating other QR information for things like staking or payments can easily connect and interact.

Just some rough thoughts but what about something like:

[scheme] [authority] [version] [data]
web+cardano: //connect /v1 ?hash=

web+cardano://connect/v1?hash=ABC123

(@rphair for visibility) :)

@rphair
Copy link
Collaborator

rphair commented Jul 31, 2023

thanks @Crypto2099 - yes @fabianbormann I think using URIs could be useful here, as per the recently drafted broadened grammar in CIP-13 (via #559).

@rphair
Copy link
Collaborator

rphair commented Aug 1, 2023

@fabianbormann since you seemed to endorse the suggestion in #395 (comment) we agreed at the CIP editors' meeting today to wait for you to approve or refuse the suggestion to add some kind of URI scheme to what you already have here.

If making no further additions, the consensus is that this is ready to merge as-is... but we'll wait on any further editorial review until that last question is answered.

@fabianbormann
Copy link
Contributor Author

@rphair I agree and it makes 💯% sense. I don't know if I will manage to look into it this week (as I'm on holiday with the family 😉), but I will invest some time next week and add it to the cip.

@stevenj
Copy link
Contributor

stevenj commented Aug 2, 2023

While this CIP is about establishing a connection via WebRTC, I wonder where the RPC protocol that is used between dapp and wallet is specified. I guess, it will be derived from CIP-30. But maybe it is worth to make it another CIP?

I came here to ask the same thing :)
I think we should have a way of standardizing CIP-30 + Extension APIs using Json RPC or similar serialization. Which any wallet interface can use, so that we don't need to continuously redefine the wallet API's for different non browser based connection methods.

@stevenj
Copy link
Contributor

stevenj commented Aug 2, 2023

I would be concerned about a potential MITM attack here.
What would prevent the Signalling server from directing you to an adversary, which then forwards your traffic where you expect it to go?
https://security.stackexchange.com/questions/84875/how-can-mitm-be-performed-in-webrtc

@hazae41
Copy link

hazae41 commented Aug 7, 2023

I would be concerned about a potential MITM attack here.
What would prevent the Signalling server from directing you to an adversary, which then forwards your traffic where you expect it to go?
https://security.stackexchange.com/questions/84875/how-can-mitm-be-performed-in-webrtc

Maybe with some kind authentication based on pre-exchange of information

You scan a QR code generated by the app using the wallet, then both know some information that the middle man doesn't know

They can then use this data to do a Diffie-Hellman and start an encrypted and authenticated channel

@fabianbormann
Copy link
Contributor Author

@hazae41 @stevenj We (@marcuspuchalla and I) decided to go with the solution of using an identicon in the center of the qr code. It's generated from both ids (dapp and wallet) https://github.com/fabianbormann/cardano-peer-connect/blob/main/src/DAppPeerConnect.ts#L572 .. I tried to play around with a few attack scenarios and as the communication itself goes via p2p and the qr code or identifier you are scanning/coping is the public key, it would not be able for a man in the middle to connect you to a different wallet. The identifier you've scanned/copied and the public key coming from the signed messages would be different. The mitm could not fake the signature matching to the public key that the dapp would expect from the wallet and therfore not start the handshake communication that would be necessary to establish the connection. But it would be nice if someone else could also try to attack the connection to make sure to think of as much attack vectors as possible.

@rphair
Copy link
Collaborator

rphair commented Aug 8, 2023

thanks @fabianbormann ... marking Waiting for Author due to #395 (comment) so we can give you all the time you might need for that & also for others to review security scenarios as per the more recent discussion. I'll keep a close eye on this in any case & please also post here when you think it's ready for review again 😎

@rphair rphair added the State: Waiting for Author Proposal showing lack of documented progress by authors. label Aug 8, 2023
@fabianbormann
Copy link
Contributor Author

Thanks @rphair 🙏 I started to look into CIP13 and web+cardano://connect/v1?identifier=ABC123 (as suggested by @Crypto2099) makes a lot of sense. I will add it to the Readme.md and to the cardano-peer-connect + demo application as soon as possible 😊

@fabianbormann
Copy link
Contributor Author

@rphair, I've included additional details about the identifier and the information relating to CIP13. Moving forward, my next steps will involve updating the cardano-peer-connect repository and the demo repository regarding the URI scheme. Additionally, I'll be reaching out to the Eternl team to discuss these minor changes. As of now, the CIP itself should be unblocked and won't be impacted by these activities. 😊

@rphair rphair removed the State: Waiting for Author Proposal showing lack of documented progress by authors. label Aug 17, 2023
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@fabianbormann that looks good to me. At first it appeared a requirement to submit an ABNF grammar for any CIP-13 URI form, but @SebastienGllmt @KtorZ it seems a bit harsh in hindsight to inflict that on everyone... I would rather leave it up to those extending the URI scheme (cc @Crypto2099) especially as it would depend on the complexity of the extended syntax.

So unless there are any arguments for an ABNF syntax (as there could be eventually even in the simple cases, if there were many different //authority possibilities, to help generate a general parser for URI links), in the meantime your addition passes my own review pending any objections on that subject.

Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

👌 Not sure why we haven't merged that one already. Agree with @rphair last comment.

CIP-????/README.md Outdated Show resolved Hide resolved
@rphair
Copy link
Collaborator

rphair commented Jan 4, 2024

@fabianbormann @KtorZ I'll update the status to Active in the top level README in another PR since the version of that file here is stale. Good to see this one finally on the books.

@rphair rphair merged commit b169fa6 into cardano-foundation:master Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Wallets Proposals belonging to the 'Wallets' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.