Re: [Gen-art] [Last-Call] Genart last call review of draft-ietf-quic-transport-32

Lars Eggert <lars@eggert.org> Wed, 28 October 2020 14:26 UTC

Return-Path: <lars@eggert.org>
X-Original-To: gen-art@ietfa.amsl.com
Delivered-To: gen-art@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 1027F3A09C3; Wed, 28 Oct 2020 07:26:05 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.098
X-Spam-Level:
X-Spam-Status: No, score=-2.098 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=eggert.org
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZWVDpQAK26Um; Wed, 28 Oct 2020 07:26:01 -0700 (PDT)
Received: from mail.eggert.org (mail.eggert.org [IPv6:2a00:ac00:4000:400:211:32ff:fe22:186f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id E41483A0976; Wed, 28 Oct 2020 07:25:57 -0700 (PDT)
Received: from [IPv6:2a00:ac00:4000:400:c1b6:9abf:94c2:7012] (unknown [IPv6:2a00:ac00:4000:400:c1b6:9abf:94c2:7012]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.eggert.org (Postfix) with ESMTPSA id 42BB4611067; Wed, 28 Oct 2020 16:25:46 +0200 (EET)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=eggert.org; s=dkim; t=1603895146; bh=ZIJiiXAEALaml6MkZ02enh3Ulzv8na4BQWCPi8GIHfw=; h=From:Subject:Date:In-Reply-To:Cc:To:References; b=cY0KKny3rgPn8jLuhmy/nckPX2vbv5WIRQ8hP52z2S+b77sVMuRUnpbFsPMSstbcZ hw4wO3/gaZBi52TAgstB3QXUD2++UBVPoWXPoyANWkfslOQOHaDykK1e4NS/J53kLg w2zhX67BGFvvUPPRHzdW+NpseCIVhFmcf1FoO03A=
From: Lars Eggert <lars@eggert.org>
Message-Id: <41995733-4EB2-4210-85D5-A90710666B92@eggert.org>
Content-Type: multipart/signed; boundary="Apple-Mail=_4EB641C5-9B1D-4467-BE58-F4CD1F4DE798"; protocol="application/pgp-signature"; micalg="pgp-sha512"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Date: Wed, 28 Oct 2020 16:25:41 +0200
In-Reply-To: <160388905223.20950.16428646022919705199@ietfa.amsl.com>
Cc: gen-art@ietf.org, last-call@ietf.org, IETF QUIC WG <quic@ietf.org>, draft-ietf-quic-transport.all@ietf.org
To: Stewart Bryant <stewart.bryant@gmail.com>
References: <160388905223.20950.16428646022919705199@ietfa.amsl.com>
X-MailScanner-ID: 42BB4611067.A7492
X-MailScanner: Found to be clean
X-MailScanner-From: lars@eggert.org
Archived-At: <https://mailarchive.ietf.org/arch/msg/gen-art/YuT3rp0Rrk__C7dDO3CVwOyfzXo>
Subject: Re: [Gen-art] [Last-Call] Genart last call review of draft-ietf-quic-transport-32
X-BeenThere: gen-art@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "GEN-ART: General Area Review Team" <gen-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/gen-art>, <mailto:gen-art-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/gen-art/>
List-Post: <mailto:gen-art@ietf.org>
List-Help: <mailto:gen-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/gen-art>, <mailto:gen-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 28 Oct 2020 14:26:05 -0000

Hi Stewart,

thank you for the review. I've opened a number of GitHub issues for the points you raise below, please feel free to track their resolution there. There is also a tracking milestone at https://github.com/quicwg/base-drafts/milestone/4.

Thanks,
Lars

On 2020-10-28, at 14:44, Stewart Bryant via Datatracker <noreply@ietf.org> wrote:
> 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.

https://github.com/quicwg/base-drafts/issues/4265

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

https://github.com/quicwg/base-drafts/issues/4266

>   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

https://github.com/quicwg/base-drafts/issues/4267

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

https://github.com/quicwg/base-drafts/issues/4268

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

https://github.com/quicwg/base-drafts/issues/4269

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

https://github.com/quicwg/base-drafts/issues/4270

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

https://github.com/quicwg/base-drafts/issues/4271

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

https://github.com/quicwg/base-drafts/issues/4272

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

https://github.com/quicwg/base-drafts/issues/4273

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

https://github.com/quicwg/base-drafts/issues/4274

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

https://github.com/quicwg/base-drafts/issues/4275

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

https://github.com/quicwg/base-drafts/issues/4276

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

https://github.com/quicwg/base-drafts/issues/4277

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

https://github.com/quicwg/base-drafts/issues/4278

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

https://github.com/quicwg/base-drafts/issues/4279

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

https://github.com/quicwg/base-drafts/issues/4280

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

https://github.com/quicwg/base-drafts/issues/4281

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

https://github.com/quicwg/base-drafts/issues/4282

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

https://github.com/quicwg/base-drafts/issues/4283

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

https://github.com/quicwg/base-drafts/issues/4284

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

https://github.com/quicwg/base-drafts/issues/4285

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

https://github.com/quicwg/base-drafts/issues/4286

> 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

https://github.com/quicwg/base-drafts/issues/4287

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

https://github.com/quicwg/base-drafts/issues/4288

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

https://github.com/quicwg/base-drafts/issues/4289

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

https://github.com/quicwg/base-drafts/issues/4290

> 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),
>   }

https://github.com/quicwg/base-drafts/issues/4291


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

https://github.com/quicwg/base-drafts/issues/4292

>   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

https://github.com/quicwg/base-drafts/issues/4293

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

https://github.com/quicwg/base-drafts/issues/4294

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

https://github.com/quicwg/base-drafts/issues/4295

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

https://github.com/quicwg/base-drafts/issues/4296

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

https://github.com/quicwg/base-drafts/issues/4297

> 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 {})

https://github.com/quicwg/base-drafts/issues/4298

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

https://github.com/quicwg/base-drafts/issues/4299

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

https://github.com/quicwg/base-drafts/issues/4300

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

https://github.com/quicwg/base-drafts/issues/4301

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

https://github.com/quicwg/base-drafts/issues/4302

>   The Transport Parameter Length field contains the length of the Transport
>   Parameter Value field.
> 
> SB> Presumably the length in bytes.

https://github.com/quicwg/base-drafts/issues/4303

> 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

https://github.com/quicwg/base-drafts/issues/4304