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

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

Closed

Conversation

ckousik
Copy link
Contributor

@ckousik ckousik commented Sep 27, 2022

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
Copy link
Contributor Author

ckousik commented Sep 27, 2022

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

@MarcoPolo
Copy link
Collaborator

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

Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

}
encryptedConn = protectedConn
if (!skipEncryption) {
({
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

src/upgrader.ts Outdated Show resolved Hide resolved
src/upgrader.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/upgrader.ts Outdated
Comment on lines 263 to 273
// 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)
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/upgrader.ts Outdated Show resolved Hide resolved
@MarcoPolo
Copy link
Collaborator

Can we move this out of draft @ckousik ?

@ckousik ckousik marked this pull request as ready for review October 5, 2022 12:32
@ckousik
Copy link
Contributor Author

ckousik commented Oct 5, 2022

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

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

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

package.json Show resolved Hide resolved
// Protect
let protectedConn = maConn
const protector = this.components.getConnectionProtector()
if ((opts?.muxerFactory) == null) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@MarcoPolo MarcoPolo Oct 6, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Doe we need the extra parentheses?

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

@@ -318,7 +330,7 @@ export class DefaultUpgrader extends EventEmitter<UpgraderEvents> implements Upg
maConn,
upgradedConn,
muxerFactory,
remotePeer
remotePeer,
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
remotePeer,
remotePeer

@achingbrain 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
@@ -115,8 +115,8 @@
"@libp2p/interface-peer-store": "^1.2.1",
"@libp2p/interface-pubsub": "^2.0.1",
"@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",
Copy link
Member

Choose a reason for hiding this comment

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

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

"@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",
Copy link
Member

Choose a reason for hiding this comment

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

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

@MarcoPolo
Copy link
Collaborator

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

@achingbrain
Copy link
Member

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
Copy link
Contributor Author

ckousik commented Oct 6, 2022

@achingbrain Should I close this PR and reopen ?

@achingbrain
Copy link
Member

achingbrain commented Oct 6, 2022

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:

image

@MarcoPolo
Copy link
Collaborator

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

@ckousik
Copy link
Contributor Author

ckousik commented Oct 7, 2022

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:

image

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

@MarcoPolo
Copy link
Collaborator

superseded by #1411

@MarcoPolo MarcoPolo closed this Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants