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

security: Rate limit GetAddr responses #7955

Merged
merged 26 commits into from
Nov 21, 2023
Merged

security: Rate limit GetAddr responses #7955

merged 26 commits into from
Nov 21, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Nov 16, 2023

Motivation

We want to avoid giving out Zebra's entire address book over a short duration.

Closes #7823.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Solution

  • Updates ADDR_RESPONSE_LIMIT_DENOMINATOR to 4
  • Adds a 10 minute INBOUND_CACHED_ADDRS_REFRESH_INTERVAL constant
  • Adds a CachedPeerAddrs struct with a reference to the address book
  • Replaces the address_book field in Setup::Initialized with an instance of CachedPeerAddrs
  • Refreshes the cached addresses in the inbound service's poll_ready() method if they are stale
  • Returns cached_addrs in response to inbound::Peers requests instead of reading from the address book
  • Logs a warning if it hasn't been able to get a lock on the address book to update the cached_addrs in an unexpectedly long time

Related cleanups:

  • Moves logic for truncating list of peers for GetAddr response to a sanitized_window() method on AddressBook.

Testing

TODO: This PR still needs tests.

Review

Anyone can review.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

@arya2 arya2 added P-Medium ⚡ C-security Category: Security issues A-network Area: Network protocol updates or fixes I-remote-trigger Remote nodes can make Zebra do something bad labels Nov 16, 2023
@arya2 arya2 self-assigned this Nov 16, 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.

This looks good, I think it would be enough to just test that we get peers when we send an address message.

zebra-network/src/address_book.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound.rs Outdated Show resolved Hide resolved
zebra-network/src/constants.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound.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 we might be over-complicating this change a bit. Just change the constant?

zebra-network/src/constants.rs Outdated Show resolved Hide resolved
zebra-network/src/constants.rs Outdated Show resolved Hide resolved
teor2345
teor2345 previously approved these changes Nov 19, 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.

Nothing here is blocking, so we're good to go when you're happy with the remaining test and module changes.

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.

From the CI test results, it looks like some more test or production updates are needed.

@arya2
Copy link
Contributor Author

arya2 commented Nov 20, 2023

From the CI test results, it looks like some more test or production updates are needed.

Yeah, that real_peer_set test seems to be broken in CI (but works locally).

I think this needs another test too.

@teor2345
Copy link
Contributor

The docker CI failure is #7898

@arya2 arya2 marked this pull request as ready for review November 20, 2023 23:26
@arya2 arya2 requested a review from a team as a code owner November 20, 2023 23:26
@arya2 arya2 requested review from teor2345 and removed request for a team November 20, 2023 23:26
@arya2
Copy link
Contributor Author

arya2 commented Nov 20, 2023

I used up the Github actions limit with frequent commits. 😞

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 all the changes, this looks good for now

@mergify mergify bot merged commit 5e4c0f9 into main Nov 21, 2023
104 checks passed
@mergify mergify bot deleted the rate-limit-getaddr branch November 21, 2023 03:32
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-security Category: Security issues I-remote-trigger Remote nodes can make Zebra do something bad
Projects
None yet
Development

Successfully merging this pull request may close these issues.

security: Rate limit GetAddr responses. Credit: Ziggurat Team
2 participants