feat(iroh)!: Make retrieving NodeID and ALPN infallible #3147
Conversation
Our crypto setup makese some guarantees here. But we do need to be careful not to unwrap something that can be triggered by a modified or custom remote.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3147/docs/iroh/ Last updated: 2025-01-23T14:45:00Z |
flub
changed the title
| @@ -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), | |||
There was a problem hiding this comment.
err... unrelated, but maybe this should actually pull out the conn.alpn()
| // Note, we could totally provide this method to be on a Connection as well. But we'd | ||
| // need to wrap Connection too. |
There was a problem hiding this comment.
I think we can delete this note
| 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")) | ||
| } |
There was a problem hiding this comment.
| 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.
| 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() |
There was a problem hiding this comment.
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?).
| let data = data | ||
| .downcast::<quinn::crypto::rustls::HandshakeData>() | ||
| .expect("iroh crypto setup"); | ||
| data.protocol.expect("ALPN required by iroh crypto setup") |
There was a problem hiding this comment.
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?
Description
Our crypto setup makese some guarantees here. But we do need to be
careful not to unwrap something that can be triggered by a modified or
custom remote.
Breaking Changes
TODO
iroh
Connecting::handshake_data.Connection::handshake_data.Connection::peer_identity.Connecting::alpnchanged to ConnectionError.Connection::alpnis now infallible.Connection::remote_node_idis now infallible.Notes & open questions
Change checklist