Hi Peter,
Thanks very much for your thorough review! I've placed comments/actions taken inline below. And one question at the very end. :) Peter Yee via Datatracker <nore...@ietf.org> writes:
Reviewer: Peter Yee 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-ipsecme-iptfs-12 Reviewer: Peter Yee Review Date: 2022-05-27 IETF LC End Date: 2022-05-18 IESG Telechat date: Not scheduled for a telechat Summary: This draft specifies an improved method for countering traffic analysis of IPsec tunnels. There are some nits and minor issues that should be addressed. I did not evaluate the appendices for correctness. [Ready with issues.] Major issues: None Minor issues: Page 7, 3rd paragraph, 1st sentence (and elsewhere in the document): You make reference to the “user” and what the “user” is supposed to do. I can’t begin to imagine an ordinary user coming up with an optimal window size or do some of the other things that are being required. Do you really want to put this requirement on a user, or should it be a different entity, such as the IP-TFS implementation?
FWIW we specifically do not use the term "ordinary user" for a reason. What the text is saying is that a user should be able to set the value b/c there's no single value that works for every use case. Indeed some values that work well in some use cases may work very poorly in others.
Page 13, 1st partial paragraph: How would the referenced AGGFRAG_PAYLOAD empty payload be recognized?
The next header field would indicate AGGFRAG_PAYLOAD.
The ESP Next Header won’t indicate that the contents is an AGGFRAG_PAYLOAD because the SA isn’t an AGGFRAG_PAYLOAD SA.
An SA being AGGFRAG_PAYLOAD enabled means that *all* the SA's tunnel traffic will be encapsulated in AGGFRAG_PAYLOADs. That does not preclude other uses of AGGFRAG_PAYLOAD payloads elsewhere. The ESP next header field determines the payload.
Page 13, 2nd full paragraph: the unnumbered figure from page 17 would be really helpful here given how many disparate header fields are referenced in this and the following paragraphs. Page 15, section 6.1: RFC 4303 says, “The Next Header is a mandatory, 8-bit field that identifies the type of data contained in the Payload Data field, e.g., an IPv4 or IPv6 packet, or a next layer header and data. The value of this field is chosen from the set of IP Protocol Numbers defined on the web page of the IANA, e.g., a value of 4 indicates IPv4, a value of 41 indicates IPv6, and a value of 6 indicates TCP.” Thus, I don’t believe you can arbitrarily choose 0x5. See the registry at https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml.
The choice of the ESP next header value was discussed and understood by the WG during the documents development. While some of the original ESP next header values were taken from the IP protocol numbers, there is no requirement that all future ESP next header values must be defined as IP protocol numbers (indeed that would be unnecessarily burdensome). Additionally, this is not the first value to not be defined by that table, so no precedent is being set here.
Nits/editorial comments: General: Insert a hyphen between “congestion” and “controlled” throughout the document. This includes the “non-“ cases as well.
Done.
Insert a hyphen between “AGGFRAG_PAYLOAD” and “enabled” throughout the document. This includes the “non-“ cases as well.
Done.
Change “inner-packet” to “inner packet”. The latter already predominates throughout the document, noting also that “outer packet” never appears in hyphenated form.
Done
Ensure that all the figures have proper captions with numbers. For example, the figures on pages 16, 17, 18, and 19 aren’t labeled. The figure on page 17 could really use a number so that there can be a pointer from page 13 if the figure isn’t moved to page 13, as suggested above.
Done.
Specific: Page 1, Abstract, 1st sentence: change “payload” to “payloads”. Or change “ESP payload” to “an ESP payload”. I can see arguments either way, but the sentence needs one or the other.
Added "s".
Page 5, 1st paragraph, 2nd sentence: delete a duplicated “the” before “tunnel packets”.
Done.
Page 5, Figure 1: change “subtype” to “sub-type” to match usage in the rest of the document.
Done.
Page 6, 3rd paragraph: append a comma after “outer”.
I think this would change the meaning incorrectly. The text is talking about recovering inner packets optimally when experiencing loss of the encapsulating outer packets. It is not talking about ``encapsulating packet loss'' which is what that comma would imply I think.
Page 7, 4th paragraph, last sentence: “one” who? What entity is supposed to be making this choice?
changed "one" to "an implementation or user"
Please 7, 4th paragraph, 1st sentence: append a comma after “note”.
Done.
Page 7, 4th paragraph, 3rd sentence: delete “amount of”.
Done.
Page 7, 5th paragraph, 1st sentence: consider changing “with no gaps” to “consecutively”.
Changed.
Page 8, section 2.2.3.1, 1st paragraph, last sentence: delete the comma after “researching”.
Done.
Page 9, section 2.2.5.3, 1st sentence: append a comma after “default”.
Done.
Page 10, section 2.3, 1st sentence: insert a hyphen between “AGGFRAG_PAYLOAD” and “enabled”.
Done.
Page 10, section 2.4.1, 2nd paragraph, 3rd sentence: append a comma after “case”. Append a period after “etc”.
Done.
Page 10, section 2.4.2, 1st paragraph, 2nd sentence: append a comma after “transport”.
Actually placed a comma before "transport", as "transport overhead" is what is being minimized.
Page 11, 1st partial paragraph: change “packet” to “packets”. Append a closing parenthesis after “congestion”.
Done.
Page 11, 1st full paragraph, 1st sentence: insert a hyphen between “TCP” and “friendly”.
The term "TCP friendly rate control" without hyphen is taken directly from the given reference (RFC5348). So I'd like to leave this as is.
Page 11, 3rd paragraph, 1st sentence: insert a hyphen between “IP-TFS” and “enabled”.
Done.
Page 11, 4th paragraph: append a comma after the closing parenthesis.
Done.
Page 12, 1st partial paragraph, 1st full sentence: delete this sentence as it doesn’t really add anything. But if you are unwilling to delete the sentence, then change “are” to “is”.
Deleted.
Page 12, section 2.5, 1st paragraph: insert a hyphen between “AGGFRAG” and “enabled”.
Done.
Page 12, section 2.5, 2nd paragraph, 3rd sentence: append a comma after “For partial packets”. Delete “the” before “they”.
Done.
Page 12, section 2.5, 2nd paragraph, 4th sentence: insert “the” before “AGGFRAG_PAYLOAD”.
Done.
Page 12, section 2.5, 3rd paragraph, 1st sentence: insert “an” before “in-order”. Page 12, section 2.5 3rd paragraph, 2nd sentence: change “make sure” to “ensure”, if you care. “Tastes light” vs. “less filling”, I suppose. Change “in-order” to “in order”. Insert “a” between “when” and “lost”. Also consider breaking up the sentence into multiple sentences because of its sheer length. For example, the final parenthetical potion could be a whole sentence on its own.
Done, broke up sentence into: The cost of this method is that a lost packet will cause a delay of up to the lost packet timer interval (or the full reorder window if no lost packet timer is used). Additionally, there can be extra burstiness in the output stream. This burstiness can happen when a lost packet is dropped from the re-order window, and the remaining outer packets in the re-order window are immediately processed and sent out back to back.
Page 12, section 3, 2nd sentence: change “it’s” to “its”.
Done.
Page 13, 2nd full paragraph, 2nd sentence: change “locally, subsequent” to “locally. Subsequent”.
Done.
Page 13, 3rd paragraph, 1st sentence: expand the initialism “CC”. I’m assuming “Congestion Control”. It’s not in the RFC Editor’s list of abbreviations.
Done.
Page 13, 4th paragraph, 3rd sentence: change “senders” to “sender’s”.
Done.
Page 16, section 6.1.1, 1st paragraph: change “4 octet” to 4-octet”.
Done.
Page 16, section 6.1.1, “Reserved” definition: delete the comma after “generation”.
Done.
Page 17, section 6.1.2, “Reserved” definition: delete the comma after “generation”.
Done.
Page 17, section 6.1.2, “P” and “E” definitions: insert “that” before “if”.
Done.
Page 18, “Echo Delay” and “Transmit Delay” definitions, 2nd sentence: change “value” to “delay” because by definition, the value cannot be larger than 0x1FFFFF, while the delay can be. Change “it” to “the value”.
Done. RTT as well.
Page 18, “Datablocks” definition: 2nd sentence: change “an” to “a”. Insert a hyphen between “non-IP-TFS” and “enabled”. Consider changing “value” to “field” because DataBlocks isn’t really a value.
Done.
Page 19, section 6.1.3.1, figure: shouldn’t the “TypeOfService” field be the “DiffServ” field instead?
It's still labeled "Type Of Service" in the diagram in RFC 791 (and still referred by the TOS/DiffServ RFCs as the TOS octet), also it's just illustrative here as well.. going to leave this be.
Page 20, section 6.1.4, “0” definition: delete the comma.
Done.
Page 21, 1st paragraph, last sentence: change “it’s” to “its”.
Done (caught earlier)
Page 22, section 8, 1st paragraph, 1st sentence: change “it” to “its”.
(removed ealier)
Page 22, section 8, 3rd paragraph: append a comma after “maintained” and after “would be”.
Done.
Page 24, Appendix A, title: change “Of” to “of”.
Done.
Page 24, Appendix A, 1st paragraph, 1st sentence: append a comma after “Below”.
Done.
Page 25, Figure 3: Explain what the 1500 means.
This actually led to me changing the diagram a bit to be make more sense. I added "Each outer encapsulating ESP payload space is a fixed-size of 1404 octets the first 4 octets of which contains the AGGFRAG header." before the first sentence. Updated the figure to change 1500 to 1404, updated the final packet to be 3000 octets, and updated the offsets to 100, 2000, 600 in the diagram and the following 2 paragraphs.
Page 25, 1st paragraph: change “800 octet” to “800-octet” twice. Make a similar change for “60”, “240”, and “4000”.
Done.
Page 25, 2nd paragraph, 2nd sentence: place “ESP1” in parentheses.
Done.
Page 25, 2nd paragraph, 3rd sentence: change “packet ESP2s” to “packet’s (ESP2)”. Change “60 octet” to “60-octet”.
Done.
Page 25, 2nd paragraph, 4th sentence: place “ESP3” in parentheses. Change “4000 octet” to “4000-octet”. Change “forth” to “fourth”.
Done.
Page 25, 2nd paragraph, 5th sentence: change “packet ESP4s” to “packet’s (ESP4)”. Append a comma after “1400”. Change “4000 octet” to “4000-octet”.
Done.
Page 25, Appendix B, 1st sentence: change “TCP friendly” to “TCP-friendly”.
Done.
Page 25, Appendix B, 2nd sentence: change “TCP friendly” to “TCP-friendly”.
Done.
Page 25, Appendix B, 3rd sentence: append a comma after “[RFC4342])”.
Done.
Page 25, Appendix B, 3rd paragraph: append a comma after “addition”.
Done.
Page 26, 1st paragraph, 2nd sentence: append a comma after “[RFC5348]”.
Done.
Page 26, section C.1, 1st paragraph, 1st sentence: append a comma after “overhead”.
Done.
Page 26, section C.1.1, 1st sentence: append a comma after “For comparison”. Insert “an” before “AGGFRAG”.
Done.
Page 26 section C.1.1, 2nd sentence: append a comma after “Therefore”. Change “fractional” to “fractions”.
Done.
Page 27, 1st formula: change “Paylaod” to “Payload”.
Done.
Page 28, section C.3, 3rd sentence: insert a hyphen between “well” and “understood”.
Done.
Page 28, section C.3.1, 2nd sentence: change the second “and” to “an”. Append a comma after the closing parenthesis.
Done.
Page 28, section C.3.1, 3rd sentence: append a comma after “Additionally”.
Done.
Page 30, 1st paragraph, 1st sentence: append a hyphen after “small”. Insert a hyphen between “medium” and “sized”.
Like this? "for small- to medium-sized packets," Thanks again for the thorough review! Chris.
_______________________________________________ IPsec mailing list IPsec@ietf.org https://www.ietf.org/mailman/listinfo/ipsec
_______________________________________________ IPsec mailing list IPsec@ietf.org https://www.ietf.org/mailman/listinfo/ipsec
