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

From: janaiyengar <[email-masked]>
Date: Fri, 08 Sep 2017 21:40:18 +0000 (UTC)

janaiyengar commented on this pull request.

Comments inline

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

MUST -> SHOULD, since there's no reason to require this.
Separately, if the handshake is not complete, there's no existing connection, and this statement contradicts the advice on buffering short header packets for such connections. I would suggest starting this para with "In all other cases, packets SHOULD be discarded."

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

This isn't accurate. A server could use the long header for 1-RTT encypted packets, and 0-RTT encrypted packets are long header.

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

capitalize Client Initial

 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

With the section break now, it's not obvious that this flows from the previous, especially if you start reading at this section. I would  remove the section break.

 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

I think Ian's point is that it's not clear what "might_create_a_connection" means. I am not clear about this either, TBH. How does a server know that a received packet might create a connection if the version is unspoken? I think all you need is if (!acceptable(version)) { send_vn() }

> +
+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 or it abandons the connection attempt.
+
+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 the client sends might not change in

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

---

From: Mike Bishop <[email-masked]>
Date: Fri, 08 Sep 2017 22:26:28 +0000 (UTC)

MikeBishop commented on this pull request.

No short packet can start a connection.  Long packets with connection IDs that are currently active inherently can't start a different connection.  So, sticking with the pseudo-code, `might_create_a_connection := !ActiveConnectionIDs.Contains(packet.connection_id) && (packet.header.IsLongFormat)`

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

---

From: janaiyengar <[email-masked]>
Date: Fri, 08 Sep 2017 15:54:32 -0700

If there's an active connection with this ID, you will match the connection earlier, attempt to decrypt the packet with the stored key and fail, and drop the packet on the floor. Even if you check the version before you try to decrypt you'd send a VN packet in response.

So this boils down to `if ((flags & 0x80) && !acceptable(version)) { send_vn() }`, since whether there's an active connection or not, we still send a VN packet. In which case, I think the long header check is unnecessary.

Simply checking the version seems adequate, no?

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