feat: deny incoming connections and add allow/deny lists #1398
Conversation
When we reach our connection limit, deny incoming connections before the peer ids are exchanged instead of accepting them and trying to apply the limits after the exchange. Adds a allow and deny lists of multiaddr prefixes to ensure we can always accept connections from a given network host even when we have reached out connection limit.
| const connections = this.getConnections() | ||
|
|
||
| if (connections.length <= this.opts.minConnections || toPrune < 1) { |
There was a problem hiding this comment.
Is this handled elsewhere?
There was a problem hiding this comment.
The method prunes the passed number of connections, previously it only did so if we were over the connection limit. We initiate pruning for a number of reasons - if the event loop is lagging too much or we are sending or receiving too much traffic for example - pruning in these cases would have done nothing if we weren't over the connection limit.
| @@ -152,6 +164,8 @@ export class DefaultConnectionManager extends EventEmitter<ConnectionManagerEven | |||
| private readonly startupReconnectTimeout: number | |||
| private connectOnStartupController?: TimeoutController | |||
| private readonly dialTimeout: number | |||
| private readonly allow: Multiaddr[] | |||
There was a problem hiding this comment.
Would it make sense to use a Set here?
There was a problem hiding this comment.
I don't think so because we call .some on it in order to see if the connecting peer's multiaddr is prefixed by anything in the allow list - to do that with a Set we'd have to convert it to an array first.
We could have a separate function that iterates over the Set with a for..of doing the check that returns early, but .some is a one-liner that does the same thing so there seems little benefit?
|
|
||
| if (this.getConnections().length < this.opts.maxConnections) { | ||
| return | ||
| if (maConn.remoteAddr.isThinWaistAddress()) { |
There was a problem hiding this comment.
Is this just checking ip{4,6} + {tcp,udp}? Where does this name come from?
There was a problem hiding this comment.
Is this just checking ip{4,6} + {tcp,udp}
Yes.
Where does this name come from?
I'm actually not sure. I think it refers to just IP + port, no other protocol information - https://www.oilshell.org/cross-ref.html?tag=narrow-waist#narrow-waist
| } | ||
|
|
||
| log('maxConnections exceeded') | ||
| return false |
There was a problem hiding this comment.
The caller no longer knows why this connection wasn't accepted. Probably okay, but maybe better to return an error?
There was a problem hiding this comment.
It logs why the connection was accepted - it's for incoming connections only so it's really just informational for anyone watching the logs.
More idiomatic to throw than return an error but after the conversation here I changed it to return a boolean instead.
Temporary fix until #1398 is merged.
Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>
| @@ -31,7 +32,8 @@ const defaultOptions: Partial<ConnectionManagerInit> = { | |||
| maxEventLoopDelay: Infinity, | |||
| pollInterval: 2000, | |||
| autoDialInterval: 10000, | |||
| movingAverageInterval: 60000 | |||
| movingAverageInterval: 60000, | |||
| inboundConnectionThreshold: 5 | |||
There was a problem hiding this comment.
This will deny any host not in the allow list that tries to open more than 5x connections a second. I plucked the number out of thin air, is it reasonable?
When we reach our connection limit, deny incoming connections before the peer ids are exchanged instead of accepting them and trying to apply the limits after the exchange.
Adds a allow and deny lists of multiaddr prefixes to ensure we can always accept connections from a given network host even when we have reached out connection limit.
Outgoing behaviour remains the same, that is, you can exceed the limit while opening new connections and the connection manager will try to close 'low value' connections after the new connection has been made.
Depends on: