feat: allow skipping encryption and custom muxer factory in upgrader by ckousik · Pull Request #1395 · libp2p/js-libp2p

ckousik · 2022-09-27T14:30:13Z

Updates the DefaultUpgrader to use the new UpgraderOptions which allows skipping encryption and/or adding a custom muxer factory for transports which inherently support encryption and muxing.

ckousik · 2022-09-27T14:30:41Z

@mxinden @MarcoPolo I've used the new upgrader interface as discussed in discord. https://discordapp.com/channels/806902334369824788/942671304530747402/10200250514179564221

MarcoPolo · 2022-09-28T19:06:01Z

ah whoops! I didn't see this PR when I made #1396. My apologies

MarcoPolo

Can we also include the await for async newStream? https://github.com/libp2p/js-libp2p/pull/1396/files#diff-c199200bb38fc06cbe1654f84bb2cfaed1cf0aa9bc9ed71f646932604cbd9252R422

MarcoPolo · 2022-09-28T19:11:39Z

src/upgrader.ts

-      }
+      encryptedConn = protectedConn
+      if (!skipEncryption) {
+        ({


Can we make this a const declaration so we don't shadow outer vars (I think this is confusing).

src/upgrader.ts

package.json

achingbrain · 2022-09-29T13:03:57Z

src/upgrader.ts

+    // If the transport natively supports encryption, skip connection
+    // protector and encryption
+
    // Protect
    let protectedConn = maConn
-    const protector = this.components.getConnectionProtector()
+    if (!skipEncryption) {
+      const protector = this.components.getConnectionProtector()

-    if (protector != null) {
-      protectedConn = await protector.protect(maConn)
+      if (protector != null) {
+        protectedConn = await protector.protect(maConn)
+      }


We still want to do connection protection here - it's use case is a little different to encrypting the whole channel, instead you're essentially making a private network on top of the encrypted connection because you'll need the PSK to communicate any data.

That is, I can support XYX connection encryption scheme (noise, etc) and connect to a remote node, but I also need to know the PSK to connect to a remote node that uses protected connections.

This will break if we're using native muxing since there isn't a single underlying "connection". But I agree that we should do this even with native encryption. Maybe skip this if we have a custom stream muxer? fwiw on go-libp2p the QUIC transport doesn't support PSK.

@achingbrain Please correct me if I have misunderstood here: For a connection that does not have native encryption, we use the PreSharedKeyConnectionProtector to wrap the plaintext stream. Then, the selected encryption scheme runs on top of the already encrypted stream. Is this done for nodes to limit which other nodes can connect to them?

As @MarcoPolo has mentioned, we do not have a single underlying stream in the webrtc spec. Instead, we create an ephemeral data channel for the Noise handshake. Subsequent streams only use the native DTLS encryption.

In go-libp2p, we don't support PSK in QUIC or WebTransport.

We probably can't support a PSK with native encryption without giving this more thought. I think this change is okay.

See libp2p/js-libp2p-interfaces#290 (comment) - it's a small change but I think we need to split skipping connection protection out into it's own option - it makes the logic in the changes here easier to follow and also leaves the door open for transports that do their own encryption and muxing but also support PSK.

src/upgrader.ts

MarcoPolo · 2022-10-04T20:58:35Z

Can we move this out of draft @ckousik ?

ckousik · 2022-10-05T12:33:40Z

I can resolve merge conflicts after libp2p/js-libp2p-interfaces#290 is merged.

achingbrain

LGTM, some small nits and needs libp2p/js-libp2p-interfaces#290 landing first.

package.json

achingbrain · 2022-10-05T18:45:50Z

src/upgrader.ts

    // Protect
    let protectedConn = maConn
-    const protector = this.components.getConnectionProtector()
+    if ((opts?.muxerFactory) == null) {


After the suggested change in the interface repo we can make this decision on explicit settings rather than the lack of presence of an overriding muxer factory:

Suggested change

if ((opts?.muxerFactory) == null) {

if (opts.skipProtection !== true) {

achingbrain · 2022-10-05T18:46:27Z

src/upgrader.ts

-        protocol: cryptoProtocol
-      } = await this._encryptOutbound(protectedConn, remotePeerId))
+      encryptedConn = protectedConn
+      if (!opts?.skipEncryption) {


I think this is a bit easier to read. The linter will also probably complain about not handling boolean values explicitly.

Suggested change

if (!opts?.skipEncryption) {

if (opts?.skipEncryption === false) {

Wouldn't this be false if opts is undefined or opts.skipEncryption is undefined?

What I mean is that we want the default behavior to be to not skip encryption so we should do opts?.skipEncryption !== true. example

achingbrain · 2022-10-05T18:50:00Z

src/upgrader.ts

-      // Multiplex the connection
-      if (this.muxers.size > 0) {
+      upgradedConn = encryptedConn
+      if ((opts?.muxerFactory) != null) {


Doe we need the extra parentheses?

Suggested change

if ((opts?.muxerFactory) != null) {

if (opts?.muxerFactory != null) {

achingbrain · 2022-10-05T18:50:59Z

src/upgrader.ts

      upgradedConn,
      muxerFactory,
-      remotePeer
+      remotePeer,


I think this will upset the linter. You can run npm run lint locally to check.

Suggested change

remotePeer,

remotePeer

achingbrain · 2022-10-06T14:29:35Z

package.json

    "@libp2p/interface-registrar": "^2.0.3",
-    "@libp2p/interface-stream-muxer": "^2.0.2",
-    "@libp2p/interface-transport": "^1.0.3",
+    "@libp2p/interface-stream-muxer": "file:../js-libp2p-interfaces/packages/interface-stream-muxer",


Suggested change

"@libp2p/interface-stream-muxer": "file:../js-libp2p-interfaces/packages/interface-stream-muxer",

"@libp2p/interface-stream-muxer": "^3.0.0",

achingbrain · 2022-10-06T14:29:58Z

package.json

-    "@libp2p/interface-stream-muxer": "^2.0.2",
-    "@libp2p/interface-transport": "^1.0.3",
+    "@libp2p/interface-stream-muxer": "file:../js-libp2p-interfaces/packages/interface-stream-muxer",
+    "@libp2p/interface-transport": "file:../js-libp2p-interfaces/packages/interface-transport",


Suggested change

"@libp2p/interface-transport": "file:../js-libp2p-interfaces/packages/interface-transport",

"@libp2p/interface-transport": "^2.0.0",

MarcoPolo · 2022-10-06T15:04:28Z

@ckousik I'd love to help here, but I can't push to this branch.

achingbrain · 2022-10-06T15:29:33Z

I can't push to this branch.

Me neither - @ckousik I noticed the same thing about your PR to @libp2p/interfaces - could you please allow edits from maintainers on PRs in future?

ckousik · 2022-10-06T15:54:41Z

@achingbrain Should I close this PR and reopen ?

achingbrain · 2022-10-06T16:08:37Z

I think you should be able to grant permission by checking the "Allow edits by maintainers" box on the bottom right of this thread under the PR participants list. I can't see the box but I see it on PRs I have open to other repos:

MarcoPolo · 2022-10-06T21:03:52Z

I copied this to: #1411 with the requested changes.

ckousik · 2022-10-07T13:24:41Z

I think you should be able to grant permission by checking the "Allow edits by maintainers" box on the bottom right of this thread under the PR participants list. I can't see the box but I see it on PRs I have open to other repos:

@achingbrain Since the fork is owned by the little-bear-labs org, I do not get that option at all.

MarcoPolo · 2022-10-07T22:40:22Z

superseded by #1411

update upgrader interface

e280a39

MarcoPolo requested changes Sep 28, 2022

View reviewed changes

MarcoPolo mentioned this pull request Sep 28, 2022

feat: support UpgradeOptions and async newStream #1396

Closed

achingbrain requested changes Sep 29, 2022

View reviewed changes

MarcoPolo reviewed Sep 29, 2022

View reviewed changes

src/upgrader.ts Outdated Show resolved Hide resolved

ckousik added 2 commits September 30, 2022 17:01

address review

14ca807

address review

8131cf9

ckousik requested a review from MarcoPolo September 30, 2022 16:22

MarcoPolo mentioned this pull request Sep 30, 2022

Tracking Issue: road to v0.1.0 libp2p/js-libp2p-webtransport#1

Closed

12 tasks

mxinden mentioned this pull request Oct 3, 2022

webrtc/: Add libp2p WebRTC browser-to-server spec libp2p/specs#412

Merged

19 tasks

MarcoPolo approved these changes Oct 4, 2022

View reviewed changes

ckousik marked this pull request as ready for review October 5, 2022 12:32

achingbrain requested changes Oct 5, 2022

View reviewed changes

achingbrain changed the title ~~Allow skipping encryption and custom muxer factory in upgrader~~ feat: allow skipping encryption and custom muxer factory in upgrader Oct 6, 2022

achingbrain reviewed Oct 6, 2022

View reviewed changes

MarcoPolo mentioned this pull request Oct 6, 2022

feat: allow skipping encryption and custom muxer factory in upgrader #1411

Merged

MarcoPolo closed this Oct 7, 2022

	if ((opts?.muxerFactory) == null) {
	if (opts.skipProtection !== true) {

	if (!opts?.skipEncryption) {
	if (opts?.skipEncryption === false) {

	if ((opts?.muxerFactory) != null) {
	if (opts?.muxerFactory != null) {

	"@libp2p/interface-stream-muxer": "file:../js-libp2p-interfaces/packages/interface-stream-muxer",
	"@libp2p/interface-stream-muxer": "^3.0.0",

	"@libp2p/interface-transport": "file:../js-libp2p-interfaces/packages/interface-transport",
	"@libp2p/interface-transport": "^2.0.0",

Conversation

ckousik commented Sep 27, 2022

Uh oh!

ckousik commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoPolo commented Sep 28, 2022

Uh oh!

MarcoPolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MarcoPolo commented Oct 4, 2022

Uh oh!

ckousik commented Oct 5, 2022

Uh oh!

achingbrain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoPolo Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoPolo commented Oct 6, 2022

Uh oh!

achingbrain commented Oct 6, 2022

Uh oh!

ckousik commented Oct 6, 2022

Uh oh!

achingbrain commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoPolo commented Oct 6, 2022

Uh oh!

ckousik commented Oct 7, 2022

Uh oh!

MarcoPolo commented Oct 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ckousik commented Sep 27, 2022 •

edited

Loading

MarcoPolo Oct 6, 2022 •

edited

Loading

achingbrain commented Oct 6, 2022 •

edited

Loading