I've now done my AD review for draft-ietf-ipsecme-traffic-visibility-08. I have two substantive comments, and a bunch of minor clarifications/nits. The substantive comments first:
- A question: did the WG discuss the pros and cons of integrity protecting the WESP header? (This does make WESP more complex to implement, and currently the WESP header does not contain any data that would benefit from integrity protection in any way.) - IPv6 requires extension headers to be aligned on 8-octet boundaries, and I believe this requirement applies to ESP, too (see e.g. RFC 4303 Section 2.3, 2nd paragraph). All current ESP specs (all encryption algorithms, UDP encapsulation, etc.) meet the 8-octet alignment requirement -- but adding a new four-octet header there obviously breaks it. Some minor clarifications/editorial nits: - The text currently uses "using ESP-NULL [RFC2410]" and "unencrypted" as synonyms. This was accurate before RFC4543, but is not any more. This needs some clarifying text somewhere (perhaps Section 1). - Section 1 needs a sentence or two motivating the existence of the "E" bit -- currently it comes as a surprise to the reader later. - Section 2/2.1: In Figures 1, 2, and 3, the bit numbers should be shifted one character to the right. - Section 2: In some parts of the text, the last 8 bits of the WESP header are called "Flags"; in other parts, the last 5 bits are called "Flags". I'd suggest changing e.g. Figure 2 so that last 5 bits are called "Rsvd" or something. - Section 2: "The bits are defined LSB first, so bit 0 would be the least significant bit of the flags octet." This doesn't match the bit numbering in Figure 2 (where bit 0 is the most significant bit). However, the bit numbers are not really used anywhere, so I would just suggest deleting this sentence. - Section 2: It would be helpful if the text explicitly said that HdrLen values less than 12 are invalid (and probably HdrLen values that are not multiple of 4 are invalid, and multiple of 8 for IPv6 case). - Section 2: the text about TrailerLen is a bit unclearly written -- the offset from the end of the packet to the last byte of the payload would be a negative number. I'd suggest phrasing this something like the "The number of bytes following the payload data in the packet, in octets: i.e. the total length of TFC Padding (normally not present for unencrypted packets), ESP Trailer (Padding, Pad Length, Next Header), and ESP ICV." - Section 2: "the packet must be dropped" -> "the packet MUST be dropped" - The figures in 2.2.1 and 2.2.2 are very confusing, since they suggest WESP could be applied as a separate step after ESP processing (that was possible in some earlier versions of the draft, but not any more since ICV covers the WESP header). Since they don't really present much new compared to the figures in RFC 4303 Section 3.1.1/3.1.2, perhaps they could be omitted? Or if we want to keep them, they probably should show the packet before applying ESP? - Section 3: s/IPSec/IPsec/ - Section 4: this section is missing the allocation of SPI value 2 to indicate WESP from the "SPI Values" registry. - Section 4 should say that for the WESP Version Number, the unassigned values are 1, 2, and 3. - Section 6: [RFC4306], [RFC3948], and [RFC5226] should be normative references, not informative. Best regards, Pasi _______________________________________________ IPsec mailing list IPsec@ietf.org https://www.ietf.org/mailman/listinfo/ipsec
