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

fix(network): Limit number of peer connections per IP address, Ignore new peer connections from the same IP and port #6980

Merged
merged 7 commits into from
Jun 19, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jun 15, 2023

Motivation

This PR drops additional peer connections from a given IP after there are already MAX_CONNS_PER_IP in the peer set.

Closes #6936.

Solution

  • Define MAX_CONNS_PER_IP
  • Count the number of keys in PeerSet.ready_services and PeerSet.cancel_handles before inserting new connections into the peer set
  • Drop new connections when there are already MAX_CONNS_PER_IP

Review

This is a routine security fix.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

@arya2 arya2 added C-bug Category: This is a bug P-Medium ⚡ C-security Category: Security issues A-network Area: Network protocol updates or fixes labels Jun 15, 2023
@arya2 arya2 requested a review from a team as a code owner June 15, 2023 22:27
@arya2 arya2 self-assigned this Jun 15, 2023
@arya2 arya2 requested review from oxarbitrage and teor2345 and removed request for a team June 15, 2023 22:27
@github-actions github-actions bot added the C-feature Category: New features label Jun 15, 2023
zebra-network/src/constants.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think this change needs some tests, what do you think?

zebra-network/src/peer_set/set.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/set.rs Show resolved Hide resolved
zebra-network/src/peer_set/set.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #6980 (703a653) into main (484f3d7) will increase coverage by 0.02%.
The diff coverage is 93.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6980      +/-   ##
==========================================
+ Coverage   77.50%   77.52%   +0.02%     
==========================================
  Files         310      310              
  Lines       41534    41582      +48     
==========================================
+ Hits        32192    32238      +46     
- Misses       9342     9344       +2     

@teor2345 teor2345 changed the title fix(network): Limit number of peer connections Zebra will keep in the peer set per IP address fix(network): Limit number of peer connections Zebra will keep in the peer set per IP address, Ignore new peer connections from the same IP and port Jun 18, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks for the tests!

zebra-network/src/peer_set/set.rs Show resolved Hide resolved
@teor2345 teor2345 changed the title fix(network): Limit number of peer connections Zebra will keep in the peer set per IP address, Ignore new peer connections from the same IP and port fix(network): Limit number of peer connections per IP address, Ignore new peer connections from the same IP and port Jun 18, 2023
@teor2345 teor2345 added I-remote-node-overload Zebra can overload other nodes on the network I-remote-trigger Remote nodes can make Zebra do something bad and removed C-feature Category: New features labels Jun 18, 2023
mergify bot added a commit that referenced this pull request Jun 18, 2023
mergify bot added a commit that referenced this pull request Jun 18, 2023
@mergify mergify bot merged commit 73ce8fb into main Jun 19, 2023
@mergify mergify bot deleted the limit-peers-per-ip branch June 19, 2023 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes C-bug Category: This is a bug C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network I-remote-trigger Remote nodes can make Zebra do something bad
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zebra node establishes multiple inbound or outbound connections to the same peer
2 participants