Re: [quicwg/base-drafts] Rewrite text about Version Negotiation (#1039)

[Martin Thomson <[e-mail]>]

I think that @mikkelfj is almost right here, but I think that there's a more general problem with your pseudo-c-code.

I don't think that we need to proscribe a particular software architecture (we're not THAT far gone just yet), but the way that I think of approaching this problem is to start with what the software needs to do first.  And for me the first step isn't to just process the packet, but to determine which connection it should be handled by.  The code here mixes connection selection into the handing more than I'd like.

I also see a few places where you assume that long header is synonymous with handshake, and that's not something we're assuming when writing these descriptions (it's true of the current set of types, but I don't think that we're prepared to commit to that just yet).

I'd like to think that we could have a simple bit of logic there.  Something like:

```python
connection = lookup_by_connection_id(packet.connection_id)
if connection is None:
    connection = lookup_by_tuple(packet.address)
if connection is not None:
    connection.handle_packet(packet)
    return
```

After which you engage the logic for handling packets that don't belong to existing connections, which includes server-side version negotiation.

This is what I ended up with when following that to its conclusion:

```python
# Lookup by connection ID.  A client that has just one connection might
# implement this lookup simply by checking the connection ID against their one
# connection.  The connection ID might be None for short headers, but that's OK.
# Note that the lookup needs to be able to return the same connection against
# multiple connection IDs in order to support NEW_CONNECTION_ID.
connection = lookup_by_connection_id(packet.connection_id)
if connection is not None:
    connection.handle_packet(packet)
    return

# The lookup by tuple is run in two steps.  The first check works only when the
# configuration allows the peer to omit the connection ID.  In that case we
# should only have one connection on each tuple.
if config.allow_omit_connection_id:
    connections = lookup_by_tuple(packet.address)
    assert(len(connections) <= 1)
    if len(connections) == 1:
        connections[0].handle_packet(packet)
        return

# Clients that are handshaking might receive a 1-RTT packet with a
# server-selected connection ID if there is packet loss or reordering.  Clients
# that allow multiple connections on the same address tuple need to pass the
# packet to all potential connections.  The client packet handler is responsible
# for buffering packets that it isn't ready for yet.
if is_client:
    connections = lookup_by_tuple(packet.address)
    assert(config.allow_shared_tuple or len(connections) <= 1)
    for c in connections:
        if not c.handshake_done():
            c.handle_packet(packet)
    return

# The server might need to make a new connection now.

# First check that the version is supported.  (Note that we can assume that
# short headers, which have a version of None, are unsupported on the basis that we
# should have found a connection already for any of those.  Only the client
# needs to deal with short headers before version negotiation completes.)
if not is_supported_version(packet.version):
    if packet.maybe_initial(): # True if long header and if size is plausible.
        send_version_negotiation(packet.address.src)
    return # drop it

if packet.is_initial():
    connection = make_connection()
    connection.version_negotiation_done = true
    connection.handle_packet(packet)
    return

# Servers will want to be very careful here.
if is_maybe_0rtt(packet):
    buffer_0rtt(packet)

# Anything not handled by this point is junk and can be dropped.
```

Admittedly, this is a fair bit simpler without the buffering complicating things, but I think that we have to cover that possibility properly when we write text.  Also, it could probably be factored more cleanly, but it turns out to be quite complicated once you add buffering.

I was surprised to see that the addition of multiple connections on the same address tuple didn't complicate things that much if you accept that you want to buffer 1-RTT packets at the client.  While that's likely only because of the structure I chose to use here, it's still something of a positive outcome, I guess.

When the packet arrives at a connection, you run the version check first.  The client has to do version negotiation.  This is pretty simple:

```python
if not this.version_negotiation_done:
    # This branch only applies to clients because the server always sets
    # version_negotiation_done to true before starting.
    assert(is_client)
    if packet.version == 0:
        this.version = pick_version(packet.versions)
        this.version_negotiation_done = true
        this.start_over()
        return false # packet will be discarded
    if packet.version == this.version:
        this.version_negotiation_done = true
        return true
    # This includes short headers, which we aren't ready for yet.
    return false

if packet.version is not None and packet.version != this.version:
    return false # discard it

return true # OK to process this packet

```

Noting that this doesn't handle the case where the client needs to buffer 1-RTT, so any potential buffering check needs to come before this is run.

And then it's on to the standard processing, decryption and so forth.  There's also a bunch of tail-end processing necessary, such as that needed to maintain the lookup tables, and that needs to be conditional on the success of the decryption, but that's an exercise for the reader.

One thing that I think is important is that I barely pay attention to whether the packet is long or short in this logic.  Though that determines whether or not we have a version field in the packet (and thus whether we consider the version supported), and similarly whether the connection ID might be absent, that effect is constrained to those places that check those fields.  Otherwise the code doesn't care about packet format.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1039#issuecomment-358194841