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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e280a39
update upgrader interface
ckousik Sep 26, 2022
14ca807
address review
ckousik Sep 30, 2022
8131cf9
address review
ckousik Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-transport": "file:../js-libp2p-interfaces/packages/interface-transport",
achingbrain marked this conversation as resolved.
Show resolved Hide resolved
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",

"@libp2p/interfaces": "^3.0.3",
"@libp2p/logger": "^2.0.1",
"@libp2p/multistream-select": "^3.0.0",
Expand Down
60 changes: 36 additions & 24 deletions src/upgrader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type { MultiaddrConnection, Connection, Stream } from '@libp2p/interface-
import type { ConnectionEncrypter, SecuredConnection } from '@libp2p/interface-connection-encrypter'
import type { StreamMuxer, StreamMuxerFactory } from '@libp2p/interface-stream-muxer'
import type { PeerId } from '@libp2p/interface-peer-id'
import type { Upgrader, UpgraderEvents } from '@libp2p/interface-transport'
import type { Upgrader, UpgraderEvents, UpgraderOptions } from '@libp2p/interface-transport'
import type { Duplex } from 'it-stream-types'
import { Components, isInitializable } from '@libp2p/components'
import type { AbortOptions } from '@libp2p/interfaces'
Expand Down Expand Up @@ -228,7 +228,7 @@ export class DefaultUpgrader extends EventEmitter<UpgraderEvents> implements Upg
/**
* Upgrades an outbound connection
*/
async upgradeOutbound (maConn: MultiaddrConnection): Promise<Connection> {
async upgradeOutbound (maConn: MultiaddrConnection, opts?: UpgraderOptions): Promise<Connection> {
const idStr = maConn.remoteAddr.getPeerId()
if (idStr == null) {
throw errCode(new Error('outbound connection must have a peer id'), codes.ERR_INVALID_MULTIADDR)
Expand Down Expand Up @@ -258,39 +258,51 @@ export class DefaultUpgrader extends EventEmitter<UpgraderEvents> implements Upg

log('Starting the outbound connection upgrade')

// If the transport natively supports encryption, skip connection
// protector and encryption

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

const protector = this.components.getConnectionProtector()

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

try {
// Encrypt the connection
({
conn: encryptedConn,
remotePeer,
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 •
edited
Loading

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

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

conn: encryptedConn,
remotePeer,
protocol: cryptoProtocol
} = await this._encryptOutbound(protectedConn, remotePeerId))

if (await this.components.getConnectionGater().denyOutboundEncryptedConnection(remotePeer, {
...protectedConn,
...encryptedConn
})) {
throw errCode(new Error('The multiaddr connection is blocked by gater.acceptEncryptedConnection'), codes.ERR_CONNECTION_INTERCEPTED)
if (await this.components.getConnectionGater().denyOutboundEncryptedConnection(remotePeer, {
...protectedConn,
...encryptedConn
})) {
throw errCode(new Error('The multiaddr connection is blocked by gater.acceptEncryptedConnection'), codes.ERR_CONNECTION_INTERCEPTED)
}
} else {
cryptoProtocol = 'native'
remotePeer = remotePeerId
}

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

muxerFactory = opts.muxerFactory
} else if (this.muxers.size > 0) {
// Multiplex the connection
const multiplexed = await this._multiplexOutbound({
...protectedConn,
...encryptedConn
}, this.muxers)
muxerFactory = multiplexed.muxerFactory
upgradedConn = multiplexed.stream
} else {
upgradedConn = encryptedConn
}
} catch (err: any) {
log.error('Failed to upgrade outbound connection', err)
Expand All @@ -307,7 +319,7 @@ export class DefaultUpgrader extends EventEmitter<UpgraderEvents> implements Upg

if (metrics != null) {
metrics.updatePlaceholder(proxyPeer, remotePeer)
setPeer(remotePeer)
setPeer(remotePeerId)
}

log('Successfully upgraded outbound connection')
Expand All @@ -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

})
}

Expand Down Expand Up @@ -411,7 +423,7 @@ export class DefaultUpgrader extends EventEmitter<UpgraderEvents> implements Upg
}

log('%s: starting new stream on %s', direction, protocols)
const muxedStream = muxer.newStream()
const muxedStream = await muxer.newStream()
const metrics = this.components.getMetrics()
let controller: TimeoutController | undefined

Expand Down Expand Up @@ -609,7 +621,7 @@ export class DefaultUpgrader extends EventEmitter<UpgraderEvents> implements Upg
* Selects one of the given muxers via multistream-select. That
* muxer will be used for all future streams on the connection.
*/
async _multiplexOutbound (connection: MultiaddrConnection, muxers: Map<string, StreamMuxerFactory>): Promise<{ stream: Duplex<Uint8Array>, muxerFactory?: StreamMuxerFactory}> {
async _multiplexOutbound (connection: MultiaddrConnection, muxers: Map<string, StreamMuxerFactory>): Promise<{stream: Duplex<Uint8Array>, muxerFactory?: StreamMuxerFactory}> {
const protocols = Array.from(muxers.keys())
log('outbound selecting muxer %s', protocols)
try {
Expand All @@ -629,7 +641,7 @@ export class DefaultUpgrader extends EventEmitter<UpgraderEvents> implements Upg
* Registers support for one of the given muxers via multistream-select. The
* selected muxer will be used for all future streams on the connection.
*/
async _multiplexInbound (connection: MultiaddrConnection, muxers: Map<string, StreamMuxerFactory>): Promise<{ stream: Duplex<Uint8Array>, muxerFactory?: StreamMuxerFactory}> {
async _multiplexInbound (connection: MultiaddrConnection, muxers: Map<string, StreamMuxerFactory>): Promise<{stream: Duplex<Uint8Array>, muxerFactory?: StreamMuxerFactory}> {
const protocols = Array.from(muxers.keys())
log('inbound handling muxers %s', protocols)
try {
Expand Down