iroh icon indicating copy to clipboard operation
iroh copied to clipboard

ProtocolHandler accept needs to get the connection at the earliest possible state

Open rklaehn opened this issue 1 year ago • 4 comments

ProtocolHandler::accept currently get a Connecting. We want custom protocols to make decisiosn about the connection even earlier than that, so this needs to be a Incoming.

rklaehn avatar Oct 10 '24 12:10 rklaehn

This is not as easy as originally anticipated. I think at the incoming stage you don't yet know the ALPN, so you can not dispatch before getting the connecting stage.

We probably need a hook to reject connections at the incoming stage, but it can not be in the per-ALPN protocol handler. Or am I wrong @flub ?

rklaehn avatar Oct 22 '24 08:10 rklaehn

You are right, when you receive the first packet you do not yet have an ALPN on the Incoming. You only have the ALPN once you have called Incoming::accept which returns you a Connecting and in turns provides the ALPN. Note however that Incoming::accept is a sync function, there is no network involved! The only difference is that it will parse the packet it already has.

To be fair, my guess would be that the kind of rejecting you want to do at the Incoming level is protocol-agnostic. So for the new Router proposal I don't think each protocol handler needs to be able to hook into handling Incoming: the router can have a protocol-independent handler for this. So I think giving protocol handlers a Connecting is reasonable.

flub avatar Oct 22 '24 08:10 flub

So accept really involves only parsing and no network layer stuff whatsoever? So in principle you could have refuse/retry/ignore even on Connecting? Or is there something triggered in the background when going from Incoming to Connecting?

Getting the ALPN from a Connecting also is just parsing, right? But the fn is async.

rklaehn avatar Oct 22 '24 17:10 rklaehn

Hum, on the surface of it you could do refuse/retry/ignore on Connecting. I think the main reason it is split off is to save the effort of parsing. However as you point out maybe something already happens in the background - we'd have to check this carefully. And if we do this maybe see if upstream wants it as that would prevent that from becoming an issue in the future. But also what happens if you drop Connecting? My guess would be that it closes the connection attempt by sending a CONNECTION_CLOSE frame but this might be worth checking (and documenting!).

Getting the ALPN from a Connecting also is just parsing, right? But the fn is async.

Without looking at the code I think this is because there is no guarantee that the first packet contains all the handshake TLS data. Especially for normal QUIC connections certificates chains can be rather large and may not fit in one packet. So some of the time (I think for us most of the time, I've only ever seen one client->server handshake packet when I captured trafic) you'll already have the ALPN data but some of the time you may have to wait for more packets.

flub avatar Oct 23 '24 07:10 flub

Closing because the ProtocolHandler trait has changed, we now have an accept that takes a Connection and a on_connecting that takes a Connecting

ramfox avatar Sep 23 '25 15:09 ramfox