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

From: Martin Thomson <[email-masked]>
Date: Sun, 08 Oct 2017 22:56:52 -0700

martinthomson commented on this pull request.

> +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
+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 an existing connection
+MUST be discarded.  Discarded packets MAY be logged for diagnostic or security

I have revised this so that it allows for the case of buffering.

 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

I like the section breaks.  In fact, so much that I would like to add another.  The text is cleanly split into "what the server does" and "what the client does", so I would like to take advantage of that.

 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

The text (that this should have referred to) boils down to almost exactly what you describe, but with a little more nuance.  "Might have created a connection" is defined further up (in this self-same PR), but I neglected to cross-reference it properly.

In short, the condition is expanded to include one more check: that the packet is big enough to generate a new connection on any version that the server supports.  This means that if you are doing 0-RTT, then you might get a Version Negotiation packet only in response to the Client Initial and any 0-RTT packet that is large enough.  This avoids the server sending Version Negotiation in response to smaller packets (and thereby acting as an amplifier).

> +
+## 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 existing 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

Actually, it is correct, but it wasn't clear what it was referring to.  It refers to the long-form packets and short-form headers sent by the server as it completes the handshake.  It sends a long header for the handshake, then short headers for packets that follow that.  The packets with a short header can arrive before the packet containing the handshake.

I will expand on this a little.

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

---

From: Mike Bishop <[email-masked]>
Date: Mon, 09 Oct 2017 17:13:39 +0000 (UTC)

MikeBishop commented on this pull request.

I see a few corner cases on another read-through, but it's looking good.  Let's land this soon.

+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, any packet that cannot be associated with an existing connection
+SHOULD be discarded if it not buffered.  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
+that are at least the minimum size defined for the protocol version can be
+initial packets.  Unless the server is buffering 0-RTT packets, a server MUST
+discard packets that cannot be associated with a connection if they use the
+short header form, or they are smaller than the smallest minimum size for any

Doesn't this rule out the very case Stateless Reset is intended for?  Can't be associated with an active connection, short header, but includes the Connection ID.

> +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
+that are at least the minimum size defined for the protocol version can be
+initial packets.  Unless the server is buffering 0-RTT packets, a server MUST
+discard packets that cannot be associated with a connection if they use the
+short header form, or they 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 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

Side-note which may not require a change:  If the server and client have any overlap between versions, this will work.  If they don't have any overlap between versions, however, this potentially leads to a connection time-out rather than a VN exchange and immediate conclusion that there's no overlap.

Consider the case where the minima of the versions supported by the client are (1040,680,256), and the minima of the version supported by the server are (1280,1280,1600).  Even following this advice, the server will always ignore the client's Client Initial packets, and the client will ultimately time out.

I'm not sure we need to fix this, given that otherwise we risk spewing VN packets in response to random noise.  However, we might want to be clear that this isn't guaranteed to avoid the problem.

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

Another potential timing issue:  Client sends Client Initial + 0-RTT data.  Client Initial is lost in transit.  Version Negotiation comes back responding to 0-RTT packets because the server only received those.  Per the current text, it MUST ignore the server's VN packets until it has retransmitted the CI and gotten *that* VN packet.  Hopefully rare, since if you're doing 0-RTT you're unlikely to use an unsupported version -- but not impossible.

Maybe we should simply say "match the values it sent in a packet on this connection"?

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 10 Oct 2017 09:14:13 +0000 (UTC)

Thanks Mike, tweaked accordingly.

Oops.  I'll fix that.  I see the stateless reset text overriding this, but no point having a direct contradiction.

+

Yeah, if the client sends a 256 octet packet, that's never going to trigger a VN packet from the server.  That's OK I think.  We have a certain incentive to keep the minimum at 1200, and the case you describe is one example of that.

Yes, I think that is a good tweak.

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 10 Oct 2017 02:15:00 -0700

@martinthomson pushed 2 commits.

39ea0bb  Allow a stateless reset in addition to discard
cec3573  Allow consumption of VN in response to 0-RTT

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

---

From: ianswett <[email-masked]>
Date: Wed, 11 Oct 2017 00:54:00 +0000 (UTC)

ianswett commented on this pull request.

A few small comments.

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

It seems worth mentioning packet type as well here, since there are only two packet types that carry client hellos.

+
+For clients, any packet that cannot be associated with an existing connection
+SHOULD be discarded if it not buffered.  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
+that are at least the minimum size defined for the protocol version can be
+initial packets.  Unless the server is buffering 0-RTT packets, a server MUST
+either discard a packet or generate a stateless reset ({{stateless-reset}}) if
+the packet cannot be associated with a connection if they use the short header
+form, or they 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.

What has to be 1200 bytes?  Is this including or excluding IP + UDP overhead?

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 10 Oct 2017 19:55:10 -0700

+

That is defined elsewhere.  I'm starting to think that we need to consolidate that text, it's all over the place.  (Another patch though.)

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

---

From: Martin Thomson <[email-masked]>
Date: Wed, 11 Oct 2017 02:56:52 +0000 (UTC)

There is only one type of packet that could be an initial packet.  However... this allows for the possibility of defining a new version of the protocol that has more than one type.  We decided not to make the initial packet type something that is invariant across versions though.

If you want to make it an invariant (and potentially prevent new versions from defining multiple opening packet types), then it's probably best to open an issue for that.

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

---

From: ianswett <[email-masked]>
Date: Wed, 11 Oct 2017 13:13:38 +0000 (UTC)

Fair point, I'm happy with this as is.

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

---

From: ianswett <[email-masked]>
Date: Wed, 11 Oct 2017 06:13:49 -0700

+

SG

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

---

From: ianswett <[email-masked]>
Date: Wed, 11 Oct 2017 06:14:12 -0700

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

---

From: janaiyengar <[email-masked]>
Date: Thu, 12 Oct 2017 15:37:33 -0700

janaiyengar commented on this pull request.

a few comments

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

This is redundant and a bit confusing. The earlier para says "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", which applies to 0-RTT packets as well. I think the para above is adequate, and this one about 0-RTT is redundant.

The para above says reordered packets SHOULD be buffered, this one says MAY.  Either is fine with me, I'd lean towards a SHOULD.

> +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.
+
+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, any packet that cannot be associated with an existing connection
+SHOULD be discarded if it not buffered.  Discarded packets MAY be logged for

nit: missing "is" in "if it not buffered"

The server could be buffering 1-RTT packets as well (consider out-of-band key negotiation).

This sentence needs work. It's a run-on and confusing.

-To avoid packet amplification attacks a server MUST NOT send a Version
-Negotiation packet that is larger than the packet it responds to.  It is
-anticipated that this is ample space for all QUIC versions that a single server
-might need to advertise.
+A server sends a Version Negotiation packet for any packet with an unaccepable

typo: "unacceptable"

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 12 Oct 2017 15:51:23 -0700

9d7cdb1  Add forward ref to packetization section
e5fb23b  Jana's 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/cec3573cec22311684545514ab7768c0ea6f6cc4..e5fb23b9c15c45a6447fec8d3e4945045337fcb8

---

From: Martin Thomson <[email-masked]>
Date: Thu, 12 Oct 2017 22:52:12 +0000 (UTC)

I plan to merge this - we can continue to tweak the details more over time.

0-RTT uses the long header.  The last paragraph talks about short headers.  Though they are related, these are completely different conditions.

The requirements have to be different.  For packets that are reordered over the last flight of the handshake, you already have an assumption that the connection is going to be made and the corresponding state commitment.  So buffering isn't a huge burden.

For 0-RTT, you receive random 0-RTT packets from some unspecified endpoints prior to any handshake packets.  Buffering of those packets is risky - they might never be followed by a handshake packet.

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

---

From: janaiyengar <[email-masked]>
Date: Thu, 12 Oct 2017 23:11:03 +0000 (UTC)

Please remove the restriction that a server MUST discard packets unless they're 0-RTT packets before merging.

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 12 Oct 2017 16:12:44 -0700

I'll downgrade to MAY

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 12 Oct 2017 23:14:55 +0000 (UTC)

@martinthomson pushed 1 commit.

aa50d14  Downgrade discard requirement to MAY prior to handshake

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

---

From: janaiyengar <[email-masked]>
Date: Thu, 12 Oct 2017 23:22:23 +0000 (UTC)

> @@ -910,7 +910,7 @@ 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
 that are at least the minimum size defined for the protocol version can be
-initial packets.  Unless the server is buffering 0-RTT packets, a server MUST
+initial packets.  Unless the server is buffering 0-RTT packets, a server MAY

This is an odd sentence now. Remove the phrase "Unless the server is buffering 0-RTT packets" 

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 12 Oct 2017 23:36:26 +0000 (UTC)

d816e03  Remove text

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

---

From: janaiyengar <[email-masked]>
Date: Fri, 13 Oct 2017 00:50:55 +0000 (UTC)

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

---

From: janaiyengar <[email-masked]>
Date: Fri, 13 Oct 2017 00:51:09 +0000 (UTC)

Merged #724.

-- 
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#event-1291415483