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: connection gater #987

Closed

Conversation

greenSnot
Copy link
Contributor

@greenSnot
Copy link
Contributor Author

#175
#744
#769

@BigLep BigLep added the P2 Medium: Good to have, but can wait until someone steps up label Sep 24, 2021
@BigLep BigLep added the need/maintainer-input Needs input from the current maintainer(s) label Sep 24, 2021
@BigLep
Copy link
Contributor

BigLep commented Nov 12, 2021

@vasco-santos : is this desirable to land soon, and is this a PR you can look at, or is there someone else we can delegate to?

@@ -18,3 +18,39 @@ The following is a list of available options for setting limits for the Connecti
- `pollInterval`: sets the poll interval (in milliseconds) for assessing the current state and determining if this peer needs to force a disconnect. Defaults to `2000` (2 seconds).
- `movingAverageInterval`: the interval used to calculate moving averages (in milliseconds). Defaults to `60000` (1 minute). This must be an available interval configured in `Metrics`
- `defaultPeerValue`: number between 0 and 1. Defaults to 1.
- `gater`: gater options.

### Gater Options
Copy link
Member

Choose a reason for hiding this comment

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

It may be nice to have the comments from https://github.com/libp2p/go-libp2p-core/blob/master/connmgr/gater.go#L11-L43 included here or wherever is most appropriate

@@ -118,6 +124,10 @@ class Dialer {
* @returns {Promise<Connection>}
*/
async connectToPeer (peer, options = {}) {
const { id } = getPeer(peer)
if (this.connectionManager && await this.connectionManager.gater.interceptPeerDial(id)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a case when this.connectionManager will be undefined? In the upgrader, it seems like it isn't treated that way.

@@ -180,10 +190,12 @@ class Dialer {
const { id, multiaddrs } = getPeer(peer)

if (multiaddrs) {
this.peerStore.addressBook.add(id, multiaddrs)
const filteredMultiaddrs = await all(filter(multiaddrs, async (multiaddr) => !(this.connectionManager && await this.connectionManager.gater.interceptAddrDial(id, multiaddr))))
this.peerStore.addressBook.add(id, filteredMultiaddrs)
Copy link
Member

Choose a reason for hiding this comment

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

Should the gater be called here?
The intercept below (L198) will filter addrs that are attempting to be dialed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To prevent malicious multiaddrs from adding to addressBook

if (await this.connectionManager.gater.interceptSecured('outbound', remotePeer, encryptedConn)) {
throw errCode(new Error('The multiaddr connection is blocked by gater.interceptSecured'), codes.ERR_CONNECTION_INTERCEPTED)
}

Copy link
Member

Choose a reason for hiding this comment

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

interceptUpgraded is needed for outbound too

@greenSnot
Copy link
Contributor Author

Too many conflicts from master, may postpone until next month.

@lidel lidel assigned wemeetagain and unassigned vasco-santos Jan 21, 2022
@lidel lidel marked this pull request as draft January 21, 2022 16:03
@lidel lidel changed the title Feat/connection gater feat: connection gater Jan 21, 2022
@achingbrain
Copy link
Member

Continued in #1142 since this PR does not allow maintainer commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/maintainer-input Needs input from the current maintainer(s) P2 Medium: Good to have, but can wait until someone steps up
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants