define compound packets #1262

kazuho · 2018-04-04T11:45:21Z

Discussed at editors' meeting. We seemed to agree on something like option 4, with some specific rules: create "compound packet types" which have a length, and encode HS+0RTT and HS+1RTT long header packet types. To keep packet processing simple, the long header can be repeated so that a receiver can process parts of the packet under different keys in a straightforward manner.

This doesn't have to be the final resolution, but we should drive this issue to an end.

@kazuho : Would you be interested in driving this? Would you be able to write up a PR with a proposed design (use the one above if you like that design) and propose to the wg?

Thank you for your patience. This is the PR that does what is being suggested. Please take a look.

Some notes:

a Compound-Initial packet is sent by client
a Compound-Handshake packet is sent by both endpoints
- a client can use it to send 0-RTT data along with packets ACKing the server's handshake packets prior to ServerFinished
- a server can use it to send 0.5-RTT data along with ServerFinished
- a client can use it to send 1-RTT data along with ClientFinished
an endpoint is required to process the handshake portion of a compound packet even if the protected part is broken
- the rationale behind is to not give an on-path attacker the chance of terminating the handshake simply by appending several bytes to the packet (currently, we require an on-path attacker to do AES based on the handshake protection key)

MikeBishop

Good start -- a few comments, but I think this is the right general outline.

MikeBishop · 2018-04-04T16:11:36Z

draft-ietf-quic-transport.md

@@ -364,6 +364,8 @@ keys are established.
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                       Packet Number (32)                      |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|     [Payload Length (16)]     |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                          Payload (*)                        ...


Totally editorial: You could put Payload in the second half of the previous line to avoid having a jagged illustration. Variable-length fields like Payload don't have to have any particular width.

Thank you very much for the review. I've fixed this in a52715a. I will address other issues later (hopefully tomorrow).

MikeBishop · 2018-04-04T16:12:36Z

draft-ietf-quic-transport.md

@@ -678,6 +687,34 @@ packet protection in detail.  After decryption, the plaintext consists of a
 sequence of frames, as described in {{frames}}.


+## Compound Packets {#packet-compound}
+
+Compound packets allow the sender to assemble an Initial ({{packet-initial}})


I might use a different verb than "assemble"; perhaps "combine"?

MikeBishop · 2018-04-04T16:14:58Z

draft-ietf-quic-transport.md

+or a Handshake ({{packet-handshake}}) packet and a Protected
+({{packet-protected}}) packet into one UDP packet, thereby reducing the number
+of packets required to be emitted when application data can be sent during the
+handshake.


Question, and important to discuss either way: When sending Compound Initial + 0-RTT, does the minimum packet size apply to the entire compound and not just the Initial section? If so, might be worth mentioning that one intended use-case of this is to meet the minimum with 0-RTT data instead of PADDING.

MikeBishop · 2018-04-04T16:17:39Z

draft-ietf-quic-transport.md

+exception being that it has the Payload Length field.  The field designates the
+length of the payload of the Handshake packet.  The remainder of the UDP packet
+contains either a 0-RTT Protected packet or a short header packet
+({{short-header}}).


It might be worth explaining or giving an example that all the packet header fields are there for the subsequent packet. If that includes the type, by the way, you probably don't actually have to specify what type will be there, since the packet will declare it.

MikeBishop · 2018-04-04T16:19:30Z

draft-ietf-quic-transport.md

+The receiver of a Compound packet MUST individually process the Cryptographic
+Handshake packet and the Protected packet being contained.  It MUST process the
+Cryptographic Handshake packet of a Compound packet even when the Protected
+packet being contained is deemed invalid.


I think I'd simplify away these terms for the sections of the packet. Simply say that the thing which follows is processed as a separate packet, which might be discarded, buffered, or processed depending on the receiver's current state.

...and contains its own packet number, and therefore gets acknowledged separately.

martinthomson

I think that the name is wrong. Compound implies that there are multiple packets in the one packet. Which is true, but not from the perspective of the Initial or Handshake packet that is included, which is where the label is applied.

That is, rename the packet types to something like Initial-With-Length. The section title is fine.

martinthomson · 2018-04-05T02:36:28Z

draft-ietf-quic-transport.md

+A Compound-Initial packet is identical to an Initial packet with the exception
+being that it has the Payload Length field.  The field designates the length of
+the payload of the Initial packet.  The remainder of the UDP packet contains a
+0-RTT Protected packet.


What if the remainder of the packet doesn't contain a 0-RTT packet?

kazuho · 2018-04-09T05:35:19Z

@MikeBishop @martinthomson Thank you for your comments. I think I have addressed all of them. Please take a look.

And I think that the text has become clearer by saying that a compound UDP packet is something that consists of a *-with-Length packet and a protected packet. Thank you to @martinthomson for the suggestion.

ianswett

I'm happy with this, though it reminds me that 32 bit packet numbers in the long header is almost certainly overkill.

ianswett · 2018-04-09T15:41:21Z

draft-ietf-quic-transport.md

@@ -364,7 +364,7 @@ keys are established.
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                       Packet Number (32)                      |


This makes me think we should move to 2 byte packet numbers.

ekr · 2018-04-09T18:28:24Z

This seems OK, but why bother to have extra packet types. We've already expanded the long header quite a bit to make room for the two directional CIDs. Why not just add a length varint and have only one header form

MikeBishop

Looks good to me.

ianswett · 2018-04-09T19:51:16Z

In regards to @ekr. If we're going to make the length varint and always present(which I'm happy with), can we make the packet number varint(but not relative) too for consistency?

mikkelfj · 2018-04-09T20:02:19Z

Note that varint packet numbers has an effect on my proposed variation of #1079 PNE because it is not immediately obvious where the PN begins if you encrypt and attach to the end. This is not an argument against, but a note to consider exposing length info elsewhere +/- privacy concerns.

ianswett · 2018-04-09T20:08:58Z

For the context of this PR, I'd slightly favor adding a varint length to all long headers, as EKR suggested.

We can discuss packet numbers elsewhere, because as you said @mikkelfj, it may be complicated.

mikkelfj · 2018-04-09T20:33:25Z

EDIT: removed longwinded PNE concern - placed here by mistake, moved to 1079.

kazuho · 2018-04-09T22:14:56Z

@MikeBishop @ianswett @ekr @mikkelfj Thank you all.

Why not just add a length varint and have only one header form?

I will do this.

OTOH, I agree with @ianswett that how PN of a long header is encoded should better be discussed separately. Reasons: i) they can be discussed separately ii) representation of a PN depends on #1079 iii) representation of a PN may depend on the outcome of the stream 0 redesign.

mikkelfj · 2018-04-09T22:54:46Z

Oh sorry, I meant to post this PN scheme elsewhere - completely ignoring Ians request by mistake.

martinthomson · 2018-04-10T00:47:19Z

Yes, let's solve one problem at a time. The idea of a varint packet number is pretty appealing to me, and I don't think that the stream 0 changes will necessarily change it, but we should probably wait and see.

…onal

martinthomson

This seems like a good change. Nits only, the structure and content is basically fine.

martinthomson · 2018-04-10T04:48:22Z

draft-ietf-quic-tls.md

@@ -552,7 +552,7 @@ older than 1.3 is negotiated.

 QUIC requires that the initial handshake packet from a client fit within the
 payload of a single packet.  The size limits on QUIC packets mean that a record
-containing a ClientHello needs to fit within 1171 octets.
+containing a ClientHello needs to fit within 1170 octets.


An 1170 octet ClientHello will have a two octet length, so this needs to be 1169.

At the moment the PR states that Payload Length field is a varint, and therefore the minimum overhead is 1 octet. Do you think that we should change it to a fix length of 2 octets?

If the packet is this big, then the length will be 2 octets. You can't have encode 1170 (or 1169) in one octet. Obviously, if the ClientHello is smaller, then the length might only be 1 octet, but I think that one octet is impossible. ClientHello.random is 32 octets and either a shares is at least 32 octets or a PSK binder is that big too.

Ah! Thank you for very much for the clarification.

martinthomson · 2018-04-10T04:48:36Z

draft-ietf-quic-transport.md

@@ -403,10 +405,13 @@ Packet Number:
 : Octets 13 to 16 contain the packet number.  {{packet-numbers}} describes the
  use of packet numbers.

+Payload Length:
+
+: The length of the Payload field in octets.


...encoded as a variable-length integer.

martinthomson · 2018-04-10T04:50:05Z

draft-ietf-quic-transport.md

+## Compound Packets {#packet-compound}
+
+A sender can combine multiple QUIC packets (typically a Cryptographic Handshake
+packet and a Protected packet) into one UDP packet, thereby reducing the number


s/UDP packet/UDP datagram/

Done in 4504fdc.

martinthomson · 2018-04-10T04:51:35Z

draft-ietf-quic-transport.md

+
+A sender can combine multiple QUIC packets (typically a Cryptographic Handshake
+packet and a Protected packet) into one UDP packet, thereby reducing the number
+of UDP packets required to be emitted when application data can be sent during


End the sentence at the comma. Then "This can reduce the number of UDP datagrams that are needed to complete the handshake when multiple types of packets are sent at the same time."

Done in 5ed67ea.

martinthomson · 2018-04-10T04:52:27Z

draft-ietf-quic-transport.md

+A sender can combine multiple QUIC packets (typically a Cryptographic Handshake
+packet and a Protected packet) into one UDP packet, thereby reducing the number
+of UDP packets required to be emitted when application data can be sent during
+the handshake.


Add: "A packet with a short header doesn't include a length, so it has to be the last packet included in a UDP datagram."

Done in 9dd629e.

martinthomson · 2018-04-10T04:53:08Z

draft-ietf-quic-transport.md

+always starts at an offset of 0 (see {{stateless-retry}}) and the complete
+cryptographic handshake message MUST fit in a single packet (see {{handshake}}).
+
+The payload of a UDP packet carrying the Initial packet MUST be expanded to at


UDP datagram.

Done in 4504fdc.

martinthomson · 2018-04-10T04:55:11Z

draft-ietf-quic-transport.md

+The payload of a UDP packet carrying the Initial packet MUST be expanded to at
+least 1200 octets (see {{packetization}}), by adding PADDING frames to the
+Initial packet and/or by combining the Initial packet with a Protected packet
+(see {{packet-compound}}).


s/Protected packet/0-RTT packet/

No point in being any less precise than needed.

Done in d5b7312.

martinthomson · 2018-04-10T04:55:16Z

draft-ietf-quic-transport.md

+the handshake.
+
+The sender MUST NOT combine QUIC packets belonging to different QUIC
+connections into a single UDP packet.


Done in 4504fdc.

martinthomson · 2018-04-10T04:56:02Z

draft-ietf-quic-transport.md

+Every QUIC packet that is being conveyed in a compound UDP packet is a complete
+QUIC packet.  No fields in the packet header are omitted.  The receiver of a
+compound UDP packet MUST individually process each QUIC packet and separately
+acknowledge them, as if they were received as distinct UDP packets.


s/as distinct UDP packets/as the payload of different UDP datagrams/

Done in 49d6f96.

kazuho · 2018-04-10T05:28:24Z

@martinthomson Thank you for the super quick feedback. I think that I have addressed all your comments (as well as merging master) with the exception of #1262 (comment). Please take a look.

…te field

kazuho · 2018-04-10T06:33:11Z

I believe that all known issues have been fixed in commits up to 93fe0be.

ianswett

Two small editorial suggestions, but looks great, thanks for the fast updates.

ianswett · 2018-04-10T13:18:12Z

draft-ietf-quic-transport.md

+The sender MUST NOT combine QUIC packets belonging to different QUIC
+connections into a single UDP datagram.
+
+Every QUIC packet that is being conveyed in a compound UDP datagram is a


nit: I'd drop "that is being"

ianswett · 2018-04-10T13:19:07Z

draft-ietf-quic-transport.md

+
+A sender can combine multiple QUIC packets (typically a Cryptographic Handshake
+packet and a Protected packet) into one UDP datagram.  This can reduce the
+number of UDP datagrams required to be emitted when application data can be sent


For clarity, how about "number of UDP datagrams sent when application data is sent during the handshake."?

nibanks

I really like how clean this design is. Good improvement!

mikkelfj · 2018-04-10T15:39:36Z

draft-ietf-quic-transport.md

+
+The sender MUST NOT combine QUIC packets belonging to different QUIC
+connections into a single UDP datagram.
+


What about packets in the gray zone such as version negotiation and resets?

Yes, I will address those in a follow-up.

mikkelfj · 2018-04-10T15:40:03Z

draft-ietf-quic-transport.md

+
+: The length of the Payload field in octets, encoded as a variable-length
+  integer ({{integer-encoding}}).
+


I came to think a bit about the variable payload length:

It makes it difficult to build a packet when you don't know the length in advance. Of course you can always just start with enough space. The intial packet starts with 1200 bytes (not sure about 0-RTT), and I don't think it can be more than 64K? So wouldn't a fixed 2 byte length field make more sense here?

The variable length is primarily for consistency. There are a few cases where packets will be small enough to benefit from the 6-bit length (Finished primarily), but the main benefit is that this is the same code throughout. Better if we can manage to make the packet number encoding variable length as well (but that interacts in interesting ways with #1079, so that's not as clear a win as this).

janaiyengar · 2018-04-12T21:15:51Z

Coming in late, but I like the final shape of this. Thanks, @kazuho, for driving this quickly!

Some editorial cleanup for #1262

kazuho added 2 commits April 4, 2018 20:40

define compound packets

87af6f9

fold in 80 chars

d9c7f0d

MikeBishop reviewed Apr 4, 2018

View reviewed changes

Payload Length and Payload in one line

a52715a

martinthomson reviewed Apr 5, 2018

View reviewed changes

kazuho added 5 commits April 5, 2018 14:30

switch typenames to *-Initial

77f90aa

remove redundancy

55b806d

more clarification on how a succeeding QUIC packet looks like

487d887

compound packet can be used to expand the Initial packet

4e01ff7

typos

9e73eab

ianswett approved these changes Apr 9, 2018

View reviewed changes

MikeBishop approved these changes Apr 9, 2018

View reviewed changes

kazuho added 3 commits April 10, 2018 13:33

Payload Length on all long header packets

9b47a09

better clarify that every QUIC packet in a compound UDP packet are equal

8aae10e

decrement maximum CHLO length due to Payload Length becoming non-opti…

ff01058

…onal

martinthomson reviewed Apr 10, 2018

View reviewed changes

kazuho added 5 commits April 10, 2018 14:08

Merge branch 'master' into kazuho/compound-packet

f0575f6

s/UDP packet/UDP datagram/g

4504fdc

clarify that Payload Length is varint

b4dfcbb

split the sentence

5ed67ea

short header packet needs to be the last one

9dd629e

kazuho added 2 commits April 10, 2018 14:20

be more precise

d5b7312

as "the payload of"

49d6f96

decrement by two, since at this size Payload Length would be a 2-by…

93fe0be

…te field

ianswett approved these changes Apr 10, 2018

View reviewed changes

nibanks approved these changes Apr 10, 2018

View reviewed changes

mikkelfj reviewed Apr 10, 2018

View reviewed changes

martinthomson merged commit 0d1a97c into quicwg:master Apr 11, 2018

martinthomson added a commit that referenced this pull request Apr 11, 2018

Some editorial cleanup for #1262.

8dbc2e4

martinthomson added a commit that referenced this pull request Apr 12, 2018

Merge pull request #1285 from quicwg/pr1262-followup

3e0c70e

Some editorial cleanup for #1262

marten-seemann mentioned this pull request Apr 18, 2018

implement payload length for long headers quic-go/quic-go#1305

Closed

This was referenced Apr 18, 2018

make payload length 0 special #1301

Closed

What to do when part of a coalesced packet is invalid? #1319

Closed

martinthomson mentioned this pull request May 10, 2018

handshake and 0rtt data in a single packet #630

Closed

		@@ -364,7 +364,7 @@ keys are established.
		+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
		\| Packet Number (32) \|


		The sender MUST NOT combine QUIC packets belonging to different QUIC
		connections into a single UDP datagram.


		: The length of the Payload field in octets, encoded as a variable-length
		integer ({{integer-encoding}}).

define compound packets #1262

define compound packets #1262

Conversation

kazuho commented Apr 4, 2018

MikeBishop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuho commented Apr 9, 2018

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekr commented Apr 9, 2018

MikeBishop left a comment

Choose a reason for hiding this comment

ianswett commented Apr 9, 2018

mikkelfj commented Apr 9, 2018 • edited

ianswett commented Apr 9, 2018

mikkelfj commented Apr 9, 2018 • edited

kazuho commented Apr 9, 2018

mikkelfj commented Apr 9, 2018

martinthomson commented Apr 10, 2018

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuho commented Apr 10, 2018

kazuho commented Apr 10, 2018

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar commented Apr 12, 2018

mikkelfj commented Apr 9, 2018 •

edited

mikkelfj commented Apr 9, 2018 •

edited