From 6748e170f28713c541e80fa828af4704c8b09643 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Tue, 14 Nov 2023 19:32:24 +0100 Subject: [PATCH 01/18] feat: MVP verification of the expected `PeerId` as part of a protocol handshake. --- core/src/transport/upgrade.rs | 24 ++++------ core/src/upgrade.rs | 31 ++++++++++-- core/src/upgrade/secure.rs | 88 +++++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 20 deletions(-) create mode 100644 core/src/upgrade/secure.rs diff --git a/core/src/transport/upgrade.rs b/core/src/transport/upgrade.rs index 8525ab741ff..6207c2c877b 100644 --- a/core/src/transport/upgrade.rs +++ b/core/src/transport/upgrade.rs @@ -31,11 +31,11 @@ use crate::{ }, upgrade::{ self, apply_inbound, apply_outbound, InboundConnectionUpgrade, InboundUpgradeApply, - OutboundConnectionUpgrade, OutboundUpgradeApply, UpgradeError, + OutboundConnectionUpgrade, OutboundUpgradeApply, SecurityUpgrade, UpgradeError, }, Negotiated, }; -use futures::{prelude::*, ready}; +use futures::{future::LocalBoxFuture, prelude::*, ready}; use libp2p_identity::PeerId; use multiaddr::Multiaddr; use std::{ @@ -99,16 +99,15 @@ where ) -> Authenticated Authenticate + Clone>> where T: Transport, - C: AsyncRead + AsyncWrite + Unpin, + C: AsyncRead + AsyncWrite + Unpin + 'static, D: AsyncRead + AsyncWrite + Unpin, - U: InboundConnectionUpgrade, Output = (PeerId, D), Error = E>, - U: OutboundConnectionUpgrade, Output = (PeerId, D), Error = E> + Clone, + U: SecurityUpgrade, Output = (PeerId, D), Error = E> + Clone + 'static, E: Error + 'static, { let version = self.version; Authenticated(Builder::new( self.inner.and_then(move |conn, endpoint| Authenticate { - inner: upgrade::apply(conn, upgrade, endpoint, version), + inner: upgrade::secure(conn, upgrade, endpoint, version).boxed_local(), }), version, )) @@ -123,23 +122,18 @@ where pub struct Authenticate where C: AsyncRead + AsyncWrite + Unpin, - U: InboundConnectionUpgrade> + OutboundConnectionUpgrade>, + U: SecurityUpgrade>, { #[pin] - inner: EitherUpgrade, + inner: LocalBoxFuture<'static, Result>>, } impl Future for Authenticate where C: AsyncRead + AsyncWrite + Unpin, - U: InboundConnectionUpgrade> - + OutboundConnectionUpgrade< - Negotiated, - Output = >>::Output, - Error = >>::Error, - >, + U: SecurityUpgrade>, { - type Output = as Future>::Output; + type Output = Result>; fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { let this = self.project(); diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index 69561fbebd8..e9c516190ee 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -63,19 +63,21 @@ mod either; mod error; mod pending; mod ready; +mod secure; mod select; +pub use self::{ + denied::DeniedUpgrade, pending::PendingUpgrade, ready::ReadyUpgrade, select::SelectUpgrade, +}; +pub use crate::Negotiated; pub(crate) use apply::{ apply, apply_inbound, apply_outbound, InboundUpgradeApply, OutboundUpgradeApply, }; pub(crate) use error::UpgradeError; use futures::future::Future; - -pub use self::{ - denied::DeniedUpgrade, pending::PendingUpgrade, ready::ReadyUpgrade, select::SelectUpgrade, -}; -pub use crate::Negotiated; +use libp2p_identity::PeerId; pub use multistream_select::{NegotiatedComplete, NegotiationError, ProtocolError, Version}; +pub(crate) use secure::secure; /// Common trait for upgrades that can be applied on inbound substreams, outbound substreams, /// or both. @@ -152,3 +154,22 @@ pub trait OutboundConnectionUpgrade: UpgradeInfo { /// The `info` is the identifier of the protocol, as produced by `protocol_info`. fn upgrade_outbound(self, socket: T, info: Self::Info) -> Self::Future; } + +/// Possible security upgrade on a connection +pub trait SecurityUpgrade: UpgradeInfo { + /// Output after the upgrade has been successfully negotiated and the handshake performed. + type Output; + /// Possible error during the handshake. + type Error; + /// Future that performs the handshake with the remote. + type Future: Future>; + + /// After we have determined that the remote supports one of the protocols we support, this + /// method is called to start the handshake. + /// + /// The `info` is the identifier of the protocol, as produced by `protocol_info`. Security + /// transports use the optional `peer_id` parameter on outgoing upgrades to validate validate + /// the expected `PeerId`. + fn upgrade_security(self, socket: T, info: Self::Info, peer_id: Option) + -> Self::Future; +} diff --git a/core/src/upgrade/secure.rs b/core/src/upgrade/secure.rs new file mode 100644 index 00000000000..6c075e1d8e0 --- /dev/null +++ b/core/src/upgrade/secure.rs @@ -0,0 +1,88 @@ +//! After a successful protocol negotiation as part of the upgrade process, the `SecurityUpgrade::upgrade_security` +//! method is called and a [`Future`] that performs a handshake is returned. + +use crate::upgrade::{SecurityUpgrade, UpgradeError}; +use crate::{connection::ConnectedPoint, Negotiated}; +use futures::prelude::*; + +use libp2p_identity::PeerId; +use multiaddr::Protocol; +use multistream_select::Version; + +/// Applies a security upgrade to the inbound and outbound direction of a connection or substream. +pub(crate) async fn secure( + conn: C, + up: U, + cp: ConnectedPoint, + v: Version, +) -> Result> +where + C: AsyncRead + AsyncWrite + Unpin, + U: SecurityUpgrade>, +{ + match cp { + ConnectedPoint::Dialer { role_override, .. } if role_override.is_dialer() => { + let peer_id = cp + .get_remote_address() + .iter() + .find_map(|protocol| match protocol { + Protocol::P2p(peer_id) => Some(peer_id), + _ => None, + }) + .expect("It should have /p2p as part of the address"); + secure_outbound(conn, up, Some(peer_id), v).await + } + _ => secure_inbound(conn, up, None).await, + } +} + +/// Tries to perform a security upgrade on an inbound connection or substream. +async fn secure_inbound( + conn: C, + up: U, + peer_id: Option, +) -> Result> +where + C: AsyncRead + AsyncWrite + Unpin, + U: SecurityUpgrade>, +{ + let (info, stream) = + multistream_select::listener_select_proto(conn, up.protocol_info()).await?; + let name = info.as_ref().to_owned(); + match up.upgrade_security(stream, info, peer_id).await { + Ok(x) => { + tracing::trace!(up=%name, "Secured inbound stream"); + Ok(x) + } + Err(e) => { + tracing::trace!(up=%name, "Failed to secure inbound stream"); + Err(UpgradeError::Apply(e)) + } + } +} + +/// Tries to perform a security upgrade on an outbound connection or substream. +async fn secure_outbound( + conn: C, + up: U, + peer_id: Option, + v: Version, +) -> Result> +where + C: AsyncRead + AsyncWrite + Unpin, + U: SecurityUpgrade>, +{ + let (info, stream) = + multistream_select::dialer_select_proto(conn, up.protocol_info(), v).await?; + let name = info.as_ref().to_owned(); + match up.upgrade_security(stream, info, peer_id).await { + Ok(x) => { + tracing::trace!(up=%name, "Secured outbound stream"); + Ok(x) + } + Err(e) => { + tracing::trace!(up=%name, "Failed to secure outbound stream"); + Err(UpgradeError::Apply(e)) + } + } +} From dbedc1d16d290b3bd24df235ff273ca27c699d2c Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Wed, 15 Nov 2023 13:41:20 +0100 Subject: [PATCH 02/18] chore: fix a typo in the doc comments --- core/src/upgrade.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index e9c516190ee..4f3f03cbd91 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -168,8 +168,8 @@ pub trait SecurityUpgrade: UpgradeInfo { /// method is called to start the handshake. /// /// The `info` is the identifier of the protocol, as produced by `protocol_info`. Security - /// transports use the optional `peer_id` parameter on outgoing upgrades to validate validate - /// the expected `PeerId`. + /// transports use the optional `peer_id` parameter on outgoing upgrades to validate the + /// expected `PeerId`. fn upgrade_security(self, socket: T, info: Self::Info, peer_id: Option) -> Self::Future; } From d8003203a2c476357deecf93a7ce049c710af013 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Thu, 16 Nov 2023 13:18:29 +0100 Subject: [PATCH 03/18] fix: introduce `Builder::authenticate2` as an alternative code path Introduce `Builder::authenticate2` as an alternative code path given the supplied upgrade implements `SecurityUpgrade` instead of `InboundConnectionUpgrade`/`OutboundConnectionUpgrade`. Inline `secure_inbound` and `secure_outound` functions into `secure`. Fix `PeerId` in the output of `SecurityUpgrade::upgrade_security`. Remove the unnecessary call to `panic`. Co-authored-by: Thomas Eizinger --- core/src/transport/upgrade.rs | 73 ++++++++++++++++++++++++++++-- core/src/upgrade.rs | 2 +- core/src/upgrade/secure.rs | 84 ++++++++++++----------------------- 3 files changed, 99 insertions(+), 60 deletions(-) diff --git a/core/src/transport/upgrade.rs b/core/src/transport/upgrade.rs index 6207c2c877b..30abd3d8372 100644 --- a/core/src/transport/upgrade.rs +++ b/core/src/transport/upgrade.rs @@ -97,6 +97,39 @@ where self, upgrade: U, ) -> Authenticated Authenticate + Clone>> + where + T: Transport, + C: AsyncRead + AsyncWrite + Unpin, + D: AsyncRead + AsyncWrite + Unpin, + U: InboundConnectionUpgrade, Output = (PeerId, D), Error = E>, + U: OutboundConnectionUpgrade, Output = (PeerId, D), Error = E> + Clone, + E: Error + 'static, + { + let version = self.version; + Authenticated(Builder::new( + self.inner.and_then(move |conn, endpoint| Authenticate { + inner: upgrade::apply(conn, upgrade, endpoint, version), + }), + version, + )) + } + + /// Upgrades the transport to perform authentication of the remote + /// + /// The supplied upgrade receives the I/O resource `C` and must + /// produce a pair `(PeerId, D)`, where `D` is a new I/O resource. + /// The upgrade must thus at a minimum identify the remote, which typically + /// involves the use of a cryptographic authentication protocol in the + /// context of establishing a secure channel. + /// + /// ## Transitions + /// + /// * I/O upgrade: `C -> (PeerId, D)`. + /// * Transport output: `C -> (PeerId, D)` + pub fn authenticate2( + self, + upgrade: U, + ) -> Authenticated Authenticate2 + Clone>> where T: Transport, C: AsyncRead + AsyncWrite + Unpin + 'static, @@ -106,7 +139,7 @@ where { let version = self.version; Authenticated(Builder::new( - self.inner.and_then(move |conn, endpoint| Authenticate { + self.inner.and_then(move |conn, endpoint| Authenticate2 { inner: upgrade::secure(conn, upgrade, endpoint, version).boxed_local(), }), version, @@ -122,18 +155,50 @@ where pub struct Authenticate where C: AsyncRead + AsyncWrite + Unpin, - U: SecurityUpgrade>, + U: InboundConnectionUpgrade> + OutboundConnectionUpgrade>, { #[pin] - inner: LocalBoxFuture<'static, Result>>, + inner: EitherUpgrade, } impl Future for Authenticate +where + C: AsyncRead + AsyncWrite + Unpin, + U: InboundConnectionUpgrade> + + OutboundConnectionUpgrade< + Negotiated, + Output = >>::Output, + Error = >>::Error, + >, +{ + type Output = as Future>::Output; + + fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + let this = self.project(); + Future::poll(this.inner, cx) + } +} + +/// An upgrade that authenticates the remote peer, typically +/// in the context of negotiating a secure channel. +/// +/// Configured through [`Builder::authenticate`]. +#[pin_project::pin_project] +pub struct Authenticate2 +where + C: AsyncRead + AsyncWrite + Unpin, + U: SecurityUpgrade>, +{ + #[pin] + inner: LocalBoxFuture<'static, Result<(PeerId, U::Output), UpgradeError>>, +} + +impl Future for Authenticate2 where C: AsyncRead + AsyncWrite + Unpin, U: SecurityUpgrade>, { - type Output = Result>; + type Output = Result<(PeerId, U::Output), UpgradeError>; fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { let this = self.project(); diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index 4f3f03cbd91..46e9565e775 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -162,7 +162,7 @@ pub trait SecurityUpgrade: UpgradeInfo { /// Possible error during the handshake. type Error; /// Future that performs the handshake with the remote. - type Future: Future>; + type Future: Future>; /// After we have determined that the remote supports one of the protocols we support, this /// method is called to start the handshake. diff --git a/core/src/upgrade/secure.rs b/core/src/upgrade/secure.rs index 6c075e1d8e0..e63d59edf21 100644 --- a/core/src/upgrade/secure.rs +++ b/core/src/upgrade/secure.rs @@ -15,7 +15,7 @@ pub(crate) async fn secure( up: U, cp: ConnectedPoint, v: Version, -) -> Result> +) -> Result<(PeerId, U::Output), UpgradeError> where C: AsyncRead + AsyncWrite + Unpin, U: SecurityUpgrade>, @@ -28,61 +28,35 @@ where .find_map(|protocol| match protocol { Protocol::P2p(peer_id) => Some(peer_id), _ => None, - }) - .expect("It should have /p2p as part of the address"); - secure_outbound(conn, up, Some(peer_id), v).await + }); + let (info, stream) = + multistream_select::dialer_select_proto(conn, up.protocol_info(), v).await?; + let name = info.as_ref().to_owned(); + match up.upgrade_security(stream, info, peer_id).await { + Ok(x) => { + tracing::trace!(up=%name, "Secured outbound stream"); + Ok(x) + } + Err(e) => { + tracing::trace!(up=%name, "Failed to secure outbound stream"); + Err(UpgradeError::Apply(e)) + } + } } - _ => secure_inbound(conn, up, None).await, - } -} - -/// Tries to perform a security upgrade on an inbound connection or substream. -async fn secure_inbound( - conn: C, - up: U, - peer_id: Option, -) -> Result> -where - C: AsyncRead + AsyncWrite + Unpin, - U: SecurityUpgrade>, -{ - let (info, stream) = - multistream_select::listener_select_proto(conn, up.protocol_info()).await?; - let name = info.as_ref().to_owned(); - match up.upgrade_security(stream, info, peer_id).await { - Ok(x) => { - tracing::trace!(up=%name, "Secured inbound stream"); - Ok(x) - } - Err(e) => { - tracing::trace!(up=%name, "Failed to secure inbound stream"); - Err(UpgradeError::Apply(e)) - } - } -} - -/// Tries to perform a security upgrade on an outbound connection or substream. -async fn secure_outbound( - conn: C, - up: U, - peer_id: Option, - v: Version, -) -> Result> -where - C: AsyncRead + AsyncWrite + Unpin, - U: SecurityUpgrade>, -{ - let (info, stream) = - multistream_select::dialer_select_proto(conn, up.protocol_info(), v).await?; - let name = info.as_ref().to_owned(); - match up.upgrade_security(stream, info, peer_id).await { - Ok(x) => { - tracing::trace!(up=%name, "Secured outbound stream"); - Ok(x) - } - Err(e) => { - tracing::trace!(up=%name, "Failed to secure outbound stream"); - Err(UpgradeError::Apply(e)) + _ => { + let (info, stream) = + multistream_select::listener_select_proto(conn, up.protocol_info()).await?; + let name = info.as_ref().to_owned(); + match up.upgrade_security(stream, info, None).await { + Ok(x) => { + tracing::trace!(up=%name, "Secured inbound stream"); + Ok(x) + } + Err(e) => { + tracing::trace!(up=%name, "Failed to secure inbound stream"); + Err(UpgradeError::Apply(e)) + } + } } } } From af4827079bb000bb7ebfc53453ef2e8837e22d53 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Thu, 16 Nov 2023 13:48:02 +0100 Subject: [PATCH 04/18] docs: improve comments related to `Builder::authenticate2` --- core/src/transport/upgrade.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/core/src/transport/upgrade.rs b/core/src/transport/upgrade.rs index 30abd3d8372..6350732e946 100644 --- a/core/src/transport/upgrade.rs +++ b/core/src/transport/upgrade.rs @@ -126,6 +126,12 @@ where /// /// * I/O upgrade: `C -> (PeerId, D)`. /// * Transport output: `C -> (PeerId, D)` + /// + /// ## Alternative state + /// + /// This function is an alternative code path to [`Builder::authenticate`] + /// given the supplied upgrade implements `SecurityUpgrade` instead of + /// `InboundConnectionUpgrade`/`OutboundConnectionUpgrade`. pub fn authenticate2( self, upgrade: U, @@ -182,7 +188,7 @@ where /// An upgrade that authenticates the remote peer, typically /// in the context of negotiating a secure channel. /// -/// Configured through [`Builder::authenticate`]. +/// Configured through [`Builder::authenticate2`]. #[pin_project::pin_project] pub struct Authenticate2 where From 31c7e614a61c89a35da85ea26f2434e60ea77197 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Fri, 17 Nov 2023 12:10:32 +0100 Subject: [PATCH 05/18] fix: add `Send` constraint to `Builder::authenticate2` parameters Use an owned dynamically typed `Future` that is `Send` (i.e. `BoxFuture`). --- core/src/transport/upgrade.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/core/src/transport/upgrade.rs b/core/src/transport/upgrade.rs index 6350732e946..8678065afd1 100644 --- a/core/src/transport/upgrade.rs +++ b/core/src/transport/upgrade.rs @@ -33,9 +33,9 @@ use crate::{ self, apply_inbound, apply_outbound, InboundConnectionUpgrade, InboundUpgradeApply, OutboundConnectionUpgrade, OutboundUpgradeApply, SecurityUpgrade, UpgradeError, }, - Negotiated, + Negotiated, UpgradeInfo, }; -use futures::{future::LocalBoxFuture, prelude::*, ready}; +use futures::{future::BoxFuture, prelude::*, ready}; use libp2p_identity::PeerId; use multiaddr::Multiaddr; use std::{ @@ -138,15 +138,18 @@ where ) -> Authenticated Authenticate2 + Clone>> where T: Transport, - C: AsyncRead + AsyncWrite + Unpin + 'static, + C: AsyncRead + AsyncWrite + Unpin + Send + 'static, D: AsyncRead + AsyncWrite + Unpin, - U: SecurityUpgrade, Output = (PeerId, D), Error = E> + Clone + 'static, + U: SecurityUpgrade, Output = (PeerId, D), Error = E> + Clone + Send + 'static, + ::Info: Send, + >>::Future: Send, + <::InfoIter as std::iter::IntoIterator>::IntoIter: Send, E: Error + 'static, { let version = self.version; Authenticated(Builder::new( self.inner.and_then(move |conn, endpoint| Authenticate2 { - inner: upgrade::secure(conn, upgrade, endpoint, version).boxed_local(), + inner: upgrade::secure(conn, upgrade, endpoint, version).boxed(), }), version, )) @@ -196,7 +199,7 @@ where U: SecurityUpgrade>, { #[pin] - inner: LocalBoxFuture<'static, Result<(PeerId, U::Output), UpgradeError>>, + inner: BoxFuture<'static, Result<(PeerId, U::Output), UpgradeError>>, } impl Future for Authenticate2 From 5c5b7288d35d54b0065c11d67bb3a411113703d3 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Fri, 17 Nov 2023 14:04:06 +0100 Subject: [PATCH 06/18] refactor: split `SecurityUpgrade` into two alternative traits Introduce `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` instead. --- core/src/transport/upgrade.rs | 33 ++++++---- core/src/upgrade.rs | 27 ++++++-- core/src/upgrade/secure.rs | 120 +++++++++++++++++++++++----------- 3 files changed, 126 insertions(+), 54 deletions(-) diff --git a/core/src/transport/upgrade.rs b/core/src/transport/upgrade.rs index 8678065afd1..075e380dd11 100644 --- a/core/src/transport/upgrade.rs +++ b/core/src/transport/upgrade.rs @@ -30,12 +30,13 @@ use crate::{ TransportError, TransportEvent, }, upgrade::{ - self, apply_inbound, apply_outbound, InboundConnectionUpgrade, InboundUpgradeApply, - OutboundConnectionUpgrade, OutboundUpgradeApply, SecurityUpgrade, UpgradeError, + self, apply_inbound, apply_outbound, EitherSecurityFuture, InboundConnectionUpgrade, + InboundSecurityUpgrade, InboundUpgradeApply, OutboundConnectionUpgrade, + OutboundSecurityUpgrade, OutboundUpgradeApply, UpgradeError, }, Negotiated, UpgradeInfo, }; -use futures::{future::BoxFuture, prelude::*, ready}; +use futures::{prelude::*, ready}; use libp2p_identity::PeerId; use multiaddr::Multiaddr; use std::{ @@ -140,16 +141,21 @@ where T: Transport, C: AsyncRead + AsyncWrite + Unpin + Send + 'static, D: AsyncRead + AsyncWrite + Unpin, - U: SecurityUpgrade, Output = (PeerId, D), Error = E> + Clone + Send + 'static, + U: InboundSecurityUpgrade, Output = (PeerId, D), Error = E> + Send + 'static, + U: OutboundSecurityUpgrade, Output = (PeerId, D), Error = E> + + Clone + + Send + + 'static, + E: Error + 'static, ::Info: Send, - >>::Future: Send, + >>::Future: Send, + >>::Future: Send, <::InfoIter as std::iter::IntoIterator>::IntoIter: Send, - E: Error + 'static, { let version = self.version; Authenticated(Builder::new( self.inner.and_then(move |conn, endpoint| Authenticate2 { - inner: upgrade::secure(conn, upgrade, endpoint, version).boxed(), + inner: upgrade::secure(conn, upgrade, endpoint, version), }), version, )) @@ -196,18 +202,23 @@ where pub struct Authenticate2 where C: AsyncRead + AsyncWrite + Unpin, - U: SecurityUpgrade>, + U: InboundSecurityUpgrade> + OutboundSecurityUpgrade>, { #[pin] - inner: BoxFuture<'static, Result<(PeerId, U::Output), UpgradeError>>, + inner: EitherSecurityFuture, } impl Future for Authenticate2 where C: AsyncRead + AsyncWrite + Unpin, - U: SecurityUpgrade>, + U: InboundSecurityUpgrade> + + OutboundSecurityUpgrade< + Negotiated, + Output = >>::Output, + Error = >>::Error, + >, { - type Output = Result<(PeerId, U::Output), UpgradeError>; + type Output = as Future>::Output; fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { let this = self.project(); diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index 46e9565e775..a7c0cf7604d 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -77,7 +77,7 @@ pub(crate) use error::UpgradeError; use futures::future::Future; use libp2p_identity::PeerId; pub use multistream_select::{NegotiatedComplete, NegotiationError, ProtocolError, Version}; -pub(crate) use secure::secure; +pub(crate) use secure::{secure, EitherSecurityFuture}; /// Common trait for upgrades that can be applied on inbound substreams, outbound substreams, /// or both. @@ -155,8 +155,8 @@ pub trait OutboundConnectionUpgrade: UpgradeInfo { fn upgrade_outbound(self, socket: T, info: Self::Info) -> Self::Future; } -/// Possible security upgrade on a connection -pub trait SecurityUpgrade: UpgradeInfo { +/// Possible security upgrade on an inbound connection +pub trait InboundSecurityUpgrade: UpgradeInfo { /// Output after the upgrade has been successfully negotiated and the handshake performed. type Output; /// Possible error during the handshake. @@ -170,6 +170,23 @@ pub trait SecurityUpgrade: UpgradeInfo { /// The `info` is the identifier of the protocol, as produced by `protocol_info`. Security /// transports use the optional `peer_id` parameter on outgoing upgrades to validate the /// expected `PeerId`. - fn upgrade_security(self, socket: T, info: Self::Info, peer_id: Option) - -> Self::Future; + fn secure_inbound(self, socket: T, info: Self::Info, peer_id: Option) -> Self::Future; +} + +/// Possible security upgrade on an outbound connection +pub trait OutboundSecurityUpgrade: UpgradeInfo { + /// Output after the upgrade has been successfully negotiated and the handshake performed. + type Output; + /// Possible error during the handshake. + type Error; + /// Future that performs the handshake with the remote. + type Future: Future>; + + /// After we have determined that the remote supports one of the protocols we support, this + /// method is called to start the handshake. + /// + /// The `info` is the identifier of the protocol, as produced by `protocol_info`. Security + /// transports use the optional `peer_id` parameter on outgoing upgrades to validate the + /// expected `PeerId`. + fn secure_outbound(self, socket: T, info: Self::Info, peer_id: Option) -> Self::Future; } diff --git a/core/src/upgrade/secure.rs b/core/src/upgrade/secure.rs index e63d59edf21..681bc06eec8 100644 --- a/core/src/upgrade/secure.rs +++ b/core/src/upgrade/secure.rs @@ -1,62 +1,106 @@ //! After a successful protocol negotiation as part of the upgrade process, the `SecurityUpgrade::upgrade_security` //! method is called and a [`Future`] that performs a handshake is returned. -use crate::upgrade::{SecurityUpgrade, UpgradeError}; +use std::iter::IntoIterator; + +use crate::upgrade::UpgradeError; +use crate::UpgradeInfo; use crate::{connection::ConnectedPoint, Negotiated}; +use futures::future::{BoxFuture, Either}; use futures::prelude::*; use libp2p_identity::PeerId; use multiaddr::Protocol; use multistream_select::Version; +use super::{InboundSecurityUpgrade, OutboundSecurityUpgrade}; + +/// An inbound or outbound security upgrade. +pub(crate) type EitherSecurityFuture = + Either, OutboundSecurityFuture>; + +/// An inbound security upgrade represented by an owned trait object `Future`. +pub(crate) type InboundSecurityFuture = BoxFuture< + 'static, + Result< + (PeerId, >>::Output), + UpgradeError<>>::Error>, + >, +>; + +/// An outbound security upgrade represented by an owned trait object `Future`. +pub(crate) type OutboundSecurityFuture = BoxFuture< + 'static, + Result< + ( + PeerId, + >>::Output, + ), + UpgradeError<>>::Error>, + >, +>; + /// Applies a security upgrade to the inbound and outbound direction of a connection or substream. -pub(crate) async fn secure( +pub(crate) fn secure( conn: C, up: U, cp: ConnectedPoint, v: Version, -) -> Result<(PeerId, U::Output), UpgradeError> +) -> EitherSecurityFuture where - C: AsyncRead + AsyncWrite + Unpin, - U: SecurityUpgrade>, + C: AsyncRead + AsyncWrite + Unpin + Send + 'static, + U: InboundSecurityUpgrade> + + OutboundSecurityUpgrade> + + Send + + 'static, + ::Info: Send, + >>::Future: Send, + >>::Future: Send, + <::InfoIter as IntoIterator>::IntoIter: Send, { match cp { - ConnectedPoint::Dialer { role_override, .. } if role_override.is_dialer() => { - let peer_id = cp - .get_remote_address() - .iter() - .find_map(|protocol| match protocol { - Protocol::P2p(peer_id) => Some(peer_id), - _ => None, - }); - let (info, stream) = - multistream_select::dialer_select_proto(conn, up.protocol_info(), v).await?; - let name = info.as_ref().to_owned(); - match up.upgrade_security(stream, info, peer_id).await { - Ok(x) => { - tracing::trace!(up=%name, "Secured outbound stream"); - Ok(x) - } - Err(e) => { - tracing::trace!(up=%name, "Failed to secure outbound stream"); - Err(UpgradeError::Apply(e)) + ConnectedPoint::Dialer { role_override, .. } if role_override.is_dialer() => Either::Right( + async move { + let peer_id = cp + .get_remote_address() + .iter() + .find_map(|protocol| match protocol { + Protocol::P2p(peer_id) => Some(peer_id), + _ => None, + }); + let (info, stream) = + multistream_select::dialer_select_proto(conn, up.protocol_info(), v).await?; + let name = info.as_ref().to_owned(); + match up.secure_outbound(stream, info, peer_id).await { + Ok(x) => { + tracing::trace!(up=%name, "Secured outbound stream"); + Ok(x) + } + Err(e) => { + tracing::trace!(up=%name, "Failed to secure outbound stream"); + Err(UpgradeError::Apply(e)) + } } } - } - _ => { - let (info, stream) = - multistream_select::listener_select_proto(conn, up.protocol_info()).await?; - let name = info.as_ref().to_owned(); - match up.upgrade_security(stream, info, None).await { - Ok(x) => { - tracing::trace!(up=%name, "Secured inbound stream"); - Ok(x) - } - Err(e) => { - tracing::trace!(up=%name, "Failed to secure inbound stream"); - Err(UpgradeError::Apply(e)) + .boxed(), + ), + _ => Either::Left( + async move { + let (info, stream) = + multistream_select::listener_select_proto(conn, up.protocol_info()).await?; + let name = info.as_ref().to_owned(); + match up.secure_inbound(stream, info, None).await { + Ok(x) => { + tracing::trace!(up=%name, "Secured inbound stream"); + Ok(x) + } + Err(e) => { + tracing::trace!(up=%name, "Failed to secure inbound stream"); + Err(UpgradeError::Apply(e)) + } } } - } + .boxed(), + ), } } From b68680156158833d8c18199d3e904e66001ea2ca Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Fri, 17 Nov 2023 15:56:56 +0100 Subject: [PATCH 07/18] docs: mark items related to `Builder::authenticate2` as `#[doc(hidden)]` Plus fix doc comments. --- core/src/transport/upgrade.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/transport/upgrade.rs b/core/src/transport/upgrade.rs index 075e380dd11..42e1bde0669 100644 --- a/core/src/transport/upgrade.rs +++ b/core/src/transport/upgrade.rs @@ -133,6 +133,7 @@ where /// This function is an alternative code path to [`Builder::authenticate`] /// given the supplied upgrade implements `SecurityUpgrade` instead of /// `InboundConnectionUpgrade`/`OutboundConnectionUpgrade`. + #[doc(hidden)] pub fn authenticate2( self, upgrade: U, @@ -198,6 +199,7 @@ where /// in the context of negotiating a secure channel. /// /// Configured through [`Builder::authenticate2`]. +#[doc(hidden)] #[pin_project::pin_project] pub struct Authenticate2 where From c0fad3a7e7e719e1723bec61c454a6e4c8bee64c Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Fri, 17 Nov 2023 19:12:32 +0100 Subject: [PATCH 08/18] feat: impl `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` for TLS Implement the `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` traits for the TLS transport verifying the expected peer ID on outgoing upgrades. --- transports/tls/src/upgrade.rs | 64 ++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/transports/tls/src/upgrade.rs b/transports/tls/src/upgrade.rs index 84510b6bab0..0711a548483 100644 --- a/transports/tls/src/upgrade.rs +++ b/transports/tls/src/upgrade.rs @@ -24,7 +24,10 @@ use futures::future::BoxFuture; use futures::AsyncWrite; use futures::{AsyncRead, FutureExt}; use futures_rustls::TlsStream; -use libp2p_core::upgrade::{InboundConnectionUpgrade, OutboundConnectionUpgrade}; +use libp2p_core::upgrade::{ + InboundConnectionUpgrade, InboundSecurityUpgrade, OutboundConnectionUpgrade, + OutboundSecurityUpgrade, +}; use libp2p_core::UpgradeInfo; use libp2p_identity as identity; use libp2p_identity::PeerId; @@ -42,6 +45,8 @@ pub enum UpgradeError { ClientUpgrade(std::io::Error), #[error("Failed to parse certificate")] BadCertificate(#[from] certificate::ParseError), + #[error("Invalid peer ID (expected {expected:?}, found {found:?})")] + PeerIdMismatch { expected: PeerId, found: PeerId }, } #[derive(Clone)] @@ -118,6 +123,63 @@ where } } +impl InboundSecurityUpgrade for Config +where + C: AsyncRead + AsyncWrite + Send + Unpin + 'static, +{ + type Output = TlsStream; + type Error = UpgradeError; + type Future = BoxFuture<'static, Result<(PeerId, Self::Output), Self::Error>>; + + fn secure_inbound(self, socket: C, _: Self::Info, _: Option) -> Self::Future { + async move { + let stream = futures_rustls::TlsAcceptor::from(Arc::new(self.server)) + .accept(socket) + .await + .map_err(UpgradeError::ServerUpgrade)?; + + let expected = extract_single_certificate(stream.get_ref().1)?.peer_id(); + + Ok((expected, stream.into())) + } + .boxed() + } +} + +impl OutboundSecurityUpgrade for Config +where + C: AsyncRead + AsyncWrite + Send + Unpin + 'static, +{ + type Output = TlsStream; + type Error = UpgradeError; + type Future = BoxFuture<'static, Result<(PeerId, Self::Output), Self::Error>>; + + fn secure_outbound(self, socket: C, _: Self::Info, peer_id: Option) -> Self::Future { + async move { + // Spec: In order to keep this flexibility for future versions, clients that only support + // the version of the handshake defined in this document MUST NOT send any value in the + // Server Name Indication. + // Setting `ServerName` to unspecified will disable the use of the SNI extension. + let name = ServerName::IpAddress(IpAddr::V4(Ipv4Addr::UNSPECIFIED)); + + let stream = futures_rustls::TlsConnector::from(Arc::new(self.client)) + .connect(name, socket) + .await + .map_err(UpgradeError::ClientUpgrade)?; + + let expected = extract_single_certificate(stream.get_ref().1)?.peer_id(); + + match peer_id { + Some(found) if found != expected => { + Err(UpgradeError::PeerIdMismatch { expected, found }) + } + _ => Ok((expected, stream.into())), + } + } + .boxed() + } +} + fn extract_single_certificate( state: &CommonState, ) -> Result, certificate::ParseError> { From 399666bd3473a322e29296780679123a13e5b22e Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Fri, 17 Nov 2023 19:44:05 +0100 Subject: [PATCH 09/18] feat: impl `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` for Noise Implement the `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` traits for the Noise transport verifying the expected peer ID on outgoing upgrades. --- transports/noise/src/lib.rs | 61 ++++++++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/transports/noise/src/lib.rs b/transports/noise/src/lib.rs index 70fae9d7ee6..4faedb9b7b7 100644 --- a/transports/noise/src/lib.rs +++ b/transports/noise/src/lib.rs @@ -58,13 +58,17 @@ mod io; mod protocol; +use futures::future::BoxFuture; pub use io::Output; use crate::handshake::State; use crate::io::handshake; use crate::protocol::{noise_params_into_builder, AuthenticKeypair, Keypair, PARAMS_XX}; use futures::prelude::*; -use libp2p_core::upgrade::{InboundConnectionUpgrade, OutboundConnectionUpgrade}; +use libp2p_core::upgrade::{ + InboundConnectionUpgrade, InboundSecurityUpgrade, OutboundConnectionUpgrade, + OutboundSecurityUpgrade, +}; use libp2p_core::UpgradeInfo; use libp2p_identity as identity; use libp2p_identity::PeerId; @@ -219,6 +223,59 @@ where } } +impl InboundSecurityUpgrade for Config +where + T: AsyncRead + AsyncWrite + Unpin + Send + 'static, +{ + type Output = Output; + type Error = Error; + type Future = BoxFuture<'static, Result<(PeerId, Self::Output), Self::Error>>; + + fn secure_inbound(self, socket: T, _: Self::Info, _: Option) -> Self::Future { + async move { + let mut state = self.into_responder(socket)?; + + handshake::recv_empty(&mut state).await?; + handshake::send_identity(&mut state).await?; + handshake::recv_identity(&mut state).await?; + + let (pk, io) = state.finish()?; + + let expected = pk.to_peer_id(); + Ok((expected, io)) + } + .boxed() + } +} + +impl OutboundSecurityUpgrade for Config +where + T: AsyncRead + AsyncWrite + Unpin + Send + 'static, +{ + type Output = Output; + type Error = Error; + type Future = BoxFuture<'static, Result<(PeerId, Self::Output), Self::Error>>; + + fn secure_outbound(self, socket: T, _: Self::Info, peer_id: Option) -> Self::Future { + async move { + let mut state = self.into_initiator(socket)?; + + handshake::send_empty(&mut state).await?; + handshake::recv_identity(&mut state).await?; + handshake::send_identity(&mut state).await?; + + let (pk, io) = state.finish()?; + + let expected = pk.to_peer_id(); + match peer_id { + Some(found) if found != expected => Err(Error::PeerIdMismatch { expected, found }), + _ => Ok((expected, io)), + } + } + .boxed() + } +} + /// libp2p_noise error type. #[derive(Debug, thiserror::Error)] #[non_exhaustive] @@ -243,6 +300,8 @@ pub enum Error { SigningError(#[from] libp2p_identity::SigningError), #[error("Expected WebTransport certhashes ({}) are not a subset of received ones ({})", certhashes_to_string(.0), certhashes_to_string(.1))] UnknownWebTransportCerthashes(HashSet>, HashSet>), + #[error("Invalid peer ID (expected {expected:?}, found {found:?})")] + PeerIdMismatch { expected: PeerId, found: PeerId }, } #[derive(Debug, thiserror::Error)] From 71e0ac8a2d7b37531b30c58a583db48ca79a453d Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Sun, 19 Nov 2023 22:57:45 +0100 Subject: [PATCH 10/18] chore: minor doc comments fixes --- core/src/transport/upgrade.rs | 4 ++-- core/src/upgrade/secure.rs | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/core/src/transport/upgrade.rs b/core/src/transport/upgrade.rs index 42e1bde0669..0fd19f9a16c 100644 --- a/core/src/transport/upgrade.rs +++ b/core/src/transport/upgrade.rs @@ -131,8 +131,8 @@ where /// ## Alternative state /// /// This function is an alternative code path to [`Builder::authenticate`] - /// given the supplied upgrade implements `SecurityUpgrade` instead of - /// `InboundConnectionUpgrade`/`OutboundConnectionUpgrade`. + /// given the supplied upgrade implements `InboundConnectionUpgrade`/`OutboundSecurityUpgrade` + /// instead of `InboundConnectionUpgrade`/`OutboundConnectionUpgrade`. #[doc(hidden)] pub fn authenticate2( self, diff --git a/core/src/upgrade/secure.rs b/core/src/upgrade/secure.rs index 681bc06eec8..8e36933ff7b 100644 --- a/core/src/upgrade/secure.rs +++ b/core/src/upgrade/secure.rs @@ -1,5 +1,6 @@ -//! After a successful protocol negotiation as part of the upgrade process, the `SecurityUpgrade::upgrade_security` -//! method is called and a [`Future`] that performs a handshake is returned. +//! After a successful protocol negotiation as part of the upgrade process, the `InboundConnectionUpgrade::secure_inbound` +//! or `OutboundSecurityUpgrade::secure_outbound` method is called and a [`Future`] that performs a +//! handshake is returned. use std::iter::IntoIterator; From 5500663d28a03484583f97ebf127e133eaa8b604 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Sun, 19 Nov 2023 23:05:44 +0100 Subject: [PATCH 11/18] refactor: remove the `PeerId` parameter in `InboundSecurityUpgrade` The optional `PeerId` parameter in a security upgrade is never known at this point for an inbound connection. --- core/src/upgrade.rs | 6 ++---- core/src/upgrade/secure.rs | 2 +- transports/noise/src/lib.rs | 2 +- transports/tls/src/upgrade.rs | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index a7c0cf7604d..84e0c52a86f 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -167,10 +167,8 @@ pub trait InboundSecurityUpgrade: UpgradeInfo { /// After we have determined that the remote supports one of the protocols we support, this /// method is called to start the handshake. /// - /// The `info` is the identifier of the protocol, as produced by `protocol_info`. Security - /// transports use the optional `peer_id` parameter on outgoing upgrades to validate the - /// expected `PeerId`. - fn secure_inbound(self, socket: T, info: Self::Info, peer_id: Option) -> Self::Future; + /// The `info` is the identifier of the protocol, as produced by `protocol_info`. + fn secure_inbound(self, socket: T, info: Self::Info) -> Self::Future; } /// Possible security upgrade on an outbound connection diff --git a/core/src/upgrade/secure.rs b/core/src/upgrade/secure.rs index 8e36933ff7b..50d361fff89 100644 --- a/core/src/upgrade/secure.rs +++ b/core/src/upgrade/secure.rs @@ -90,7 +90,7 @@ where let (info, stream) = multistream_select::listener_select_proto(conn, up.protocol_info()).await?; let name = info.as_ref().to_owned(); - match up.secure_inbound(stream, info, None).await { + match up.secure_inbound(stream, info).await { Ok(x) => { tracing::trace!(up=%name, "Secured inbound stream"); Ok(x) diff --git a/transports/noise/src/lib.rs b/transports/noise/src/lib.rs index 4faedb9b7b7..2b095bcdf47 100644 --- a/transports/noise/src/lib.rs +++ b/transports/noise/src/lib.rs @@ -231,7 +231,7 @@ where type Error = Error; type Future = BoxFuture<'static, Result<(PeerId, Self::Output), Self::Error>>; - fn secure_inbound(self, socket: T, _: Self::Info, _: Option) -> Self::Future { + fn secure_inbound(self, socket: T, _: Self::Info) -> Self::Future { async move { let mut state = self.into_responder(socket)?; diff --git a/transports/tls/src/upgrade.rs b/transports/tls/src/upgrade.rs index 0711a548483..c63c3fd02db 100644 --- a/transports/tls/src/upgrade.rs +++ b/transports/tls/src/upgrade.rs @@ -131,7 +131,7 @@ where type Error = UpgradeError; type Future = BoxFuture<'static, Result<(PeerId, Self::Output), Self::Error>>; - fn secure_inbound(self, socket: C, _: Self::Info, _: Option) -> Self::Future { + fn secure_inbound(self, socket: C, _: Self::Info) -> Self::Future { async move { let stream = futures_rustls::TlsAcceptor::from(Arc::new(self.server)) .accept(socket) From a7260834c47bc9cf8da83ea6e5f5fe344303c8ff Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Wed, 22 Nov 2023 17:28:30 +0100 Subject: [PATCH 12/18] docs: fix several issues in the comments --- core/src/transport/upgrade.rs | 6 +++--- core/src/upgrade/secure.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/transport/upgrade.rs b/core/src/transport/upgrade.rs index 0fd19f9a16c..adae2bc4124 100644 --- a/core/src/transport/upgrade.rs +++ b/core/src/transport/upgrade.rs @@ -115,7 +115,7 @@ where )) } - /// Upgrades the transport to perform authentication of the remote + /// Upgrades the transport to perform authentication of the remote. /// /// The supplied upgrade receives the I/O resource `C` and must /// produce a pair `(PeerId, D)`, where `D` is a new I/O resource. @@ -131,8 +131,8 @@ where /// ## Alternative state /// /// This function is an alternative code path to [`Builder::authenticate`] - /// given the supplied upgrade implements `InboundConnectionUpgrade`/`OutboundSecurityUpgrade` - /// instead of `InboundConnectionUpgrade`/`OutboundConnectionUpgrade`. + /// given the supplied upgrade implements [`InboundSecurityUpgrade`]/[`OutboundSecurityUpgrade`] + /// instead of [`InboundConnectionUpgrade`]/[`OutboundConnectionUpgrade`]. #[doc(hidden)] pub fn authenticate2( self, diff --git a/core/src/upgrade/secure.rs b/core/src/upgrade/secure.rs index 50d361fff89..66ff622eb3c 100644 --- a/core/src/upgrade/secure.rs +++ b/core/src/upgrade/secure.rs @@ -1,5 +1,5 @@ -//! After a successful protocol negotiation as part of the upgrade process, the `InboundConnectionUpgrade::secure_inbound` -//! or `OutboundSecurityUpgrade::secure_outbound` method is called and a [`Future`] that performs a +//! After a successful protocol negotiation as part of the upgrade process, the [`InboundSecurityUpgrade::secure_inbound`] +//! or [`OutboundSecurityUpgrade::secure_outbound`] method is called and a [`Future`] that performs a //! handshake is returned. use std::iter::IntoIterator; From 514b22018fb6cad31fe531613eba6b2e248e9ef1 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Wed, 22 Nov 2023 18:34:48 +0100 Subject: [PATCH 13/18] refactor: delegate `upgrade_inbound` and `upgrade_outbound` methods Delegate the implementations of `InboundConnectionUpgrade`/`OutboundConnectionUpgrade` traits to `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` for Noise and TLS. --- transports/noise/src/lib.rs | 32 +++++--------------------------- transports/tls/src/upgrade.rs | 32 ++++---------------------------- 2 files changed, 9 insertions(+), 55 deletions(-) diff --git a/transports/noise/src/lib.rs b/transports/noise/src/lib.rs index 2b095bcdf47..605b2893af2 100644 --- a/transports/noise/src/lib.rs +++ b/transports/noise/src/lib.rs @@ -58,12 +58,12 @@ mod io; mod protocol; -use futures::future::BoxFuture; pub use io::Output; use crate::handshake::State; use crate::io::handshake; use crate::protocol::{noise_params_into_builder, AuthenticKeypair, Keypair, PARAMS_XX}; +use futures::future::BoxFuture; use futures::prelude::*; use libp2p_core::upgrade::{ InboundConnectionUpgrade, InboundSecurityUpgrade, OutboundConnectionUpgrade, @@ -183,19 +183,8 @@ where type Error = Error; type Future = Pin> + Send>>; - fn upgrade_inbound(self, socket: T, _: Self::Info) -> Self::Future { - async move { - let mut state = self.into_responder(socket)?; - - handshake::recv_empty(&mut state).await?; - handshake::send_identity(&mut state).await?; - handshake::recv_identity(&mut state).await?; - - let (pk, io) = state.finish()?; - - Ok((pk.to_peer_id(), io)) - } - .boxed() + fn upgrade_inbound(self, socket: T, info: Self::Info) -> Self::Future { + InboundSecurityUpgrade::secure_inbound(self, socket, info) } } @@ -207,19 +196,8 @@ where type Error = Error; type Future = Pin> + Send>>; - fn upgrade_outbound(self, socket: T, _: Self::Info) -> Self::Future { - async move { - let mut state = self.into_initiator(socket)?; - - handshake::send_empty(&mut state).await?; - handshake::recv_identity(&mut state).await?; - handshake::send_identity(&mut state).await?; - - let (pk, io) = state.finish()?; - - Ok((pk.to_peer_id(), io)) - } - .boxed() + fn upgrade_outbound(self, socket: T, info: Self::Info) -> Self::Future { + OutboundSecurityUpgrade::secure_outbound(self, socket, info, None) } } diff --git a/transports/tls/src/upgrade.rs b/transports/tls/src/upgrade.rs index c63c3fd02db..181ee6caff7 100644 --- a/transports/tls/src/upgrade.rs +++ b/transports/tls/src/upgrade.rs @@ -81,18 +81,8 @@ where type Error = UpgradeError; type Future = BoxFuture<'static, Result>; - fn upgrade_inbound(self, socket: C, _: Self::Info) -> Self::Future { - async move { - let stream = futures_rustls::TlsAcceptor::from(Arc::new(self.server)) - .accept(socket) - .await - .map_err(UpgradeError::ServerUpgrade)?; - - let peer_id = extract_single_certificate(stream.get_ref().1)?.peer_id(); - - Ok((peer_id, stream.into())) - } - .boxed() + fn upgrade_inbound(self, socket: C, info: Self::Info) -> Self::Future { + InboundSecurityUpgrade::secure_inbound(self, socket, info) } } @@ -104,22 +94,8 @@ where type Error = UpgradeError; type Future = BoxFuture<'static, Result>; - fn upgrade_outbound(self, socket: C, _: Self::Info) -> Self::Future { - async move { - // Spec: In order to keep this flexibility for future versions, clients that only support the version of the handshake defined in this document MUST NOT send any value in the Server Name Indication. - // Setting `ServerName` to unspecified will disable the use of the SNI extension. - let name = ServerName::IpAddress(IpAddr::V4(Ipv4Addr::UNSPECIFIED)); - - let stream = futures_rustls::TlsConnector::from(Arc::new(self.client)) - .connect(name, socket) - .await - .map_err(UpgradeError::ClientUpgrade)?; - - let peer_id = extract_single_certificate(stream.get_ref().1)?.peer_id(); - - Ok((peer_id, stream.into())) - } - .boxed() + fn upgrade_outbound(self, socket: C, info: Self::Info) -> Self::Future { + OutboundSecurityUpgrade::secure_outbound(self, socket, info, None) } } From 66df049e3c7c6241f801fc95ac03080a8c761406 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Wed, 22 Nov 2023 20:48:06 +0100 Subject: [PATCH 14/18] fix: references to actual and remote `PeerId` respectively The `PeerId` coming from the address is the remote `PeerId` and the one from the handshake is the actual `PeerId`. --- core/src/upgrade.rs | 9 +++++++-- transports/noise/src/lib.rs | 29 ++++++++++++++++++++--------- transports/tls/src/upgrade.rs | 27 +++++++++++++++++++-------- 3 files changed, 46 insertions(+), 19 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index 84e0c52a86f..cb69b05acf4 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -184,7 +184,12 @@ pub trait OutboundSecurityUpgrade: UpgradeInfo { /// method is called to start the handshake. /// /// The `info` is the identifier of the protocol, as produced by `protocol_info`. Security - /// transports use the optional `peer_id` parameter on outgoing upgrades to validate the + /// transports use the optional `remote_peer_id` parameter on outgoing upgrades to validate the /// expected `PeerId`. - fn secure_outbound(self, socket: T, info: Self::Info, peer_id: Option) -> Self::Future; + fn secure_outbound( + self, + socket: T, + info: Self::Info, + remote_peer_id: Option, + ) -> Self::Future; } diff --git a/transports/noise/src/lib.rs b/transports/noise/src/lib.rs index 605b2893af2..b2c23562d84 100644 --- a/transports/noise/src/lib.rs +++ b/transports/noise/src/lib.rs @@ -219,8 +219,8 @@ where let (pk, io) = state.finish()?; - let expected = pk.to_peer_id(); - Ok((expected, io)) + let peer_id = pk.to_peer_id(); + Ok((peer_id, io)) } .boxed() } @@ -234,7 +234,12 @@ where type Error = Error; type Future = BoxFuture<'static, Result<(PeerId, Self::Output), Self::Error>>; - fn secure_outbound(self, socket: T, _: Self::Info, peer_id: Option) -> Self::Future { + fn secure_outbound( + self, + socket: T, + _: Self::Info, + remote_peer_id: Option, + ) -> Self::Future { async move { let mut state = self.into_initiator(socket)?; @@ -244,10 +249,13 @@ where let (pk, io) = state.finish()?; - let expected = pk.to_peer_id(); - match peer_id { - Some(found) if found != expected => Err(Error::PeerIdMismatch { expected, found }), - _ => Ok((expected, io)), + let peer_id = pk.to_peer_id(); + match remote_peer_id { + Some(remote_peer_id) if remote_peer_id != peer_id => Err(Error::PeerIdMismatch { + peer_id, + remote_peer_id, + }), + _ => Ok((peer_id, io)), } } .boxed() @@ -278,8 +286,11 @@ pub enum Error { SigningError(#[from] libp2p_identity::SigningError), #[error("Expected WebTransport certhashes ({}) are not a subset of received ones ({})", certhashes_to_string(.0), certhashes_to_string(.1))] UnknownWebTransportCerthashes(HashSet>, HashSet>), - #[error("Invalid peer ID (expected {expected:?}, found {found:?})")] - PeerIdMismatch { expected: PeerId, found: PeerId }, + #[error("Invalid peer ID (actual {peer_id:?}, remote {remote_peer_id:?})")] + PeerIdMismatch { + peer_id: PeerId, + remote_peer_id: PeerId, + }, } #[derive(Debug, thiserror::Error)] diff --git a/transports/tls/src/upgrade.rs b/transports/tls/src/upgrade.rs index 181ee6caff7..9fbbea55c0e 100644 --- a/transports/tls/src/upgrade.rs +++ b/transports/tls/src/upgrade.rs @@ -45,8 +45,11 @@ pub enum UpgradeError { ClientUpgrade(std::io::Error), #[error("Failed to parse certificate")] BadCertificate(#[from] certificate::ParseError), - #[error("Invalid peer ID (expected {expected:?}, found {found:?})")] - PeerIdMismatch { expected: PeerId, found: PeerId }, + #[error("Invalid peer ID (actual {peer_id:?}, remote {remote_peer_id:?})")] + PeerIdMismatch { + peer_id: PeerId, + remote_peer_id: PeerId, + }, } #[derive(Clone)] @@ -130,7 +133,12 @@ where type Error = UpgradeError; type Future = BoxFuture<'static, Result<(PeerId, Self::Output), Self::Error>>; - fn secure_outbound(self, socket: C, _: Self::Info, peer_id: Option) -> Self::Future { + fn secure_outbound( + self, + socket: C, + _: Self::Info, + remote_peer_id: Option, + ) -> Self::Future { async move { // Spec: In order to keep this flexibility for future versions, clients that only support // the version of the handshake defined in this document MUST NOT send any value in the @@ -143,13 +151,16 @@ where .await .map_err(UpgradeError::ClientUpgrade)?; - let expected = extract_single_certificate(stream.get_ref().1)?.peer_id(); + let peer_id = extract_single_certificate(stream.get_ref().1)?.peer_id(); - match peer_id { - Some(found) if found != expected => { - Err(UpgradeError::PeerIdMismatch { expected, found }) + match remote_peer_id { + Some(remote_peer_id) if remote_peer_id != peer_id => { + Err(UpgradeError::PeerIdMismatch { + peer_id, + remote_peer_id, + }) } - _ => Ok((expected, stream.into())), + _ => Ok((peer_id, stream.into())), } } .boxed() From d8440e75401031943c8893c52b3bd845ed1cb7dc Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Fri, 24 Nov 2023 08:21:16 +0100 Subject: [PATCH 15/18] chore: keep grouping of module and use declarations --- core/src/upgrade.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index cb69b05acf4..56601bf2999 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -66,18 +66,20 @@ mod ready; mod secure; mod select; -pub use self::{ - denied::DeniedUpgrade, pending::PendingUpgrade, ready::ReadyUpgrade, select::SelectUpgrade, -}; -pub use crate::Negotiated; pub(crate) use apply::{ apply, apply_inbound, apply_outbound, InboundUpgradeApply, OutboundUpgradeApply, }; pub(crate) use error::UpgradeError; +pub(crate) use secure::{secure, EitherSecurityFuture}; + use futures::future::Future; use libp2p_identity::PeerId; + +pub use self::{ + denied::DeniedUpgrade, pending::PendingUpgrade, ready::ReadyUpgrade, select::SelectUpgrade, +}; +pub use crate::Negotiated; pub use multistream_select::{NegotiatedComplete, NegotiationError, ProtocolError, Version}; -pub(crate) use secure::{secure, EitherSecurityFuture}; /// Common trait for upgrades that can be applied on inbound substreams, outbound substreams, /// or both. From c386182edac8c5353cdfa40dca4742631b9451e9 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Fri, 24 Nov 2023 17:35:05 +0100 Subject: [PATCH 16/18] fix: make client and server config ad-hoc in `secure_outbound` method Introduce the `with_remote_peer_id` constructor in `Config`. --- transports/tls/src/upgrade.rs | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/transports/tls/src/upgrade.rs b/transports/tls/src/upgrade.rs index 9fbbea55c0e..f8d4c63b912 100644 --- a/transports/tls/src/upgrade.rs +++ b/transports/tls/src/upgrade.rs @@ -65,6 +65,17 @@ impl Config { client: crate::make_client_config(identity, None)?, }) } + + pub(crate) fn with_remote_peer_id( + remote_peer_id: Option, + ) -> Result { + let identity = libp2p_identity::Keypair::generate_ed25519(); + + Ok(Self { + server: crate::make_server_config(&identity)?, + client: crate::make_client_config(&identity, remote_peer_id)?, + }) + } } impl UpgradeInfo for Config { @@ -117,9 +128,9 @@ where .await .map_err(UpgradeError::ServerUpgrade)?; - let expected = extract_single_certificate(stream.get_ref().1)?.peer_id(); + let peer_id = extract_single_certificate(stream.get_ref().1)?.peer_id(); - Ok((expected, stream.into())) + Ok((peer_id, stream.into())) } .boxed() } @@ -134,12 +145,15 @@ where type Future = BoxFuture<'static, Result<(PeerId, Self::Output), Self::Error>>; fn secure_outbound( - self, + mut self, socket: C, _: Self::Info, remote_peer_id: Option, ) -> Self::Future { async move { + // Create new ad-hoc client and server configuration by passing the remote PeerId + self = Self::with_remote_peer_id(remote_peer_id)?; + // Spec: In order to keep this flexibility for future versions, clients that only support // the version of the handshake defined in this document MUST NOT send any value in the // Server Name Indication. @@ -153,15 +167,7 @@ where let peer_id = extract_single_certificate(stream.get_ref().1)?.peer_id(); - match remote_peer_id { - Some(remote_peer_id) if remote_peer_id != peer_id => { - Err(UpgradeError::PeerIdMismatch { - peer_id, - remote_peer_id, - }) - } - _ => Ok((peer_id, stream.into())), - } + Ok((peer_id, stream.into())) } .boxed() } From 3eb5f1f0cff1af1c9f3d3a780dcef2b874474db1 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Mon, 27 Nov 2023 10:52:51 +0100 Subject: [PATCH 17/18] fix: use the wording of `actual` and `expected` PeerId The `PeerId` coming from the address is the expected `PeerId` and the one from the handshake is the actual `PeerId`. --- core/src/upgrade.rs | 6 +++--- transports/noise/src/lib.rs | 18 ++++++++++-------- transports/tls/src/upgrade.rs | 16 ++++++++-------- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index 56601bf2999..0dc4f097431 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -186,12 +186,12 @@ pub trait OutboundSecurityUpgrade: UpgradeInfo { /// method is called to start the handshake. /// /// The `info` is the identifier of the protocol, as produced by `protocol_info`. Security - /// transports use the optional `remote_peer_id` parameter on outgoing upgrades to validate the - /// expected `PeerId`. + /// transports use the optional `expected_peer_id` parameter on outgoing upgrades to validate + /// the expected `PeerId`. fn secure_outbound( self, socket: T, info: Self::Info, - remote_peer_id: Option, + expected_peer_id: Option, ) -> Self::Future; } diff --git a/transports/noise/src/lib.rs b/transports/noise/src/lib.rs index b2c23562d84..91208168cd4 100644 --- a/transports/noise/src/lib.rs +++ b/transports/noise/src/lib.rs @@ -238,7 +238,7 @@ where self, socket: T, _: Self::Info, - remote_peer_id: Option, + expected_peer_id: Option, ) -> Self::Future { async move { let mut state = self.into_initiator(socket)?; @@ -250,11 +250,13 @@ where let (pk, io) = state.finish()?; let peer_id = pk.to_peer_id(); - match remote_peer_id { - Some(remote_peer_id) if remote_peer_id != peer_id => Err(Error::PeerIdMismatch { - peer_id, - remote_peer_id, - }), + match expected_peer_id { + Some(expected_peer_id) if expected_peer_id != peer_id => { + Err(Error::PeerIdMismatch { + peer_id, + expected_peer_id, + }) + } _ => Ok((peer_id, io)), } } @@ -286,10 +288,10 @@ pub enum Error { SigningError(#[from] libp2p_identity::SigningError), #[error("Expected WebTransport certhashes ({}) are not a subset of received ones ({})", certhashes_to_string(.0), certhashes_to_string(.1))] UnknownWebTransportCerthashes(HashSet>, HashSet>), - #[error("Invalid peer ID (actual {peer_id:?}, remote {remote_peer_id:?})")] + #[error("Invalid peer ID (actual {peer_id:?}, expected {expected_peer_id:?})")] PeerIdMismatch { peer_id: PeerId, - remote_peer_id: PeerId, + expected_peer_id: PeerId, }, } diff --git a/transports/tls/src/upgrade.rs b/transports/tls/src/upgrade.rs index f8d4c63b912..9db2d50632b 100644 --- a/transports/tls/src/upgrade.rs +++ b/transports/tls/src/upgrade.rs @@ -45,10 +45,10 @@ pub enum UpgradeError { ClientUpgrade(std::io::Error), #[error("Failed to parse certificate")] BadCertificate(#[from] certificate::ParseError), - #[error("Invalid peer ID (actual {peer_id:?}, remote {remote_peer_id:?})")] + #[error("Invalid peer ID (actual {peer_id:?}, expected {expected_peer_id:?})")] PeerIdMismatch { peer_id: PeerId, - remote_peer_id: PeerId, + expected_peer_id: PeerId, }, } @@ -66,14 +66,14 @@ impl Config { }) } - pub(crate) fn with_remote_peer_id( - remote_peer_id: Option, + pub(crate) fn with_expected_peer_id( + expected_peer_id: Option, ) -> Result { let identity = libp2p_identity::Keypair::generate_ed25519(); Ok(Self { server: crate::make_server_config(&identity)?, - client: crate::make_client_config(&identity, remote_peer_id)?, + client: crate::make_client_config(&identity, expected_peer_id)?, }) } } @@ -148,11 +148,11 @@ where mut self, socket: C, _: Self::Info, - remote_peer_id: Option, + expected_peer_id: Option, ) -> Self::Future { async move { - // Create new ad-hoc client and server configuration by passing the remote PeerId - self = Self::with_remote_peer_id(remote_peer_id)?; + // Create new ad-hoc client and server configuration by passing the expected PeerId + self = Self::with_expected_peer_id(expected_peer_id)?; // Spec: In order to keep this flexibility for future versions, clients that only support // the version of the handshake defined in this document MUST NOT send any value in the From 2ede11c5a720022d239ace68a36560649abf4a70 Mon Sep 17 00:00:00 2001 From: Denis Deniz Date: Mon, 27 Nov 2023 11:19:38 +0100 Subject: [PATCH 18/18] refactor: store the `identity::Keypair` within `Config` Explicitly call `make_server_config` and `make_client_config` as part of the upgrade. --- transports/tls/src/upgrade.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/transports/tls/src/upgrade.rs b/transports/tls/src/upgrade.rs index 9db2d50632b..17f8f498a99 100644 --- a/transports/tls/src/upgrade.rs +++ b/transports/tls/src/upgrade.rs @@ -56,6 +56,7 @@ pub enum UpgradeError { pub struct Config { server: rustls::ServerConfig, client: rustls::ClientConfig, + keypair: identity::Keypair, } impl Config { @@ -63,17 +64,7 @@ impl Config { Ok(Self { server: crate::make_server_config(identity)?, client: crate::make_client_config(identity, None)?, - }) - } - - pub(crate) fn with_expected_peer_id( - expected_peer_id: Option, - ) -> Result { - let identity = libp2p_identity::Keypair::generate_ed25519(); - - Ok(Self { - server: crate::make_server_config(&identity)?, - client: crate::make_client_config(&identity, expected_peer_id)?, + keypair: identity.clone(), }) } } @@ -152,7 +143,10 @@ where ) -> Self::Future { async move { // Create new ad-hoc client and server configuration by passing the expected PeerId - self = Self::with_expected_peer_id(expected_peer_id)?; + let keypair = libp2p_identity::Keypair::generate_ed25519(); + self.server = crate::make_server_config(&keypair)?; + self.client = crate::make_client_config(&keypair, expected_peer_id)?; + self.keypair = keypair; // Spec: In order to keep this flexibility for future versions, clients that only support // the version of the handshake defined in this document MUST NOT send any value in the