Rewrite text about Version Negotiation #1039

martinduke · 2018-01-08T05:05:50Z

This is meant to resolve #1038.

I made the following changes:

Explicitly state the conditions for each possible server action on receipt of the first packet
Use IETF terms like "MUST" when missing.
Eliminate redundant text
Eliminate all explicit mentions of the 1200-byte limit, as this is defined later in the text, where it also explains exactly how to measure packet size. I'm concerned that just saying "1200 bytes" here might cause people to count IP headers or something. Make them read the definition!

I don't believe there's any substantive change to the spec.

This is meant to resolve #1038. I made the following changes: 1) Explicitly state the conditions for each possible server action on receipt of the first packet 2) Use IETF terms like "MUST" when missing. 3) Eliminate redundant text 4) Eliminate all explicit mentions of the 1200-byte limit, as this is defined later in the text, where it also explains exactly how to measure packet size. I'm concerned that just saying "1200 bytes" here might cause people to count IP headers or something. Make them read the definition! I don't believe there's any substantive change to the spec.

martinthomson

This appears to be a solid improvement, but I think that we could structure it a little better.

This is, I think, a better logical flow:

if (short || too_small) {
  discard_buffer_or_stateless_reset();
} else if (supported_version) {
  if (initial) {
    make_new_connection();
  } else {
    discard_buffer_or_stateless_reset();
  }
} else {
  send_Version_Negotiation();
}

What you have is close to this, but I would merge the handling of packets from a supported version into the same paragraph.

I'm also not sure about the logic regarding stateless reset. You don't offer that as an option for a non-initial packet. That might be sensible for the protocol as currently structured, but that assumes that we don't have a use for packets with the long header after connection setup. I don't think that is safe.

The logic for deciding whether to discard or buffer might be different for packets of a supported version. For instance, 0-RTT packets might never trigger stateless reset, because you can identify them as being for a new connection and therefore unlikely to be from before you died and were revived. At this level though, we only need to list the valid options and I think that includes stateless reset.

martinthomson · 2018-01-08T05:46:49Z

draft-ietf-quic-transport.md

+packet for that version, the server MUST either drop the packet or buffer it in
+anticipation of additional packets.
+
+If a supported version, and a valid Initial packet for that version, the server


"If the packet contains a supported version and is a valid Initial packet for that version,"

martinthomson · 2018-01-08T05:52:44Z

draft-ietf-quic-transport.md

 proceeds with the handshake ({{handshake}}).  This commits the server to the
 version that the client selected.

+If an unsupported version, the server MUST send a version negotiation packet


"If the packet contains an unsupported version and the packet is large enough, the server MUST [...]"

Capitalize "Version Negotiation" when referring to the packet.

Does this need to be a MUST? If the server has sent a Version Negotiation on this flow recently, we should allow suppression. Maybe a "MUST...unless".

martinthomson · 2018-01-08T05:55:14Z

draft-ietf-quic-transport.md

+a new connection. A series of tests dictate the allowed actions.
+
+If the packet uses a short form header, or is not long enough for the size
+required for the initial packet of any of its supported versions (as defined


The subject of the "its" is unclear here. "required for the initial packet in any QUIC version that the server supports" (see {{packet-size}} for the minimum size in this version of QUIC)"

martinthomson · 2018-01-08T05:55:45Z

draft-ietf-quic-transport.md

-successfully receives a response or it abandons the connection attempt.
-
+This system allows a server to process packets with unsupported versions without
+retaining state.  Though either the Initial packet or the version negotiation


Capitalize VN

martinthomson · 2018-01-08T06:26:44Z

draft-ietf-quic-transport.md

-packets that are the minimum size of all QUIC versions they support.
-
-
-## Version Negotiation {#version-negotiation}


Losing this header is a problem.

@martinthomson

Addressed @martinthomson's issues. Made further edits to be concise and well-organized.

mikkelfj · 2018-01-08T19:20:04Z

draft-ietf-quic-transport.md

@@ -876,56 +876,62 @@ Packets without connection IDs and long-form packets for connections that have
 incomplete cryptographic handshakes are associated with an existing connection
 using the tuple of source and destination IP addresses and ports.



This is ambigious: "packets without connection ID and long-form packets". Does it mean packets without connection ID, or packets that are in long form during handshake?

A long form always has a connection ID in the current version. During handshake a tuple is insufficient if the peer uses a non-unique port. So the handshake should always use the initial client connection ID for association. Also because without the connection ID the handshake crypto won't work. For other versions this might be different, but then the current text is not very clear.

I recall a conversation where we were going to assume that the 4-tuple does not change over the life of the handshake, and that we were willing to accept connection failure if it did.

At the server, there's a little ambiguity as to whether the next packet will contain the original client connection ID or the server-generated one.

Just setting aside connection ID for these purposes seems cleaner, though with a bunch more verbiage we could force endpoints to consider conn ID as well. I dunno.

Yes, we agreed that changing 4-tuple during the handshake was not permitted. It's too complicated otherwise. The questions of address validation alone are too complex to consider.

My concern is not changing tuple but having two conns with the same. It happens when avoiding ephemeral port exhaustion.

I agree we should try to write this text in a way that allows two connections on the same 4-tuple.

Is it too much to ask to limit it to one overloaded handshake at a time? Once the handshake is complete, there's no restriction on using the same tuple for another handshake.

One handshake at a time would complicate reverse proxies significantly and force them to use multiple ports or limit rates.

Proposed text:

Clients and servers associate packets with an existing or a new connection by using the connection ID or the tuple of port address pairs. The connection ID may be absent from a packet header in which case the tuple must be unique. The tuple may be non-unique when a client reuses the source port so the connection ID SHOULD be used when available. A server MAY choose to associate handshake packets using a combination of both the tuple and the client connection ID in order to protect against accidental or intentional collisions. Post handshake both the client and the server can rely on just the connection ID when available in the header because of packet protection, otherwise the tuple will be sufficient. Connection migration necessitates also considering other tuples and connection IDs as advertised by the NEW_CONNECTION_ID ({{frame-new-connection-id}}) frame.

A few edits to Mikkel's text, but the right ideas are there:

Clients and servers associate packets with an existing or a new connection by using the connection ID if one is present, or the tuple of port-address pairs otherwise. When the connection ID is omitted from short packet headers, correct association requires that the tuple be unique. The tuple might be non-unique when a client reuses the source port, so the connection ID SHOULD be included in the client-to-server direction. A server MAY choose to associate handshake packets using a combination of both the tuple and the client connection ID in order to protect against accidental or intentional collisions. After the handshake completes, the tuple and connection ID might change over the course of the connection; see {{frame-new-connection-id}}) and {{wherever-migration-lives}}.

I'm a little dubious of the "accidental or intentional collisions," though. I think we could reasonably drop that sentence.

The collision part originally referred to the handshake which is why the server was singled out, but after the handshake the same idea also applies to the client when the server chooses a new connection ID. I'm not sure how important that sentence is, but if included, the edited version might be better off noting that: "Potential collisions can be avoided by associating connections with both the connection ID and the tuple together.", but this is not 100% fool proof either, so perhaps leave that as an implementation detail.

The lead-in is good, but I think that the point we need to acknowledge is that during a handshake, uniqueness of the connection ID is not guaranteed because it is not chosen by the server. Therefore, new connections might be unsuccessful due to a collision on connection ID. Using the address tuple in addition to the connection ID narrows this enough that it probably isn't going to cause problems.

Presumably the client won't choose a connection ID that it has seen, so the risk is small: a server and client simultaneously choosing a colliding connection ID. For the server this can be either during the handshake or for NEW_CONNECTION_ID. Right now we don't have a use for long headers after the handshake, so those can probably be recognized as new connections, but that's possibly asking too much of servers for this somewhat marginal use case.

@mikkelfj

Addressed @mikkelfj's issue.

martinthomson

I like this structure much better. I do have a number of nits and I'd also like review from @janaiyengar in case I missed something, but I think that this is good.

martinthomson · 2018-01-08T22:14:02Z

draft-ietf-quic-transport.md

+with an existing connection, or - for servers - potentially create a new
+connection.
+
+Host handle packets that can be associated with an existing connection


martinthomson · 2018-01-08T22:15:04Z

draft-ietf-quic-transport.md

+Host handle packets that can be associated with an existing connection
+according to the current state of that connection. Short form packets without
+connection IDs, and long-form packets for connections that have incomplete
+cryptographic handshakes, are associated with an existing connection using the


Too many commas. You can remove these two commas.

martinthomson · 2018-01-08T22:16:32Z

draft-ietf-quic-transport.md

+If a server receives a packet not associated with an existing connection, it
+executes the following steps, in order:
+
+The server MUST check if the packet uses a short form header, or is not long


Let's go with a numbered list here.

martinthomson · 2018-01-08T22:19:51Z

draft-ietf-quic-transport.md

-
+Clients that support multiple QUIC versions SHOULD pad their Initial packets
+to reflect the largest minimum Initial packet size of all their versions.
+This ensures that that the server respond if there are any mutually supported


martinthomson · 2018-01-08T22:21:56Z

draft-ietf-quic-transport.md


+{{packet-handling}} describes the conditions under which endpoints negotiate
+versions.


I think that we could keep some of the exposition here. Maybe: "Version negotiation ensures that client and server agree to a QUIC version that is mutually supported. A server sends a Version Negotiation packet in response to a packet that might initiate a new connection, see {{packet-handling}} for details."

martinthomson · 2018-01-08T22:22:39Z

draft-ietf-quic-transport.md

-proceeds with the handshake ({{handshake}}).  This commits the server to the
-version that the client selected.
-
+Clients that support multiple QUIC versions SHOULD pad their Initial packets


Needs a bit more context. "The size of the first packet sent by a client will determine whether a server sends a Version Negotiation packet."

martinthomson · 2018-01-08T22:24:24Z

draft-ietf-quic-transport.md

+executes the following steps, in order:
+
+The server MUST check if the packet uses a short form header, or is not long
+enough for the size required for the initial packet of any QUIC version that


Let's avoid using "initial" and instead use "first" here. It could be confused with the packet type.

martinthomson · 2018-01-08T22:24:41Z

draft-ietf-quic-transport.md

+The server MUST check if the packet uses a short form header, or is not long
+enough for the size required for the initial packet of any QUIC version that
+the server supports. See {{packet-size}} for the definition of packet size
+and the minimum size in this version of QUIC. If either condition is true,


"and the minimum size of the first client packet in this version of QUIC"

martinthomson · 2018-01-08T22:25:14Z

draft-ietf-quic-transport.md

+### Handshake Buffering {{#handshake-buffer}}
+
+Due to packet reordering or loss, subsequent packets for a handshake might
+arrive at the server prior to the intended Initial packet. As described above,


Remove "intended" here.

martinthomson · 2018-01-08T22:25:48Z

draft-ietf-quic-transport.md

+commits the server to the version that the client selected. If not an Initial
+packet, the server MUST either buffer the packet or silently drop it.
+
+### Handshake Buffering {{#handshake-buffer}}


Use one curly brace and a hash for anchors: {#handshake-buffer}.

Addressed Martin's issues and fixed one error (first packets might not be "handshake" packets -- they might be 0RTT)

Whoops, forgot the last of Martin's comments.

ianswett

Some more concrete suggestions.

ianswett · 2018-01-09T02:16:19Z

draft-ietf-quic-transport.md

-in response could be lost, the client will send new packets until it
-successfully receives a response or it abandons the connection attempt.
-
+This system allows a server to process packets with unsupported versions without


The server isn't really processing the packets if it doesn't support the version. How about "This allows a server to respond to packets with unsupported versions without retaining state."

ianswett · 2018-01-09T02:20:05Z

draft-ietf-quic-transport.md

-proceeds with the handshake ({{handshake}}).  This commits the server to the
-version that the client selected.
-
+The size of the first packet sent by a client will determine whether a server


I find this sentence a bit confusing. I'd remove it.

I agree it's unnecessary. I'll strike it.

ianswett · 2018-01-09T02:34:15Z

draft-ietf-quic-transport.md

+with an existing connection, or - for servers - potentially create a new
+connection.
+
+Hosts handle packets that can be associated with an existing connection


I would change this to:
"Hosts associate packets with an existing connection if the connection ID matches an existing connection. This might include connection IDs that were advertised using NEW_CONNECTION_ID ({{frame-new-connection-id}}). If the connection ID is omitted, packets are associated using 4-tuple. Long form packets for existing connections with different 4-tuples SHOULD be discarded."

But I realize that's fairly similar to the current first paragraph, so maybe just keep that?

That could be interpreted as preventing the use of packets with the long header after connection setup. We've not made that decision yet.

Ian's proposal is the third rephrasing of the second paragraph, and the differences between them are very subtle. I think the difference is in a handful of corner cases:

a long header comes in on the same conn id as an established connection, but with a different 4-tuple, this might be due to NAT rebinding, or a coincidence (original: assign to established connection; mine: treat it as new; ian: discard it)

same as above, but the same 4-tuple. (original: to established connection; mine: to established connection; ian: discard it)

two clients attempt a handshake with the same conn id (original and mine: 2 separate connections; Ian: these are the same connection)

a client opens two connections simultaneously with the same 4-tuple but different conn ids. (original and mine: same connection; ian: two separate connections)

I slightly prefer Ian's text in the first two, but in 3 and 4 things can get a little hairy when the server changes the connection ID. This whole section gets pretty intricate, and I wonder if we're better off with pseudocode. Or alternatively, under-specify it and provide hosts with a little flexibility to do the right thing for then. I'll do a little more thinking on these corner cases, and welcome further input.

In that case, I think that we have a problem here.

On the first two, the original needs to be correct or we won't ever be able to use long headers after the handshake completes. For 3 it's always possible for a new client to pick a collision on connection ID (in fact, the odds are surprisingly good at scale), so the original is fine. Only the change in 4 is what was intended.

Personally, I am with Ian here. Delivery of a received packet should be based on the connection ID. Only if the connection ID is omitted, then the 4-tuple would be used.

For (1) & (2) above, I don't care too much if the packets are dropped or delivered to the existing connection. I think delivering to the connection is easier because it doesn't require any knowledge of the connection's state beyond it exists. Depending on the implementation, the UDP receive thread/processing might not have access to the connection's state.

For (3) above, I agree that ideally they would be separate connections, but how do you differentiate the two connection scenario from the retransmission w/ NAT rebinding scenario? Just delivering them to the same connection is simplest.

For (4) above, we should definitely allow a client to create two connections simultaneously with the same 4-tuple. #1041 is also necessary to support this scenario.

I don't think it is possible to entirely ignore at tuple even when given an ID. A host may assume an ID is approximately (modulo long time) unique when originating from one source. One peer does not control the ID. Internally it makes sense to hash the tuple with connection ID for lookup. However, it may be sufficient to require the ID be used, since an implementation may decide how tolerant it wants to be agasint collisions.

ianswett · 2018-01-09T02:36:22Z

draft-ietf-quic-transport.md

+the connection ID in the header; this might include connection IDs that were
+advertised using NEW_CONNECTION_ID ({{frame-new-connection-id}}).
+
+Clients SHOULD discard any packet that cannot be associated with an existing


This paragraph is very odd right here. This seems to imply connections are never created. Possibly remove this paragraph?

The key word here is "Clients". This finishes up instructions for clients, and the rest of the section is about servers. That said, a week after I wrote this it took me a minute to notice that. Any suggestions on how to make the word "clients" and its precise meaning more salient?

ianswett · 2018-01-09T02:38:26Z

draft-ietf-quic-transport.md

+server MUST either buffer the packet (see {{handshake-buffer}}), send a
+stateless reset ({{stateless-reset}}), or silently drop it.
+
+2. Otherwise, the server checks the version field in the long header. If the


I'd put sending VN as 1, because I think it's easier to get that out of the way early in the logical flow and it's what I'd expect implementations to do.

I actually implemented this in the order listed. For writing this, I found other arrangements to have more cumbersome conditions to express in English:

if (long header) && (big enough) && (version not OK)
send VN

if (long header) && (big enough) && (version OK) - i.e. I have to write out the first two conditions twice consecutively, or nest the paragraphs somehow.
deliver payload to TLS, etc.

Drop/Buffer

but obviously this is a purely wordsmithing concern.

martinthomson · 2018-01-16T03:26:49Z

I'm happy with this now, but it looks like @ianswett doesn't like some of the changes I specifically requested.

martinduke · 2018-01-16T07:08:42Z

Alright, I thought through the corner cases. I believe there are three ambiguities where we may have to adjust the wording:

In an established connection, are long headers with the same conn id legal? I don't think they are, because there's no way to mark the key phase. In the exceedingly unlikely event a different host guessed the same conn id, they'll just have to try again.
When a connection is doing a handshake, what do we do with a long header with the same ID but a different tuple? The keys will be the same assuming the same version. So do we accept the packet as a rebinding, reject the second one, or abandon the connection (probably a DoS)?
During the handshake, if it comes with the same tuple but a different conn id, what do we do? Are there special rules when it's the first packet from the server?

martinduke · 2018-01-16T17:26:16Z

Writing some pseudocode clarified it, at least for me. I think the answer, in most of the cases above, is just to drop it. If we can agree that the below is correct, I'll find the language to put in the spec:

Client Processing:

if (short_header)
    if (has_conn_id)
        if (conn_id_matches)
            if (1rtt_keys_exist)
                decrypt();
            else
                buffer_or_drop();
        else
            drop();
    else
        if (tuple_matches)
            if (allow_omit_conn_id)
                if (1rtt_keys_exist)
                    decrypt();
                else
                    buffer_or_drop();             
            else
                drop();
        else
            drop();
else /* long header */
    if (conn_id_matches)
        if (handshake_recent)
            if (version == 0)
                if (is_first_pkt)
                    process_VN();
                else
                    drop();
            else
                decrypt();
        else
            drop();
    else if (tuple_matches && is_first_pkt && version_matches)
        use_new_conn_id()
        decrypt()
    else
        drop()

Server processing:

if (short_header)
    if (has_conn_id)
        if (conn_id_matches)
            if (1rtt_keys_exist)
                decrypt();
            else
                buffer_or_drop();
        else
             reset_or_drop(); /* This is the only difference from client short header processing */
    else
        if (tuple_matches)
            if (allow_omit_conn_id)
                if (1rtt_keys_exist)
                    decrypt();
                else
                    buffer_or_drop();             
            else
                drop();
        else
           drop();
else /* long header */
    if (conn_id_matches)
        if (recent_handshake && version_matches)
            decrypt();
        else
            drop();
    else if (version_matches)
        decrypt()
    else
        send_vn();

mikkelfj · 2018-01-16T19:51:03Z

Nice work, C in pseudo code :)

It mostly looks good to me, except for where the client upgrades to the new version id:

else if (tuple_matches && is_first_pkt && version_matches)
        use_new_conn_id()
        decrypt()

The client needs to use the new connection id only after it has got a connection id match. So it would be something along:
client:

In Client:
...
else /* long header */
    if (conn_id_matches)
        if (handshake_recent)
            if (version == 0)
                if (is_first_pkt)
                    process_VN();
                else
                    drop();
            else
                decrypt();
                if (got_new_conn_id)
                    use_new_conn_id()
        else
            drop();
    else
        drop()

Otherwise we are back to matching in tuples at one stage.

mikkelfj · 2018-01-16T20:01:24Z

Also, the client may have to buffer some long headers it does not understand when the server changes CID, but I don't recall the details. Later it may be fine to drop.

And the client may have to handle a reset matching on ID as well, but that might be covered in the decrypt part.

martinthomson · 2018-01-17T04:37:34Z

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:

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:

# 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:

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.

@martinthomson

@martinthomson's suggestions make this much cleaner. Please read it over for mistakes, but I think we're close.

marten-seemann · 2018-02-13T02:42:52Z

draft-ietf-quic-transport.md

-response from the server. Servers that do not support a version of QUIC that
-allows early short headers MUST either respond with a stateless reset or drop
-the packet silently.
+First, hosts try to associate the packet with an existing connection. If the


Wouldn’t you first look at the packet type, in order to handle Initial packets statelessly?

That's a good point. It certainly makes sense for a server to push any stateless processing to the front.

I don't know if it makes sense to consider that as an optional amendment, or whether it isn't just easier to reorder the processing. It depends on whether it is easier to say "a server might move any stateless processing of potential Initial packets to the start of this process", or whether it is easier to rearrange the order from the outset.

"Initial" is a version-dependent concept, so you still check the version first. If you understand the version, then you can check whether it's an Initial packet. However, since that's for long headers, the version will always be explicitly present, so you don't have to look up the connection ID first.

MikeBishop

Looks good. Technically, I have no objections. Minor editorial stuff.

MikeBishop · 2018-02-13T04:42:29Z

draft-ietf-quic-transport.md

+packet, which is handled in accordance with {{handle-vn}}. 
+
+Due to packet reordering or loss, clients might receive packets associated
+with a connection for which it does not yet have the keys to decrypt it.


This sentence is awkward. Maybe "packets encrypted with a key which the client has not yet computed."?

Computation isn't the problem, it's receipt of the server handshake. Maybe "packets that are protected with 1-RTT keys prior to receiving handshake messages. Packet protection cannot be removed from these packets until the cryptographic handshake produces the necessary keys."

MikeBishop · 2018-02-13T04:44:46Z

draft-ietf-quic-transport.md

+### Server-Specific Behaviors {#server-specific-behaviors}
+
+If a server receives a packet with an unknown connection ID, an unsupported
+version, and is long enough to be an Initial packet for some version


"If a server receives a packet with... is long enough...." doesn't parse.

"...a packet which has an unknown connection ID, an unsupported version, and a sufficient length....", maybe?

"and length sufficient for the packet to be an Initial packet in any version supported by the server" maybe

MikeBishop · 2018-02-13T04:45:52Z

draft-ietf-quic-transport.md

+
+Servers MUST drop other packets that contain unsupported versions.
+
+If the packet is a supported version, and an Initial Packet fully


Lower-case "packet" for consistency, I think.

Yes, Initial packet.

MikeBishop · 2018-02-13T04:47:36Z

draft-ietf-quic-transport.md

+Servers MUST drop other packets that contain unsupported versions.
+
+If the packet is a supported version, and an Initial Packet fully
+conforming with the specification, the server MUST proceed with the


I think you want a different phrasing here. The server is never obligated to proceed with the connection or the handshake; it can abort at any time. Perhaps we should just avoid the normative language and say that the server proceeds?

MikeBishop · 2018-02-13T04:48:49Z

draft-ietf-quic-transport.md

+handshake ({{handshake}}).  This commits the server to the version that the
+client selected.
+
+If the packet is a supported version, and a Handshake or 0RTT packet, the


"0-RTT" is used elsewhere, I think.

MikeBishop · 2018-02-13T04:52:03Z

draft-ietf-quic-transport.md

+If the packet is a supported version, and a Handshake or 0RTT packet, the
+server MAY buffer a limited number of these packets in anticipation of
+a late-arriving Initial Packet. In the event the server later generates
+a RETRY packet, this buffer should be purged. Servers MUST NOT send packets


We haven't all-capsed packet types elsewhere. Also, is this really a "should", or is it a "MUST"? Finally, the "MUST NOT send packets" seems superfluous, since the server doesn't have the context to respond without the contents of the Initial. "cannot" might be sufficient.

On second thought, the buffer purge might be irrelevant. If the server processes these packets according to the normal rules once it receives the Initial and replies with a Retry, it would drop them, wouldn't it? So it should just process these packets after it's gotten the delayed Initial, and do what it otherwise would.

"the server doesn't have the context to respond without the contents of the Initial. "

I was trying to make sure no acks were sent, but I see now that you're right. We can't decrypt (and therefore can't ack) 0-RTT packets, and we couldn't possibly receive other Handshake packets if we havent' seen the Initial. Good catch.

The buffer purge is probably a bad idea. If you consider how you might implement this, the 0-RTT buffer is likely to be global across the entire server. You can't know when you receive them which connection to associate them with, so it has to be a little bit global. Choosing to send Retry on one connection shouldn't cause other 0-RTT packets to be purged. Also, choosing not to accept 0-RTT is another reason you might purge them.

It might be better not to get into details here. I would probably instead maintain a deque of 0-RTT packets and let new 0-RTT packets push older ones out. No point ever purging other than when you are scrubbing through looking for unclaimed packets. Initiating a purge is CPU cycles you don't need to spend.

MikeBishop · 2018-02-13T04:56:46Z

draft-ietf-quic-transport.md

-in response could be lost, the client will send new packets until it
-successfully receives a response or it abandons the connection attempt.
+This system allows a server to process packets with unsupported versions without
+retaining state.  Though either the Initial packet or the version negotiation


Capitalize "Version Negotiation", since that's a packet type. Also, since we're talking about a hypothetical unknown version of QUIC here, we don't actually know that the packet which provokes version negotiation is of type "Initial" -- it might be worth lower-casing that (and maybe just saying "the client's packet" so someone else doesn't try to get you to capitalize it another time!).

MikeBishop · 2018-02-13T04:58:08Z

draft-ietf-quic-transport.md

+
+Version negotiation ensures that client and server agree to a QUIC version
+that is mutually supported. A server sends a Version Negotiation packet in
+response to a packet that might initiate a new connection, see


The point this text was making in the old location might be clearer if you say "in response to each packet", i.e. if the client sends six packets in its first flight, it will get 1 < N <= 6 VN packets back.

MikeBishop · 2018-02-13T05:04:19Z

draft-ietf-quic-transport.md

+do not meet these criteria.
+
+Note that a successfully associated packet may be a Version Negotiation
+packet, which is handled in accordance with {{handle-vn}}. 


Your build break is trailing whitespace here, looks like.

kazuho · 2018-02-13T05:36:31Z

draft-ietf-quic-transport.md

+
+First, hosts try to associate the packet with an existing connection. If the
+packet has a connection ID corresponding to an existing connection, QUIC
+processes that packet accordingly. Note that a NEW_CONNECTION_ID frame


If the packet has a connection ID corresponding to an existing connection, QUIC processes that packet accordingly.

May I ask if this is true?

In current form of QUIC, a server is allowed to specify any Connection ID that it likes, which may collide with a Connection ID that a client chooses for a new connection.

I believe that a client needs to identify 1 RTT packets by their 5-tuple to avoid the issue.

I think that this fits nicely with @marten-seemann's comment about handling Initial packets specially. My pseudocode didn't do this, but this motivates moving the version and Initial packet checks right up front.

My instinct is that this is one of a few cases where extremely rare Connection ID collisions cause a decryption failure and a discarded packet. If a host has two connections with the same ID there's going to be trouble, so we might as well kill one of them off right away.

I would agree, if it weren't for the point about stateless handling. I think that's together it's worth addressing. I think that all you need to do is put in a "Before attempting to identify an existing connection a server might process Initial packets and generate Version Negotiation or Retry packets. Efficient and stateless generation of these packets can be important in reducing server load." Or some such blather.

As @MikeBishop pointed out, Initial packets are a version-dependent concept. So although I think we have to be careful to check the version first, I can put something together along these lines.

On the other hand, an Initial packet might be a retransmission due to loss of the first server flight. Wouldn't we want this to be associated with the existing connection context, especially since there might be 0-RTT packets?

I think that it would be OK for a retransmission of an Initial to be handled statelessly. If that results in progressing to establishing a connection, then I think that it might pay to check if you don't already have one for the same client-chosen connection ID. That is special logic, but probably necessary (unless the server copies the client connection ID).

I am fine with moving version checking to the front, and will shortly publish a revision to that effect.

But I do think processing the initial right off opens up several cans of worms.

For instance:

What if the content of Initial Transport draft is missing #2 differs from Initial Mapping draft #1 (in the same Stream 0 sequence numbers)? What if I process, send a RETRY, and then it turns out I'd already responded to the Initial with a Server Hello?

If I process the second Initial, and then find I already have this connection with 0-RTT, I've passed this stuff up to TLS for no reason. If we check conn ID first, we've already thrown out the Stream 0 contents as duplicative.

Perhaps this requires more discussion, but I'd really like to commit this very large change and then haggle over the order of checking conn ID/processing Initials for supported versions in a separate issue.

martinthomson

This is much better. I think that we can address the stateless VN/Retry thing pretty easily by prefacing the server-specific packet handling section appropriately (you already have the text in the right order).

martinthomson · 2018-02-13T06:12:44Z

draft-ietf-quic-transport.md

+
+Servers MUST drop other packets that contain unsupported versions.
+
+If the packet is a supported version, and an Initial packet fully


I think that you can remove the if (supported(version)) check from these paragraphs. You already did the early return on !supported

My thinking was that this section is (no longer) pseudocode in prose, so we might as well be completely explicit. If you think this is too cumbersome, I can delete it.

I think that your previous paragraphs effectively close of all options for not-supported versions, so this is probably a little redundant.

OK, removed the reference.

martinthomson · 2018-02-13T06:13:14Z

draft-ietf-quic-transport.md

+
+
+### Server-Specific Behaviors {#server-specific-behaviors}
+


This is probably where you need to finesse the stateless generation of VN and Retry packets.

VN handled, Retry not so much.

martinthomson · 2018-02-13T06:15:23Z

draft-ietf-quic-transport.md

+
+First, hosts try to associate the packet with an existing connection. If the
+packet has a connection ID corresponding to an existing connection, QUIC
+processes that packet accordingly. Note that a NEW_CONNECTION_ID frame


I would agree, if it weren't for the point about stateless handling. I think that's together it's worth addressing. I think that all you need to do is put in a "Before attempting to identify an existing connection a server might process Initial packets and generate Version Negotiation or Retry packets. Efficient and stateless generation of these packets can be important in reducing server load." Or some such blather.

martinthomson · 2018-02-13T06:17:16Z

draft-ietf-quic-transport.md

+Initial Packet. Clients are forbidden from sending Handshake packets prior
+to receiving a server response, so servers SHOULD ignore any such packets.
+
+Servers MUST drop incoming packets under all other circumstances.


What about regular packets for a connection? I know that this section only lists the exceptional cases, but because it doesn't mention packets that are matched to a connection, this could be read as requiring servers to discard all packets that belong to connections.

I agree; this is the same point as the "supported version" above; if reading in sequence, it reads fine, but there's no reason not be explicit. I'll revise once we reach consensus on the other items.

I think I'm fixing this is in the latest revision.

martinthomson · 2018-02-13T06:19:15Z

draft-ietf-quic-transport.md

+with an existing connection, or - for servers - potentially create a new
+connection.
+
+First, hosts try to associate the packet with an existing connection. If the


Remove the first here and forward-reference the server-specific section.

martinthomson · 2018-02-13T06:19:45Z

draft-ietf-quic-transport.md

+allow it to compute the key.
+
+
+### Server-Specific Behaviors {#server-specific-behaviors}


s/Behaviors/Packet Handling/

A shorter anchor might be nice too.

I don't think we've converged on whether it is desirable to process an Initial Packet before checking if the connection ID matches on open one; I would like to submit these extensive changes (assuming no other issues) and continue that specific debate in a new issue/PR.

martinthomson · 2018-02-16T00:39:48Z

@martinduke, I agree, we should try to land this and then iterate.

What if the content of Initial #2 differs from Initial #1 (in the same Stream 0 sequence numbers)? What if I process, send a RETRY, and then it turns out I'd already responded to the Initial with a Server Hello?
If I process the second Initial, and then find I already have this connection with 0-RTT, I've passed this stuff up to TLS for no reason. If we check conn ID first, we've already thrown out the Stream 0 contents as duplicative.

I think that I mentioned that problem before. The client should probably ignore Retry if it has moved on. More generally, we should probably encourage (or insist on) consistent routing of Initial. #1115

martinthomson requested changes Jan 8, 2018

View reviewed changes

martinthomson reviewed Jan 8, 2018

View reviewed changes

Update to PR #1039

bae2777

Addressed @martinthomson's issues. Made further edits to be concise and well-organized.

mikkelfj reviewed Jan 8, 2018

View reviewed changes

Update draft-ietf-quic-transport.md

61c1003

Addressed @mikkelfj's issue.

martinthomson requested changes Jan 8, 2018

View reviewed changes

martinduke added 2 commits January 8, 2018 14:53

Update draft-ietf-quic-transport.md

a847126

Addressed Martin's issues and fixed one error (first packets might not be "handshake" packets -- they might be 0RTT)

Update draft-ietf-quic-transport.md

f1fe446

Whoops, forgot the last of Martin's comments.

ianswett reviewed Jan 9, 2018

View reviewed changes

martinthomson added 2 commits January 16, 2018 10:41

Merge branch 'master' into patch-8

a15f67a

Merge branch 'master' into patch-8

256ad6b

martinthomson approved these changes Jan 16, 2018

View reviewed changes

Rewrote completely based on Martin's feedback

b33d04e

@martinthomson's suggestions make this much cleaner. Please read it over for mistakes, but I think we're close.

marten-seemann reviewed Feb 13, 2018

View reviewed changes

MikeBishop approved these changes Feb 13, 2018

View reviewed changes

MikeBishop reviewed Feb 13, 2018

View reviewed changes

kazuho reviewed Feb 13, 2018

View reviewed changes

Made Mike Bishop's changes

5271b59

martinthomson reviewed Feb 13, 2018

View reviewed changes

Couple of typos

80a47db

martinthomson merged commit 80a47db into quicwg:master Feb 16, 2018

		packets that are the minimum size of all QUIC versions they support.


		## Version Negotiation {#version-negotiation}

		@@ -876,56 +876,62 @@ Packets without connection IDs and long-form packets for connections that have
		incomplete cryptographic handshakes are associated with an existing connection
		using the tuple of source and destination IP addresses and ports.


		{{packet-handling}} describes the conditions under which endpoints negotiate
		versions.


		Servers MUST drop other packets that contain unsupported versions.

		If the packet is a supported version, and an Initial Packet fully

		allow it to compute the key.


		### Server-Specific Behaviors {#server-specific-behaviors}

Rewrite text about Version Negotiation #1039

Rewrite text about Version Negotiation #1039

Conversation

martinduke commented Jan 8, 2018

martinthomson 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

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

martinthomson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett 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

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

martinthomson commented Jan 16, 2018

martinduke commented Jan 16, 2018

martinduke commented Jan 16, 2018

mikkelfj commented Jan 16, 2018 • edited Loading

mikkelfj commented Jan 16, 2018

martinthomson commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeBishop 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

MikeBishop Feb 13, 2018 • 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

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

Choose a reason for hiding this comment

mikkelfj commented Jan 16, 2018 •

edited

Loading

MikeBishop Feb 13, 2018 •

edited

Loading

martinduke Feb 15, 2018 •

edited

Loading