Subject: [quicwg/base-drafts] Alternative header proposal (#361)

From: janaiyengar <[email-masked]>
Date: Mon, 06 Mar 2017 16:52:28 -0800

This PR proposes an alternative header format for QUIC.  It is based on a previous proposal and incorporates feedback received on the list. This PR is unfortunately quite invasive.

Closes:
#35: Starting packet number
#56: Extending flags
#119: Server-proposed connection ID
#133: Connection ID in version negotiation
#135: DoS using Version Negotiation Packets
#147: Reflection Attack Resistance
#185: Reliable identification of the initial packet for a connection
#193: Flags section is kind of confusing
#244: Need a NONCE in version negotiation packets
#293: Does the connection ID need to be in a consistent location
#295: Connection ID on a version negotiation packet

(also addresses some of:
#148: QUIC packet header complexity
#170: Connection ID collisions)
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Partial commit of new header format
  * Everything except connection ID.
  * Connection ID added.
  * Two key phases for long headers
  * Nits.
  * Clarifications.
  * Public reset proof not optional.

-- File Changes --

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

-- Patch Links --

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

---

From: janaiyengar <[email-masked]>
Date: Mon, 06 Mar 2017 17:50:23 -0800

@janaiyengar pushed 3 commits.

c3dedf3  Fix formatting of long- and short-form headers
d37a977  Add version specific fields to short form
87037ab  Merge pull request #362 from quicwg/headerfix

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/361/files/644c2c7dfbfd398c6d9c41b90dc36981f1f1575d..87037ab8bcb7615b541f339caa64e23cbb314e8b

---

From: Martin Thomson <[email-masked]>
Date: Mon, 06 Mar 2017 17:52:46 -0800

@martinthomson pushed 2 commits.

da2da74  Fix long line
f4f84e4  Merge branch 'headerfix' into header

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/361/files/87037ab8bcb7615b541f339caa64e23cbb314e8b..f4f84e443d179ab6d2b4c71299659a337090376b

---

From: Alexis La Goutte <[email-masked]>
Date: Mon, 06 Mar 2017 23:50:12 -0800

LGTM

-- 
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/361#issuecomment-284647236

---

From: ekr <[email-masked]>
Date: Tue, 07 Mar 2017 11:07:45 -0800

ekr requested changes on this pull request.

> @@ -322,175 +322,356 @@ all field sizes are in bits.  When discussing individual bits of fields, the
 least significant bit is referred to as bit 0.  Hexadecimal notation is used for
 describing the value of fields.
 
+QUIC packet headers can be separated into long and short headers. Long headers

I don't think "separated" is the word you want here.  Rather
"Any QUIC packet has either a long or a short header"

> @@ -322,175 +322,356 @@ all field sizes are in bits.  When discussing individual bits of fields, the
 least significant bit is referred to as bit 0.  Hexadecimal notation is used for
 describing the value of fields.
 
+QUIC packet headers can be separated into long and short headers. Long headers
+are expected to be used for the initial phase of the connection and are used for
+version negotiation, connection establishment, 0-RTT, and public reset
+packets. Short headers are minimal version-specific headers, which can be used
+after version negotiation and 1-RTT keys are established, and MUST NOT be used
+earlier.

I would say here that they are distinguished by the leading bit (which it seems they are).

-   * 0x02 = PUBLIC_RESET.  Set to indicate that the packet is a Public Reset
-     packet.
+: The most significant bit (0x80) of the first octet (octet 0) is set to 1 for

"octet 0" is confusing. I would say "the first octet" everywhere.

-     + 11 indicates that 6 bytes of the packet number are present
-     + 10 indicates that 4 bytes of the packet number are present
-     + 01 indicates that 2 bytes of the packet number are present
-     + 00 indicates that 1 byte of the packet number is present
+: Octets 1 through 8 include a connection ID.  {{connection-id}} describes the
+  use of this field in more details.

"in more detail". Also "the connection ID", no?

-   * 0x80 is currently unused, and must be set to 0.
+: Octets 9 to 12 contain a packet number.  {{packet-numbers} describes the use
+  of packet numbers.

"the packet number"

+: Octets 13 to 16 contain the selected protocol version.  This field indicates
+  which version of QUIC is in use and determines how the some protocol fields
+  are interpreted.

"how the some" -> "how the rest of the"? 

Anyway, "how the some" is ungrammatical

-While all QUIC packets have the same common header, there are three types of
-packets: Regular packets, Version Negotiation packets, and Public Reset packets.
-The flowchart below shows how a packet is classified into one of these three
-packet types:
+: Octets from 17 onwards are the payload of the packet.

As I understand it, it's the rest of the UDP datagram, right? You should say that.

+| Type | Name                   | Section                        |
+|:-----|:-----------------------|:-------------------------------|
+| 01   | Version Negotiation    | {{version-negotiation-packet}} |
+| 02   | Public Reset           | {{public-reset-packet}}        |
+| 03   | Client Cleartext       | {{cleartext-packet}}           |
+| 04   | Final Server Cleartext | {{cleartext-packet}}           |
+| 05   | Other Server Cleartext | {{cleartext-packet}}           |

Not: I feel like "other" should have a lower number than "final". Maybe "non-final" would be a better name.

+| Type | Name                   | Section                        |
+|:-----|:-----------------------|:-------------------------------|
+| 01   | Version Negotiation    | {{version-negotiation-packet}} |
+| 02   | Public Reset           | {{public-reset-packet}}        |
+| 03   | Client Cleartext       | {{cleartext-packet}}           |
+| 04   | Final Server Cleartext | {{cleartext-packet}}           |
+| 05   | Other Server Cleartext | {{cleartext-packet}}           |
+| 06   | 0-RTT Encrypted        | {{encrypted-packet}}           |
+| 07   | 1-RTT Encrypted (key phase 0) | {{encrypted-packet}}    |
+| 08   | 1-RTT Encrypted (key phase 1) | {{encrypted-packet}}    |

It's not clear what mechanic you have in mind here. Were you thinking that when you change to key n+1 you would send a long-header on each packet until you got it acked? That seems nasty. 

-### Handling Packets from Different Versions
+The header form, long packet type, connection ID, packet number and version

s/long packet type/packet tpe/

+: The second bit (0x40) of octet 0 indicates whether the connection ID field is
+  present.  If set to 1, then the connection ID field is present; if set to 0,
+  the connection ID field is zero length.

As I think I mentioned earlier, I don't think this is needed. You should just negotiate this and the packet number and then have it be constant.

-Each Regular packet contains additional header fields followed by an encrypted
-payload, as shown below:
+: The third bit (0x20) of octet 0 indicates the key phase, which allows a
+  recipient of a packet to identify the packet protection keys that are used to
+  protect the packet.  See {{QUIC-TLS}} for details.
+
+Short Packet Type:
+
+: The remaining 5 bits of octet 0 include one of 32 packet types.
+  {{short-packet-types}} lists the types that are defined for short packets.

Can't we just use the same types?

> +  can be 1, 2 or 4 octets long depending on the short packet type.
+
+Encrypted Payload:
+
+: Packets with a short header always include a 1-RTT protected payload.
+
+The packet type in a short header currently determines only the size of the
+packet number field.  Additional types can be used to signal the presence of
+other fields.
+
+| Type | Packet Number Size     |
+|:-----|:-----------------------|
+| 01   | 1 octet                |
+| 02   | 2 octets               |
+| 03   | 4 octets               |
+{: #short-packet-types title="Short Header Packet Types"}

Not a huge fan of this. Why do you need to change this on a per-packet basis. Also, why are you burning both a bit on this *and* a type field instead of having one of the types be "no packet #"

-* Packet Number: The lower 8, 16, 32, or 48 bits of the packet number, based on
-  the PACKET_NUMBER_SIZE flag.  Each Regular packet is assigned a packet number
-  by the sender.  The first packet number is randomized (see
-  {{initial-packet-number}}).
+* Octet 0: 0x81 (Flags indicating long header and Version Negotiation packet
+  type)
+* Octets 1-8: Connection ID (echoed)
+* Octets 9-12: Packet Number (echoed)
+* Octets 13-16: Version (echoed)
+* Octets 17+: Payload (version list, containing 0 or more acceptable versions)

Why 0? Don't we just want some other error?

> +A server that generates a version negotiation packet SHOULD include one of the
+reserved version numbers (those following the pattern 0x?a?a?a?a).  This ensures
+that clients are able to correctly handle an unsupported version.
+
+The design of version negotiation permits a server to avoid maintaining state
+for packets that it rejects in this fashion.  However, when the server generates
+a version negotiation packet, it cannot randomly generate a reserved version
+number.  This is because the server is required to include the same value in its
+transport parameters (see {{version-validation}}).  To avoid the selected
+version number changing during connection establishment, the reserved version
+can be generated as a function of values that will be available to the server
+when later generating its handshake packets.
+
+A pseudorandom function that takes client address information (IP and port) and
+the client selected version as input would ensure that there is sufficient
+variability in the values that a server uses.

I would put this in a subsection, b/c it's (a) not required behavior and (b) not really core to the version negotiation mechanics.

> +
+The design of version negotiation permits a server to avoid maintaining state
+for packets that it rejects in this fashion.  However, when the server generates
+a version negotiation packet, it cannot randomly generate a reserved version
+number.  This is because the server is required to include the same value in its
+transport parameters (see {{version-validation}}).  To avoid the selected
+version number changing during connection establishment, the reserved version
+can be generated as a function of values that will be available to the server
+when later generating its handshake packets.
+
+A pseudorandom function that takes client address information (IP and port) and
+the client selected version as input would ensure that there is sufficient
+variability in the values that a server uses.
+
+A client MAY send a packet using a reserved version number.  This can be used to
+solicit a list of supported versions from a server.

This should go somewhere else.

> +* Octets 17+: Public Reset Proof
+
+A Public Reset packet sent by a server indicates that it does not have the
+state necessary to continue with a connection.  In this case, the server will
+include the fields that prove that it originally participated in the connection
+(see {{public-reset-proof}} for details).
+
+Upon receipt of a Public Reset packet that contains a valid proof, a client
+MUST tear down state associated with the connection.  The client MUST then
+cease sending packets on the connection and SHOULD discard any subsequent
+packets that arrive. A Public Reset that does not contain a valid proof MUST be
+ignored.
+
+### Public Reset Proof
+
+Details to be added.

Nit: can you mark this as a TODO so we can find it again.

> +
+### Public Reset Proof
+
+Details to be added.
+
+
+## Cleartext Packets {#cleartext-packet}
+
+Cleartext packets are sent during the handshake prior to key negotiation. A
+Client Cleartext packet contains:
+
+* Octet 0: 0x83 (Flags indicating long header and client Cleartext packet type)
+* Octets 1-8: Connection ID (randomly chosen)
+* Octets 9-12: Packet number (low 4 octets, starts at a random 31-bit value)
+* Octets 13-16: Version
+* Octets 17+: Payload

I feel like cleartext would be better to go first. Lets do the happy path.

> +
+Other Server Cleartext packets contain:
+
+* Octet 0: 0x85 (Flags indicating Long header and appropriate server Cleartext
+  packet type)
+* Octets 1-8: Connection ID (echoed)
+* Octets 9-12: Packet Number (low 4 octets, random 31-bit initial value)
+* Octets 13-16: Version (echoed)
+* Octets 17+: Payload
+
+The client MUST choose a random value and use it as the Connection ID until the
+server replies with a server-selected Connection ID. Server-selected Connection
+IDs are used after a successful handshake, cleanly distinguishing packets that
+use them from packets using client-selected Connection IDs. All packets
+including and following a Server Cleartext End-of-Handshake packet use a
+server-selected Connection ID, as described in {{connection-id}}.

I would consider not interleaving the description of the semantics and the packet format. The packet format tells you which bytes are bytes, so you just need the semantics here.

-## Packet Numbers
+## Connection ID {#connection-id}
+
+QUIC connections are identified by their 64-bit Connection ID. All long headers
+contain a Connection ID. Short headers indicate the presence of a Connection ID
+using the CONNECTION_ID flag. When present, the Connection ID is in the same
+location in all packet headers, making it straightforward for middleboxes, such
+as load balancers, to locate and use it.
+
+When a connection is initiated, the client MUST choose a random value and use it
+as the Connection ID in all Cleartext packets and all 0-RTT Encrypted
+packets. The client's Connection ID is a suggestion to the server. The server
+echoes this value in all packets until the handshake is successful (see
+{{QUIC-TLS}}. In its final successful handshake completion packet, the server

"handshake completion packet" does not seem to be a defined term.

-- 
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/361#pullrequestreview-25591223

---

From: ianswett <[email-masked]>
Date: Tue, 07 Mar 2017 12:09:34 -0800

ianswett commented on this pull request.

Looking great, thanks for writing this up.

nit: "and are used for" => "including"?

-The fields in the Common Header are the following:
+Long headers are used for packets that are sent prior to the completion of
+version negotiation or are not encrypted with 1-RTT keys. Once both conditions

How about: "Long headers MUST be used for packets that are sent prior..."

zero length or omitted?

+when later generating its handshake packets.
+
+A pseudorandom function that takes client address information (IP and port) and
+the client selected version as input would ensure that there is sufficient
+variability in the values that a server uses.
+
+A client MAY send a packet using a reserved version number.  This can be used to
+solicit a list of supported versions from a server.
+
+See {{version-negotiation}} for a description of the version negotiation
+process.
+
+## Public Reset Packet {#public-reset-packet}
+
+A Public Reset packet is sent by only a server and is used to abruptly terminate

nit: only sent by a server

+variability in the values that a server uses.
+
+A client MAY send a packet using a reserved version number.  This can be used to
+solicit a list of supported versions from a server.
+
+See {{version-negotiation}} for a description of the version negotiation
+process.
+
+## Public Reset Packet {#public-reset-packet}
+
+A Public Reset packet is sent by only a server and is used to abruptly terminate
+communications. Public Reset is provided as an option of last resort for a
+server that does not have access to the state of a connection.  This is intended
+for use by a server that has lost state (for example, through a crash or
+outage). A server that wishes to indicate fatal errors with a connection MUST

nit: "A server that wishes to communicate a fatal connection error MUST use a CONNECTION_CLOSE frame if it has sufficient state to do so."

-- 
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/361#pullrequestreview-25565963

---

From: Mike Bishop <[email-masked]>
Date: Tue, 07 Mar 2017 12:25:21 -0800

MikeBishop requested changes on this pull request.

Generally okay with the proposal on technical grounds, but lots of nitpicks on the actual text and diagrams.  Should be easy fixes.

Not necessarily, since we're also saying that the server can change it.  The client includes *a* connection ID before it learns *the* connection ID from the server.  I'm fine with either wording.

For the scenario where the sender is opting to use long-header packets in 1-RTT, they need to be able to communicate a key phase.  There's a corresponding bit in the short-form.

Whether that's true depends how you read it, so I think this needs to be expanded a little bit.  The *presence and interpretation* of the packet type field will not change between versions.  The set of packet types very well might.

-### Handling Packets from Different Versions
+The header form, long packet type, connection ID, packet number and version
+fields of a long header packet are version independent. The rest of the packet
+is specific to a version. See {{version-specific}} for details on how packets

Since you've already called it the "payload" earlier, pull that in here.  The payload is specific to a version *and* packet type, I think.  Anyone who speaks QUIC can find the version and type fields.  You have to know the version to interpret the type; you have to know the version and type to interpret the payload.

I partially disagree.  There's value in a self-descriptive packet, so that a middlebox (e.g. load-balancer) can know whether to expect the Connection ID field to be present.  However, if you're expecting and accommodating such a middlebox, you'd just always negotiate the field to be on, so there are ways around this.

Agreed, Non-Final.

 ~~~
  0                   1                   2                   3
  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                          Frame 1 (*)                        ...
+|                    Supported Version 1 (32)                 ...

Per the notation conventions in the document, you've said this is a required, variable-length field.  But it's optional (0 or more) and fixed-length.

We could unify the spaces, but there will be types that are only valid for long (public reset, version negotiation), and possibly types that are only valid for short.  If we did unify them, though, we could reclaim the key phase bit and just use the same 1-RTT (KP0) and 1-RTT (KP1) types the long header defines.

-* the location and size of the Flags field,
+```
+ 0                   1                   2                   3
+ 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
++-+-+-+-+-+-+-+-+
+|0|C|K|  Type   |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                                                               |
++                   Connection ID (optional)                    +

We have a notation convention for optional fields:  This would be [Connection ID (64)].

+
+Encrypted Payload:
+

What bit are they burning?  This appears to only be indicated by the type byte.

+: Packets with a short header always include a 1-RTT protected payload.
+
+The packet type in a short header currently determines only the size of the
+packet number field.  Additional types can be used to signal the presence of
+other fields.
+
+| Type | Packet Number Size     |
+|:-----|:-----------------------|
+| 01   | 1 octet                |
+| 02   | 2 octets               |
+| 03   | 4 octets               |
+{: #short-packet-types title="Short Header Packet Types"}
+
+The header form, connection ID flag and connection ID of a short header packet
+are version independent.  The remaining fields are specific to the selected QUIC

Hyphenate:  version-independent.

Yeah, it seems a little odd to allow the server to respond "I speak just enough QUIC to say 'I don't speak QUIC.'"

>  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 ~~~
-{: #regular-packet-frames title="Contents of Encrypted Payload"}
+{: #version-negotiation-format title="Version Negotiation Packet"}
+
+A server that generates a version negotiation packet SHOULD include one of the
+reserved version numbers (those following the pattern 0x?a?a?a?a).  This ensures

"helps ensure." Nothing can ensure well-written clients.

>  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 ~~~
-{: #regular-packet-frames title="Contents of Encrypted Payload"}
+{: #version-negotiation-format title="Version Negotiation Packet"}
+

SHOULD?

"only sent by servers"

+Details to be added.
+
+
+A Server Cleartext End-of-Handshake packet contains:

You called this Final above, not End-of-Handshake.

+1.  Define the fields, then describe how they're used by reference to those fields, not the underlying bytes.

> +(TODO: Add hash before frames.)
+
+
+## Encrypted Packets {#encrypted-packet}
+
+Packets encrypted with either 0-RTT or 1-RTT keys may be sent with long headers.
+Different packet types explicitly indicate the encryption level for ease of
+decryption. These packets contain:
+* Octet 0: 0x86, 0x87 or 0x88 (Flags indicating long header and an encrypted
+  packet type)
+* Octets 1-8: Connection ID (client or server-selected, see {{connection-id}})
+* Octets 9-12: Packet Number (low 4 octets)
+* Octets 13-16: Version
+* Octets 17+: Encrypted Payload
+
+A flags octet of 0x86 indicates a 0-RTT packet. Key phases are used by the QUIC

You're pivoting away from 0-RTT without lead-in.  I'd suggest "...indicates a 0-RTT packet.  After the 1-RTT keys are established, key phases...."

-## Packet Numbers

And are there multiple successful handshake completion packets?  Let's reference the specific packet type.

-## Packet Numbers
+MUST use a server-selected Connection ID. All subsequent packets from the server

Wait, if the Connection ID is a suggestion to the server, but the server MUST use a server-selected ID, what's the point of the suggestion?

Perhaps that packet type MUST contain the final Connection ID, which MAY be the one proposed by the client, or MAY be a new server-selected value?

> @@ -541,12 +721,54 @@ packet number.  Once any packet has been acknowledged, subsequent packets can
 use a shorter packet number encoding.

-## Frames and Frame Types {#frames}
+## Handling Packets from Different Versions {#version-specific}
+
+Between different versions the following things are guaranteed to remain
+constant:
+
+* the location and size of the Flags field in both header forms,

You didn't originally call that field Flags in this version, though I see it has cropped up other places through the document.

> @@ -541,12 +721,54 @@ packet number.  Once any packet has been acknowledged, subsequent packets can
 use a shorter packet number encoding.

-## Frames and Frame Types {#frames}
+## Handling Packets from Different Versions {#version-specific}
+
+Between different versions the following things are guaranteed to remain
+constant:
+
+* the location and size of the Flags field in both header forms,
+
+* the location of the HEADER_FORM bit in the Flags field in both header forms,
+
+* the location of the CONNECTION_ID bit in the Flags field in short headers,
+
+* the location and size of the Connection ID field in both header forms,
+
+* the location and value of the Version field in long headers, and

The value of the Version field can't change between versions?  That's really going to screw with version negotiation logic....  :-)  I suspect this should be location and meaning.

> +## Handling Packets from Different Versions {#version-specific}
+
+Between different versions the following things are guaranteed to remain
+constant:
+
+* the location and size of the Flags field in both header forms,
+
+* the location of the HEADER_FORM bit in the Flags field in both header forms,
+
+* the location of the CONNECTION_ID bit in the Flags field in short headers,
+
+* the location and size of the Connection ID field in both header forms,
+
+* the location and value of the Version field in long headers, and
+
+* the location and value of the Packet Number field in long headers.

Likewise.

> +constant:
+
+* the location and size of the Flags field in both header forms,
+
+* the location of the HEADER_FORM bit in the Flags field in both header forms,
+
+* the location of the CONNECTION_ID bit in the Flags field in short headers,
+
+* the location and size of the Connection ID field in both header forms,
+
+* the location and value of the Version field in long headers, and
+
+* the location and value of the Packet Number field in long headers.
+
+Implementations MUST assume that an unsupported version uses an unknown packet
+format. All other values MUST be ignored when processing a packet that contains

Other fields, not just other values.

> +* the location of the CONNECTION_ID bit in the Flags field in short headers,
+
+* the location and size of the Connection ID field in both header forms,
+
+* the location and value of the Version field in long headers, and
+
+* the location and value of the Packet Number field in long headers.
+
+Implementations MUST assume that an unsupported version uses an unknown packet
+format. All other values MUST be ignored when processing a packet that contains
+an unsupported version.
+
+
+# Frames and Frame Types {#frames}
+
+The payload of cleartext packets and the plaintext after decryption of encrypted

After decryption of the payload -- you're not decrypting the entire 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/361#pullrequestreview-25607265

---

From: ekr <[email-masked]>
Date: Tue, 07 Mar 2017 13:09:37 -0800

ekr commented on this pull request.

Oh, I see that I missed this in the short header. nm.

-- 
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/361#discussion_r104779377

---

From: janaiyengar <[email-masked]>
Date: Tue, 07 Mar 2017 18:54:11 -0800

janaiyengar commented on this pull request.

Done

-- 
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/361#discussion_r104834134

---

From: Martin Thomson <[email-masked]>
Date: Tue, 07 Mar 2017 22:43:11 -0800

martinthomson approved this pull request.

This is a pretty big and important change.  Overall, I think that the direction is good.  Like others, I think that there are plenty of opportunities for this to be improved, but that should not stop us from making forward progress.

s/expected to be used/used/

"prior to the completion of both version negotiation and the cryptographic handshake "

same end result, no?

I thinking that this is just @janaiyengar trolling us all.

 ~~~

Agreed, in spades, though this isn't new text for this PR.

 ~~~

Not really.  The requirement here is that the bogus version is constant.  This is only suggesting one way that a server might be implemented to comply with that requirement.

Hmm, I don't remember this being a restriction on its use.  Why only servers?

This is an artifact of me partially fixing the formatting.

-## Packet Numbers

I had the same question as @MikeBishop here.  I believe that the answer is that the server sends the "Final" long packet type on all of the packets after ServerHello.  It would only echo the client's connection ID if it needed to send a Version Negotiation packet or a HelloRetryReqest.

-## Packet Numbers

Yeah, I would just say any value.  The fact that the server might pick the value that the client did isn't particularly interesting.  If anything, we might point out that the ID doesn't have to be *new*, just unique.

I would simply refer to this as the first octet, which sorts the both the "location" and "size" aspects.  Maybe:

> New versions of QUIC can be specified as long as the invariants in this section are maintained.
>
> The first octet always indicates the type of packet:
>  * The header form bit (0x80) always indicates a long or short header.
>  * In the short header, the next bit (0x40) indicates whether the Connection ID field is present.
>  * The remaining bits of this octet are interpreted as determined by the protocol version.
>
> Between all protocol versions, the following is guaranteed to be constant:
> * The location, size, and semantics of the Connection ID in both header forms (conditional on the field being present in the short form),
> * The location, size, and semantics of the Version field in the long form header, and
> * The location and size (but not semantics) of the Packet Number field in the long form header.

+

And let us talk about "(successful) removal of packet protection"

-- 
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/361#pullrequestreview-25705349

---

From: Mike Bishop <[email-masked]>
Date: Wed, 08 Mar 2017 08:25:12 -0800

MikeBishop commented on this pull request.

That was my initial reaction as well.  A client implementation on top of a UDP stack will never receive a packet on a closed connection, because there's no UDP socket open there.  The UDP stack will drop packets before they reach QUIC.  However, a server will have a listening socket open and receive all traffic that's sent to that port -- new connections, existing connections, and stale connections.

-- 
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/361#discussion_r104957139

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:10:10 -0800

Done

-- 
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/361#discussion_r105010906

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:11:44 -0800

Done

-- 
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/361#discussion_r105011235

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:12:47 -0800

changed to "contain the connection ID"

-- 
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/361#discussion_r105011479

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:13:01 -0800

Done

-- 
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/361#discussion_r105011538

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:13:59 -0800

done.

-- 
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/361#discussion_r105011764

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:15:27 -0800

I've added "(the rest of the QUIC packet)" for consistency since we don't really need to talk about UDP datagram anywhere else after QUIC packet is defined.

-- 
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/361#discussion_r105012056

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:21:43 -0800

Changed consistently to Final and Non-Final

-- 
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/361#discussion_r105013250

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:25:54 -0800

I've added a sentence saying the packet types defined here are version-specific. I'm not sure about this though, so I've added a TODO as well.

-- 
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/361#discussion_r105014128

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:27:34 -0800

As mentioned on the list, and in agreement with Mike, there's value in having the packet be self-descriptive for connection ID.

-- 
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/361#discussion_r105014492

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:30:11 -0800

Yeah, there will be holes or overlap between the header types. I don't see the value in unifying since there's got to be a branch before reading the types about which bits to read from the packet anyways, and two small maps aren't really that expensive to maintain.

-- 
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/361#discussion_r105014973

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:30:39 -0800

Good point. Changed to 1 or more.

-- 
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/361#discussion_r105015058

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:52:24 -0800

moved this down to version negotiation section in a subsection called "Using Reserved Versions"

-- 
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/361#discussion_r105019462

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:52:55 -0800

+

done

-- 
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/361#discussion_r105019582

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:52:33 -0800

moved

-- 
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/361#discussion_r105019484

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 12:55:19 -0800

+### Public Reset Proof
+
+Details to be added.
+
+

I'm doing it in the order that the packet types are defined. I've moved the types and the sections to have happy path first.

-- 
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/361#discussion_r105020020

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 16:18:35 -0800

Simplified all this significantly, and moved descriptions down to prose below the layouts.

-- 
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/361#discussion_r105059300

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 16:21:32 -0800

-## Packet Numbers

clarified and changed text. Yes there can be several Final packets, and the server-selected value is used for all of them. 

-- 
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/361#discussion_r105059665

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 16:31:48 -0800

There's no way to send those packets otherwise, so this is definitional. I don't think we need to mandate behavior when no alternative is present.

-- 
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/361#discussion_r105061074

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 16:32:17 -0800

changed to "omitted"

-- 
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/361#discussion_r105061134

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 16:33:43 -0800

changing to "sent only by a 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/361#discussion_r105061386

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 16:35:00 -0800

done

-- 
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/361#discussion_r105061549

---

From: Martin Thomson <[email-masked]>
Date: Wed, 08 Mar 2017 16:38:10 -0800

martinthomson commented on this pull request.

It's not definitional if you accept your example of out-of-band keying.

-- 
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/361#discussion_r105061911

---

From: Martin Thomson <[email-masked]>
Date: Wed, 08 Mar 2017 17:14:56 -0800

I think that we should just land this now:

![land it](https://images.duckduckgo.com/iu/?u=http%3A%2F%2Fwww.jokeoftheday.com%2Fpictures%2Fpymtrsi1ndxq7zj0ve3kfh9a86ugl4wob52c_Badlanding.jpg&f=1)

-- 
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/361#issuecomment-285223431

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 23:01:58 -0800

+
+Encrypted Payload:
+

Not sure what this comment means

-- 
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/361#discussion_r105097788

---

From: Martin Thomson <[email-masked]>
Date: Wed, 08 Mar 2017 23:15:59 -0800

+
+Encrypted Payload:
+

Can't help you there.

-- 
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/361#discussion_r105099241

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 23:22:40 -0800

@janaiyengar pushed 1 commit.

e105bbe  addresses mikebishop's and mt's comments

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

---

From: janaiyengar <[email-masked]>
Date: Wed, 08 Mar 2017 23:23:44 -0800

I've addressed all comments, PTAL. I hope to land this Thu/Fri, and I hope it's state is better than mt's plane :-)

-- 
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/361#issuecomment-285275106

---

From: ekr <[email-masked]>
Date: Thu, 09 Mar 2017 06:09:40 -0800

ekr approved this pull request.

I'm happy to land this as-is as a step in the right direction

I actually would prefer to not have the middlebox parse this (see previous comments about encrypting everything) so I'm not sure I'm excited about features that just are designed to accomodate that.

+
+Encrypted Payload:
+

Nm. I misread this.

What is the purpose of this being random?

-- 
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/361#pullrequestreview-25631273

---

From: Mike Bishop <[email-masked]>
Date: Thu, 09 Mar 2017 07:48:54 -0800

+
+Encrypted Payload:
+

Ekr had a comment (deleted?) that talked about both burning a bit _and_ using a frame type to communicate... something.  Packet number length, maybe?  My comment was that there no longer appears to be a dedicated bit, so I think his comment was invalid.  If he deleted it subsequently, apparently he thought so too.

-- 
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/361#discussion_r105195771

---

From: janaiyengar <[email-masked]>
Date: Thu, 09 Mar 2017 08:27:38 -0800

The client then provides a suggestion that may be reasonably for servers to simply use. Only a server that cares about structure in the ID changes it, others use the clients suggested value 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/361#discussion_r105206706

---

From: janaiyengar <[email-masked]>
Date: Thu, 09 Mar 2017 11:57:40 -0800

Mike, you still have "Changes Requested".  I've addressed your comments though, so I'll go ahead and merge. Feel free to let me know if there's more you'd like me to do, happy to do it in a follow up PR.

-- 
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/361#issuecomment-285463222

---

From: janaiyengar <[email-masked]>
Date: Thu, 09 Mar 2017 11:58:25 -0800

Merged #361.

-- 
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/361#event-993785398