diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index 5e8697b1..348300c6 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -1162,6 +1162,14 @@ where if self.sessions.inbound().count() >= self.config.limits.connection.inbound { return false; } + match self.db.addresses().is_banned(&addr) { + Ok(banned) => { + if banned { + return false; + } + } + Err(e) => error!(target: "service", "Error querying ban status for {addr}: {e}"), + } let host: HostName = addr.into(); if self.limiter.limit( @@ -2185,7 +2193,7 @@ where // even if it's in a disconnected state. Those sessions are re-attempted automatically. let mut peers = entries .filter(|entry| !entry.address.banned) - .filter(|entry| !entry.penalty.is_threshold_reached()) + .filter(|entry| !entry.penalty.is_connect_threshold_reached()) .filter(|entry| !self.sessions.contains_key(&entry.node)) .filter(|entry| !self.config.external_addresses.contains(&entry.address.addr)) .filter(|entry| &entry.node != self.nid()) diff --git a/radicle/src/node.rs b/radicle/src/node.rs index d19cd586..9874b27b 100644 --- a/radicle/src/node.rs +++ b/radicle/src/node.rs @@ -53,7 +53,9 @@ pub const DEFAULT_TIMEOUT: time::Duration = time::Duration::from_secs(30); /// Maximum length in bytes of a node alias. pub const MAX_ALIAS_LENGTH: usize = 32; /// Penalty threshold at which point we avoid connecting to this node. -pub const PENALTY_THRESHOLD: u8 = 32; +pub const PENALTY_CONNECT_THRESHOLD: u8 = 32; +/// Penalty threshold at which point we ban this node. +pub const PENALTY_BAN_THRESHOLD: u8 = 64; /// Filename of node database under the node directory. pub const NODE_DB_FILE: &str = "node.db"; /// Filename of policies database under the node directory. @@ -162,8 +164,12 @@ pub struct Penalty(u8); impl Penalty { /// If the penalty threshold is reached, at which point we should just avoid /// connecting to this node. - pub fn is_threshold_reached(&self) -> bool { - self.0 >= PENALTY_THRESHOLD + pub fn is_connect_threshold_reached(&self) -> bool { + self.0 >= PENALTY_CONNECT_THRESHOLD + } + + pub fn is_ban_threshold_reached(&self) -> bool { + self.0 >= PENALTY_BAN_THRESHOLD } } diff --git a/radicle/src/node/address.rs b/radicle/src/node/address.rs index d3c69e97..d31689af 100644 --- a/radicle/src/node/address.rs +++ b/radicle/src/node/address.rs @@ -132,6 +132,8 @@ pub struct Node { pub timestamp: Timestamp, /// Node connection penalty. pub penalty: Penalty, + /// Whether the node is banned. + pub banned: bool, } /// A known address. diff --git a/radicle/src/node/address/store.rs b/radicle/src/node/address/store.rs index e7bb6303..398cb40e 100644 --- a/radicle/src/node/address/store.rs +++ b/radicle/src/node/address/store.rs @@ -63,6 +63,9 @@ pub trait Store { fn is_empty(&self) -> Result { self.len().map(|l| l == 0) } + /// Check if an address is banned. Also returns `true` if the node this address belongs + /// to is banned. + fn is_banned(&self, addr: &Address) -> Result; /// Get the address entries in the store. fn entries(&self) -> Result>, Error>; /// Mark a node as attempted at a certain time. @@ -80,10 +83,9 @@ pub trait Store { impl Store for Database { fn get(&self, node: &NodeId) -> Result, Error> { - let mut stmt = self - .db - .prepare("SELECT features, alias, pow, penalty, timestamp FROM nodes WHERE id = ?")?; - + let mut stmt = self.db.prepare( + "SELECT features, alias, pow, penalty, banned, timestamp FROM nodes WHERE id = ?", + )?; stmt.bind((1, node))?; if let Some(Ok(row)) = stmt.into_iter().next() { @@ -93,6 +95,7 @@ impl Store for Database { let pow = row.read::("pow") as u32; let penalty = row.read::("penalty").min(u8::MAX as i64); let penalty = Penalty(penalty as u8); + let banned = row.read::("banned").is_positive(); let addrs = self.addresses_of(node)?; Ok(Some(Node { @@ -102,12 +105,34 @@ impl Store for Database { timestamp, penalty, addrs, + banned, })) } else { Ok(None) } } + fn is_banned(&self, addr: &Address) -> Result { + let mut stmt = self.db.prepare( + "SELECT a.banned, n.banned + FROM addresses AS a + JOIN nodes AS n ON a.node = n.id + WHERE value = ?1 AND type =?2", + )?; + stmt.bind((1, addr))?; + stmt.bind((2, AddressType::from(addr)))?; + + if let Some(row) = stmt.into_iter().next() { + let row = row?; + let addr_banned = row.read::(0).is_positive(); + let node_banned = row.read::(1).is_positive(); + + Ok(node_banned || addr_banned) + } else { + Ok(false) + } + } + fn addresses_of(&self, node: &NodeId) -> Result, Error> { let mut addrs = Vec::new(); let mut stmt = self.db.prepare( @@ -306,17 +331,29 @@ impl Store for Database { _addr: &Address, severity: Severity, ) -> Result<(), Error> { - let mut stmt = self.db.prepare( - "UPDATE `nodes` - SET penalty = penalty + ?2 - WHERE id = ?1", - )?; + transaction(&self.db, |db| { + let mut stmt = self.db.prepare( + "UPDATE `nodes` + SET penalty = penalty + ?2 + WHERE id = ?1", + )?; + stmt.bind((1, nid))?; + stmt.bind((2, severity as i64))?; + stmt.next()?; - stmt.bind((1, nid))?; - stmt.bind((2, severity as i64))?; - stmt.next()?; + // If the ban threshold is reached, we ban the node and its addresses. + let node = self.get(nid)?.ok_or(Error::NoRows)?; + if node.penalty.is_ban_threshold_reached() { + let mut stmt = db.prepare("UPDATE `nodes` SET banned = 1 WHERE id = ?1")?; + stmt.bind((1, nid))?; + stmt.next()?; - Ok(()) + let mut stmt = db.prepare("UPDATE `addresses` SET banned = 1 WHERE node = ?1")?; + stmt.bind((1, nid))?; + stmt.next()?; + } + Ok(()) + }) } } @@ -684,4 +721,46 @@ mod test { let node = cache.get(&alice).unwrap().unwrap(); assert_eq!(node.penalty, Penalty(4)); } + + #[test] + fn test_disconnected_ban() { + let alice = arbitrary::gen::(1); + let ka1 = arbitrary::gen::(1); + let ka2 = arbitrary::gen::(1); + let mut db = Database::memory().unwrap(); + let features = node::Features::SEED; + let timestamp = Timestamp::from(LocalTime::now()); + + db.insert( + &alice, + features, + Alias::new("alice"), + 16, + timestamp, + [ka1.clone(), ka2.clone()], + ) + .unwrap(); + let node = db.get(&alice).unwrap().unwrap(); + assert_eq!(node.penalty, Penalty::default()); + + for _ in 0..7 { + db.disconnected(&alice, &ka1.addr, Severity::High).unwrap(); + let node = db.get(&alice).unwrap().unwrap(); + + assert!(!node.penalty.is_ban_threshold_reached()); + assert!(!node.banned); + } + + db.disconnected(&alice, &ka1.addr, Severity::High).unwrap(); + let node = db.get(&alice).unwrap().unwrap(); + + assert!(node.penalty.is_ban_threshold_reached()); + assert!(node.banned); + + for addr in node.addrs { + assert!(addr.banned); + } + assert!(db.is_banned(&ka1.addr).unwrap()); + assert!(db.is_banned(&ka2.addr).unwrap()); // Banned because node is banned. + } } diff --git a/radicle/src/test/arbitrary.rs b/radicle/src/test/arbitrary.rs index dbab87c7..8c584e15 100644 --- a/radicle/src/test/arbitrary.rs +++ b/radicle/src/test/arbitrary.rs @@ -18,8 +18,8 @@ use crate::identity::{ project::Project, Did, }; -use crate::node::address::AddressType; -use crate::node::{Address, Alias, Timestamp}; +use crate::node::address::{AddressType, Source}; +use crate::node::{Address, Alias, KnownAddress, Timestamp}; use crate::storage; use crate::storage::refs::{Refs, RefsAt, SignedRefs}; use crate::test::storage::{MockRepository, MockStorage}; @@ -303,6 +303,12 @@ impl Arbitrary for Address { } } +impl Arbitrary for KnownAddress { + fn arbitrary(g: &mut qcheck::Gen) -> Self { + KnownAddress::new(Address::arbitrary(g), Source::Peer) + } +} + impl Arbitrary for Alias { fn arbitrary(g: &mut qcheck::Gen) -> Self { let s = g