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.

Sign up for GitHub

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

Jump to bottom

Security: Stop routing inventory requests by peer address #3090

Merged
merged 8 commits into from
Nov 24, 2021
Merged

Security: Stop routing inventory requests by peer address #3090

merged 8 commits into from
Nov 24, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 23, 2021 •
edited
Loading

Motivation

  1. The peer set uses integer-based indexing, which makes it harder to drop peers. Using address-based keys will make ticket After network upgrade activation, close existing connections to outdated peers #2262 easier to implement.

  2. The peer set routes inventory requests based on the peer address order in a HashMap. This is a security issue, because it allows a single peer to control our inventory responses. It could also overload peers by directing all the requests to them.

Solution

  • Replace peer set integer indexes with address-based indexes
    • This lets us drop the indexmap dependency, which is nice
  • Security: Stop using peer addresses to choose inventory routing order
  • Add documentation and improve PeerSet logging

Review

@jvff can review this PR, it blocks #2262.

This PR is based on PR #3091 to avoid unrelated test failures.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

We might want to do more refactors on the peer set, but we're focusing on bug fixes at the moment.

@teor2345 teor2345 added A-dependencies Area: Dependency file updates P-Medium C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-network Area: Network protocol updates or fixes labels Nov 23, 2021
@teor2345 teor2345 requested a review from jvff November 23, 2021 00:42
@teor2345 teor2345 self-assigned this Nov 23, 2021
@teor2345 teor2345 changed the title Peer set addr key Security: Stop routing inventory requests by peer address Nov 23, 2021
Base automatically changed from peer-set-task-drop to main November 23, 2021 01:29
@teor2345 teor2345 mentioned this pull request Nov 23, 2021
Merged
3 tasks
@teor2345 teor2345 changed the base branch from main to startup-slow-services-last November 23, 2021 04:23
Base automatically changed from startup-slow-services-last to main November 23, 2021 17:42
jvff
jvff previously approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@jvff jvff left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. I just added a few minor optional suggestions.

zebra-network/src/peer_set/set.rs Show resolved Hide resolved
zebra-network/src/peer_set/set.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/set.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 enabled auto-merge (squash) November 23, 2021 22:55
@teor2345
Copy link
Contributor Author

I applied the suggested minor changes, so I'm going to admin-merge this to unblock #2262.

@teor2345 teor2345 merged commit f6abb15 into main Nov 24, 2021
@teor2345 teor2345 deleted the peer-set-addr-key branch November 24, 2021 00:31
@teor2345 teor2345 mentioned this pull request Nov 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvff jvff jvff left review comments

Assignees

@teor2345 teor2345

Labels
A-dependencies Area: Dependency file updates A-network Area: Network protocol updates or fixes C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-remote-node-overload Zebra can overload other nodes on the network
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@teor2345 @jvff