Security: only apply the outbound connection rate-limit to actual connections #2278

teor2345 · 2021-06-10T06:56:09Z

Motivation

Zebra advances the next peer timer, even when there are no eligible peers. (And the method is about to return None.)

This creates large peer wait times when there are no eligible peers. This can be a significant issue on testnet.

Solution

Only advance the timer when we're actually going to open a connection
Test that there is no delay when there are no candidate peers
Test that the maximum delay is bounded

Review

@jvff can review this PR.

Reviewer Checklist

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

Previously, we were advancing the timer even when we returned `None`. This created large wait times when there were no eligible peers.

zebra-network/src/peer_set/candidate_set.rs

dconnolly · 2021-06-11T20:52:34Z

@jvff would you mind adding a proptest to catch this issue?

If next() returns None, the sleep timer shouldn't be advanced at all.

Is that proptest a blocker to this merging?

Also refactor peer numbers into constants.

Also cleanup the test constants.

teor2345 · 2021-06-13T07:21:30Z

@jvff would you mind adding a proptest to catch this issue?
If next() returns None, the sleep timer shouldn't be advanced at all.

Is that proptest a blocker to this merging?

This is my third attempt at implementing/reviewing this rate-limit, so I need better tests for it.
(I've added two different proptests to this PR. So we should be good if this PR reliably passes CI.)

I have an idea for one more proptest, but it needs to done after PR #2275 merges to avoid conflicts. The existing proptests put pretty strict bounds on the wait time. So I don't think we need extra tests any time soon.

macOS VMs seem to need this extra time to pass their tests.

This change avoids test failures due to cumulative errors. Also use a single call to `Instant::now` for each test round. And print the times when the tests fail.

The candidate set proptests will fail if enough generated peers are invalid for outbound connections.

teor2345 · 2021-06-13T08:55:49Z

zebra-network/proptest-regressions/peer_set/candidate_set/tests/prop.txt

+cc e27ea62986a1ace4e16dde4b99d8ae8557a6b1e92e3f9a7c6aba0d7b179eb817 # shrinks to peers = [MetaAddr { addr: 0.0.0.0:0, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:0, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:1151, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 129.65.108.101:9019, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 95.68.199.21:46887, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 167.206.253.131:43927, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 160.138.155.239:63430, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: [6a4e:a543:8c93:e074:8a2a:5cc4:96f8:ce95]:51071, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }], initial_candidates = 0, extra_candidates = 2
+cc 788fe2c47a4af559e9a921764d1ce8fcfd0743361c19e6e7a06adc093bc19078 # shrinks to peers = [MetaAddr { addr: [::e9:8050:dbf7:5665]:27188, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 106.27.232.177:51742, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 127.0.0.1:30827, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:21948, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:38186, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: [4db2:96e6:38c5:fd62:e964:9338:4ce8:bfa0]:32686, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:22952, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 114.164.73.233:10203, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }], initial_candidates = 3, extra_candidates = 3


Addresses containing either:

IPv4: 0.0.0.0

IPv6: ::

Port: 0

Are invalid for outbound connections.

I did about 5000 runs of this test overnight, seems like the generated test cases are (almost) all valid now.

teor2345 · 2021-06-13T09:15:10Z

Coverage failed with:

warning: spurious network error (2 tries remaining): failed to get 200 response from https://crates.io/api/v1/crates/anyhow/1.0.40/download, got 502
warning: spurious network error (1 tries remaining): failed to get 200 response from https://crates.io/api/v1/crates/anyhow/1.0.40/download, got 502
error: failed to download from https://crates.io/api/v1/crates/anyhow/1.0.40/download

Caused by:
failed to get 200 response from https://crates.io/api/v1/crates/anyhow/1.0.40/download, got 502

This seems to be happening a bit lately. It would be good to wait between retries, but there are no retry or timeout options on the action:
https://github.com/actions-rs/toolchain#inputs

So this likely needs a cargo or actions-rs/toolchain code fix.

teor2345 · 2021-06-13T09:17:31Z

zebra-network/src/meta_addr/arbitrary.rs

+    pub fn alternate_strategy() -> BoxedStrategy<Self> {
+        (canonical_socket_addr(), any::<PeerServices>())
+            .prop_map(|(socket_addr, services)| MetaAddr::new_alternate(&socket_addr, &services))
+            .boxed()
+    }


Fix the alternate strategy so it returns the full range of possible service bits. (Node and client.)

teor2345 · 2021-06-13T09:18:12Z

zebra-network/src/meta_addr/arbitrary.rs

+                //
+                // TODO: create a "Zebra supported services" constant
+                let meta_addr = MetaAddr::new_alternate(&address, &PeerServices::NODE_NETWORK);
+                if meta_addr.is_valid_for_outbound() {


Create a specific strategy for valid and ready outbound peers.

This was the missing check in the previous code:

if meta_addr.is_valid_for_outbound() {

teor2345 · 2021-06-13T09:19:19Z

zebra-network/src/peer_set/candidate_set.rs

+        // SECURITY: rate-limit new outbound peer connections
+        (&mut self.wait_next_handshake).await;
+        let mut sleep = sleep(constants::MIN_PEER_CONNECTION_INTERVAL);
+        mem::swap(&mut self.wait_next_handshake, &mut sleep);


Two fixes:

only wait if we're returning Some

schedule the next sleep time after the previous sleep returns, avoiding issues with overlapping times

teor2345 · 2021-06-13T09:21:00Z

zebra-network/src/peer_set/candidate_set/tests/prop.rs

+        // Allow enough time for the maximum number of candidates,
+        // plus some extra time for test machines with high CPU load.
+        // (The max delay asserts usually fail before hitting this timeout.)
+        let max_rate_limit_sleep = 3 * MAX_TEST_CANDIDATES * MIN_PEER_CONNECTION_INTERVAL;
+        let max_extra_delay = (2 * MAX_TEST_CANDIDATES + 1) * MAX_SLEEP_EXTRA_DELAY;
+        assert!(runtime.block_on(timeout(max_rate_limit_sleep + max_extra_delay, checks)).is_ok());


Increase the test timeout to match any increases in the candidates or times.
(Currently it's 10.2 seconds rather than 10 seconds!)

Also use named constants rather than integer literals.

teor2345 · 2021-06-13T09:22:48Z

zebra-network/src/peer_set/candidate_set/tests/prop.rs

-    let mut minimum_reconnect_instant = Instant::now();
+    let mut now = Instant::now();
+    let mut minimum_reconnect_instant = now;
+    // Allow extra time for test machines with high CPU load
+    let mut maximum_reconnect_instant = now + MAX_SLEEP_EXTRA_DELAY;

    for _ in 0..candidates {
-        assert!(candidate_set.next().await.is_some());
-        assert!(Instant::now() >= minimum_reconnect_instant);
-
-        minimum_reconnect_instant += MIN_PEER_CONNECTION_INTERVAL;
+        assert!(
+            candidate_set.next().await.is_some(),
+            "there are enough available candidates"
+        );
+
+        now = Instant::now();
+        assert!(
+            now >= minimum_reconnect_instant,
+            "all candidates should obey the minimum rate-limit: now: {:?} min: {:?}",
+            now,
+            minimum_reconnect_instant,
+        );
+        assert!(
+            now <= maximum_reconnect_instant,
+            "rate-limited candidates should not be delayed too long: now: {:?} max: {:?}. Hint: is the test machine overloaded?",
+            now,
+            maximum_reconnect_instant,
+        );
+
+        minimum_reconnect_instant = now + MIN_PEER_CONNECTION_INTERVAL;
+        maximum_reconnect_instant = now + MIN_PEER_CONNECTION_INTERVAL + MAX_SLEEP_EXTRA_DELAY;


Two test changes:

add a maximum time that's a second after the minimum time (unfortunately macOS tends to fail on shorter times)

get the value of now once per loop iteration

teor2345 · 2021-06-13T09:24:23Z

zebra-network/src/peer_set/candidate_set/tests/prop.rs

+    /// TODO: after PR #2275 merges, add a similar proptest,
+    ///       using a "not ready for attempt" peer generation strategy
+    #[test]
+    fn skipping_outbound_peer_connection_skips_rate_limit(next_peer_attempts in 0..TEST_ADDRESSES) {


The skipping_outbound_peer_connection_skips_rate_limit test checks the bug fix in the code.

(The other test is useful for making sure we're not delaying way too long when we actually have peers, but it can't be too strict due to OS scheduling delays.)

…nections (#2278) * Only advance the outbound connection timer when it returns an address Previously, we were advancing the timer even when we returned `None`. This created large wait times when there were no eligible peers. * Refactor to avoid overlapping sleep timers * Add a maximum next peer delay test Also refactor peer numbers into constants. * Make the number of proptests overridable by the standard env var Also cleanup the test constants. * Test that skipping peer connections also skips their rate limits * Allow an extra second after each sleep on loaded machines macOS VMs seem to need this extra time to pass their tests. * Restart test time bounds from the current time This change avoids test failures due to cumulative errors. Also use a single call to `Instant::now` for each test round. And print the times when the tests fail. * Stop generating invalid outbound peers in proptests The candidate set proptests will fail if enough generated peers are invalid for outbound connections.

Only advance the outbound connection timer when it returns an address

05a3673

Previously, we were advancing the timer even when we returned `None`. This created large wait times when there were no eligible peers.

teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code I-slow Problems with performance or responsiveness A-network Area: Network protocol updates or fixes labels Jun 10, 2021

teor2345 added this to the 2021 Sprint 11 - Zcon2 milestone Jun 10, 2021

teor2345 requested a review from jvff June 10, 2021 06:56

teor2345 assigned jvff Jun 10, 2021

zfnd-bot bot assigned teor2345 Jun 10, 2021

teor2345 force-pushed the only-rate-limit-actual-handshakes branch from e8e2f23 to 05a3673 Compare June 10, 2021 06:56

teor2345 changed the base branch from dynamic-local-listener-to-peers to main June 10, 2021 06:56

This comment has been minimized.

Sign in to view

dconnolly reviewed Jun 11, 2021

View reviewed changes

zebra-network/src/peer_set/candidate_set.rs Show resolved Hide resolved

dconnolly previously approved these changes Jun 11, 2021

View reviewed changes

teor2345 added 4 commits June 13, 2021 16:14

Refactor to avoid overlapping sleep timers

4fec4cd

Add a maximum next peer delay test

bb422f2

Also refactor peer numbers into constants.

Make the number of proptests overridable by the standard env var

ceab84a

Also cleanup the test constants.

Test that skipping peer connections also skips their rate limits

e9cfed4

teor2345 dismissed dconnolly’s stale review via e9cfed4 June 13, 2021 07:12

teor2345 unassigned jvff Jun 13, 2021

teor2345 added 3 commits June 13, 2021 18:01

Allow an extra second after each sleep on loaded machines

131fe4d

macOS VMs seem to need this extra time to pass their tests.

Restart test time bounds from the current time

20d8f4b

This change avoids test failures due to cumulative errors. Also use a single call to `Instant::now` for each test round. And print the times when the tests fail.

Stop generating invalid outbound peers in proptests

3e7d29e

The candidate set proptests will fail if enough generated peers are invalid for outbound connections.

teor2345 commented Jun 13, 2021

View reviewed changes

teor2345 requested a review from dconnolly June 14, 2021 01:08

mpguerra removed this from the 2021 Sprint 11 - Zcon2 milestone Jun 14, 2021

jvff approved these changes Jun 14, 2021

View reviewed changes

teor2345 changed the title ~~Only apply the outbound connection rate-limit to actual connections~~ Security: only apply the outbound connection rate-limit to actual connections Jun 14, 2021

teor2345 added I-usability Zebra is hard to understand or use I-hang A Zebra component stops responding to requests labels Jun 14, 2021

teor2345 merged commit 86f23f7 into main Jun 14, 2021

teor2345 deleted the only-rate-limit-actual-handshakes branch June 14, 2021 22:29

teor2345 mentioned this pull request Jun 15, 2021

Security: stop gossiping failure and attempt times as last_seen times #2273

Merged

3 tasks

teor2345 mentioned this pull request Jun 15, 2021

Security: Stop disconnecting from nodes that send unexpected messages, to prevent disconnection attacks, Credit: Equilibrium #2107

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security: only apply the outbound connection rate-limit to actual connections #2278

Security: only apply the outbound connection rate-limit to actual connections #2278

teor2345 commented Jun 10, 2021 •

edited

Loading

This comment has been minimized.

dconnolly commented Jun 11, 2021

teor2345 commented Jun 13, 2021

teor2345 Jun 13, 2021 •

edited

Loading

teor2345 Jun 14, 2021

teor2345 commented Jun 13, 2021

teor2345 Jun 13, 2021

teor2345 Jun 13, 2021

teor2345 Jun 13, 2021

teor2345 Jun 13, 2021 •

edited

Loading

teor2345 Jun 13, 2021

teor2345 Jun 13, 2021

		cc e27ea62986a1ace4e16dde4b99d8ae8557a6b1e92e3f9a7c6aba0d7b179eb817 # shrinks to peers = [MetaAddr { addr: 0.0.0.0:0, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:0, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:1151, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 129.65.108.101:9019, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 95.68.199.21:46887, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 167.206.253.131:43927, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 160.138.155.239:63430, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: [6a4e:a543:8c93:e074:8a2a:5cc4:96f8:ce95]:51071, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623572152, calendar: 2021-06-13T08:15:52Z }, last_connection_state: NeverAttemptedAlternate }], initial_candidates = 0, extra_candidates = 2
		cc 788fe2c47a4af559e9a921764d1ce8fcfd0743361c19e6e7a06adc093bc19078 # shrinks to peers = [MetaAddr { addr: [::e9:8050:dbf7:5665]:27188, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 106.27.232.177:51742, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 127.0.0.1:30827, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:21948, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:38186, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: [4db2:96e6:38c5:fd62:e964:9338:4ce8:bfa0]:32686, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 0.0.0.0:22952, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }, MetaAddr { addr: 114.164.73.233:10203, services: NODE_NETWORK, last_seen: DateTime32 { timestamp: 1623573029, calendar: 2021-06-13T08:30:29Z }, last_connection_state: NeverAttemptedAlternate }], initial_candidates = 3, extra_candidates = 3

Security: only apply the outbound connection rate-limit to actual connections #2278

Security: only apply the outbound connection rate-limit to actual connections #2278

Conversation

teor2345 commented Jun 10, 2021 • edited Loading

Motivation

Solution

Review

Reviewer Checklist

This comment has been minimized.

dconnolly commented Jun 11, 2021

teor2345 commented Jun 13, 2021

teor2345 Jun 13, 2021 • edited Loading

Choose a reason for hiding this comment

teor2345 Jun 14, 2021

Choose a reason for hiding this comment

teor2345 commented Jun 13, 2021

teor2345 Jun 13, 2021

Choose a reason for hiding this comment

teor2345 Jun 13, 2021

Choose a reason for hiding this comment

teor2345 Jun 13, 2021

Choose a reason for hiding this comment

teor2345 Jun 13, 2021 • edited Loading

Choose a reason for hiding this comment

teor2345 Jun 13, 2021

Choose a reason for hiding this comment

teor2345 Jun 13, 2021

Choose a reason for hiding this comment

teor2345 commented Jun 10, 2021 •

edited

Loading

teor2345 Jun 13, 2021 •

edited

Loading

teor2345 Jun 13, 2021 •

edited

Loading