Skip to content
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

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

Open
rklaehn opened this issue Oct 10, 2024 · 4 comments
Labels
c-iroh-net feat New feature or request

Comments

@rklaehn
Copy link
Contributor

rklaehn commented Oct 10, 2024

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 rklaehn added c-iroh-net feat New feature or request labels Oct 10, 2024
@dignifiedquire dignifiedquire added this to the v0.27.0 milestone Oct 10, 2024
@rklaehn
Copy link
Contributor Author

rklaehn commented Oct 22, 2024

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 ?

@flub
Copy link
Contributor

flub commented Oct 22, 2024

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.

@rklaehn
Copy link
Contributor Author

rklaehn commented Oct 22, 2024

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.

@flub
Copy link
Contributor

flub commented Oct 23, 2024

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.

@dignifiedquire dignifiedquire removed this from the v0.27.0 milestone Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-iroh-net feat New feature or request
Projects
Status: No status
Development

No branches or pull requests

3 participants