Hi, thanks for the feedback! Please find our comments inline.
On Fri, Jan 03, 2025 at 09:26:29AM +0000, Panwei (William) wrote: > Hi, > > I’ve read the current v01. I know this is still in the early version, and I’m > happy to see it evolving and contribute to it. Here below are my initial > comments. > > 1. Under Figure 1 of Section 2.1, in the descriptions of all headers, the > Base Header is the only place where middlebox processing requirements are > mentioned. To keep consistency, it’s better to mention the middlebox > processing requirements for other Headers as well (or not mention the > middlebox processing requirements for the Base Header either). > We added some text to make that more clear. > 2. In the ‘Fixed Base Header’, after the ‘Version’ field is the 1-bit field > of ‘Packet Format”. This ‘Packet Format’ field can have the value of 0 and 1. > But in Figure 2, this field is written with a fixed value 1. This may cause > confusion, and I suggest changing it to “P” or “F” in the figure. We changed the 1-bit Packet Format field into a 6-bit flags field and updated all the figures accordinly. > > 3. In the Peer Header of Section 2.3, it’s said that the Peer Header contains > an optional ‘Sequence Number’ field and an optional ‘Initialization Vector’ > field. If both fields are optional, is this Peer Header optional too? What if > neither of these fields are carried? If we don't do replay protection and the used cipher does not need an IV, we don't need the peer header. So yes, it is optional. > In Section 2.3.1, it isn’t said in what case the Sequence Number field can be > omitted. So my understanding is that the Sequence Number field is mandatory > but it can be used to encode the IV, i.e., the IV field is omitted and the > value of Sequence Number field is used as the IV. Good point, the document is still a bit vague here. The Sequence Number field is mandatory for ESP, while the IV is not. I think that's why it is used to carry an implicit IV for counter mode algorithms. > 4. In Section 2.3.1, there seems to be some ambiguity in that last sentence: > Unless any future Option > defining this for a multi-sender SA, the anti-replay features of ESP > > are not available. > It seems like the anti-replay feature will always be unavailable in any case > before the Option for the multi-sender SA is defined. But, in fact, it’s just > the case of multi-sender SA that the anti-replay feature is not available. > (Also one nit here: s/ESP/EESP) We plan to add a sender ID option, this would make the anti-replay feature available to multi-sender SAS. So we likely can remove that text in the next version. > > 5. In Section 2.3.2, the suggested changes to the first sentence is as > follows, to avoid ambiguity: > OLD: > > If the algorithm used to encrypt the payload requires cryptographic > > synchronization data, e.g., an Initialization Vector (IV), then this > > data is carried explicitly in front of the encrypted part of the > > packet in the 'Peer Header'. > NEW: > > If the algorithm used to encrypt the payload requires cryptographic > > synchronization data, e.g., an Initialization Vector (IV), then this > > data is carried explicitly in the 'Peer Header' which is in front of > > the encrypted part of the packet. Changed. > > 6. Regarding the Payload Info Header, I’m happy to see it being moved to the > front of Payload Data. > I noticed that this header is designed to be optional in the tunnel mode, and > it’s called Optimized Packet Format. > The first thing I want to suggest is that the default situation in the tunnel > mode should be the Full Packet Format, and the Optimized Packet Format should > only be an optional choice. Using the Optimized Packet Format should be > explicitly negotiated by the two peers via the control channel. > Implementations can choose to only support the Full Packet Format and not to > support the Optimized Packet Format. Because this optimization may not always > be cost-effective when considering the cost of its implementation, especially > when implementing in the hardware. OK, changed. > The second thing is that if we want to minimize the overhead of the EESP > header, should we consider the optimization thoroughly? Not only to omit the > Payload Info Header, but also to consider what other fields can be optimized. Which other fields could be optimized? > The third thing is that I suggest moving Section 2.8 to Section 2.1. Putting > the whole packet format in the front seems will be clearer and easier to > understand, just like the style of RFC 4303. And the first paragraph of > Section 2.8, i.e., the corresponding processing logic when Payload Info > Header is omitted, is better to be moved to Section 2.4. > We renamed Section 2 to 'Enhanced Encapsulating Security Payload Packet Format' so that Section 2.8 belongs to this. We mentioned the processing logic in Section 2.4 but left the exact definition in Section 2.8. > 7. In the beginnings of Section 2.5 and Section 2.6, it’s said that Payload > Data and Padding is from ESP but adjusted to apply to EESP. It’s better to > clearly clarify what the adjustments are. > For example, one adjustment of Payload Data is that the ‘IV’, ‘Next Header’, > and ‘Pad Length’ fields in ESP’s Payload Data is moved out to other headers. > In the new version of the draft we have even more sections like these. It is not a new version of ESP, so maybe we should just mention in the introduction that we took text from ESP (with adjustments). > 8. In Section 2.5, the last paragraph (copied below) is the same as the > alignment requirement written in ESP. But I think this paragraph can be > omitted in EESP. > > Note that the beginning of the next layer protocol header MUST be > > aligned relative to the beginning of the EESP header as follows. For > > IPv4, this alignment is a multiple of 4 bytes. For IPv6, the > > alignment is a multiple of 8 bytes. > In ESP, there is an IV field in front of the actual payload data. So I think > the requirement above is actually for the IV field, i.e., the IV field should > be a multiple of 4 / 8 bytes. > But the Payload Data in EESP doesn’t have the IV field, and it begins with > the “next layer protocol header”. So the beginning of the next layer protocol > header is the beginning of Payload Data, and it will naturally fit the > alignment requirement. > I think the alignment of the next layer protocol header depends on the final EESP header layout with variable length options. So not sure if that text should be removed. > 9. In Section 2.6, suggested changes: > OLD: > > * If an encryption algorithm is employed that requires the plaintext > > to be a multiple of some number of bytes, e.g., the block size of > > a block cipher, the Padding field is used to fill the plaintext > > (consisting of the Payload Data, Padding, Pad Length, and Next > > Header fields) to the size required by the algorithm. > NEW: > > * If an encryption algorithm is employed that requires the plaintext > > to be a multiple of some number of bytes, e.g., the block size of > > a block cipher, the Padding field is used to fill the plaintext > > (consisting of the Payload Data, Padding, and Payload Info Header) > > to the size required by the algorithm. Yes, changed. > > 10. In Section 2.6, in the following sentence: > > For the purposes of ensuring that the ICV is aligned on a 4-byte > > boundary (second bullet above), the padding computation applies to > > the Payload Data inclusive of the Payload Info Header, if present. > Because the Payload Info Header is already 4 bytes, so would the padding > computation applying to the Payload Data itself be enough to ensure the ICV > is aligned on a 4-byte boundary? Yes, I guess we can remove 'inclusive of the Payload Info Header, if present'. > > 11. In Section 2.6, in the following sentence: > > If a combined mode algorithm is used, any replicated data and ICV- > > equivalent data are included in the Payload Data covered by the > > padding computation. > Would it be better to add some descriptions to “combined mode algorithm” and > “replicated data” for the readers who are not familiar with ESP? > Hm, that was taken from ESP witout too much thinking :) I do not know of any combined mode algorithm that replicates data, so I think we can remove this. > 12. In Section 2.6, in the last paragraph, there is a nit to change ESP to > EESP. Fixed. > 13. In Table 1 in Section 2.8, the “Required” option for Payload can be “D”, > which means dummy packet in ESP. But EESP doesn’t support TFC like ESP does. > So maybe “D” (Dummy packet) can be deleted here. Yes, deleted. > 14. In Section 4.1, it’s said that the SAD and SPD will change frequently in > the high data rate scenario. > > A high data rate, combined with frequent changes in the SAD and SPD, > > can slow down the system. As the data flow increases, adding and > > removing entries in the control plane creates locking issues and > > contention in both software and hardware. > I’m not quite clear of the issue described here. Why the SAD and SPD will > change frequently? Is it because the high traffic volume will lead to > frequent SA rekeys and therefore SAD and SPD will change frequently? This was added to cover the PSP datacenter usecase. We will add some text to make that more clear. This also might be moved to another draft specifying the PSP usecase, as this does not really match RFC 4301 arcitecture. We will publish a new version with your feedback incorporated soon. Thanks again for the review, Steffen _______________________________________________ IPsec mailing list -- ipsec@ietf.org To unsubscribe send an email to ipsec-le...@ietf.org
