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

From: martinduke <[email-masked]>
Date: Sun, 07 Jan 2018 21:05:52 -0800

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.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/1039

-- Commit Summary --

  * Rewrite text about Version Negotiation

-- File Changes --

    M draft-ietf-quic-transport.md (72)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1039.patch
https://github.com/quicwg/base-drafts/pull/1039.diff

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

---

From: Martin Thomson <[email-masked]>
Date: Mon, 08 Jan 2018 06:13:58 +0000 (UTC)

martinthomson requested changes on this pull request.

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:

```c
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.

> -
-If the packet contains a version that is acceptable to the server, the server
+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
+in {{packet-size}}), it cannot create a new connection. In this case, the server
+MUST either buffer the packet in case more packets arrive, send a stateless
+reset ({{stateless-reset}}), or silently drop it.
+
+If the packet could create a new connection, the server checks the version
+field in the packet header. If a supported version, but not a valid Initial
+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,"

>  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".

> -A QUIC connection begins with a client sending an Initial packet
-({{packet-initial}}). The details of the handshake mechanisms are described in
-{{handshake}}, but any Initial packet sent from the client to the server MUST
-use the long header format - which includes the version of the protocol being
-used - and they MUST be padded to at least 1200 octets.
-
-The server receives this packet and determines whether it potentially creates a
-new connection (see {{packet-handling}}).  If the packet might generate a new
-connection, the server then checks whether it understands the version that the
-client has selected.
-
-If the packet contains a version that is acceptable to the server, the server
+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)"

 ### Sending Version Negotiation Packets {#send-vn}
 
 If the version selected by the client is not acceptable to the server, the
 server responds with a Version Negotiation packet ({{packet-version}}).  This
 includes a list of versions that the server will accept.
 
-A server sends a Version Negotiation packet for any packet with an unacceptable
-version if that packet could create a new connection.  This allows a server to
-process packets with unsupported versions without retaining state.  Though
-either the Client Initial packet or the version negotiation packet that is sent
-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 VN

-- 
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#pullrequestreview-87129801

---

From: Martin Thomson <[email-masked]>
Date: Mon, 08 Jan 2018 06:26:46 +0000 (UTC)

martinthomson commented on this pull request.

> -initial packets.  A server MAY discard packets with a short header or packets
-that are smaller than the smallest minimum size for any version that the server
-supports.  A server that discards a packet that cannot be associated with a
-connection MAY also generate a stateless reset ({{stateless-reset}}).
-
-This version of QUIC defines a minimum size for initial packets of 1200 octets
-(see {{packetization}}).  Versions of QUIC that define smaller minimum initial
-packet sizes need to be aware that initial packets will be discarded without
-action by servers that only support versions with larger minimums.  Clients that
-support multiple QUIC versions can avoid this problem by ensuring that they
-increase the size of their initial packets to the largest minimum size across
-all of the QUIC versions they support.  Servers need to recognize initial
-packets that are the minimum size of all QUIC versions they support.
-
-
-## Version Negotiation {#version-negotiation}

Losing this header is a problem.

-- 
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#pullrequestreview-87132485

---

From: martinduke <[email-masked]>
Date: Mon, 08 Jan 2018 18:33:36 +0000 (UTC)

@martinduke pushed 1 commit.

bae2777  Update to PR #1039

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1039/files/61977b518599e3441fa473651588921c8dd2e783..bae27776631f1254750fdcef16cf10acc935f024

---

From: MikkelFJ <[email-masked]>
Date: Mon, 08 Jan 2018 11:20:08 -0800

mikkelfj commented on this pull request.

> @@ -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.

-- 
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#pullrequestreview-87311246

---

From: martinduke <[email-masked]>
Date: Mon, 08 Jan 2018 11:25:48 -0800

61c1003  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1039/files/bae27776631f1254750fdcef16cf10acc935f024..61c10035ada05379a6d1b7efe54238f383882b2b

---

From: Martin Thomson <[email-masked]>
Date: Mon, 08 Jan 2018 22:28:00 +0000 (UTC)

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.

> -A packet that uses the short header could be associated with an existing
-connection with an incomplete cryptographic handshake.  Such a packet could be a
-valid packet that has been reordered with respect to the long-form packets that
-will complete the cryptographic handshake.  This might happen after the final
-set of cryptographic handshake messages from either peer.  These packets are
-expected to be correlated with a connection using the tuple of IP addresses and
-ports.  Packets that might be reordered in this fashion SHOULD be buffered in
-anticipation of the handshake completing.
-
-0-RTT packets might be received prior to a Client Initial packet at a server.
-If the version of these packets is acceptable to the server, it MAY buffer these
-packets in anticipation of receiving a reordered Client Initial packet.
+with an existing connection, or - for servers - potentially create a new
+connection.
+
+Host handle packets that can be associated with an existing connection

Host**s**

> -will complete the cryptographic handshake.  This might happen after the final
-set of cryptographic handshake messages from either peer.  These packets are
-expected to be correlated with a connection using the tuple of IP addresses and
-ports.  Packets that might be reordered in this fashion SHOULD be buffered in
-anticipation of the handshake completing.
-
-0-RTT packets might be received prior to a Client Initial packet at a server.
-If the version of these packets is acceptable to the server, it MAY buffer these
-packets in anticipation of receiving a reordered Client Initial packet.
+with an existing connection, or - for servers - potentially create a new
+connection.
+
+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.

> +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
+tuple of source and destination IP addresses and ports. Other packets are
+associated with existing connections using connection ID 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
+connection.  Discarded packets MAY be logged for diagnostic or security
+purposes.
+
+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.

-use the long header format - which includes the version of the protocol being
-used - and they MUST be padded to at least 1200 octets.
-
-The server receives this packet and determines whether it potentially creates a
-new connection (see {{packet-handling}}).  If the packet might generate a new
-connection, the server then checks whether it understands the version that the
-client has selected.
-
-If the packet contains a version that is acceptable to the server, the server
-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
+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

respond**s**

+{{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."

-({{packet-initial}}). The details of the handshake mechanisms are described in
-{{handshake}}, but any Initial packet sent from the client to the server MUST
-use the long header format - which includes the version of the protocol being
-used - and they MUST be padded to at least 1200 octets.
-
-The server receives this packet and determines whether it potentially creates a
-new connection (see {{packet-handling}}).  If the packet might generate a new
-connection, the server then checks whether it understands the version that the
-client has selected.
-
-If the packet contains a version that is acceptable to the server, the server
-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."

> +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. Other packets are
+associated with existing connections using connection ID 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
+connection.  Discarded packets MAY be logged for diagnostic or security
+purposes.
+
+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
+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.

> +tuple of source and destination IP addresses and ports. Other packets are
+associated with existing connections using connection ID 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
+connection.  Discarded packets MAY be logged for diagnostic or security
+purposes.
+
+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
+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"

+Otherwise, the server checks the version field in the long header. If the
+server does not support the chosen version, it MUST send a Version
+Negotiation packet as described in {{send-vn}}.
+
+If the server supports the version, the server operates in accordance with
+the specification of that version. For the version described in this
+specification, it checks if the packet is a correctly formatted Initial packet.
+If so, the server MUST proceed with the handshake ({{handshake}}).  This
+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}}
+
+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.

> +the packet cannot create a new connection. In this case, the server MUST
+either buffer the packet (see {{handshake-buffer}}), send a stateless reset
+({{stateless-reset}}), or silently drop it.
+
+Otherwise, the server checks the version field in the long header. If the
+server does not support the chosen version, it MUST send a Version
+Negotiation packet as described in {{send-vn}}.
+
+If the server supports the version, the server operates in accordance with
+the specification of that version. For the version described in this
+specification, it checks if the packet is a correctly formatted Initial packet.
+If so, the server MUST proceed with the handshake ({{handshake}}).  This
+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}`.

-- 
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#pullrequestreview-87357105

---

From: martinduke <[email-masked]>
Date: Mon, 08 Jan 2018 22:36:07 +0000 (UTC)

martinduke commented on this pull request.

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.

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

---

From: martinduke <[email-masked]>
Date: Mon, 08 Jan 2018 14:53:09 -0800

a847126  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1039/files/61c10035ada05379a6d1b7efe54238f383882b2b..a847126f9351898ff5cfa2e4f92762d03169575b

---

From: martinduke <[email-masked]>
Date: Mon, 08 Jan 2018 14:55:15 -0800

f1fe446  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1039/files/a847126f9351898ff5cfa2e4f92762d03169575b..f1fe44676ef81ee931ebeea871a849b05932bd16

---

From: Martin Thomson <[email-masked]>
Date: Mon, 08 Jan 2018 23:55:29 +0000 (UTC)

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.

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

---

From: MikkelFJ <[email-masked]>
Date: Mon, 08 Jan 2018 16:22:04 -0800

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

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

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jan 2018 01:17:56 +0000 (UTC)

ianswett commented on this pull request.

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

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

---

From: Martin Thomson <[email-masked]>
Date: Mon, 08 Jan 2018 17:21:24 -0800

I see the point, but I'm sure that Martin would appreciate concrete text.  It's not easy to get right.

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

---

From: martinduke <[email-masked]>
Date: Mon, 08 Jan 2018 18:05:57 -0800

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.

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

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jan 2018 02:40:42 +0000 (UTC)

Some more concrete suggestions.

 

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."

+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.

+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?

-If the version of these packets is acceptable to the server, it MAY buffer these
-packets in anticipation of receiving a reordered Client Initial packet.
+with an existing connection, or - for servers - potentially create a new
+connection.
+
+Hosts handle packets that can be associated with an existing connection
+according to the current state of that connection. Both 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 tuple of source and destination IP addresses and ports.
+Other packets are associated with existing connections using connection ID
+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?

+connection.  Discarded packets MAY be logged for diagnostic or security
+purposes.
+
+If a server receives a packet not associated with an existing connection, it
+executes the following steps, in order:
+
+1. The server MUST check if the packet uses a short form header, or is not
+long enough for the size required for the first packet of any QUIC version
+that the server supports. See {{packet-size}} for the definition of packet size
+and the minimum size of the first packet in this version of QUIC. If either
+condition is true, the packet cannot create a new connection. In this case, the
+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.

-- 
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#pullrequestreview-87397385

---

From: Martin Thomson <[email-masked]>
Date: Mon, 08 Jan 2018 18:58:16 -0800

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

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

---

From: MikkelFJ <[email-masked]>
Date: Mon, 08 Jan 2018 19:27:49 -0800

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.

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

---

From: Mike Bishop <[email-masked]>
Date: Tue, 09 Jan 2018 17:52:33 +0000 (UTC)

MikeBishop commented on this pull request.

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.

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

---

From: MikkelFJ <[email-masked]>
Date: Tue, 09 Jan 2018 10:30:49 -0800

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.

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

---

From: Martin Thomson <[email-masked]>
Date: Wed, 10 Jan 2018 02:50:28 +0000 (UTC)

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.

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

---

From: Martin Thomson <[email-masked]>
Date: Mon, 15 Jan 2018 23:41:25 +0000 (UTC)

@martinthomson pushed 1 commit.

a15f67a  Merge branch 'master' into patch-8

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1039/files/f1fe44676ef81ee931ebeea871a849b05932bd16..a15f67a0e8fff62a3f575e75b48bcd0905db2a2f

---

From: Martin Thomson <[email-masked]>
Date: Tue, 16 Jan 2018 03:26:52 +0000 (UTC)

martinthomson approved this pull request.

-- 
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#pullrequestreview-88977830

---

From: Martin Thomson <[email-masked]>
Date: Mon, 15 Jan 2018 19:26:52 -0800

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

-- 
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-357844049

---

From: martinduke <[email-masked]>
Date: Mon, 15 Jan 2018 22:14:42 -0800

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:
1) 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)
2) same as above, but the same 4-tuple. (original: to established connection; mine: to established connection; ian: discard it)
3) two clients attempt a handshake with the same conn id (original and mine: 2 separate connections; Ian: these are the same connection)
4) 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.

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

---

From: martinduke <[email-masked]>
Date: Mon, 15 Jan 2018 22:18:36 -0800

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?

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

---

From: martinduke <[email-masked]>
Date: Tue, 16 Jan 2018 06:28:39 +0000 (UTC)

I actually implemented this in the order listed. For writing this, I found other arrangements to have more cumbersome conditions to express in English:
1) if (long header) && (big enough) && (version not OK)
send VN
2) 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.
3) Drop/Buffer

but obviously this is a purely wordsmithing concern.

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

---

From: martinduke <[email-masked]>
Date: Tue, 16 Jan 2018 06:38:56 +0000 (UTC)

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

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

---

From: martinduke <[email-masked]>
Date: Tue, 16 Jan 2018 06:39:24 +0000 (UTC)

OK

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

---

From: martinduke <[email-masked]>
Date: Tue, 16 Jan 2018 07:08:44 +0000 (UTC)

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

1) 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.
2) 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)?
3) 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?

-- 
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-357871734

---

From: Martin Thomson <[email-masked]>
Date: Mon, 15 Jan 2018 23:52:26 -0800

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.

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

---

From: Nick Banks <[email-masked]>
Date: Tue, 16 Jan 2018 07:16:05 -0800

nibanks commented on this pull request.

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.

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

---

From: MikkelFJ <[email-masked]>
Date: Tue, 16 Jan 2018 15:47:46 +0000 (UTC)

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.

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

---

From: martinduke <[email-masked]>
Date: Tue, 16 Jan 2018 09:26:19 -0800

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();
```

-- 
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-358038506

---

From: MikkelFJ <[email-masked]>
Date: Tue, 16 Jan 2018 19:51:06 +0000 (UTC)

Nice work, C in pseudo code :)

        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.

-- 
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-358083675

---

From: MikkelFJ <[email-masked]>
Date: Tue, 16 Jan 2018 20:01:26 +0000 (UTC)

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.

-- 
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-358086713

---

From: Martin Thomson <[email-masked]>
Date: Wed, 17 Jan 2018 04:37:40 +0000 (UTC)

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