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

[Lucas Pardue <lucaspardue.24.7@xxxxxxxxx>]

Hello,

As mentioned previously, review comments raised during the genart transport review were captured as issues on the QUIC WG GitHub repository. These issues have been assessed by the document editor(s) and shepherd, please see each individual issue for more-specific discussion. As a summary, the following resolutions for issues are:

Addressed via Pull Request, changes will appear in the next I-D
=======================================================
https://github.com/quicwg/base-drafts/issues/4284
  - https://github.com/quicwg/base-drafts/pull/4308
 
https://github.com/quicwg/base-drafts/issues/4297
  - https://github.com/quicwg/base-drafts/pull/4309
 
https://github.com/quicwg/base-drafts/issues/4271
  - https://github.com/quicwg/base-drafts/pull/4307
 
https://github.com/quicwg/base-drafts/issues/4303
  - https://github.com/quicwg/base-drafts/pull/4311
 
https://github.com/quicwg/base-drafts/issues/4295
  - https://github.com/quicwg/base-drafts/pull/4312
 
https://github.com/quicwg/base-drafts/issues/4269
  - https://github.com/quicwg/base-drafts/pull/4306
 
https://github.com/quicwg/base-drafts/issues/4299
  - https://github.com/quicwg/base-drafts/pull/4310
 
https://github.com/quicwg/base-drafts/issues/4285
  - https://github.com/quicwg/base-drafts/pull/4317
 
https://github.com/quicwg/base-drafts/issues/4289
  - https://github.com/quicwg/base-drafts/pull/4313
 
https://github.com/quicwg/base-drafts/issues/4290
  - https://github.com/quicwg/base-drafts/pull/4313
 
https://github.com/quicwg/base-drafts/issues/4266
  - https://github.com/quicwg/base-drafts/pull/4313
 
https://github.com/quicwg/base-drafts/issues/4301
  - https://github.com/quicwg/base-drafts/pull/4315
 
https://github.com/quicwg/base-drafts/issues/4300
  - https://github.com/quicwg/base-drafts/pull/4316
 
https://github.com/quicwg/base-drafts/issues/4302
  - https://github.com/quicwg/base-drafts/pull/4319
 
https://github.com/quicwg/base-drafts/issues/4304
  - https://github.com/quicwg/base-drafts/pull/4320
 
https://github.com/quicwg/base-drafts/issues/4286
  - https://github.com/quicwg/base-drafts/pull/4317
 
https://github.com/quicwg/base-drafts/issues/4273
  - https://github.com/quicwg/base-drafts/pull/4342
 
https://github.com/quicwg/base-drafts/issues/4265
  - https://github.com/quicwg/base-drafts/pull/4329

Close with no action
====================
https://github.com/quicwg/base-drafts/issues/4267
https://github.com/quicwg/base-drafts/issues/4268
https://github.com/quicwg/base-drafts/issues/4274
https://github.com/quicwg/base-drafts/issues/4275
https://github.com/quicwg/base-drafts/issues/4277
https://github.com/quicwg/base-drafts/issues/4278
https://github.com/quicwg/base-drafts/issues/4279
https://github.com/quicwg/base-drafts/issues/4281
https://github.com/quicwg/base-drafts/issues/4282
https://github.com/quicwg/base-drafts/issues/4287
https://github.com/quicwg/base-drafts/issues/4288
https://github.com/quicwg/base-drafts/issues/4291
https://github.com/quicwg/base-drafts/issues/4292
https://github.com/quicwg/base-drafts/issues/4293
https://github.com/quicwg/base-drafts/issues/4294
https://github.com/quicwg/base-drafts/issues/4296
https://github.com/quicwg/base-drafts/issues/4298


Duplicate/redundant issue
================
https://github.com/quicwg/base-drafts/issues/4270 (duplicate of 4268)
https://github.com/quicwg/base-drafts/issues/4272 (fixed alongside 4271

Kinds Regards

Lars and Lucas

QUIC WG Co-chairs

On Wed, Oct 28, 2020 at 2:26 PM Lars Eggert <lars@xxxxxxxxxx> wrote:

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@xxxxxxxx> 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

-- 
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call