Subject: [quicwg/base-drafts] Define requirements for handling packets (#724)

From: Martin Thomson <[email-masked]>
Date: Fri, 11 Aug 2017 06:53:22 +0000 (UTC)

We never really said how to handle incoming packets.  This adds a section on
that.  This was originally motivated by #570, where the rules for what to
generate a Version Negotiation packet for were a little unclear.

Now though, this is a far more comprehensive description of what to do with
a packet that is received.

For #570, I've chosen to limit packet size.  If the packet is too small, then
this requires the server to drop the packet.  In response to the potential for
a future version of QUIC to define a smaller initial packet (@mikkelfj raised
this point), I've included advice on what might be done to avoid having packets
discarded.

I realize that we don't have a patch that sets the packet size to 1200.  I'm
going to write that one now.  This depends on that.

Closes #570.  

I would also like to do the same to #568, but that will be down to @mcmanus.  To the extent that we treat VN as special (see discussion on #693 regarding this), then that will be addressed by the change proposed there.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Define requirements for handling packets

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/724.patch
https://github.com/quicwg/base-drafts/pull/724.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/724

---

From: Patrick McManus <[email-masked]>
Date: Fri, 11 Aug 2017 14:35:42 +0000 (UTC)

I suppose fixing the long-header-bit and the min-size is enough for closing #568 in practice.

I really appreciate this PR btw - good stuff.

-- 
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/724#issuecomment-321830582

---

From: ianswett <[email-masked]>
Date: Wed, 16 Aug 2017 14:55:18 +0000 (UTC)

ianswett commented on this pull request.

Nice clarifications, a few editorial nits.

> @@ -860,39 +860,93 @@ different IP or port at either endpoint, due to NAT rebinding or mobility, as
 described in {{migration}}.  Finally a connection may be terminated by either
 endpoint, as described in {{termination}}.
 
+
+## Matching Packets to Connections
+
+Incoming packets are classified on receipt.  Packets can either be associated
+with an existing connection, be discarded, or - for servers - potentially create
+a new connection.
+
+A packet that can be identified as belonging to an existing connection is

nit: "Packets identified as belonging to an existing connection are handled according to the current state of that connection."

> +valid packet that has been reordered with respect to the long-form packets that
+will complete the cryptographic handshake.  These packets SHOULD be buffered in
+anticipation of the handshake completing.
+
+Buffering ensures that data is not lost, which improves performance; conversely,
+discarding these packets could create false loss signals for the congestion
+controllers.  However, limiting the number and size of buffered packets might be
+needed to prevent exposure to denial of service.
+
+For clients, all packets that cannot be associated with a connection MUST be
+discarded.  Discarded packets MAY be logged for diagnostic or security purposes.
+
+For servers, packets that aren't associated with a connection potentially create
+a new connection.  However, only packets that use the long packet header and
+packets that meet the minimum size defined for the protocol version can be
+initial packets.  A server MUST discard packets that use the short header form,

nit: "A server MUST discard un-associable packets ..." to clarify what's being discarded.

> +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.  These packets SHOULD be buffered in
+anticipation of the handshake completing.
+
+Buffering ensures that data is not lost, which improves performance; conversely,
+discarding these packets could create false loss signals for the congestion
+controllers.  However, limiting the number and size of buffered packets might be
+needed to prevent exposure to denial of service.
+
+For clients, all packets that cannot be associated with a connection MUST be
+discarded.  Discarded packets MAY be logged for diagnostic or security purposes.
+
+For servers, packets that aren't associated with a connection potentially create
+a new connection.  However, only packets that use the long packet header and
+packets that meet the minimum size defined for the protocol version can be

nit: "and packets that meet" -> "and meet" for brevity

> +
+A packet that uses the short header form 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.  These packets SHOULD be buffered in
+anticipation of the handshake completing.
+
+Buffering ensures that data is not lost, which improves performance; conversely,
+discarding these packets could create false loss signals for the congestion
+controllers.  However, limiting the number and size of buffered packets might be
+needed to prevent exposure to denial of service.
+
+For clients, all packets that cannot be associated with a connection MUST be
+discarded.  Discarded packets MAY be logged for diagnostic or security purposes.
+
+For servers, packets that aren't associated with a connection potentially create

nit: What about "aren't associated with an existing connection"

 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.
 
-When the client receives a Version Negotiation packet from the server, it should
-select an acceptable protocol version.  If the server lists an acceptable
-version, the client selects that version and reattempts to create a connection
-using that version.  Though the contents of a packet might not change in
+
+### Sending and Handling 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 unaccepable
+version if that packet could create a new connection.  This allows a server to

Does it make sense to specify only sending VN in response to client initial or are there other types you have in mind for "that packet could create a new connection."?

> -select an acceptable protocol version.  If the server lists an acceptable
-version, the client selects that version and reattempts to create a connection
-using that version.  Though the contents of a packet might not change in
+
+### Sending and Handling 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 unaccepable
+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.

At some point, the handshake might timeout.

> +
+A server sends a Version Negotiation packet for any packet with an unaccepable
+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.
+
+When the client receives a Version Negotiation packet, it first checks that the
+packet number and connection ID match the values it sent in a client initial
+packet.  If this check fails, the packet MUST be discarded.
+
+Once the Version Negotiation packet is determined to be valid, the client then
+selects an acceptable protocol version from the list provided by the server.
+The client then attempts to create a connection using that version.  Though the
+contents of the client initial packet that the client sends might not change in

nit: that the -> the

> @@ -860,39 +860,93 @@ different IP or port at either endpoint, due to NAT rebinding or mobility, as
 described in {{migration}}.  Finally a connection may be terminated by either
 endpoint, as described in {{termination}}.
 
+
+## Matching Packets to Connections
+
+Incoming packets are classified on receipt.  Packets can either be associated
+with an existing connection, be discarded, or - for servers - potentially create
+a new connection.
+
+A packet that can be identified as belonging to an existing connection is
+handled according to the current state of that connection.  Packets are
+associated with established connections using connection ID if it is present;

nit: A switch from existing to established here.  Is that intentional?

> +
+## Matching Packets to Connections
+
+Incoming packets are classified on receipt.  Packets can either be associated
+with an existing connection, be discarded, or - for servers - potentially create
+a new connection.
+
+A packet that can be identified as belonging to an existing connection is
+handled according to the current state of that connection.  Packets are
+associated with established connections using connection ID if it is present;
+this might include connection IDs that were advertised using NEW_CONNECTION_ID
+({{frame-new-connection-id}}).  Packets without connection IDs and long-form
+packets for connections that have incomplete cryptographic handshakes are
+associated using the tuple of source and destination IP addresses and ports.
+
+A packet that uses the short header form could be associated with an existing

It'd be nice to just say "Short header packets" here and elsewhere, since the extra passive voice makes these passages longer and a harder to read.  That's probably a larger change than just this new section, so I can do a followup editorial change for that if you think it'll work?

-- 
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/724#pullrequestreview-56492263

---

From: ekr <[email-masked]>
Date: Wed, 16 Aug 2017 18:14:03 +0000 (UTC)

ekr commented on this pull request.

This seems generally OK, but see comment.s

+

You say "discarded", but many discarded packets trigger behavior (VN, stateless reset)

+needed to prevent exposure to denial of service.
+
+For clients, all packets that cannot be associated with a connection MUST be
+discarded.  Discarded packets MAY be logged for diagnostic or security purposes.
+
+For servers, packets that aren't associated with a connection potentially create
+a new connection.  However, only packets that use the long packet header and
+packets that meet the minimum size defined for the protocol version can be
+initial packets.  A server MUST discard packets that use the short header form,
+or packets that are smaller than the smallest minimum size for any version that
+the server supports.
+
+This version of QUIC defines a minimum size for initial packets of 1200 octets.
+Versions of QUIC that define smaller minimum initial packet sizes need to be
+aware that initial packets will be discarded by servers that only support

It's not that they will be discarded but rather that they will be *silently* discarded rather than eliciting VN

+
+For servers, packets that aren't associated with a connection potentially create
+a new connection.  However, only packets that use the long packet header and
+packets that meet the minimum size defined for the protocol version can be
+initial packets.  A server MUST discard packets that use the short header form,
+or packets that are smaller than the smallest minimum size for any version that
+the server supports.
+
+This version of QUIC defines a minimum size for initial packets of 1200 octets.
+Versions of QUIC that define smaller minimum initial packet sizes need to be
+aware that initial packets will be discarded 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.

You need to provide the appropriate guidance for servers of accepting the minimum size, regardless of version.

Suppose we have versions 1, 2, 3 with minimum sizes 1200, 1201 and 1202 respectively. A client which supports (1, 2) sends a packet of size 1201. A server which supports (2, 3) and doesn't recognize 1 needs to accept [1, 1201] and send VN even though [3, 1201] would not be acceptable

>  ## Version Negotiation {#version-negotiation}
 
 QUIC's connection establishment begins with version negotiation, since all
 communication between the endpoints, including packet and frame formats, relies
 on the two endpoints agreeing on a version.
 
-A QUIC connection begins with a client sending a handshake packet. The details
-of the handshake mechanisms are described in {{handshake}}, but all of the
-initial packets sent from the client to the server MUST use the long header
-format and MUST specify the version of the protocol being used.
-
-When the server receives a packet from a client with the long header format, it
-compares the client's version to the versions it supports.
-
-If the version selected by the client is not acceptable to the server, the
-server discards the incoming packet and responds with a Version Negotiation
-packet ({{packet-version}}).  This includes a list of versions that the server
-will accept.
+A QUIC connection begins with a client sending a client initial packet
+({{packet-client-initial}}). The details of the handshake mechanisms are
+described in {{handshake}}, but all of the initial packets sent from the client

s/initial/first flight/

-- 
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/724#pullrequestreview-56716181

---

From: Mike Bishop <[email-masked]>
Date: Wed, 16 Aug 2017 20:18:31 +0000 (UTC)

MikeBishop approved this pull request.

In general, looks good -- and a good platform for future building.

+

I think those fall under existing (stateless reset is for connections that have reached a terminal state) or new (VN).

Packet types are version-specific.  If the server doesn't speak the version, it doesn't know it's a Client Initial.

-- 
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/724#pullrequestreview-56749571

---

From: ianswett <[email-masked]>
Date: Wed, 16 Aug 2017 13:24:04 -0700

Ok, that wasn't obvious to me.  Can we clarify when to send VN then?

-- 
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/724#discussion_r133557002

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Aug 2017 07:14:44 +0000 (UTC)

martinthomson commented on this pull request.

+

they mean the same thing, is that 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/724#discussion_r134158811

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Aug 2017 00:15:18 -0700

I'd be ok with a PR that fixed that.  It's just that we don't have clearly established definitions that would allow for the shorter usage.

-- 
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/724#discussion_r134158890

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Aug 2017 07:16:55 +0000 (UTC)

I was trying to save words here.  I figured that this was used enough to be implicit by now.

-- 
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/724#discussion_r134159107

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Aug 2017 07:20:30 +0000 (UTC)

In what way is this text not providing that clarification?  `if (might_create_a_connection && !acceptable(version)) { 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/724#discussion_r134159670

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Aug 2017 07:22:33 +0000 (UTC)

+

Pro tip: when reviewing something and you have a new comment, don't reply to another comment, it makes it hard to respond.

-- 
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/724#discussion_r134159977

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Aug 2017 07:25:01 +0000 (UTC)

Initial is correct

-- 
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/724#discussion_r134160322

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Aug 2017 00:25:43 -0700

@martinthomson pushed 1 commit.

f46a09d  Review comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/724/files/594ab51b68a4b451a613ab89d1cca55cb29d63e7..f46a09d148edf17d9f975f0a545fe3793cc1ec2e