Security: Avoid reconnecting to peers that are likely unreachable (#3030

)

* Add a `Duration32::from_days` constructor

Make it simpler to construct a `Duration32` representing a certain
number of days.

* Add `MetaAddr::was_not_recently_seen` method

A helper method to check if a peer was never seen before or if it was
last seen a long time ago. This will be one of the conditions to
consider a peer as unreachable.

* Add `MetaAddr::is_probably_unreachable` method

A helper method to check if a peer should be considered unreachable. It
is considered unreachable if recent connection attempts have failed and
it was not recently seen.

If a peer is considered unreachable, Zebra shouldn't attempt to connect
to it again.

* Do not keep trying to connect to unreachable peer

A peer is probably unreachable if it was last seen a long time ago and
if it's last connection attempt failed.

* Test `was_not_recently_seen`

Redo the calculation on arbitrary `MetaAddr`s.

* Test `is_probably_unreachable`

Redo the calculation on arbitrary `MetaAddr`s.

* Test if probably unreachable peers are ignored

Given an `AddressBook` with a list of arbitrary `MetaAddr`s, check that
none of the peers listed for a reconnection is probably unreachable.

* Rename unit test to improve clarity

Remove the double negative from the name.

Co-authored-by: teor <teor@riseup.net>

* Rename constant to `MAX_RECENT_PEER_AGE`

Make the purpose of the constant clearer.

Co-authored-by: teor <teor@riseup.net>

* Rename method to `last_seen_is_recent`

Remove the double negative from the name.

* Rename method to `is_probably_reachable`

Avoid having to negate the result of the method in security critical
filter.

* Move check into `is_ready_for_connection_attempt`

Make sure the check is used in any place that requires a peer that's
ready for a connection attempt.

* Improve test documention

Describe the goal of the test better.

Co-authored-by: teor <teor@riseup.net>

* Improve `is_probably_reachable` documentation

List the conditions as bullet points.

Co-authored-by: teor <teor@riseup.net>

* Document what happens when peers have no last seen time

Co-authored-by: teor <teor@riseup.net>

zebra-chain/src/serialization/date_time.rs

@@ -138,6 +138,14 @@ impl Duration32 {
         Duration32::from_minutes(hours.saturating_mul(60))
     }
 
+    /// Creates a new [`Duration32`] to represent the given amount of days.
+    ///
+    /// If the resulting number of seconds does not fit in a [`u32`], [`Duration32::MAX`] is
+    /// returned.
+    pub const fn from_days(days: u32) -> Self {
+        Duration32::from_hours(days.saturating_mul(24))
+    }
+
     /// Returns the number of seconds in this duration.
     pub fn seconds(&self) -> u32 {
         self.seconds

zebra-network/src/address_book/tests/prop.rs

@@ -33,4 +33,19 @@ proptest! {
             prop_assert!(duration_since_last_seen <= MAX_PEER_ACTIVE_FOR_GOSSIP);
         }
     }
+
+    /// Test that only peers that are reachable are listed for reconnection attempts.
+    #[test]
+    fn only_reachable_addresses_are_attempted(
+        local_listener in any::<SocketAddr>(),
+        addresses in vec(any::<MetaAddr>(), 0..MAX_META_ADDR),
+    ) {
+        zebra_test::init();
+
+        let address_book = AddressBook::new_with_addrs(local_listener, Span::none(), addresses);
+
+        for peer in address_book.reconnection_peers() {
+            prop_assert!(peer.is_probably_reachable(), "peer: {:?}", peer);
+        }
+    }
 }

zebra-network/src/constants.rs

@@ -78,6 +78,15 @@ pub const MIN_PEER_RECONNECTION_DELAY: Duration = Duration::from_secs(60 + 20 +
 /// within the last three hours."
 pub const MAX_PEER_ACTIVE_FOR_GOSSIP: Duration32 = Duration32::from_hours(3);
 
+/// The maximum duration since a peer was last seen to consider reconnecting to it.
+///
+/// Peers that haven't been seen for more than three days and that had its last connection attempt
+/// fail are considered to be offline and Zebra will stop trying to connect to them.
+///
+/// This is to ensure that Zebra can't have a denial-of-service as a consequence of having too many
+/// offline peers that it constantly and uselessly retries to connect to.
+pub const MAX_RECENT_PEER_AGE: Duration32 = Duration32::from_days(3);
+
 /// Regular interval for sending keepalive `Ping` messages to each
 /// connected peer.
 pub const HEARTBEAT_INTERVAL: Duration = Duration::from_secs(60);

zebra-network/src/meta_addr.rs

@@ -501,6 +501,7 @@ impl MetaAddr {
             && !self.has_connection_recently_responded()
             && !self.was_connection_recently_attempted()
             && !self.has_connection_recently_failed()
+            && self.is_probably_reachable()
     }
 
     /// Is the [`SocketAddr`] we have for this peer valid for outbound
@@ -528,6 +529,38 @@ impl MetaAddr {
         is_node && self.address_is_valid_for_outbound()
     }
 
+    /// Should this peer considered reachable?
+    ///
+    /// A peer is probably reachable if:
+    /// - it has never been attempted, or
+    /// - the last connection attempt was successful, or
+    /// - the last successful connection was less than 3 days ago.
+    ///
+    /// # Security
+    ///
+    /// This is used by [`Self::is_ready_for_connection_attempt`] so that Zebra stops trying to
+    /// connect to peers that are likely unreachable.
+    ///
+    /// The `untrusted_last_seen` time is used as a fallback time if the local node has never
+    /// itself seen the peer. If the reported last seen time is a long time ago or `None`, then the local
+    /// node will attempt to connect the peer once, and if that attempt fails it won't
+    /// try to connect ever again. (The state can't be `Failed` until after the first connection attempt.)
+    pub fn is_probably_reachable(&self) -> bool {
+        self.last_connection_state != PeerAddrState::Failed || self.last_seen_is_recent()
+    }
+
+    /// Was this peer last seen recently?
+    ///
+    /// Returns `true` if this peer was last seen at most
+    /// [`MAX_RECENT_PEER_AGE`][constants::MAX_RECENT_PEER_AGE] ago.
+    /// Returns false if the peer is outdated, or it has no last seen time.
+    pub fn last_seen_is_recent(&self) -> bool {
+        match self.last_seen() {
+            Some(last_seen) => last_seen.saturating_elapsed() <= constants::MAX_RECENT_PEER_AGE,
+            None => false,
+        }
+    }
+
     /// Return a sanitized version of this `MetaAddr`, for sending to a remote peer.
     ///
     /// Returns `None` if this `MetaAddr` should not be sent to remote peers.

zebra-network/src/meta_addr/tests/prop.rs

@@ -15,8 +15,10 @@ use tokio::{runtime, time::Instant};
 use tower::service_fn;
 use tracing::Span;
 
+use zebra_chain::serialization::DateTime32;
+
 use crate::{
-    constants::MIN_PEER_RECONNECTION_DELAY,
+    constants::{MAX_RECENT_PEER_AGE, MIN_PEER_RECONNECTION_DELAY},
     meta_addr::{
         arbitrary::{MAX_ADDR_CHANGE, MAX_META_ADDR},
         MetaAddr, MetaAddrChange,
@@ -356,4 +358,41 @@ proptest! {
             prop_assert!(max_attempts - min_attempts <= 1);
         }
     }
+
+    /// Make sure check if a peer was recently seen is correct.
+    #[test]
+    fn last_seen_is_recent_is_correct(peer in any::<MetaAddr>()) {
+        let time_since_last_seen = peer
+            .last_seen()
+            .map(|last_seen| last_seen.saturating_elapsed());
+
+        let recently_seen = time_since_last_seen
+            .map(|elapsed| elapsed <= MAX_RECENT_PEER_AGE)
+            .unwrap_or(false);
+
+        prop_assert_eq!(
+            peer.last_seen_is_recent(),
+            recently_seen,
+            "last seen: {:?}, now: {:?}",
+            peer.last_seen(),
+            DateTime32::now(),
+        );
+    }
+
+    /// Make sure a peer is correctly determined to be probably reachable.
+    #[test]
+    fn probably_rechable_is_determined_correctly(peer in any::<MetaAddr>()) {
+        let last_attempt_failed = peer.last_connection_state == Failed;
+        let not_recently_seen = !peer.last_seen_is_recent();
+
+        let probably_unreachable = last_attempt_failed && not_recently_seen;
+
+        prop_assert_eq!(
+            peer.is_probably_reachable(),
+            !probably_unreachable,
+            "last_connection_state: {:?}, last_seen: {:?}",
+            peer.last_connection_state,
+            peer.last_seen()
+        );
+    }
 }