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.

Sign up for GitHub

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

More robust peer banning #3086

Merged
merged 4 commits into from
Oct 4, 2019
Merged

More robust peer banning #3086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b8600c
More robust peer banning
quentinlesceller Oct 4, 2019
40a6751
Typo
quentinlesceller Oct 4, 2019
28d3d96
Fix whitespace
quentinlesceller Oct 4, 2019
4dff58b
Hashmap comments
quentinlesceller Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions api/src/handlers/peers_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,20 @@ impl Handler for PeerHandler {
};

match command {
"ban" => w_fut!(&self.peers).ban_peer(addr, ReasonForBan::ManualBan),
"unban" => w_fut!(&self.peers).unban_peer(addr),
"ban" => match w_fut!(&self.peers).ban_peer(addr, ReasonForBan::ManualBan) {
Ok(_) => response(StatusCode::OK, "{}"),
Err(e) => response(
StatusCode::INTERNAL_SERVER_ERROR,
format!("ban failed: {:?}", e),
),
},
"unban" => match w_fut!(&self.peers).unban_peer(addr) {
Ok(_) => response(StatusCode::OK, "{}"),
Err(e) => response(
StatusCode::INTERNAL_SERVER_ERROR,
format!("unban failed: {:?}", e),
),
},
_ => return response(StatusCode::BAD_REQUEST, "invalid command"),
};

Expand Down
119 changes: 63 additions & 56 deletions p2p/src/peers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,10 @@ impl Peers {
/// Adds the peer to our internal peer mapping. Note that the peer is still
/// returned so the server can run it.
pub fn add_connected(&self, peer: Arc<Peer>) -> Result<(), Error> {
let mut peers = match self.peers.try_write_for(LOCK_TIMEOUT) {
Some(peers) => peers,
None => {
error!("add_connected: failed to get peers lock");
return Err(Error::Timeout);
}
};
let mut peers = self.peers.try_write_for(LOCK_TIMEOUT).ok_or_else(|| {
error!("add_connected: failed to get peers lock");
Error::Timeout
})?;
let peer_data = PeerData {
addr: peer.info.addr,
capabilities: peer.info.capabilities,
Expand Down Expand Up @@ -102,13 +99,10 @@ impl Peers {
/// and this attempt fails then return an error allowing the caller
/// to decide how best to handle this.
pub fn is_known(&self, addr: PeerAddr) -> Result<bool, Error> {
let peers = match self.peers.try_read_for(LOCK_TIMEOUT) {
Some(peers) => peers,
None => {
error!("is_known: failed to get peers lock");
return Err(Error::Internal);
}
};
let peers = self.peers.try_read_for(LOCK_TIMEOUT).ok_or_else(|| {
error!("is_known: failed to get peers lock");
Error::Internal
})?;
Ok(peers.contains_key(&addr))
}

Expand Down Expand Up @@ -253,50 +247,38 @@ impl Peers {
}
false
}

/// Ban a peer, disconnecting it if we're currently connected
pub fn ban_peer(&self, peer_addr: PeerAddr, ban_reason: ReasonForBan) {
if let Err(e) = self.update_state(peer_addr, State::Banned) {
error!("Couldn't ban {}: {:?}", peer_addr, e);
return;
}

if let Some(peer) = self.get_connected_peer(peer_addr) {
debug!("Banning peer {}", peer_addr);
// setting peer status will get it removed at the next clean_peer
match peer.send_ban_reason(ban_reason) {
Err(e) => error!("failed to send a ban reason to{}: {:?}", peer_addr, e),
Ok(_) => debug!("ban reason {:?} was sent to {}", ban_reason, peer_addr),
};
peer.set_banned();
peer.stop();

let mut peers = match self.peers.try_write_for(LOCK_TIMEOUT) {
Some(peers) => peers,
None => {
pub fn ban_peer(&self, peer_addr: PeerAddr, ban_reason: ReasonForBan) -> Result<(), Error> {
self.update_state(peer_addr, State::Banned)?;

match self.get_connected_peer(peer_addr) {
Some(peer) => {
debug!("Banning peer {}", peer_addr);
// setting peer status will get it removed at the next clean_peer
peer.send_ban_reason(ban_reason)?;
peer.set_banned();
peer.stop();
let mut peers = self.peers.try_write_for(LOCK_TIMEOUT).ok_or_else(|| {
error!("ban_peer: failed to get peers lock");
return;
}
};
peers.remove(&peer.info.addr);
Error::PeerException
})?;
peers.remove(&peer.info.addr);
Ok(())
}
None => return Err(Error::PeerNotFound),
}
}

/// Unban a peer, checks if it exists and banned then unban
pub fn unban_peer(&self, peer_addr: PeerAddr) {
pub fn unban_peer(&self, peer_addr: PeerAddr) -> Result<(), Error> {
debug!("unban_peer: peer {}", peer_addr);
match self.get_peer(peer_addr) {
Ok(_) => {
if self.is_banned(peer_addr) {
if let Err(e) = self.update_state(peer_addr, State::Healthy) {
error!("Couldn't unban {}: {:?}", peer_addr, e);
}
} else {
error!("Couldn't unban {}: peer is not banned", peer_addr);
}
}
Err(e) => error!("Couldn't unban {}: {:?}", peer_addr, e),
};
// check if peer exist
self.get_peer(peer_addr)?;
if self.is_banned(peer_addr) {
return self.update_state(peer_addr, State::Healthy);
} else {
return Err(Error::PeerNotBanned);
}
}

fn broadcast<F>(&self, obj_name: &str, inner: F) -> u32
Expand Down Expand Up @@ -605,7 +587,12 @@ impl ChainAdapter for Peers {
"Received a bad block {} from {}, the peer will be banned",
hash, peer_info.addr,
);
self.ban_peer(peer_info.addr, ReasonForBan::BadBlock);
self.ban_peer(peer_info.addr, ReasonForBan::BadBlock)
.map_err(|e| {
let err: chain::Error =
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below - perhaps just return error chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into() instaead of defining a temp var and returnning it?

Copy link
Member Author

@quentinlesceller quentinlesceller Oct 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I have no choice otherwise Rust complains (cannot infer type thing).

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately cannot be done. Rust need type annotations for that one.

chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(false)
} else {
Ok(true)
Expand All @@ -625,7 +612,12 @@ impl ChainAdapter for Peers {
"Received a bad compact block {} from {}, the peer will be banned",
hash, peer_info.addr
);
self.ban_peer(peer_info.addr, ReasonForBan::BadCompactBlock);
self.ban_peer(peer_info.addr, ReasonForBan::BadCompactBlock)
.map_err(|e| {
let err: chain::Error =
chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(false)
} else {
Ok(true)
Expand All @@ -640,7 +632,12 @@ impl ChainAdapter for Peers {
if !self.adapter.header_received(bh, peer_info)? {
// if the peer sent us a block header that's intrinsically bad
// they are either mistaken or malevolent, both of which require a ban
self.ban_peer(peer_info.addr, ReasonForBan::BadBlockHeader);
self.ban_peer(peer_info.addr, ReasonForBan::BadBlockHeader)
.map_err(|e| {
let err: chain::Error =
chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(false)
} else {
Ok(true)
Expand All @@ -655,7 +652,12 @@ impl ChainAdapter for Peers {
if !self.adapter.headers_received(headers, peer_info)? {
// if the peer sent us a block header that's intrinsically bad
// they are either mistaken or malevolent, both of which require a ban
self.ban_peer(peer_info.addr, ReasonForBan::BadBlockHeader);
self.ban_peer(peer_info.addr, ReasonForBan::BadBlockHeader)
.map_err(|e| {
let err: chain::Error =
chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(false)
} else {
Ok(true)
Expand Down Expand Up @@ -701,7 +703,12 @@ impl ChainAdapter for Peers {
"Received a bad txhashset data from {}, the peer will be banned",
peer_info.addr
);
self.ban_peer(peer_info.addr, ReasonForBan::BadTxHashSet);
self.ban_peer(peer_info.addr, ReasonForBan::BadTxHashSet)
.map_err(|e| {
let err: chain::Error =
chain::ErrorKind::Other(format!("ban peer error :{:?}", e)).into();
err
})?;
Ok(true)
} else {
Ok(false)
Expand Down
2 changes: 2 additions & 0 deletions p2p/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ pub enum Error {
peer: Hash,
},
Send(String),
PeerNotFound,
PeerNotBanned,
PeerException,
Internal,
}
Expand Down
4 changes: 3 additions & 1 deletion servers/src/grin/seed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,9 @@ fn monitor_peers(
let interval = Utc::now().timestamp() - x.last_banned;
// Unban peer
if interval >= config.ban_window() {
peers.unban_peer(x.addr);
if let Err(e) = peers.unban_peer(x.addr) {
error!("failed to unban peer {}: {:?}", x.addr, e);
}
debug!(
"monitor_peers: unbanned {} after {} seconds",
x.addr, interval
Expand Down
8 changes: 6 additions & 2 deletions servers/src/grin/sync/header_sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,12 @@ impl HeaderSync {
if now > *stalling_ts + Duration::seconds(120)
&& header_head.total_difficulty < peer.info.total_difficulty()
{
self.peers
.ban_peer(peer.info.addr, ReasonForBan::FraudHeight);
if let Err(e) = self
.peers
.ban_peer(peer.info.addr, ReasonForBan::FraudHeight)
{
error!("failed to ban peer {}: {:?}", peer.info.addr, e);
}
info!(
"sync: ban a fraud peer: {}, claimed height: {}, total difficulty: {}",
peer.info.addr,
Expand Down