From 2c96d644f9a91aa6ac7d9d9da805d1bd1496e661 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 23 Nov 2022 11:51:47 +1100 Subject: [PATCH] feat: Better error reporting when features are disabled (#2972) In case support for e.g. RSA keys is disabled at compile-time, we will now print a better error message. For example: > Failed to dial Some(PeerId("QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt")): Failed to negotiate transport protocol(s): [(/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt): : Handshake failed: Handshake failed: Invalid public key: Key decoding error: RSA keys are unsupported)] Fixes #2971. --- core/CHANGELOG.md | 3 + core/Cargo.toml | 5 +- core/src/identity.rs | 47 +++----- core/src/identity/ecdsa.rs | 12 +-- core/src/identity/ed25519.rs | 6 +- core/src/identity/error.rs | 52 ++++++++- core/src/identity/rsa.rs | 4 +- core/src/identity/secp256k1.rs | 17 ++- core/src/upgrade/error.rs | 4 +- examples/ipfs-kad.rs | 79 +++++++------- swarm/src/lib.rs | 60 ++++++++++- transports/noise/CHANGELOG.md | 3 + transports/noise/Cargo.toml | 1 + transports/noise/src/error.rs | 107 ------------------- transports/noise/src/io/framed.rs | 37 +++---- transports/noise/src/io/handshake.rs | 9 +- transports/noise/src/lib.rs | 38 ++++++- transports/noise/src/protocol/x25519.rs | 2 +- transports/noise/src/protocol/x25519_spec.rs | 2 +- transports/noise/tests/smoke.rs | 4 +- 20 files changed, 245 insertions(+), 247 deletions(-) delete mode 100644 transports/noise/src/error.rs diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index ba80474b..4cc0f2ff 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -10,10 +10,13 @@ - Update `multistream-select` to `v0.12.1`. See [PR 3090]. +- Improve error messages in case keys cannot be decoded because of missing feature flags. See [PR 2972]. + [PR 3031]: https://github.com/libp2p/rust-libp2p/pull/3031 [PR 3058]: https://github.com/libp2p/rust-libp2p/pull/3058 [PR 3097]: https://github.com/libp2p/rust-libp2p/pull/3097 [PR 3090]: https://github.com/libp2p/rust-libp2p/pull/3090 +[PR 2972]: https://github.com/libp2p/rust-libp2p/pull/2972 # 0.37.0 diff --git a/core/Cargo.toml b/core/Cargo.toml index cdc50ee1..cb95d814 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -24,20 +24,21 @@ log = "0.4" multiaddr = { version = "0.16.0" } multihash = { version = "0.16", default-features = false, features = ["std", "multihash-impl", "identity", "sha2"] } multistream-select = { version = "0.12.1", path = "../misc/multistream-select" } -p256 = { version = "0.11.1", default-features = false, features = ["ecdsa"], optional = true } +p256 = { version = "0.11.1", default-features = false, features = ["ecdsa", "std"], optional = true } parking_lot = "0.12.0" pin-project = "1.0.0" prost = "0.11" once_cell = "1.16.0" rand = "0.8" rw-stream-sink = { version = "0.3.0", path = "../misc/rw-stream-sink" } +sec1 = { version = "0.3.0", features = ["std"] } # Activate `std` feature until https://github.com/RustCrypto/traits/pull/1131 is released. +serde = { version = "1", optional = true, features = ["derive"] } sha2 = "0.10.0" smallvec = "1.6.1" thiserror = "1.0" unsigned-varint = "0.7" void = "1" zeroize = "1" -serde = { version = "1", optional = true, features = ["derive"] } [target.'cfg(not(target_arch = "wasm32"))'.dependencies] ring = { version = "0.16.9", features = ["alloc", "std"], default-features = false, optional = true} diff --git a/core/src/identity.rs b/core/src/identity.rs index af5dceb6..45ed6be2 100644 --- a/core/src/identity.rs +++ b/core/src/identity.rs @@ -155,23 +155,11 @@ impl Keypair { data: data.encode().into(), }, #[cfg(all(feature = "rsa", not(target_arch = "wasm32")))] - Self::Rsa(_) => { - return Err(DecodingError::new( - "Encoding RSA key into Protobuf is unsupported", - )) - } + Self::Rsa(_) => return Err(DecodingError::encoding_unsupported("RSA")), #[cfg(feature = "secp256k1")] - Self::Secp256k1(_) => { - return Err(DecodingError::new( - "Encoding Secp256k1 key into Protobuf is unsupported", - )) - } + Self::Secp256k1(_) => return Err(DecodingError::encoding_unsupported("secp256k1")), #[cfg(feature = "ecdsa")] - Self::Ecdsa(_) => { - return Err(DecodingError::new( - "Encoding ECDSA key into Protobuf is unsupported", - )) - } + Self::Ecdsa(_) => return Err(DecodingError::encoding_unsupported("ECDSA")), }; Ok(pk.encode_to_vec()) @@ -182,26 +170,19 @@ impl Keypair { use prost::Message; let mut private_key = keys_proto::PrivateKey::decode(bytes) - .map_err(|e| DecodingError::new("Protobuf").source(e)) + .map_err(|e| DecodingError::bad_protobuf("private key bytes", e)) .map(zeroize::Zeroizing::new)?; - let key_type = keys_proto::KeyType::from_i32(private_key.r#type).ok_or_else(|| { - DecodingError::new(format!("unknown key type: {}", private_key.r#type)) - })?; + let key_type = keys_proto::KeyType::from_i32(private_key.r#type) + .ok_or_else(|| DecodingError::unknown_key_type(private_key.r#type))?; match key_type { keys_proto::KeyType::Ed25519 => { ed25519::Keypair::decode(&mut private_key.data).map(Keypair::Ed25519) } - keys_proto::KeyType::Rsa => Err(DecodingError::new( - "Decoding RSA key from Protobuf is unsupported.", - )), - keys_proto::KeyType::Secp256k1 => Err(DecodingError::new( - "Decoding Secp256k1 key from Protobuf is unsupported.", - )), - keys_proto::KeyType::Ecdsa => Err(DecodingError::new( - "Decoding ECDSA key from Protobuf is unsupported.", - )), + keys_proto::KeyType::Rsa => Err(DecodingError::decoding_unsupported("RSA")), + keys_proto::KeyType::Secp256k1 => Err(DecodingError::decoding_unsupported("secp256k1")), + keys_proto::KeyType::Ecdsa => Err(DecodingError::decoding_unsupported("ECDSA")), } } } @@ -268,7 +249,7 @@ impl PublicKey { use prost::Message; let pubkey = keys_proto::PublicKey::decode(bytes) - .map_err(|e| DecodingError::new("Protobuf").source(e))?; + .map_err(|e| DecodingError::bad_protobuf("public key bytes", e))?; pubkey.try_into() } @@ -310,7 +291,7 @@ impl TryFrom for PublicKey { fn try_from(pubkey: keys_proto::PublicKey) -> Result { let key_type = keys_proto::KeyType::from_i32(pubkey.r#type) - .ok_or_else(|| DecodingError::new(format!("unknown key type: {}", pubkey.r#type)))?; + .ok_or_else(|| DecodingError::unknown_key_type(pubkey.r#type))?; match key_type { keys_proto::KeyType::Ed25519 => { @@ -323,7 +304,7 @@ impl TryFrom for PublicKey { #[cfg(any(not(feature = "rsa"), target_arch = "wasm32"))] keys_proto::KeyType::Rsa => { log::debug!("support for RSA was disabled at compile-time"); - Err(DecodingError::new("Unsupported")) + Err(DecodingError::missing_feature("rsa")) } #[cfg(feature = "secp256k1")] keys_proto::KeyType::Secp256k1 => { @@ -332,7 +313,7 @@ impl TryFrom for PublicKey { #[cfg(not(feature = "secp256k1"))] keys_proto::KeyType::Secp256k1 => { log::debug!("support for secp256k1 was disabled at compile-time"); - Err(DecodingError::new("Unsupported")) + Err(DecodingError::missing_feature("secp256k1")) } #[cfg(feature = "ecdsa")] keys_proto::KeyType::Ecdsa => { @@ -341,7 +322,7 @@ impl TryFrom for PublicKey { #[cfg(not(feature = "ecdsa"))] keys_proto::KeyType::Ecdsa => { log::debug!("support for ECDSA was disabled at compile-time"); - Err(DecodingError::new("Unsupported")) + Err(DecodingError::missing_feature("ecdsa")) } } } diff --git a/core/src/identity/ecdsa.rs b/core/src/identity/ecdsa.rs index 4accdde2..9c2ef903 100644 --- a/core/src/identity/ecdsa.rs +++ b/core/src/identity/ecdsa.rs @@ -31,6 +31,7 @@ use p256::{ }, EncodedPoint, }; +use void::Void; /// An ECDSA keypair. #[derive(Clone)] @@ -107,7 +108,7 @@ impl SecretKey { /// Decode a secret key from a byte buffer. pub fn from_bytes(buf: &[u8]) -> Result { SigningKey::from_bytes(buf) - .map_err(|err| DecodingError::new("failed to parse ecdsa p256 secret key").source(err)) + .map_err(|err| DecodingError::failed_to_parse("ecdsa p256 secret key", err)) .map(SecretKey) } } @@ -134,12 +135,11 @@ impl PublicKey { /// Decode a public key from a byte buffer without compression. pub fn from_bytes(k: &[u8]) -> Result { - let enc_pt = EncodedPoint::from_bytes(k).map_err(|_| { - DecodingError::new("failed to parse ecdsa p256 public key, bad point encoding") - })?; + let enc_pt = EncodedPoint::from_bytes(k) + .map_err(|e| DecodingError::failed_to_parse("ecdsa p256 encoded point", e))?; VerifyingKey::from_encoded_point(&enc_pt) - .map_err(|err| DecodingError::new("failed to parse ecdsa p256 public key").source(err)) + .map_err(|err| DecodingError::failed_to_parse("ecdsa p256 public key", err)) .map(PublicKey) } @@ -157,7 +157,7 @@ impl PublicKey { /// Decode a public key into a DER encoded byte buffer as defined by SEC1 standard. pub fn decode_der(k: &[u8]) -> Result { let buf = Self::del_asn1_header(k).ok_or_else(|| { - DecodingError::new("failed to parse asn.1 encoded ecdsa p256 public key") + DecodingError::failed_to_parse::("ASN.1-encoded ecdsa p256 public key", None) })?; Self::from_bytes(buf) } diff --git a/core/src/identity/ed25519.rs b/core/src/identity/ed25519.rs index eef934d4..48cddf10 100644 --- a/core/src/identity/ed25519.rs +++ b/core/src/identity/ed25519.rs @@ -55,7 +55,7 @@ impl Keypair { kp.zeroize(); Keypair(k) }) - .map_err(|e| DecodingError::new("Ed25519 keypair").source(e)) + .map_err(|e| DecodingError::failed_to_parse("Ed25519 keypair", e)) } /// Sign a message using the private key of this keypair. @@ -169,7 +169,7 @@ impl PublicKey { /// Decode a public key from a byte array as produced by `encode`. pub fn decode(k: &[u8]) -> Result { ed25519::PublicKey::from_bytes(k) - .map_err(|e| DecodingError::new("Ed25519 public key").source(e)) + .map_err(|e| DecodingError::failed_to_parse("Ed25519 public key", e)) .map(PublicKey) } } @@ -215,7 +215,7 @@ impl SecretKey { pub fn from_bytes(mut sk_bytes: impl AsMut<[u8]>) -> Result { let sk_bytes = sk_bytes.as_mut(); let secret = ed25519::SecretKey::from_bytes(&*sk_bytes) - .map_err(|e| DecodingError::new("Ed25519 secret key").source(e))?; + .map_err(|e| DecodingError::failed_to_parse("Ed25519 secret key", e))?; sk_bytes.zeroize(); Ok(SecretKey(secret)) } diff --git a/core/src/identity/error.rs b/core/src/identity/error.rs index 32c7edc5..18430565 100644 --- a/core/src/identity/error.rs +++ b/core/src/identity/error.rs @@ -31,17 +31,61 @@ pub struct DecodingError { } impl DecodingError { - pub(crate) fn new(msg: S) -> Self { + #[cfg(not(all( + feature = "ecdsa", + feature = "rsa", + feature = "secp256k1", + not(target_arch = "wasm32") + )))] + pub(crate) fn missing_feature(feature_name: &'static str) -> Self { Self { - msg: msg.to_string(), + msg: format!("cargo feature `{feature_name}` is not enabled"), source: None, } } - pub(crate) fn source(self, source: impl Error + Send + Sync + 'static) -> Self { + pub(crate) fn failed_to_parse(what: &'static str, source: S) -> Self + where + E: Error + Send + Sync + 'static, + S: Into>, + { Self { + msg: format!("failed to parse {what}"), + source: match source.into() { + None => None, + Some(e) => Some(Box::new(e)), + }, + } + } + + pub(crate) fn bad_protobuf( + what: &'static str, + source: impl Error + Send + Sync + 'static, + ) -> Self { + Self { + msg: format!("failed to decode {what} from protobuf"), source: Some(Box::new(source)), - ..self + } + } + + pub(crate) fn unknown_key_type(key_type: i32) -> Self { + Self { + msg: format!("unknown key-type {key_type}"), + source: None, + } + } + + pub(crate) fn decoding_unsupported(key_type: &'static str) -> Self { + Self { + msg: format!("decoding {key_type} key from Protobuf is unsupported"), + source: None, + } + } + + pub(crate) fn encoding_unsupported(key_type: &'static str) -> Self { + Self { + msg: format!("encoding {key_type} key to Protobuf is unsupported"), + source: None, } } } diff --git a/core/src/identity/rsa.rs b/core/src/identity/rsa.rs index 54dbe47f..e3c10a6e 100644 --- a/core/src/identity/rsa.rs +++ b/core/src/identity/rsa.rs @@ -48,7 +48,7 @@ impl Keypair { /// [RFC5208]: https://tools.ietf.org/html/rfc5208#section-5 pub fn from_pkcs8(der: &mut [u8]) -> Result { let kp = RsaKeyPair::from_pkcs8(der) - .map_err(|e| DecodingError::new("RSA PKCS#8 PrivateKeyInfo").source(e))?; + .map_err(|e| DecodingError::failed_to_parse("RSA PKCS#8 PrivateKeyInfo", e))?; der.zeroize(); Ok(Keypair(Arc::new(kp))) } @@ -111,7 +111,7 @@ impl PublicKey { /// structure. See also `encode_x509`. pub fn decode_x509(pk: &[u8]) -> Result { Asn1SubjectPublicKeyInfo::decode(pk) - .map_err(|e| DecodingError::new("RSA X.509").source(e)) + .map_err(|e| DecodingError::failed_to_parse("RSA X.509", e)) .map(|spki| spki.subjectPublicKey.0) } } diff --git a/core/src/identity/secp256k1.rs b/core/src/identity/secp256k1.rs index bfecc33e..51aa073f 100644 --- a/core/src/identity/secp256k1.rs +++ b/core/src/identity/secp256k1.rs @@ -100,7 +100,7 @@ impl SecretKey { pub fn from_bytes(mut sk: impl AsMut<[u8]>) -> Result { let sk_bytes = sk.as_mut(); let secret = libsecp256k1::SecretKey::parse_slice(&*sk_bytes) - .map_err(|_| DecodingError::new("failed to parse secp256k1 secret key"))?; + .map_err(|e| DecodingError::failed_to_parse("parse secp256k1 secret key", e))?; sk_bytes.zeroize(); Ok(SecretKey(secret)) } @@ -112,13 +112,12 @@ impl SecretKey { pub fn from_der(mut der: impl AsMut<[u8]>) -> Result { // TODO: Stricter parsing. let der_obj = der.as_mut(); - let obj: Sequence = DerDecodable::decode(der_obj) - .map_err(|e| DecodingError::new("Secp256k1 DER ECPrivateKey").source(e))?; - let sk_obj = obj - .get(1) - .map_err(|e| DecodingError::new("Not enough elements in DER").source(e))?; - let mut sk_bytes: Vec = - asn1_der::typed::DerDecodable::load(sk_obj).map_err(DecodingError::new)?; + + let mut sk_bytes = Sequence::decode(der_obj) + .and_then(|seq| seq.get(1)) + .and_then(Vec::load) + .map_err(|e| DecodingError::failed_to_parse("secp256k1 SecretKey bytes", e))?; + let sk = SecretKey::from_bytes(&mut sk_bytes)?; sk_bytes.zeroize(); der_obj.zeroize(); @@ -217,7 +216,7 @@ impl PublicKey { /// by `encode`. pub fn decode(k: &[u8]) -> Result { libsecp256k1::PublicKey::parse_slice(k, Some(libsecp256k1::PublicKeyFormat::Compressed)) - .map_err(|_| DecodingError::new("failed to parse secp256k1 public key")) + .map_err(|e| DecodingError::failed_to_parse("secp256k1 public key", e)) .map(PublicKey) } } diff --git a/core/src/upgrade/error.rs b/core/src/upgrade/error.rs index 2bbe95ec..3d349587 100644 --- a/core/src/upgrade/error.rs +++ b/core/src/upgrade/error.rs @@ -55,8 +55,8 @@ where { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - UpgradeError::Select(e) => write!(f, "select error: {}", e), - UpgradeError::Apply(e) => write!(f, "upgrade apply error: {}", e), + UpgradeError::Select(_) => write!(f, "Multistream select failed"), + UpgradeError::Apply(_) => write!(f, "Handshake failed"), } } } diff --git a/examples/ipfs-kad.rs b/examples/ipfs-kad.rs index 2c370472..3764f8c9 100644 --- a/examples/ipfs-kad.rs +++ b/examples/ipfs-kad.rs @@ -23,16 +23,15 @@ //! You can pass as parameter a base58 peer ID to search for. If you don't pass any parameter, a //! peer ID will be generated randomly. -use async_std::task; use futures::StreamExt; use libp2p::kad::record::store::MemoryStore; use libp2p::kad::{GetClosestPeersError, Kademlia, KademliaConfig, KademliaEvent, QueryResult}; use libp2p::{ development_transport, identity, swarm::{Swarm, SwarmEvent}, - Multiaddr, PeerId, + PeerId, }; -use std::{env, error::Error, str::FromStr, time::Duration}; +use std::{env, error::Error, time::Duration}; const BOOTNODES: [&str; 4] = [ "QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN", @@ -63,58 +62,54 @@ async fn main() -> Result<(), Box> { // Add the bootnodes to the local routing table. `libp2p-dns` built // into the `transport` resolves the `dnsaddr` when Kademlia tries // to dial these nodes. - let bootaddr = Multiaddr::from_str("/dnsaddr/bootstrap.libp2p.io")?; for peer in &BOOTNODES { - behaviour.add_address(&PeerId::from_str(peer)?, bootaddr.clone()); + behaviour.add_address(&peer.parse()?, "/dnsaddr/bootstrap.libp2p.io".parse()?); } Swarm::with_async_std_executor(transport, behaviour, local_peer_id) }; // Order Kademlia to search for a peer. - let to_search: PeerId = if let Some(peer_id) = env::args().nth(1) { - peer_id.parse()? - } else { - identity::Keypair::generate_ed25519().public().into() - }; + let to_search = env::args() + .nth(1) + .map(|p| p.parse()) + .transpose()? + .unwrap_or_else(PeerId::random); - println!("Searching for the closest peers to {to_search:?}"); + println!("Searching for the closest peers to {to_search}"); swarm.behaviour_mut().get_closest_peers(to_search); - // Kick it off! - task::block_on(async move { - loop { - let event = swarm.select_next_some().await; - if let SwarmEvent::Behaviour(KademliaEvent::OutboundQueryCompleted { - result: QueryResult::GetClosestPeers(result), - .. - }) = event - { - match result { - Ok(ok) => { - if !ok.peers.is_empty() { - println!("Query finished with closest peers: {:#?}", ok.peers) - } else { - // The example is considered failed as there - // should always be at least 1 reachable peer. - println!("Query finished with no closest peers.") - } + loop { + let event = swarm.select_next_some().await; + if let SwarmEvent::Behaviour(KademliaEvent::OutboundQueryCompleted { + result: QueryResult::GetClosestPeers(result), + .. + }) = event + { + match result { + Ok(ok) => { + if !ok.peers.is_empty() { + println!("Query finished with closest peers: {:#?}", ok.peers) + } else { + // The example is considered failed as there + // should always be at least 1 reachable peer. + println!("Query finished with no closest peers.") } - Err(GetClosestPeersError::Timeout { peers, .. }) => { - if !peers.is_empty() { - println!("Query timed out with closest peers: {peers:#?}") - } else { - // The example is considered failed as there - // should always be at least 1 reachable peer. - println!("Query timed out with no closest peers."); - } + } + Err(GetClosestPeersError::Timeout { peers, .. }) => { + if !peers.is_empty() { + println!("Query timed out with closest peers: {peers:#?}") + } else { + // The example is considered failed as there + // should always be at least 1 reachable peer. + println!("Query timed out with no closest peers."); } - }; + } + }; - break; - } + break; } + } - Ok(()) - }) + Ok(()) } diff --git a/swarm/src/lib.rs b/swarm/src/lib.rs index c467b99c..6d12f289 100644 --- a/swarm/src/lib.rs +++ b/swarm/src/lib.rs @@ -1660,17 +1660,45 @@ impl fmt::Display for DialError { f, "Dial error: Pending connection attempt has been aborted." ), - DialError::InvalidPeerId(multihash) => write!(f, "Dial error: multihash {:?} is not a PeerId", multihash), - DialError::WrongPeerId { obtained, endpoint} => write!(f, "Dial error: Unexpected peer ID {} at {:?}.", obtained, endpoint), + DialError::InvalidPeerId(multihash) => { + write!(f, "Dial error: multihash {:?} is not a PeerId", multihash) + } + DialError::WrongPeerId { obtained, endpoint } => write!( + f, + "Dial error: Unexpected peer ID {} at {:?}.", + obtained, endpoint + ), DialError::ConnectionIo(e) => write!( f, - "Dial error: An I/O error occurred on the connection: {:?}.", e + "Dial error: An I/O error occurred on the connection: {:?}.", + e ), - DialError::Transport(e) => write!(f, "An error occurred while negotiating the transport protocol(s) on a connection: {:?}.", e), + DialError::Transport(errors) => { + write!(f, "Failed to negotiate transport protocol(s): [")?; + + for (addr, error) in errors { + write!(f, "({addr}")?; + print_error_chain(f, error)?; + write!(f, ")")?; + } + write!(f, "]")?; + + Ok(()) + } } } } +fn print_error_chain(f: &mut fmt::Formatter<'_>, e: &dyn error::Error) -> fmt::Result { + write!(f, ": {e}")?; + + if let Some(source) = e.source() { + print_error_chain(f, source)?; + } + + Ok(()) +} + impl error::Error for DialError { fn source(&self) -> Option<&(dyn error::Error + 'static)> { match self { @@ -1745,13 +1773,16 @@ mod tests { use futures::future::poll_fn; use futures::future::Either; use futures::{executor, future, ready}; + use libp2p_core::either::EitherError; use libp2p_core::multiaddr::multiaddr; + use libp2p_core::transport::memory::MemoryTransportError; use libp2p_core::transport::TransportEvent; - use libp2p_core::Endpoint; use libp2p_core::{identity, multiaddr, transport, upgrade}; + use libp2p_core::{Endpoint, UpgradeError}; use libp2p_plaintext as plaintext; use libp2p_yamux as yamux; use quickcheck::*; + use void::Void; // Test execution state. // Connection => Disconnecting => Connecting. @@ -2610,4 +2641,23 @@ mod tests { e => panic!("Unexpected swarm event {:?}.", e), } } + + #[test] + fn dial_error_prints_sources() { + // This constitutes a fairly typical error for chained transports. + let error = DialError::Transport(vec![( + "/ip4/127.0.0.1/tcp/80".parse().unwrap(), + TransportError::Other(io::Error::new( + io::ErrorKind::Other, + EitherError::<_, Void>::A(EitherError::::B(UpgradeError::Apply( + MemoryTransportError::Unreachable, + ))), + )), + )]); + + let string = format!("{error}"); + + // Unfortunately, we have some "empty" errors that lead to multiple colons without text but that is the best we can do. + assert_eq!("Failed to negotiate transport protocol(s): [(/ip4/127.0.0.1/tcp/80: : Handshake failed: No listener on the given port.)]", string) + } } diff --git a/transports/noise/CHANGELOG.md b/transports/noise/CHANGELOG.md index 24bd7d11..15b72e33 100644 --- a/transports/noise/CHANGELOG.md +++ b/transports/noise/CHANGELOG.md @@ -6,8 +6,11 @@ - Update `rust-version` to reflect the actual MSRV: 1.60.0. See [PR 3090]. +- Introduce more variants to `NoiseError` to better differentiate between failure cases during authentication. See [PR 2972]. + [PR 3058]: https://github.com/libp2p/rust-libp2p/pull/3058 [PR 3090]: https://github.com/libp2p/rust-libp2p/pull/3090 +[PR 2972]: https://github.com/libp2p/rust-libp2p/pull/2972 # 0.40.0 diff --git a/transports/noise/Cargo.toml b/transports/noise/Cargo.toml index a64d7fe3..fda1ca93 100644 --- a/transports/noise/Cargo.toml +++ b/transports/noise/Cargo.toml @@ -19,6 +19,7 @@ prost = "0.11" rand = "0.8.3" sha2 = "0.10.0" static_assertions = "1" +thiserror = "1.0.37" x25519-dalek = "1.1.0" zeroize = "1" diff --git a/transports/noise/src/error.rs b/transports/noise/src/error.rs deleted file mode 100644 index 4a9df3a6..00000000 --- a/transports/noise/src/error.rs +++ /dev/null @@ -1,107 +0,0 @@ -// Copyright 2019 Parity Technologies (UK) Ltd. -// -// Permission is hereby granted, free of charge, to any person obtaining a -// copy of this software and associated documentation files (the "Software"), -// to deal in the Software without restriction, including without limitation -// the rights to use, copy, modify, merge, publish, distribute, sublicense, -// and/or sell copies of the Software, and to permit persons to whom the -// Software is furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS -// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING -// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER -// DEALINGS IN THE SOFTWARE. - -use libp2p_core::identity; -use snow::error::Error as SnowError; -use std::{error::Error, fmt, io}; - -/// libp2p_noise error type. -#[derive(Debug)] -#[non_exhaustive] -pub enum NoiseError { - /// An I/O error has been encountered. - Io(io::Error), - /// An noise framework error has been encountered. - Noise(SnowError), - /// A public key is invalid. - InvalidKey, - /// Authentication in a [`NoiseAuthenticated`](crate::NoiseAuthenticated) - /// upgrade failed. - AuthenticationFailed, - /// A handshake payload is invalid. - InvalidPayload(DecodeError), - /// A signature was required and could not be created. - SigningError(identity::error::SigningError), -} - -#[derive(Debug)] -pub struct DecodeError(prost::DecodeError); - -impl fmt::Display for DecodeError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0.fmt(f) - } -} - -impl Error for DecodeError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - self.0.source() - } -} - -impl fmt::Display for NoiseError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - NoiseError::Io(e) => write!(f, "{}", e), - NoiseError::Noise(e) => write!(f, "{}", e), - NoiseError::InvalidKey => f.write_str("invalid public key"), - NoiseError::InvalidPayload(e) => write!(f, "{}", e), - NoiseError::AuthenticationFailed => f.write_str("Authentication failed"), - NoiseError::SigningError(e) => write!(f, "{}", e), - } - } -} - -impl Error for NoiseError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - match self { - NoiseError::Io(e) => Some(e), - NoiseError::Noise(_) => None, // TODO: `SnowError` should implement `Error`. - NoiseError::InvalidKey => None, - NoiseError::AuthenticationFailed => None, - NoiseError::InvalidPayload(e) => Some(e), - NoiseError::SigningError(e) => Some(e), - } - } -} - -impl From for NoiseError { - fn from(e: io::Error) -> Self { - NoiseError::Io(e) - } -} - -impl From for NoiseError { - fn from(e: SnowError) -> Self { - NoiseError::Noise(e) - } -} - -impl From for NoiseError { - fn from(e: prost::DecodeError) -> Self { - NoiseError::InvalidPayload(DecodeError(e)) - } -} - -impl From for NoiseError { - fn from(e: identity::error::SigningError) -> Self { - NoiseError::SigningError(e) - } -} diff --git a/transports/noise/src/io/framed.rs b/transports/noise/src/io/framed.rs index 4ca228eb..b0486037 100644 --- a/transports/noise/src/io/framed.rs +++ b/transports/noise/src/io/framed.rs @@ -94,28 +94,23 @@ impl NoiseFramed { where C: Protocol + AsRef<[u8]>, { - let dh_remote_pubkey = match self.session.get_remote_static() { - None => None, - Some(k) => match C::public_from_bytes(k) { - Err(e) => return Err(e), - Ok(dh_pk) => Some(dh_pk), - }, + let dh_remote_pubkey = self + .session + .get_remote_static() + .map(C::public_from_bytes) + .transpose()?; + + let io = NoiseFramed { + session: self.session.into_transport_mode()?, + io: self.io, + read_state: ReadState::Ready, + write_state: WriteState::Ready, + read_buffer: self.read_buffer, + write_buffer: self.write_buffer, + decrypt_buffer: self.decrypt_buffer, }; - match self.session.into_transport_mode() { - Err(e) => Err(e.into()), - Ok(s) => { - let io = NoiseFramed { - session: s, - io: self.io, - read_state: ReadState::Ready, - write_state: WriteState::Ready, - read_buffer: self.read_buffer, - write_buffer: self.write_buffer, - decrypt_buffer: self.decrypt_buffer, - }; - Ok((dh_remote_pubkey, NoiseOutput::new(io))) - } - } + + Ok((dh_remote_pubkey, NoiseOutput::new(io))) } } diff --git a/transports/noise/src/io/handshake.rs b/transports/noise/src/io/handshake.rs index c32eef96..e57288d5 100644 --- a/transports/noise/src/io/handshake.rs +++ b/transports/noise/src/io/handshake.rs @@ -25,10 +25,10 @@ mod payload_proto { include!(concat!(env!("OUT_DIR"), "/payload.proto.rs")); } -use crate::error::NoiseError; use crate::io::{framed::NoiseFramed, NoiseOutput}; use crate::protocol::{KeypairIdentity, Protocol, PublicKey}; use crate::LegacyConfig; +use crate::NoiseError; use bytes::Bytes; use futures::prelude::*; use libp2p_core::identity; @@ -118,7 +118,7 @@ impl State { if C::verify(&id_pk, &dh_pk, &self.dh_remote_pubkey_sig) { RemoteIdentity::IdentityKey(id_pk) } else { - return Err(NoiseError::InvalidKey); + return Err(NoiseError::BadSignature); } } }; @@ -208,11 +208,10 @@ where let pb = pb_result?; if !pb.identity_key.is_empty() { - let pk = identity::PublicKey::from_protobuf_encoding(&pb.identity_key) - .map_err(|_| NoiseError::InvalidKey)?; + let pk = identity::PublicKey::from_protobuf_encoding(&pb.identity_key)?; if let Some(ref k) = state.id_remote_pubkey { if k != &pk { - return Err(NoiseError::InvalidKey); + return Err(NoiseError::UnexpectedKey); } } state.id_remote_pubkey = Some(pk); diff --git a/transports/noise/src/lib.rs b/transports/noise/src/lib.rs index d710e163..1837bdbd 100644 --- a/transports/noise/src/lib.rs +++ b/transports/noise/src/lib.rs @@ -55,11 +55,9 @@ #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] -mod error; mod io; mod protocol; -pub use error::NoiseError; pub use io::handshake::RemoteIdentity; pub use io::NoiseOutput; pub use protocol::{x25519::X25519, x25519_spec::X25519Spec}; @@ -241,6 +239,42 @@ where } } +/// libp2p_noise error type. +#[derive(Debug, thiserror::Error)] +#[non_exhaustive] +pub enum NoiseError { + #[error(transparent)] + Io(#[from] std::io::Error), + #[error(transparent)] + Noise(#[from] snow::Error), + #[error("Invalid public key")] + InvalidKey(#[from] identity::error::DecodingError), + #[error("Only keys of length 32 bytes are supported")] + InvalidLength, + #[error("Remote authenticated with an unexpected public key")] + UnexpectedKey, + #[error("The signature of the remote identity's public key does not verify")] + BadSignature, + #[error("Authentication failed")] + AuthenticationFailed, + #[error(transparent)] + InvalidPayload(DecodeError), + #[error(transparent)] + SigningError(#[from] identity::error::SigningError), +} + +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct DecodeError(prost::DecodeError); + +impl From for NoiseError { + fn from(e: prost::DecodeError) -> Self { + NoiseError::InvalidPayload(DecodeError(e)) + } +} + +// Handshake pattern IX ///////////////////////////////////////////////////// + /// Implements the responder part of the `IX` noise handshake pattern. /// /// `IX` is a single round-trip (2 messages) handshake in which each party sends their identity over to the other party. diff --git a/transports/noise/src/protocol/x25519.rs b/transports/noise/src/protocol/x25519.rs index a1d542ae..067f67ca 100644 --- a/transports/noise/src/protocol/x25519.rs +++ b/transports/noise/src/protocol/x25519.rs @@ -117,7 +117,7 @@ impl Protocol for X25519 { fn public_from_bytes(bytes: &[u8]) -> Result, NoiseError> { if bytes.len() != 32 { - return Err(NoiseError::InvalidKey); + return Err(NoiseError::InvalidLength); } let mut pk = [0u8; 32]; pk.copy_from_slice(bytes); diff --git a/transports/noise/src/protocol/x25519_spec.rs b/transports/noise/src/protocol/x25519_spec.rs index b0ae73d9..e114f107 100644 --- a/transports/noise/src/protocol/x25519_spec.rs +++ b/transports/noise/src/protocol/x25519_spec.rs @@ -123,7 +123,7 @@ impl Protocol for X25519Spec { fn public_from_bytes(bytes: &[u8]) -> Result, NoiseError> { if bytes.len() != 32 { - return Err(NoiseError::InvalidKey); + return Err(NoiseError::InvalidLength); } let mut pk = [0u8; 32]; pk.copy_from_slice(bytes); diff --git a/transports/noise/tests/smoke.rs b/transports/noise/tests/smoke.rs index 7ec1f396..b9fe0027 100644 --- a/transports/noise/tests/smoke.rs +++ b/transports/noise/tests/smoke.rs @@ -27,10 +27,10 @@ use libp2p::core::identity; use libp2p::core::transport::{self, Transport}; use libp2p::core::upgrade::{self, apply_inbound, apply_outbound, Negotiated}; use libp2p::noise::{ - Keypair, NoiseAuthenticated, NoiseConfig, NoiseError, NoiseOutput, RemoteIdentity, X25519Spec, - X25519, + Keypair, NoiseAuthenticated, NoiseConfig, NoiseOutput, RemoteIdentity, X25519Spec, X25519, }; use libp2p::tcp; +use libp2p_noise::NoiseError; use log::info; use quickcheck::*; use std::{convert::TryInto, io, net::TcpStream};