-
Notifications
You must be signed in to change notification settings - Fork 474
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
docs: add limit configuration documentation #1421
Conversation
To help people better configure their libp2p nodes to defend against malicious peers, document all the limit settings in one place.
Instead of duplicating advice in https://github.com/libp2p/docs/blob/master/content/reference/dos-mitigation.md I thought I'd link out from there to the sections being added here - what do you think @BigLep ? |
I'm going to merge this to move the dos docs forward, there may be a follow up PR to this one with any required changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together. It's helpful to see the end customer experience. Also definitely agreed of linking to this from https://docs.libp2p.io/reference/dos-mitigation/ . No copy and paste!
A few things:
-
How do we handle transient connections? Is there a way to limit them? It looks like yes?
js-libp2p/src/connection-manager/index.ts
Line 161 in b717beb
maxIncomingPendingConnections?: number -
Sometimes I see notes about how there are default values and other times there aren't defaults. What happens in the cases where these values aren't set? Can we link to the authoritative place for defaults (e.g.,
js-libp2p/src/connection-manager/index.ts
Line 26 in b717beb
const defaultOptions: Partial<ConnectionManagerInit> = { -
Is it possible to control the max amount of data buffered across all js-libp2p connections in total? If not, is at least possible to set a max buffer limit at the connection level? I'm assuming not. It looks like
(maxInboundStreams + maxOutboundStreams) * maxStreamBufferSize
is the upper bound that a single connection could consume (if all streams appraoched the maxStreamBufferSize). I wonder if it would be easier for a user to reason about if we had a limit likemaxConnectionBuufferSize
ormaxMuxerBufferSize
. Anyways, assuming this is a no go, maybe documenting this a bit more? -
LIMITS.md answers specifics about js-libp2p. I think it would be good to link out to https://docs.libp2p.io/reference/dos-mitigation/, which give a wholistic take on DoS mitigation (potentially providing useful context).
I'll do my best to respond to feedback, but please don't block on me. Opening and merging a PR like this works for me.
* A remote peer may attempt to open up to this many connections per second, | ||
* any more than that will be automatically rejected | ||
*/ | ||
inboundConnectionThreshold: 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we're not going to change now, but given this is a "rate", I think it would be ideal to get the word "rate" into the name.
- [TCP](#tcp) | ||
- [Allow/deny lists](#allowdeny-lists) | ||
|
||
## Connection limits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to specify different limits for inbound vs. outbound?
I can imagine applications taking the posture of trusting their outbound connections (and thus no limits) but being defensive on inbound connections (and thus having limits).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at the moment, but I can see why this is something you might want to control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@achingbrain : I think this is worth backlogging. Then maybe lets add a note here like:
"There currently isn't a way to specify different limits for incoming vs. outgoing. Connection limits are applied across both incoming and outgoing connections combined. There is a backlog item for this here."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this as the first paragraph:
It's possible to limit the total amount of connections a node is able to make (combining incoming and outgoing). When this limit is reached and an attempt to open a new connection is made, existing connections may be closed to make room for the new connection (see [Closing connections][#closing-connections]).
- Note: there currently isn't a way to specify different limits for incoming vs. outgoing. Connection limits are applied across both incoming and outgoing connections combined. There is a backlog item for this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #1508
|
||
/** | ||
* If the number of open connections goes below this number, the node | ||
* will try to connect to nearby peers from the peer store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put "nearby" in quotes?
Where is "nearby" defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it'll actually sort-of-randomly select some peers from the peer store (preferring ones we have the public key for or protocol list they support), will update.
It should really choose peers that have been turned up by the periodic "findClosestPeers" query we run. "close" here is defined by the peer routing implementation, so KAD-DHT would choose KAD-close peers, etc.
Also, not required, but it wouldn't hurt to flag for @MarcoPolo in case he wants to look given his work on DoS docs and familiarity with multiple of the libp2p implementations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @BigLep that we should use the word "rate" instead of threshold when we're talking about rate limits.
Also a couple other comments.
|
||
When registering listeners for custom protocols, the maximum number of simultaneously open inbound and outbound streams per-connection can be specified. If not specified these will default to 32 inbound streams and 64 outbound streams. | ||
|
||
If more than this number of streams for the given protocol are opened on a single connection, subsequent new streams for that protocol will be immediately reset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a mechanism to coalesce connections? What happens if peer B opens 10 connections to peer A. Does peer A see that there already is a connection to peer B and close the older ones?
I'm wondering because this limit is per connection, and I'm curious why it isn't per peer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, all we can do is just close duplicate connections I think, at least until the stream migration protocol gets a bit more fleshed out.
allow: [ | ||
'/ip4/43.123.5.23/tcp/3984', | ||
'/ip4/234.243.64.2', | ||
'/ip4/52.55', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a valid multiaddr, isn't that confusing? I think we should prefer to use the standard IP cidr format (I know this is just the docs, so this will involve impl changes) e.g. "/ip4/52.55" should be "/ip4/52.55.0.0/ipcidr/16".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are treated as strings prefixes not full multiaddrs. It'd probably be more correct to make them multiaddrs and less error prone but I don't think the @multiformats/multiaddr
module will let us match address ranges.
A good improvement for the future.
Incorporate feedback from #1421
Incorporate feedback from #1421 Co-authored-by: Steve Loeppky <biglep@protocol.ai>
To help people better configure their libp2p nodes to defend against malicious peers, document all the limit settings and protections in one place.