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

Add a total established connection limit #2137

Merged
merged 8 commits into from
Jul 30, 2021

Conversation

AgeManning
Copy link
Contributor

I think it would be useful to be able to specify a total established connection limit, rather than just incoming and outgoing limits.

If a user wants to set an incoming limit of 10 and an outgoing limit of 5, currently, this has a maximum connection limit of 15, but the ratio of incoming to outgoing is fixed. If a user wishes to specify the total and is happy to have a variable ratio of incoming/outgoing, I think this PR is useful to enable this at the Network level.

This shouldn't break anything as its an optional addition to the ConnectionLimit struct.

@AgeManning AgeManning requested a review from mxinden July 14, 2021 04:24
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

In general, I am not opposed to this change, though I want to make sure it is not used in the wrong way.

Would you mind expanding on your use-case?

When running rust-libp2p in untrusted environments, I find the differentiation in incoming and outgoing connections valuable as an eclipse-attack defense mechanism.

core/src/connection/pool.rs Outdated Show resolved Hide resolved
Co-authored-by: Max Inden <mail@max-inden.de>
@AgeManning
Copy link
Contributor Author

Yep agree inbound/outbound limits are useful for preventing eclipses.

My use-case is that we allow the user essentially to specify an amount of peers (connections) they want to connect to. With this PR we can enforce this limit at the libp2p level, preventing us from having to establish connections and performing initial behaviour tasks before dropping them.

Although the user specifies essentially a hard limit in connections, we enforce a percentage maximum of those to be inbound. I think this PR allows a state where all of the total connections are outbound. We don't allow all the total connections to be inbound however. Essentially the ratio of inbound/outbound can vary for reaching the total connection limit.

I think your point in important though. At the moment, users can choose not to define limits and be vulnerable to eclipses. Perhaps adding more info in the comment around setting a total limit might be useful.

@mxinden
Copy link
Member

mxinden commented Jul 23, 2021

Thanks for the additional context @AgeManning.

Would you mind:

  • Adding a changelog entry.
  • Documenting the importance of enforcing a limit to incoming connections to prevent eclipse attacks on either with_max_established or with_max_established_outgoing?

@AgeManning
Copy link
Contributor Author

Added your suggestions. Feel free to modify the added comment.

core/CHANGELOG.md Show resolved Hide resolved
core/src/connection/pool.rs Outdated Show resolved Hide resolved
core/src/connection/pool.rs Outdated Show resolved Hide resolved
@mxinden
Copy link
Member

mxinden commented Jul 26, 2021

Feel free to modify the added comment.

I am unable to commit suggestions to your branch. Can you either give me permissions or apply the suggestions above yourself?

AgeManning and others added 4 commits July 29, 2021 11:44
Co-authored-by: Max Inden <mail@max-inden.de>
Co-authored-by: Max Inden <mail@max-inden.de>
Co-authored-by: Max Inden <mail@max-inden.de>
@mxinden mxinden merged commit 76f1fcb into libp2p:master Jul 30, 2021
@AgeManning AgeManning deleted the total-connection-limit branch September 21, 2021 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants