[IPsec] AD review comments for draft-ietf-ipsecme-traffic-visibility

[<[e-mail]>]

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