Skip to content

feat(iroh)!: Make retrieving NodeID and ALPN infallible #3147

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion iroh/examples/dht_discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ async fn chat_server(args: Args) -> anyhow::Result<()> {
};
tokio::spawn(async move {
let connection = connecting.await?;
let remote_node_id = connection.remote_node_id()?;
let remote_node_id = connection.remote_node_id();
println!("got connection from {}", remote_node_id);
// just leave the tasks hanging. this is just an example.
let (mut writer, mut reader) = connection.accept_bi().await?;
Expand Down
2 changes: 1 addition & 1 deletion iroh/examples/echo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ impl ProtocolHandler for Echo {
// Wait for the connection to be fully established.
let connection = connecting.await?;
// We can get the remote's node id from the connection.
let node_id = connection.remote_node_id()?;
let node_id = connection.remote_node_id();
println!("accepted connection from {node_id}");

// Our protocol is a simple request-response protocol, so we expect the
Expand Down
6 changes: 3 additions & 3 deletions iroh/examples/listen-unreliable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ async fn main() -> anyhow::Result<()> {
// accept incoming connections, returns a normal QUIC connection

while let Some(incoming) = endpoint.accept().await {
let mut connecting = match incoming.accept() {
let connecting = match incoming.accept() {
Ok(connecting) => connecting,
Err(err) => {
warn!("incoming connection failed: {err:#}");
Expand All @@ -67,9 +67,9 @@ async fn main() -> anyhow::Result<()> {
continue;
}
};
let alpn = connecting.alpn().await?;
let conn = connecting.await?;
let node_id = conn.remote_node_id()?;
let alpn = conn.alpn();
let node_id = conn.remote_node_id();
info!(
"new (unreliable) connection from {node_id} with ALPN {} (coming from {})",
String::from_utf8_lossy(&alpn),
Expand Down
6 changes: 3 additions & 3 deletions iroh/examples/listen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ async fn main() -> anyhow::Result<()> {
);
// accept incoming connections, returns a normal QUIC connection
while let Some(incoming) = endpoint.accept().await {
let mut connecting = match incoming.accept() {
let connecting = match incoming.accept() {
Ok(connecting) => connecting,
Err(err) => {
warn!("incoming connection failed: {err:#}");
Expand All @@ -68,9 +68,9 @@ async fn main() -> anyhow::Result<()> {
continue;
}
};
let alpn = connecting.alpn().await?;
let conn = connecting.await?;
let node_id = conn.remote_node_id()?;
let alpn = conn.alpn();
let node_id = conn.remote_node_id();
info!(
"new connection from {node_id} with ALPN {} (coming from {})",
String::from_utf8_lossy(&alpn),
Expand Down
2 changes: 1 addition & 1 deletion iroh/examples/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ impl ProtocolHandler for BlobSearch {
// Wait for the connection to be fully established.
let connection = connecting.await?;
// We can get the remote's node id from the connection.
let node_id = connection.remote_node_id()?;
let node_id = connection.remote_node_id();
println!("accepted connection from {node_id}");

// Our protocol is a simple request-response protocol, so we expect the
Expand Down
2 changes: 1 addition & 1 deletion iroh/examples/transfer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ async fn provide(
}
};
let conn = connecting.await?;
let node_id = conn.remote_node_id()?;
let node_id = conn.remote_node_id();
info!(
"new connection from {node_id} with ALPN {} (coming from {})",
String::from_utf8_lossy(TRANSFER_ALPN),
Copy link
Member

Choose a reason for hiding this comment

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

err... unrelated, but maybe this should actually pull out the conn.alpn()

Expand Down
99 changes: 27 additions & 72 deletions iroh/src/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
//! [module docs]: crate

use std::{
any::Any,
collections::BTreeSet,
future::{Future, IntoFuture},
net::{IpAddr, SocketAddr, SocketAddrV4, SocketAddrV6},
Expand Down Expand Up @@ -1277,11 +1276,6 @@ impl Connecting {
}
}

/// Parameters negotiated during the handshake
pub async fn handshake_data(&mut self) -> Result<Box<dyn Any>, ConnectionError> {
self.inner.handshake_data().await
}

/// The local IP address which was used when the peer established the connection.
pub fn local_ip(&self) -> Option<IpAddr> {
self.inner.local_ip()
Expand All @@ -1295,15 +1289,12 @@ impl Connecting {
/// Extracts the ALPN protocol from the peer's handshake data.
// Note, we could totally provide this method to be on a Connection as well. But we'd
// need to wrap Connection too.
Comment on lines 1290 to 1291
Copy link
Member

Choose a reason for hiding this comment

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

I think we can delete this note

pub async fn alpn(&mut self) -> Result<Vec<u8>> {
let data = self.handshake_data().await?;
match data.downcast::<quinn::crypto::rustls::HandshakeData>() {
Ok(data) => match data.protocol {
Some(protocol) => Ok(protocol),
None => bail!("no ALPN protocol available"),
},
Err(_) => bail!("unknown handshake type"),
}
pub async fn alpn(&mut self) -> Result<Vec<u8>, ConnectionError> {
let data = self.inner.handshake_data().await?;
let data = data
.downcast::<quinn::crypto::rustls::HandshakeData>()
.expect("crypto setup for iroh");
Ok(data.protocol.expect("ALPN required by iroh crypto setup"))
}
Comment on lines +1292 to 1298
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
pub async fn alpn(&mut self) -> Result<Vec<u8>, ConnectionError> {
let data = self.inner.handshake_data().await?;
let data = data
.downcast::<quinn::crypto::rustls::HandshakeData>()
.expect("crypto setup for iroh");
Ok(data.protocol.expect("ALPN required by iroh crypto setup"))
}
pub async fn alpn(&mut self) -> Result<Vec<u8>, ConnectionError> {
let data = self.inner.handshake_data().await?;
let data = data
.downcast::<quinn::crypto::rustls::HandshakeData>()
.expect("invariant checked by iroh's custom rustls verifier");
Ok(data.protocol.expect("invariant upheld by iroh's custom rustls verifier"))
}

Ideally insert the function names of whatever is actually verifying these invariants here.

}

Expand Down Expand Up @@ -1539,38 +1530,13 @@ impl Connection {
self.inner.congestion_state()
}

/// Parameters negotiated during the handshake.
///
/// Guaranteed to return `Some` on fully established connections or after
/// [`Connecting::handshake_data()`] succeeds. See that method's documentations for
/// details on the returned value.
///
/// [`Connection::handshake_data()`]: crate::Connecting::handshake_data
#[inline]
pub fn handshake_data(&self) -> Option<Box<dyn Any>> {
self.inner.handshake_data()
}

/// Extracts the ALPN protocol from the peer's handshake data.
pub fn alpn(&self) -> Option<Vec<u8>> {
let data = self.handshake_data()?;
match data.downcast::<quinn::crypto::rustls::HandshakeData>() {
Ok(data) => data.protocol,
Err(_) => None,
}
}

/// Cryptographic identity of the peer.
///
/// The dynamic type returned is determined by the configured [`Session`]. For the
/// default `rustls` session, the return value can be [`downcast`] to a
/// <code>Vec<[rustls::pki_types::CertificateDer]></code>
///
/// [`Session`]: quinn_proto::crypto::Session
/// [`downcast`]: Box::downcast
#[inline]
pub fn peer_identity(&self) -> Option<Box<dyn Any>> {
self.inner.peer_identity()
pub fn alpn(&self) -> Vec<u8> {
let data = self.inner.handshake_data().expect("iroh crypto setup");
let data = data
.downcast::<quinn::crypto::rustls::HandshakeData>()
.expect("iroh crypto setup");
data.protocol.expect("ALPN required by iroh crypto setup")
Copy link
Contributor

Choose a reason for hiding this comment

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

Guaranteed to be set if a nonempty list of protocols was specified for this connection.

Are we sure this will always be the case from the public api usage?

}

/// Returns the [`NodeId`] from the peer's TLS certificate.
Expand All @@ -1581,25 +1547,17 @@ impl Connection {
/// connection.
///
/// [`PublicKey`]: iroh_base::PublicKey
// TODO: Would be nice if this could be infallible.
pub fn remote_node_id(&self) -> Result<NodeId> {
let data = self.peer_identity();
match data {
None => bail!("no peer certificate found"),
Some(data) => match data.downcast::<Vec<rustls::pki_types::CertificateDer>>() {
Ok(certs) => {
if certs.len() != 1 {
bail!(
"expected a single peer certificate, but {} found",
certs.len()
);
}
let cert = tls::certificate::parse(&certs[0])?;
Ok(cert.peer_id())
}
Err(_) => bail!("invalid peer certificate"),
},
}
pub fn remote_node_id(&self) -> NodeId {
let data = self
.inner
.peer_identity()
.expect("required by iroh crypto setup");
let certs = data
.downcast::<Vec<rustls::pki_types::CertificateDer>>()
.expect("iroh crypto setup");
debug_assert_eq!(certs.len(), 1);
let cert = tls::certificate::parse(&certs[0]).expect("TODO: is this guaranteed by now?");
cert.peer_id()
Comment on lines +1550 to +1560
Copy link
Member

Choose a reason for hiding this comment

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

I'd like better expect messages here, too. "required by iroh crypto setup" doesn't suffice IMO.
The message should mention the place that the peer_identity field is populated.
I tried figuring this out, but yeah - it's probably not that easy (flows through quinn, right?).

}

/// A stable identifier for this connection.
Expand Down Expand Up @@ -1660,10 +1618,7 @@ impl Connection {
/// function.
fn try_send_rtt_msg(conn: &Connection, magic_ep: &Endpoint) {
// If we can't notify the rtt-actor that's not great but not critical.
let Ok(node_id) = conn.remote_node_id() else {
warn!(?conn, "failed to get remote node id");
return;
};
let node_id = conn.remote_node_id();
let Ok(conn_type_changes) = magic_ep.conn_type(node_id) else {
warn!(?conn, "failed to create conn_type stream");
return;
Expand Down Expand Up @@ -1989,7 +1944,7 @@ mod tests {
info!("[server] round {i}");
let incoming = ep.accept().await.unwrap();
let conn = incoming.await.unwrap();
let node_id = conn.remote_node_id().unwrap();
let node_id = conn.remote_node_id();
info!(%i, peer = %node_id.fmt_short(), "accepted connection");
let (mut send, mut recv) = conn.accept_bi().await.unwrap();
let mut buf = vec![0u8; chunk_size];
Expand Down Expand Up @@ -2105,7 +2060,7 @@ mod tests {
let mut iconn = incoming.accept().unwrap();
let alpn = iconn.alpn().await.unwrap();
let conn = iconn.await.unwrap();
let node_id = conn.remote_node_id().unwrap();
let node_id = conn.remote_node_id();
assert_eq!(node_id, src);
assert_eq!(alpn, TEST_ALPN);
let (mut send, mut recv) = conn.accept_bi().await.unwrap();
Expand Down Expand Up @@ -2195,7 +2150,7 @@ mod tests {
async fn accept(ep: &Endpoint) -> NodeId {
let incoming = ep.accept().await.unwrap();
let conn = incoming.await.unwrap();
let node_id = conn.remote_node_id().unwrap();
let node_id = conn.remote_node_id();
tracing::info!(node_id=%node_id.fmt_short(), "accepted connection");
node_id
}
Expand Down
Loading