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: Handle small numbers of initial peers better, Credit: Equilibrium #2154

Merged
merged 2 commits into from
May 17, 2021

Conversation

teor2345
Copy link
Contributor

Motivation

  1. Zebra can appear to hang if there are a small number of initial peers. (It eventually continues after 3*75 seconds, but that's a long time to wait.)

Reported by Niklas Long of Equilibrium.

  1. Zebra does not do anything if it has zero initial peers.

Solution

  • Limit the fanout on the initial candidate set update to the number of initial peers.
  • Log an explicit error if there are zero initial peers. Do not attempt DNS resolution at all.

The code in this pull request has:

  • Documentation Comments
  • Existing Integration Tests

Review

@dconnolly or @conradoplg can review. This PR blocks #1849, so it would be nice to get it in soon.

Follow Up Work

It would be nice to avoid the initial update entirely. But it's a workaround for a zcashd network protocol choice, so there's not much we can do here.

teor2345 added 2 commits May 14, 2021 12:21
Instead, log an error and return immediately.
If there is a small number of initial peers, and they are slow, the
initial candidate set update can appear to hang. To avoid this issue,
limit the initial candidate set fanout to the number of initial peers.

Once the initial peers have sent us more peer addresses, there is no need
to limit the fanouts for future updates.

Reported by Niklas Long of Equilibrium.
@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code P-Medium C-security Category: Security issues I-hang A Zebra component stops responding to requests I-usability Zebra is hard to understand or use A-network Area: Network protocol updates or fixes labels May 14, 2021
@teor2345 teor2345 added this to the 2021 Sprint 9 milestone May 14, 2021
@teor2345 teor2345 requested review from conradoplg and dconnolly May 14, 2021 02:31
@teor2345 teor2345 self-assigned this May 14, 2021
@teor2345 teor2345 changed the title Security: Handle small numbers of initial peers better, Credit: Niklas Long of Equilibrium Security: Handle small numbers of initial peers better, Credit: Equilibrium May 14, 2021
@teor2345 teor2345 enabled auto-merge (rebase) May 17, 2021 00:02
Comment on lines +70 to +72
if peers.is_empty() {
error!("no initial peers in the network config. Hint: you must configure at least one peer or DNS seeder to run Zebra");
return HashSet::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -159,7 +178,7 @@ where
// called while the peer set is already loaded.
let mut responses = FuturesUnordered::new();
trace!("sending GetPeers requests");
for _ in 0..constants::GET_ADDR_FANOUT {
for _ in 0..fanout_limit.unwrap_or(constants::GET_ADDR_FANOUT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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 A-rust Area: Updates to Rust code C-bug Category: This is a bug C-security Category: Security issues I-hang A Zebra component stops responding to requests I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants