Hi Erik, Thanks for the review. I've captured your comments as issues on the QUIC WG GItHub repository. Links to each are provided as in-line responses.
On Tue, Jan 5, 2021 at 6:18 AM Erik Kline via Datatracker <[email protected]> wrote: > Erik Kline has entered the following ballot position for > draft-ietf-quic-transport-33: Yes > > When responding, please keep the subject line intact and reply to all > email addresses included in the To and CC lines. (Feel free to cut this > introductory paragraph, however.) > > > Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html > for more information about IESG DISCUSS and COMMENT positions. > > > The document, along with other ballot positions, can be found here: > https://datatracker.ietf.org/doc/draft-ietf-quic-transport/ > > > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > [[ comments ]] > > [ section 8.2.3 ] > > * I found this wording a tad odd: > > If the PATH_CHALLENGE frame that resulted in successful path > validation was sent in a datagram that was not expanded to at least > 1200 bytes, the endpoint can regard the address as valid. > > It seems like whether the frame was padded to 1200 or not, if the > response > data matches the challenge data the address can be considered validated. > > I think the point at the end of the sentence is to say that *only* the > address, but not the MTU, can be taken as validated. > https://github.com/quicwg/base-drafts/issues/4513 > [ section 9.6.3 ] > > * Entirely optional, but I wonder if it's worth noting that in certain > situations, for example an IPv6-only client and IPv4-only server, the > client might be required to evaluate use of an alternate address family > address if, for example, some transition mechanism (a la NAT64) was in > use. > https://github.com/quicwg/base-drafts/issues/4514 > [ section 9.7 ] > > * "as this would enable..." reads to me like the opposite of what's > intended. > Perhaps: "as failure to do so would enable..."? > https://github.com/quicwg/base-drafts/issues/4515 > [ section 14.1 ] > > * I think it might be important to note that this strategy places some > restrictions on the use of things like IPv6 extension headers that can be > used in these packets. > > For example, on an IPv6-only link with a 1280 MTU, enforcing a 1200 byte > UDP payload in these packets leaves only 32 bytes of space for any > extension headers. > > I think this is likely fine for these initial packets (vis. section 8.1 > and > so on ), but as a general requirement for all packets this could > artificially constrain use of new extension headers. > > https://github.com/quicwg/base-drafts/issues/4516 [ section 19.3.1 ] > > * This seems intricate enough that it might be nice if there were an > Appendix (A.5?) section walking through an example computation or two. > > https://github.com/quicwg/base-drafts/issues/4517 [ section 19.18 ] > > * I'm idly wondering if there would be any debugging value in the response > including the IP & port to which the response is being sent (i.e. from > which the path challenge was received) ... assuming the packet with the > PATH_RESPONSE frame is protected. > > Not important though, and perhaps it was already discussed and rejected. > (or maybe it's better as some future, entirely separate PATH_INFO frame) > > https://github.com/quicwg/base-drafts/issues/4518 > [[ questions ]] > > [ section 8.2.4 ] > > * To be clear, this document is effectively saying that it takes no > position > on the interpretation of any ICMP errors received? Is it up to the > implementer to decide if "validated" (in as much as ICMP messages can be > validated) Admin Prohibited messages, for example, should constitute a > positive confirmation of path failure? Or is there some very specific > stance that should be taken ("never trust that lyin' ICMP!")? > > https://github.com/quicwg/base-drafts/issues/4519 [ section 10.3 ] > > * Does this "datagram ends with stateless reset token" scheme mean that > implementations must check the output of every packet, including post > encryption, and take some action if a (very low probability) collision > occurs (meaning the output accidentally produces this 16 byte value > but the packet is not intended to be a stateless reset)? > > https://github.com/quicwg/base-drafts/issues/4520 > [[ nits ]] > > [ section 7 ] > > * There seem to be two paragraphs with the same text about how an endpoint > validates ECN support. Seems like maybe only the first paragraph is > really > necessary (or, put another way: I can't see what new information the > second > paragraph adds). > > (the paragraph below Figure 4 seems to be repeated information) > https://github.com/quicwg/base-drafts/issues/4521 > [ section 8.1.1 ] > > * "a NEW_TOKEN frames" -> "a NEW_TOKEN frame" or "NEW_TOKEN frames" > https://github.com/quicwg/base-drafts/issues/4522 > [ section 17.2.3 ] > > * ", as defined Section" -> ", as defined in Section" > > https://github.com/quicwg/base-drafts/issues/4523 Cheers, Lucas On behalf of QUIC WG Chairs
