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

fix peer_id in case of DialError::InvalidPeerId (#2425) #2428

Closed
wants to merge 4 commits into from
Closed

fix peer_id in case of DialError::InvalidPeerId (#2425) #2428

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
1a8e772
fix peer_id in case of DialError::InvalidPeerId (#2425)
rkuhn Jan 11, 2022
4bf8267
Apply suggestions from code review
rkuhn Jan 12, 2022
561b922
include remote address in InvalidPeerId error
rkuhn Jan 12, 2022
020f9b9
fix changelog entries (InvalidPeerId -> WrongPeerId)
rkuhn Jan 15, 2022
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
9 changes: 6 additions & 3 deletions core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,23 @@
- Enable overriding _dial concurrency factor_ per dial via
`DialOpts::override_dial_concurrency_factor`.

- Introduces `libp2p_core::DialOpts` mirroring `libp2p_swarm::DialOpts`.
- Introduces `libp2p_core::DialOpts` mirroring `libp2p_swarm::DialOpts`.
Passed as an argument to `Network::dial`.
- Removes `Peer::dial` in favor of `Network::dial`.
- Removes `Peer::dial` in favor of `Network::dial`.

See [PR 2404].

- Implement `Serialize` and `Deserialize` for `PeerId` (see [PR 2408])

- Report negotiated and expected PeerId as well as remote address in DialError::WrongPeerId (see [PR 2428])

[PR 2339]: https://github.com/libp2p/rust-libp2p/pull/2339
[PR 2350]: https://github.com/libp2p/rust-libp2p/pull/2350
[PR 2352]: https://github.com/libp2p/rust-libp2p/pull/2352
[PR 2392]: https://github.com/libp2p/rust-libp2p/pull/2392
[PR 2404]: https://github.com/libp2p/rust-libp2p/pull/2404
[PR 2408]: https://github.com/libp2p/rust-libp2p/pull/2408
[PR 2428]: https://github.com/libp2p/rust-libp2p/pull/2428

# 0.30.1 [2021-11-16]

Expand Down Expand Up @@ -115,7 +118,7 @@

# 0.28.3 [2021-04-26]

- Fix build with secp256k1 disabled [PR 2057](https://github.com/libp2p/rust-libp2p/pull/2057].
- Fix build with secp256k1 disabled [PR 2057](https://github.com/libp2p/rust-libp2p/pull/2057).

# 0.28.2 [2021-04-13]

Expand Down
35 changes: 29 additions & 6 deletions core/src/connection/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.

use crate::connection::ConnectionLimit;
use crate::transport::TransportError;
use crate::Multiaddr;
use crate::{connection::ConnectionLimit, PeerId};
use std::{fmt, io};

/// Errors that can occur in the context of an established `Connection`.
Expand Down Expand Up @@ -84,14 +84,33 @@ pub enum PendingConnectionError<TTransErr> {
Aborted,

/// The peer identity obtained on the connection did not
/// match the one that was expected or is otherwise invalid.
InvalidPeerId,
/// match the one that was expected or is the local one.
WrongPeerId {
obtained: PeerId,
address: Multiaddr,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
address: Multiaddr,
address: ConnectedPoint,

How about a ConnectedPoint instead of a plain Multiaddr here? Mostly for the sake of additional context.

},

/// An I/O error occurred on the connection.
// TODO: Eventually this should also be a custom error?
IO(io::Error),
}

impl<T> PendingConnectionError<T> {
pub fn map<U>(self, f: impl FnOnce(T) -> U) -> PendingConnectionError<U> {
match self {
PendingConnectionError::Transport(t) => PendingConnectionError::Transport(f(t)),
PendingConnectionError::ConnectionLimit(l) => {
PendingConnectionError::ConnectionLimit(l)
}
PendingConnectionError::Aborted => PendingConnectionError::Aborted,
PendingConnectionError::WrongPeerId { obtained, address } => {
PendingConnectionError::WrongPeerId { obtained, address }
}
PendingConnectionError::IO(e) => PendingConnectionError::IO(e),
}
}
}

impl<TTransErr> fmt::Display for PendingConnectionError<TTransErr>
where
TTransErr: fmt::Display + fmt::Debug,
Expand All @@ -110,8 +129,12 @@ where
PendingConnectionError::ConnectionLimit(l) => {
write!(f, "Connection error: Connection limit: {}.", l)
}
PendingConnectionError::InvalidPeerId => {
write!(f, "Pending connection: Invalid peer ID.")
PendingConnectionError::WrongPeerId { obtained, address } => {
write!(
f,
"Pending connection: Unexpected peer ID {} at {}.",
obtained, address
)
}
}
}
Expand All @@ -125,7 +148,7 @@ where
match self {
PendingConnectionError::IO(err) => Some(err),
PendingConnectionError::Transport(_) => None,
PendingConnectionError::InvalidPeerId => None,
PendingConnectionError::WrongPeerId { .. } => None,
PendingConnectionError::Aborted => None,
PendingConnectionError::ConnectionLimit(..) => None,
}
Expand Down
60 changes: 27 additions & 33 deletions core/src/connection/pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ where
match event {
task::PendingConnectionEvent::ConnectionEstablished {
id,
output: (peer_id, muxer),
output: (obtained_peer_id, muxer),
outgoing,
} => {
let PendingConnectionInfo {
Expand Down Expand Up @@ -769,49 +769,39 @@ where
),
};

enum Error {
ConnectionLimit(ConnectionLimit),
InvalidPeerId,
}

impl<TransportError> From<Error> for PendingConnectionError<TransportError> {
fn from(error: Error) -> Self {
match error {
Error::ConnectionLimit(limit) => {
PendingConnectionError::ConnectionLimit(limit)
}
Error::InvalidPeerId => PendingConnectionError::InvalidPeerId,
}
}
}

let error = self
let error: Result<(), PendingInboundConnectionError<_>> = self
.counters
// Check general established connection limit.
.check_max_established(&endpoint)
.map_err(Error::ConnectionLimit)
.map_err(PendingConnectionError::ConnectionLimit)
// Check per-peer established connection limit.
.and_then(|()| {
self.counters
.check_max_established_per_peer(num_peer_established(
&self.established,
peer_id,
obtained_peer_id,
))
.map_err(Error::ConnectionLimit)
.map_err(PendingConnectionError::ConnectionLimit)
})
// Check expected peer id matches.
.and_then(|()| {
if let Some(peer) = expected_peer_id {
if peer != peer_id {
return Err(Error::InvalidPeerId);
if peer != obtained_peer_id {
return Err(PendingConnectionError::WrongPeerId {
obtained: obtained_peer_id,
address: endpoint.get_remote_address().clone(),
});
}
}
Ok(())
})
// Check peer is not local peer.
.and_then(|()| {
if self.local_id == peer_id {
Err(Error::InvalidPeerId)
if self.local_id == obtained_peer_id {
Err(PendingConnectionError::WrongPeerId {
obtained: obtained_peer_id,
address: endpoint.get_remote_address().clone(),
})
} else {
Ok(())
}
Expand All @@ -824,7 +814,7 @@ where
log::debug!(
"Failed to close connection {:?} to peer {}: {:?}",
id,
peer_id,
obtained_peer_id,
e
);
}
Expand All @@ -837,9 +827,10 @@ where
ConnectedPoint::Dialer { .. } => {
return Poll::Ready(PoolEvent::PendingOutboundConnectionError {
id,
error: error.into(),
error: error
.map(|t| vec![(endpoint.get_remote_address().clone(), t)]),
handler,
peer: Some(peer_id),
peer: expected_peer_id.or(Some(obtained_peer_id)),
})
}
ConnectedPoint::Listener {
Expand All @@ -848,7 +839,7 @@ where
} => {
return Poll::Ready(PoolEvent::PendingInboundConnectionError {
id,
error: error.into(),
error,
handler,
send_back_addr,
local_addr,
Expand All @@ -858,7 +849,7 @@ where
}

// Add the connection to the pool.
let conns = self.established.entry(peer_id).or_default();
let conns = self.established.entry(obtained_peer_id).or_default();
let other_established_connection_ids = conns.keys().cloned().collect();
self.counters.inc_established(&endpoint);

Expand All @@ -867,20 +858,23 @@ where
conns.insert(
id,
EstablishedConnectionInfo {
peer_id,
peer_id: obtained_peer_id,
endpoint: endpoint.clone(),
sender: command_sender,
},
);

let connected = Connected { peer_id, endpoint };
let connected = Connected {
peer_id: obtained_peer_id,
endpoint,
};

let connection =
super::Connection::new(muxer, handler.into_handler(&connected));
self.spawn(
task::new_for_established_connection(
id,
peer_id,
obtained_peer_id,
connection,
command_receiver,
self.established_connection_events_tx.clone(),
Expand Down
8 changes: 4 additions & 4 deletions core/src/network.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ use crate::{
Executor, Multiaddr, PeerId,
};
use either::Either;
use multihash::Multihash;
use std::{
convert::TryFrom as _,
error, fmt,
Expand Down Expand Up @@ -236,7 +237,7 @@ where
.transpose()
{
Ok(peer_id) => peer_id,
Err(_) => return Err(DialError::InvalidPeerId { handler }),
Err(multihash) => return Err(DialError::InvalidPeerId { handler, multihash }),
};

(peer_id, Either::Right(std::iter::once(address)), None)
Expand Down Expand Up @@ -565,11 +566,10 @@ pub enum DialError<THandler> {
handler: THandler,
},
/// The dialing attempt is rejected because the peer being dialed is the local peer.
LocalPeerId {
handler: THandler,
},
LocalPeerId { handler: THandler },
InvalidPeerId {
handler: THandler,
multihash: Multihash,
},
}

Expand Down
62 changes: 54 additions & 8 deletions core/tests/network_dial_error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ fn deny_incoming_connec() {

swarm1.listen_on("/memory/0".parse().unwrap()).unwrap();

let address = async_std::task::block_on(future::poll_fn(|cx| match swarm1.poll(cx) {
let address = futures::executor::block_on(future::poll_fn(|cx| match swarm1.poll(cx) {
Poll::Ready(NetworkEvent::NewListenerAddress { listen_addr, .. }) => {
Poll::Ready(listen_addr)
}
Expand All @@ -59,7 +59,7 @@ fn deny_incoming_connec() {
)
.unwrap();

async_std::task::block_on(future::poll_fn(|cx| -> Poll<Result<(), io::Error>> {
futures::executor::block_on(future::poll_fn(|cx| -> Poll<Result<(), io::Error>> {
match swarm1.poll(cx) {
Poll::Ready(NetworkEvent::IncomingConnection { connection, .. }) => drop(connection),
Poll::Ready(_) => unreachable!(),
Expand Down Expand Up @@ -88,6 +88,52 @@ fn deny_incoming_connec() {
.unwrap();
}

#[test]
fn invalid_peer_id() {
// Checks whether dialing an address containing the wrong peer id raises an error
// for the expected peer id instead of the obtained peer id.

let mut swarm1 = test_network(NetworkConfig::default());
let mut swarm2 = test_network(NetworkConfig::default());

swarm1.listen_on("/memory/0".parse().unwrap()).unwrap();
rkuhn marked this conversation as resolved.
Show resolved Hide resolved

let address = futures::executor::block_on(future::poll_fn(|cx| match swarm1.poll(cx) {
Poll::Ready(NetworkEvent::NewListenerAddress { listen_addr, .. }) => {
Poll::Ready(listen_addr)
}
Poll::Pending => Poll::Pending,
_ => panic!("Was expecting the listen address to be reported"),
}));

let other_id = PeerId::random();
let other_addr = address.with(Protocol::P2p(other_id.into()));

swarm2.dial(TestHandler(), other_addr.clone()).unwrap();

let (peer_id, error) = futures::executor::block_on(future::poll_fn(|cx| {
if let Poll::Ready(NetworkEvent::IncomingConnection { connection, .. }) = swarm1.poll(cx) {
swarm1.accept(connection, TestHandler()).unwrap();
}

match swarm2.poll(cx) {
Poll::Ready(NetworkEvent::DialError { peer_id, error, .. }) => {
Poll::Ready((peer_id, error))
}
Poll::Ready(x) => panic!("unexpected {:?}", x),
Poll::Pending => Poll::Pending,
}
}));
assert_eq!(peer_id, other_id);
match error {
PendingConnectionError::WrongPeerId { obtained, address } => {
assert_eq!(obtained, *swarm1.local_peer_id());
assert_eq!(address, other_addr);
}
x => panic!("wrong error {:?}", x),
}
}

#[test]
fn dial_self() {
// Check whether dialing ourselves correctly fails.
Expand All @@ -103,7 +149,7 @@ fn dial_self() {
let mut swarm = test_network(NetworkConfig::default());
swarm.listen_on("/memory/0".parse().unwrap()).unwrap();

let local_address = async_std::task::block_on(future::poll_fn(|cx| match swarm.poll(cx) {
let local_address = futures::executor::block_on(future::poll_fn(|cx| match swarm.poll(cx) {
Poll::Ready(NetworkEvent::NewListenerAddress { listen_addr, .. }) => {
Poll::Ready(listen_addr)
}
Expand All @@ -115,12 +161,12 @@ fn dial_self() {

let mut got_dial_err = false;
let mut got_inc_err = false;
async_std::task::block_on(future::poll_fn(|cx| -> Poll<Result<(), io::Error>> {
futures::executor::block_on(future::poll_fn(|cx| -> Poll<Result<(), io::Error>> {
loop {
match swarm.poll(cx) {
Poll::Ready(NetworkEvent::DialError {
peer_id,
error: PendingConnectionError::InvalidPeerId { .. },
error: PendingConnectionError::WrongPeerId { .. },
..
}) => {
assert_eq!(&peer_id, swarm.local_peer_id());
Expand Down Expand Up @@ -157,7 +203,7 @@ fn dial_self_by_id() {
// Trying to dial self by passing the same `PeerId` shouldn't even be possible in the first
// place.
let mut swarm = test_network(NetworkConfig::default());
let peer_id = swarm.local_peer_id().clone();
let peer_id = *swarm.local_peer_id();
assert!(swarm.peer(peer_id).into_disconnected().is_none());
}

Expand All @@ -181,13 +227,13 @@ fn multiple_addresses_err() {
swarm
.dial(
TestHandler(),
DialOpts::peer_id(target.clone())
DialOpts::peer_id(target)
.addresses(addresses.clone())
.build(),
)
.unwrap();

async_std::task::block_on(future::poll_fn(|cx| -> Poll<Result<(), io::Error>> {
futures::executor::block_on(future::poll_fn(|cx| -> Poll<Result<(), io::Error>> {
loop {
match swarm.poll(cx) {
Poll::Ready(NetworkEvent::DialError {
Expand Down
Loading