[Gen-art] Genart last call review of draft-ietf-quic-transport-32

[Stewart Bryant via Datatracker <[e-mail]>]

Reviewer: Stewart Bryant
Review result: Ready with Issues

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-quic-transport-32
Reviewer: Stewart Bryant
Review Date: 2020-10-28
IETF LC End Date: 2020-11-16
IESG Telechat date: Not scheduled for a telechat

Summary:

I would like to remind the reader that the Genart reviewer is someone was not
involved with the design and is ideally reading the text for the first time
rather than an expert. In this regard I hope that I am accuracy modelling the
reaction of such a reader in the future.

This is a very clever, well written and innovative body of work, and could be
published as is. However, I am not sure it services the needs of all of the
communities that will need to read the document and as such ought to have
additional text, or point to another supplementary document.

Specifically as the base document for the protocol it is the one that people
will naturally start with, and I found that it was not until I got to past page
100 that I started to appreciate how it worked. Not everyone needs to know the
whole protocol in detail, and some will just need to dip into important
sections. A tutorial/introduction explaining the basics of operation to provide
a framework for the rest of the document would help a lot of readers.

The text uses a new notation which is clever and succinct for representing the
detail of the transmitted protocol, but I suspect that the use of conventional
diagrams or at least a tutorial appendix that includes them would be
appreciated by other new readers looking at this for the first time.

I amplify this in the comments below.

Major issues:

Section 1 provides an ultrafast introduction to the problem, and very little on
the fundamentals of how the protocol works. It would be useful if there a
process by which the reader is gradually immersed in the detail.

========

Section 1 is the first place that you introduce the term "Frame". I got very
confused about frames and packets, and I thing the taxonomy needs to be clearly
explained. You have UDP/IP packets that carry one or more QUIC PDUs, but the
frame/packet notation made it unclear what was being discussed and whether the
terms were being used consistently. Some clarification would be useful, or if
it is in the text then making it more obvious would help.

=======

   Packet and frame diagrams in this document use a custom format.  The
   purpose of this format is to summarize, not define, protocol
   elements.

SB> I found this confusing, because in a ST protocol specification I was
looking for a protocol definition

=======

   Streams are identified within a connection by a numeric value,
   referred to as the stream ID.  A stream ID is a 62-bit integer (0 to
   2^62-1) that is unique for all streams on a connection.

SB> I found the 62 bits pulled out of a hat very confusing. Eventually the
reader realises that this is 64 bits with the two lowest bits  signifying the
parameters below.    Some explanatory text would help.

======
Stream IDs
   are encoded as variable-length integers; see Section 16.  A QUIC
   endpoint MUST NOT reuse a stream ID within a connection.

   The least significant bit (0x1) of the stream ID identifies the

SB> This was a common problem throughout the text. I found the use of (0x1) and
similar confusing in the text. Where you use this notation you mean the bit
manipulated by the mask 0x1 (and similar). It would be helpful to the reader to
be a little more pedantic in the use of the term in each case. We will get to
it later, in the comments but a lot of readers would be helped by the use of
the classic ASCII art style representation of bit fields.

==========

===========
SB> When I read the text around table 1, I found this to be confusing and was
not clear whether the object is a 64 bit object with a 62 bit instance ID or a
62 bit object with a 60 bit instance ID. If the former I am not sure what the
name of the whole is vs the name of the 62 bit instance ID is. This could
usefully be clarified.

===========
   QUIC depends upon a minimum IP packet size of at least 1280 bytes.
   This is the IPv6 minimum size ([IPv6]) and is also supported by most
   modern IPv4 networks.  Assuming the minimum IP header size of 40
   bytes for IPv6 and 20 bytes for IPv4 and a UDP header size of 8
   bytes, this results in a maximum datagram size of 1232 bytes for IPv6
   and 1252 bytes for IPv4.

   The maximum datagram size MUST be at least 1200 bytes.
SB> The previous two paras are quite confusing. I think the first para refers
to the max packet size in the network layer, but the second para is not clear,
nor is the next para.

   Any maximum datagram size larger than 1200 bytes can be discovered
   using Path Maximum Transmission Unit Discovery (PMTUD; see
   Section 14.2.1) or Datagram Packetization Layer PMTU Discovery
   (DPLPMTUD; see Section 14.3).

   Enforcement of the max_udp_payload_size transport parameter
   (Section 18.2) might act as an additional limit on the maximum
   datagram size.  A sender can avoid exceeding this limit, once the
   value is known.  However, prior to learning the value of the
   transport parameter, endpoints risk datagrams being lost if they send
   datagrams larger than the smallest allowed maximum datagram size of
   1200 bytes.

SB> Again unclear. Are you saying that the network must support a network layer
packet of at least 1200B which is used to carry Ip + UDP + UDP payload?

   UDP datagrams MUST NOT be fragmented at the IP layer.  In IPv4
   ([IPv4]), the DF bit MUST be set if possible, to prevent
   fragmentation on the path.

   Datagrams are required to be of a minimum size under some conditions.
   However, the size of the datagram is not authenticated.  Therefore,
   an endpoint MUST NOT close a connection when it receives a datagram
   that does not meet size constraints, though the endpoint MAY discard
   such datagrams.

SB> I really think this section needs re-writing for clarity.

============

14.2.1.  Handling of ICMP Messages by PMTUD

SB> I am worried about the text in this section. ICMP and UDP paths may not be
congruent as a result of different RTG ECMP treatment. The only packet you can
trust to explore the path is a QUIC packet.

============

22.1.1.  Provisional Registrations

   Provisional registration of codepoints are intended to allow for
   private use and experimentation with extensions to QUIC.  Provisional
   registrations only require the inclusion of the codepoint value and
   contact information.  However, provisional registrations could be
   reclaimed and reassigned for another purpose.

SB> Experience in other WGs suggests that reclaiming is very hard. We really
needed to do this in MPLS where we had a 16 value field and found it impossible.

============
   Additionally, each value of the format "31 * N + 27" for integer
   values of N (that is, 27, 58, 89, ...) are reserved and MUST NOT be
   assigned by IANA.

SB> We do not normally ask IANA to run an algorithm to check a value. Are we
sure that this will work?

=============
Minor issues:

SB> In looking at  "Figure 2: States for Sending Parts of Streams" it occurred
to me that this was the major states but there were possibly a number of error
states missing. It would be useful to clarify.

=========

   It is necessary to limit the amount of data that a receiver could
   buffer, to prevent a fast sender from overwhelming a slow receiver,
   or to prevent a malicious sender from consuming a large amount of
   memory at a receiver.
SB> Surely it is also necessary to do this to prevent the network from being
overwhelmed?

=======
   If a max_streams transport parameter or a MAX_STREAMS frame is
   received with a value greater than 2^60,
SB> This pulling of 2^60 out of a hat is too cryptic. As I understand it this
is because you need 2 bits to express the size of the object that holds to
identifier and two bits to hold the control parameters that go in the LSBs. It
would be helpful to the new reader to call all this out earlier rather than
making them figure it all out.
=======

6.  Version Negotiation
SB> I am surprised that there is no IANA version registry
========

  For a server to use a new version in the future, clients need to
   correctly handle unsupported versions.  Some version numbers
   (0x?a?a?a?a as defined in Section 15) are reserved for inclusion in
   fields that contain version numbers.

SB> The value 0x?a?a?a?a is somewhat pulled out of a hat, it would be useful to
explain the rational. SB> and then in Section 15 I wondered why this pattern
which leaves all sorts of holes in the version space?
=======

   Implementations need to maintain a buffer of CRYPTO data received out
   of order.  Because there is no flow control of CRYPTO frames, an
   endpoint could potentially force its peer to buffer an unbounded
   amount of data.

SB> What are the congestion consequences of this?

==========

      validation_timeout = max(3*PTO, 6*kInitialRtt)
SB>A quick look in the reference did not show what units of time you are using
in the protocol - maybe I missed it?

=======

   Stateless Reset {
     Fixed Bits (2) = 1,
     Unpredictable Bits (38..),
     Stateless Reset Token (128),
   }

                     Figure 10: Stateless Reset Packet

SB> When I first met this my reaction was that  I was sure it sends a lot more
bits on the wire than this, but so far I have not see a packet definition nor a
pointer to a packet definition.  Please could you clarify what is happening?

=====

10.3.1.  Detecting a Stateless Reset

   An endpoint detects a potential stateless reset using the trailing 16
   bytes of the UDP datagram.
SB> I find this rather opaque given that so far we do not know what a packet
looks like, and I cannot see what the encoding of this is.

SB> To add to the confusion over packets and frames we now have datagrams.

=======

SB> Having read to the end of section 10 I made a note that I was still trying
to figure out what a reset token actually looked like.

=====

12.3.  Packet Numbers

   The packet number is an integer in the range 0 to 2^62-1.
SB> It would save a bit of wondering until later if this was described as a
non-reusable number space

=======
   A QUIC endpoint MUST NOT reuse a packet number within the same packet
   number space in one connection.  If the packet number for sending
   reaches 2^62 - 1, the sender MUST close the connection without
   sending a CONNECTION_CLOSE frame or any further packets; an endpoint
   MAY send a Stateless Reset (Section 10.3) in response to further
   packets that it receives.

SB> I know that this is a lot of space, although you subdivide it so its
smaller, but I would be useful is the fact that there was a maximum data size
was stated up from and it was specified. There are some applications that
expect to send continuously without disruption and would need to take action
prior to termination of the transfer.

=====
12.4.  Frames and Frame Types

   The payload of QUIC packets, after removing packet protection,
   Packet Payload {
     Frame (8..) ...,
   }

   consists of a sequence of complete frames, as shown in Figure 11.
   Version Negotiation, Stateless Reset, and Retry packets do not
   contain frames.

SB> At this point (page 82) I am still not sure what a packet or a frame is and
have no idea what is going on on the wire.

=======
14.  Datagram Size

   A UDP datagram can include one or more QUIC packets.
SB> There is some confusion in the text between packets, frames and datagrams.
That could be helped by earlier clarification.

======
17.2.  Long Header Packets

SB> How is the following encoded? It is not clear how long the fields are.
SB> If the notation is that (x) means x bits it would be good to remind the
reader. It may have been called up earlier but that was 100 pages ago with no
back pointer. This would be so much clearer to most readers with a conventional
packet diagram

   Long Header Packet {
     Header Form (1) = 1,
     Fixed Bit (1) = 1,
     Long Packet Type (2),
     Type-Specific Bits (4),
     Version (32),
     Destination Connection ID Length (8),
     Destination Connection ID (0..160),
     Source Connection ID Length (8),
     Source Connection ID (0..160),
   }

=======
   The layout of a Version Negotiation packet is:

   Version Negotiation Packet {
     Header Form (1) = 1,
     Unused (7),
     Version (32) = 0,
     Destination Connection ID Length (8),
     Destination Connection ID (0..2040),
     Source Connection ID Length (8),
     Source Connection ID (0..2040),
     Supported Version (32) ...,
   }

                   Figure 14: Version Negotiation Packet

   The value in the Unused field is selected randomly by the server.

SB> When I first met this text the following questions arose.
SB> What does the (7) in unused mean?
SB> What length is Destination/Source Connection ID/What does (0..2040) mean?
SB> Now I have figured this out, but I really worry about how difficult this is
for the new/casual reader of the text.

========
   The server includes a connection ID of its choice in the Source
   Connection ID field.  This value MUST NOT be equal to the Destination
   Connection ID field of the packet sent by the client.
SB> It would be helpful to explain why

========

18.1.  Reserved Transport Parameters

   Transport parameters with an identifier of the form "31 * N + 27" for
   integer values of N are reserved to exercise the requirement that
   unknown transport parameters be ignored.  These transport parameters
   have no semantics, and may carry arbitrary values.

SB> Which hat did that come out of? It would be helpful to the reader to
explain the rational.

===========
19.1.  PADDING Frames
SB> The choice of padding values is itself an interesting problem, but as far
as I can see the text is silent on the subject.
==========

22.1.2.  Selecting Codepoints

   New uses of codepoints from QUIC registries SHOULD use a randomly
   selected codepoint that excludes both existing allocations and the
   first unallocated codepoint in the selected space.  Requests for
   multiple codepoints MAY use a contiguous range.

SB> Doesn’t this lead to fragment code-spaces where it becomes progressively
harder to get blocks. Why this policy?

SB> Related to which I am not sure the process in para 2 of 22.1.3 is realistic
and wonder of it is really needed.

=========
Appendix A.  Sample Packet Number Decoding Algorithm

   The pseudo-code in Figure 45 shows how an implementation can decode
   packet numbers after header protection has been removed.

SB> The variable in the code need to be defined.
SB> There should also be some commentary to make it understandable to the new
reader. SB> It would be useful for this to have a back reference to the body of
the document.

==========

Nits/editorial comments:

SB> "Example Structure"
SB> The use of white space coupled name components was strange to me and  it
too a while and a look late in the text to realise that Structure was part the
name and not part of the syntax (i.e. that a structure was not Structure {})

=======

Table 3: Frame Types

SB> The notation used in the table took a while to figure out. A little more
explanation, and possibly before the table itself might make it clearer.

=========

                   Table 4: Summary of Integer Encodings

   For example, the eight byte sequence c2 19 7c 5e ff 14 e8 8c (in
   hexadecimal) decodes to the decimal value 151288809941952652; the
   four byte sequence 9d 7f 3e 7d decodes to 494878333; the two byte
   sequence 7b bd decodes to 15293; and the single byte 25 decodes to 37
   (as does the two byte sequence 40 25).

SB> The
SB> Some hex & decimal indication would help.
SB> A picture is worth a thousand words and save the reader drawing this out
themselves to understand it and getting confused. SB> The conversion pseudo
code would be helpful, possibly in an appendix.

==============
   As a result, the size of the packet number encoding is at least one
   bit more than the base-2 logarithm of the number of contiguous
   unacknowledged packet numbers, including the new packet.
SB> That is really hard to parse.

   For example, if an endpoint has received an acknowledgment for packet
   0xabe8bc, sending a packet with a number of 0xac5c02 requires a
   packet number encoding with 16 bits or more; whereas the 24-bit
   packet number encoding is needed to send a packet with a number of
   0xace8fe.
SB> and so is that.

SB> Indeed I found the remaining paras of the section difficult to follow.
=========
   With this mechanism, the server reflects the spin value received,
   while the client 'spins' it after one RTT.  On-path observers can
   measure the time between two spin bit toggle events to estimate the
   end-to-end RTT of a connection.
SB> It would be clearer to the reader if the explanation of operation proceeded
the detail.

=========

   The Transport Parameter Length field contains the length of the Transport
   Parameter Value field.

SB> Presumably the length in bytes.

========

21.  Security Considerations
SB> An introduction to this long section would be useful

=========

21.13.  Overview of Security Properties

SB> I find it strange that having read 12 previous security analysis
subsections I arrive at this section. I am surprised that the overview is not
section 21.1

============