feat(iroh)!: Wrap the Connection struct so we own the type #3110

flub · 2025-01-09T11:03:58Z

Description

This makes iroh own the Connection type. This is desirable because:

Some: upstream APIs don't work well for us,
e.g. Connection::remote_address is not that meaningful. This allows
us to remove those and replace them with more appropriate APIs. This
has not yet been done in this PR however.
The connection type and changes are currently on the Endpoint, they
also belong on the Connection.
Connection information, like which pats are used, the latencies of
those paths etc, also belong on the Connection.
It is generally not great to expose types which you have no control
over in your public API. For the 1.0 we do not want to have any
such structs. This takes an important step in that direction.

As part of this change Connection::remote_node_id replaces the
get_remote_node_id function. Also Connection::alpn now exists.

Breaking Changes

iroh

iroh::endpoint::get_remote_node_id has been removed. Use
iroh::endpoint::Connection::remote_node_id instead.

Notes & open questions

This does not yet clean up the existing methods in the Connection,
preserving most of them as-is. It is easier to handle those
separately as each one involves careful decisions about the API.

Replaces #2768

Change checklist

Self-review.
Documentation updates following the style guide, if relevant.
Tests if relevant.
All breaking changes documented.

This makes iroh own the Connection type. This is desireable because: - Some: upstream APIs don't work well for us, e.g. Connection::remote_address is not that meaningful. This allows us to remove those and replace them with more appropriate APIs. This has not yet been done in this PR however. - The connection type and changes are currently on the Endpoint, they also belong on the Connection. - Connection information, like which pats are used, the latencies of those paths etc, also belong on the Connection. - It is generally not great to expose types which you have no control over in your public API. For the 1.0 we do not want to have any such structs. This takes an important step in that direction. As part of this change Connection::remote_node_id replaces the get_remote_node_id function. Also Connection::alpn now exists.

github-actions · 2025-01-09T11:05:56Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3110/docs/iroh/

Last updated: 2025-01-23T13:51:13Z

github-actions · 2025-01-09T11:08:57Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 3feb779

iroh/src/endpoint.rs

matheus23

I really like this.

I'd personally love to go even further and remove Connection::remote_address already, etc. but I get that you're going to do that as follow-ups instead.

matheus23 · 2025-01-13T16:18:00Z

You requested a review of this, but there weren't any changes since I last accepted this PR.
I'm still happy with it? And I don't think that discussions since have changed my view on this?

flub · 2025-01-13T17:02:50Z

You requested a review of this, but there weren't any changes since I last accepted this PR. I'm still happy with it? And I don't think that discussions since have changed my view on this?

The new review requests is to see whether you're happy with #3110 (comment) and deferring it to an issue.

dignifiedquire · 2025-01-14T09:49:35Z

The downcasts for both remote_node_id and alpn, should uses .expects, as this is not sth dynamic, but just that the rustls api doesn't give us the type back we put in, type safely

flub · 2025-01-14T12:18:46Z

The downcasts for both remote_node_id and alpn, should uses .expects, as this is not sth dynamic, but just that the rustls api doesn't give us the type back we put in, type safely

Hehe, see also the discussion here: #3123

matheus23 · 2025-01-14T12:21:09Z

succumb to the "same review twice" feedback flub. Just... succumb. 😂

flub · 2025-01-14T14:13:41Z

succumb to the "same review twice" feedback flub. Just... succumb. 😂

Fine by me! If you're all sure that these are real invariants that won't change them I'm all for it.

I don't like that they'd need an .expect() and because of that I'd need to do a bunch of digging through all these layers. I still maintain that if that is true we should be able to write this in a way that no .expect() is needed though.

So please tell me: should I unwrap/expect all these:

Connection::remote_node_id
Connection::peer_identity
Connection::alpn
Connection::handshake_data

so that they are no longer Option/Result?

matheus23 · 2025-01-14T15:41:40Z

Actually, sorry. I took a closer look at this again. I think the right thing to do would be to just ship the PR as is - and then we'll hash out a way to do this without unwrap in #3123 (writing up a comment there)

matheus23

I think checking handshake_data and whether an ALPN is present

should be an Err(ConnectionError) in Connecting::alpn and <Connecting as Future>::poll
should be an .expect in Connection::<methods>

It's entirely possible we get an Incoming and Connecting from an untrusted peer with e.g. a modified iroh. We don't want to crash because we receive such a connection attempt.

But once we actually verified the Connecting and turned it into a Connection, it should be safe to .expect, since the state in the Connection isn't determined by the remote anymore, but instead was checked by us previously when it was a Connecting.

matheus23 · 2025-01-23T11:53:47Z

iroh/src/endpoint.rs

+    pub fn peer_identity(&self) -> Box<dyn Any> {
+        self.inner
+            .peer_identity()
+            .expect("we always have a peer identity")


I like this expect, but I don't understand where this is checked.
Can we have a message similar to the above? ("checked in <...>")

I assume we have this any time we manage to establish the TLS connection with our configuration. But yes, the message can be better.

matheus23 · 2025-01-23T11:54:38Z

iroh/src/endpoint.rs

-        }
+        let certs = data
+            .downcast::<Vec<rustls::pki_types::CertificateDer>>()
+            .expect("we always have a cert");


Would prefer a message like "checked in ::poll" (and actually checking it there).

I was assuming that this type would always be correct if we got this far. But maybe it's not.

matheus23 · 2025-01-23T11:59:34Z

iroh/src/endpoint.rs

+                let handshake_data = conn.handshake_data();
+                let handshake_data = handshake_data
+                    .downcast::<quinn::crypto::rustls::HandshakeData>()
+                    .expect("fixed crypto setup for iroh");


Sorry for the back and forth on this.
I actually don't think this should be an .expect, because you cannot guarantee that the other side runs a truthful implementation of iroh.
If we keep it like this, this is a free ticket for getting DoSed from anyone who knows your NodeId, if you're accepting connections.
(I mean we're far from being DoS-safe, but let's at least try to avoid it in this case.)

It is my understanding that this downcast only depends on how we configure Rustls. If the other side does something different this does not make a difference to the type can downcast to.

matheus23 · 2025-01-23T12:00:15Z

iroh/src/endpoint.rs

-            Err(_) => bail!("unknown handshake type"),
+        let data = data
+            .downcast::<quinn::crypto::rustls::HandshakeData>()
+            .expect("fixed crypto setup for iroh");


(similar to the other comment - at this point we haven't verified that the handshake data is there for sure, so we should probably return an error if it isn't. IMO a CONNECTION_REFUSED is totally fine for this case as well.)

flub

I'm going to remove the last commit that was trying to remove some Options and Results. I think it mixes two separate efforts, it will be a lot nicer to have this merged independently with separate PRs, discussions and commit messages.

Also, it really not clear everything is correct. Even the checks I added for the ALPN are weird: because of our Rustls config we probably don't need those. But it gets worse, we can still create a server with no ALPN but then just can't connect to it. This needs more time. I'll file an issue for this instead.

Edit: I updated #3123 instead.

flub · 2025-01-23T12:52:08Z

iroh/src/endpoint.rs

+                let handshake_data = conn.handshake_data();
+                let handshake_data = handshake_data
+                    .downcast::<quinn::crypto::rustls::HandshakeData>()
+                    .expect("fixed crypto setup for iroh");


It is my understanding that this downcast only depends on how we configure Rustls. If the other side does something different this does not make a difference to the type can downcast to.

flub · 2025-01-23T12:53:12Z

iroh/src/endpoint.rs

+    pub fn peer_identity(&self) -> Box<dyn Any> {
+        self.inner
+            .peer_identity()
+            .expect("we always have a peer identity")


I assume we have this any time we manage to establish the TLS connection with our configuration. But yes, the message can be better.

flub · 2025-01-23T12:54:00Z

iroh/src/endpoint.rs

-        }
+        let certs = data
+            .downcast::<Vec<rustls::pki_types::CertificateDer>>()
+            .expect("we always have a cert");


I was assuming that this type would always be correct if we got this far. But maybe it's not.

flub · 2025-01-23T13:04:00Z

Pushed without the last commit that changes APIs. Let's not mix things up.

flub mentioned this pull request Jan 9, 2025

feat(iroh-net)!: Create our own connection struct #2768

Closed

3 tasks

flub requested a review from a team January 9, 2025 11:04

matheus23 added this to the v0.31.0 milestone Jan 9, 2025

matheus23 reviewed Jan 9, 2025

View reviewed changes

iroh/src/endpoint.rs Show resolved Hide resolved

matheus23 approved these changes Jan 9, 2025

View reviewed changes

flub self-assigned this Jan 9, 2025

flub requested review from dignifiedquire and matheus23 January 13, 2025 11:23

matheus23 approved these changes Jan 14, 2025

View reviewed changes

matheus23 modified the milestones: v0.31.0, v0.32.0 Jan 14, 2025

matheus23 requested changes Jan 23, 2025

View reviewed changes

flub commented Jan 23, 2025

View reviewed changes

flub force-pushed the flub/owned-connect-2 branch from 68ce91e to 4741be8 Compare January 23, 2025 13:03

matheus23 approved these changes Jan 23, 2025

View reviewed changes

flub added 2 commits January 23, 2025 14:10

Merge branch 'main' into flub/owned-connect-2

43344ed

fixup docs

c7b8246

flub enabled auto-merge January 23, 2025 13:50

flub added this pull request to the merge queue Jan 23, 2025

Merged via the queue into main with commit 2e61ff2 Jan 23, 2025
25 of 26 checks passed

flub deleted the flub/owned-connect-2 branch January 23, 2025 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh)!: Wrap the Connection struct so we own the type #3110

feat(iroh)!: Wrap the Connection struct so we own the type #3110

flub commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

matheus23 left a comment

matheus23 commented Jan 13, 2025

flub commented Jan 13, 2025

dignifiedquire commented Jan 14, 2025

flub commented Jan 14, 2025

matheus23 commented Jan 14, 2025

flub commented Jan 14, 2025

matheus23 commented Jan 14, 2025

matheus23 left a comment

matheus23 Jan 23, 2025

flub Jan 23, 2025

matheus23 Jan 23, 2025

flub Jan 23, 2025

matheus23 Jan 23, 2025

flub Jan 23, 2025

matheus23 Jan 23, 2025

flub left a comment •

edited

Loading

flub Jan 23, 2025

flub Jan 23, 2025

flub Jan 23, 2025

flub commented Jan 23, 2025

feat(iroh)!: Wrap the Connection struct so we own the type #3110

feat(iroh)!: Wrap the Connection struct so we own the type #3110

Conversation

flub commented Jan 9, 2025 • edited Loading

Description

Breaking Changes

iroh

Notes & open questions

Change checklist

github-actions bot commented Jan 9, 2025 • edited Loading

github-actions bot commented Jan 9, 2025 • edited Loading

matheus23 left a comment

Choose a reason for hiding this comment

matheus23 commented Jan 13, 2025

flub commented Jan 13, 2025

dignifiedquire commented Jan 14, 2025

flub commented Jan 14, 2025

matheus23 commented Jan 14, 2025

flub commented Jan 14, 2025

matheus23 commented Jan 14, 2025

matheus23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flub left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flub commented Jan 23, 2025

flub commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

flub left a comment •

edited

Loading