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

Add Secure WebSockets example #930

Merged
merged 2 commits into from
Jun 11, 2021
Merged

Add Secure WebSockets example #930

merged 2 commits into from
Jun 11, 2021

Conversation

D4nte
Copy link
Contributor

@D4nte D4nte commented Apr 28, 2021

Adding an example on how to use wss as it seems possible.

@D4nte D4nte mentioned this pull request Apr 28, 2021
@vasco-santos
Copy link
Member

The problem is in here: https:/libp2p/js-libp2p-websockets/blob/master/src/listener.js#L71

When we "reconstruct" the addresses that we are listening on in websockets transport, we have ws hardcoded, which means it will return the peer address with ws instead of wss. As a result, when you use libp2p.multiaddrs to add the address to the peerStore, the address used will not be the wss. In the websockets ssl test, the multiaddr with wss is used and that is why it works.

We should in websockets get the multiaddr component of ws or wss instead of hardcoding ws. Would you like to get a PR to fix this?

@lidel
Copy link
Member

lidel commented May 31, 2021

(triage note) this looks good, but needs libp2p/js-libp2p-websockets#130 to ship first.

@lidel lidel added status/blocked Unable to be worked further until needs are met and removed need/author-input Needs input from the original author labels May 31, 2021
@D4nte
Copy link
Contributor Author

D4nte commented Jun 1, 2021

The problem is in here: https:/libp2p/js-libp2p-websockets/blob/master/src/listener.js#L71

When we "reconstruct" the addresses that we are listening on in websockets transport, we have ws hardcoded, which means it will return the peer address with ws instead of wss. As a result, when you use libp2p.multiaddrs to add the address to the peerStore, the address used will not be the wss. In the websockets ssl test, the multiaddr with wss is used and that is why it works.

We should in websockets get the multiaddr component of ws or wss instead of hardcoding ws. Would you like to get a PR to fix this?

Hi @vasco-santos, sorry for the lack of response and thanks for the PR!

@lidel lidel added the P3 Low: Not priority right now label Jun 7, 2021
@lidel lidel requested review from yusefnapora and removed request for yusefnapora June 7, 2021 14:30
@vasco-santos
Copy link
Member

@D4nte we just shipped [email protected] fixing this. Mind updating the package.json and retry this?

@D4nte
Copy link
Contributor Author

D4nte commented Jun 9, 2021

Looks much better, thanks!

Unfortunately it fails due to the certificate being self-signed:

4.js
node 1 is listening on:
/ip4/127.0.0.1/tcp/10000/wss/p2p/QmRSqcpLbMk1ThEm8v83jSVBvXxi5DnXMMCC9RApKfH9es
node 2 is listening on:
/home/froyer/src/libp2p/js-libp2p/node_modules/p-some/index.js:31
                        reject(new AggregateError(errors));
                               ^

AggregateError: 
    Error: self signed certificate
        at TLSSocket.onConnectSecure (node:_tls_wrap:1541:34)
        at TLSSocket.emit (node:events:365:28)
        at TLSSocket._finishInit (node:_tls_wrap:952:8)
        at TLSWrap.ssl.onhandshakedone (node:_tls_wrap:723:12)
    at maybeSettle (/home/froyer/src/libp2p/js-libp2p/node_modules/p-some/index.js:31:11)
    at /home/froyer/src/libp2p/js-libp2p/node_modules/p-some/index.js:69:23
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

I am basing my test on the one in libp2p-websocket.
I assumed that rejectUnauthorized: false allows self-signed certificate:

https:/libp2p/js-libp2p-websockets/blob/62a3276b8398669f4d4a95c2a869d878033d91ac/test/node.js#L322

But it does not seem to be the case as I tried both:

const { stream } = await node2.dialProtocol(node1.peerId, '/print', { websocket: { rejectUnauthorized: false } })

and

  const { stream } = await node2.dialProtocol(node1.peerId, '/print', { rejectUnauthorized: false })

What am I missing?

@vasco-santos
Copy link
Member

@D4nte the problem here is the following:

By default, when libp2p discovers a peer (and has less than the minimum connections configured in connMgr) it will attempt to dial the peer. This way, when node2.peerStore.addressBook.set(node1.peerId, node1.multiaddrs) happens a dial will be triggered. This dial does not include the rejectUnauthorized: false.

Consequently, when you trigger the dial manually const { stream } = await node2.dialProtocol(node1.peerId, '/print', { websocket: { rejectUnauthorized: false } }), the previous dial already started and libp2p identifies that a dial is already in progress for this target peer. So, it will just "merge" both dials, with the second waiting for the result of the first.

This should not happen in production, as a self signed certificate should not happen. There are two solutions for this example:

  1. Disable autoDial in libp2p configuration:
config: {
  peerDiscovery: {
    autoDial: false
  }
}
  1. Do not add to the peerStore and dial the actual address directly
const targetAddr = `${node1.multiaddrs[0]}/p2p/${node1.peerId.toB58String()}`
const { stream } = await node2.dialProtocol(targetAddr, '/print', { websocket: { rejectUnauthorized: false } })

Probably the second solution is cleaner, given that we do not change the default config of libp2p

@D4nte
Copy link
Contributor Author

D4nte commented Jun 11, 2021

Actually, the only way for the test to work is to disable the autoDial. I tried solution 2 by itself but as part of the dialProtocol, the peer gets added to the book and the autodial kicks off before the manual dial, making the test fail. Peer added here:

this.peerStore.addressBook.add(id, multiaddrs)

@D4nte D4nte marked this pull request as ready for review June 11, 2021 01:34
@vasco-santos
Copy link
Member

Actually, the only way for the test to work is to disable the autoDial. I tried solution 2 by itself but as part of the dialProtocol, the peer gets added to the book and the autodial kicks off before the manual dial, making the test fail. Peer added here:

That is true, good catch. We can live with that for now. One of the plans with #744 is to get rid of this autoDial strategy and have the node be more intelligent on finding out the nodes it should connect to.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the example and to report the issue

@vasco-santos vasco-santos merged commit cd152f1 into libp2p:master Jun 11, 2021
@D4nte D4nte deleted the wss branch June 15, 2021 05:38
@D4nte
Copy link
Contributor Author

D4nte commented Jan 20, 2022

@vasco-santos looking back at this, is it possible to pass { rejectUnauthorized: false } to websocket when creating the libp2p instance? So that the autodial or libp2p-bootstrap can pick it up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Low: Not priority right now status/blocked Unable to be worked further until needs are met
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants