Reviewer: Peter Yee Review result: Almost Ready 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://wiki.ietf.org/en/group/gen/GenArtFAQ>. Document: draft-ietf-ippm-encrypted-pdmv2-09 Reviewer: Peter Yee Review Date: 2024-10-28 IETF LC End Date: 2024-10-15 IESG Telechat date: Not scheduled for a telechat Summary: This specification provides security to PDM (RFC 8250) by defining a version that includes the ability to pass encryption-protected data. While the document is not difficult to read, it feels like important details are missing that should really be filled out prior to it being published. My apologies for not having gotten to this review earlier. [Almost ready] Major issues: None Minor issues: General: Sometimes PDM is referred to when it’s PDMv2 that’s really meant. This leads to confusion for the naïve reader. Some examples include: o Section 4 definitions of client and server, which are being listed as part of the setup for then performing encryption. o Section 5 where a PDM data flow is discussed by it must be a PDMv2 data flow given the preceding text. o Section 5.1, last paragraph. That one ought to be obvious because PDM does not have an Epoch. o Section 6, 3rd paragraph. o Section 7.3, most uses in this section. Specific: Page 4, 2nd paragraph. It might be useful to note here that PDM and PDMv2 usage cannot overlap because they use the same option type. I’m not even sure that the receiver could differentiate them upon receipt. The document is a little light on where PDMv2 stands vs. PDM. This document is not listed as obsoleting RFC 8250, nor is it listed as updating RFC 8250. Some clarification about their standing in the same environment would be welcome. Page 6, section 3.3, 1st paragraph, 2nd sentence: the difference between unencrypted PDMv2 and encrypted PDMv2 is determined from the Options Length value. But at the bottom of page 6, unencrypted PDM has a length of 0x22 while encrypted PDMv2 at the top of page 7 also have a length of 0x22. How is that determination made? Page 6, section 3.3, header representation diagrams: in the encrypted version, Encrypted PDM Data is 16 bytes. This replaces the existing 16 bytes in the unencrypted version. Presumably what’s contained in the Encrypted PDM Data field are the replaced fields (PSN Last Received, …, Delta Time Last Received), in their encrypted form. First, it would be good to spell that out, perhaps by defining the Encrypted PDM Data field after the definitions of the other fields at the end of section 3.3. Second, where does the AEAD tag go? HPKE generally emits it at the end of ciphertext, as far as I can determine. That means that the 16 bytes of the encryption-replaced fields cannot be represented by 16 bytes in encrypted PDMv2 header. I may have misunderstood your usage of HPKE, however. Or I may have just misunderstood HPKE! Page 7, Packet Sequence Number This Packet (PSNTP) definition, 3rd paragraph. You’ve specified a 44-bit nonce. Is 44 bits sufficient for the nonce? Given that HPKE handles nonces for its AEAD algorithm internally, I'm not sure what this nonce does or how it’s applied. By itself, this nonce is not sufficient for AES-GCM-128 (specified later on in the document), which typically takes a 96-bit nonce. Page 10, 2nd and 3rd bullet items: the use of these values is not clear. Are they being passed as the info parameter to the KDF? If so, how are they arranged? Are they concatenated somehow? Padded? Page 13, 1st paragraph: What happens when a different encryption mechanism is chosen? Certainly, section 5.1 is written as though HPKE is the choice, not some recommendation. Page 18, item b: How is this different than a)? With my disagreement with even having c) [see next comment], I think this list can be collapsed to a single paragraph that extols the virtues of encryption for protecting the privacy of the data. But otherwise, it’s really taking something small and expending a lot of words on it. Page 18, item c: I do not consider integrity a privacy service and believe this paragraph should be struck, while a) and b) could be combined. Page 20, Appendix A: I didn't find this appendix particularly informative. It is not the purported sample implementation of registration that title would leave you to believe as its much too high level for that. It's a very generic description of minimally described steps to use HPKE, but it provides such shallow detail as to be not terribly applicable to a PDMv2 implementation specifically. It would be much better, in my opinion, to make HPKE mandatory and then provide concrete steps in the document on how it is used, what fields feed into which inputs to HPKE, and how the HPKE outputs are placed into the PDMv2 header. This lack of overall detail is why I have submitted my review marked as “Almost ready”. It’s not badly broken, but it doesn’t have enough detail to implement it either, especially for something that’s on the Standards Track. Nits/editorial comments: General: Some of your lettered lists uses the form a), b), while others use a., b., etc. for no obvious reason. It would be nice to be consistent. Perhaps that’s just my little mind’s hobgoblin. Specific: Page 1, Abstract, 1st sentence: change “RFC8250” to “RFC 8250”, as given in the RFC Editor’s style guide. Page 1, Abstract, 3rd sentence: append a comma after “PDMv2”. Page 1, Abstract, 4th sentence: change “which” to “that”. Page 3, section 1, 1st paragraph, 1st sentence: change “which” to “that”. Delete “the” before “metrics”. Page 4, 2nd paragraph: change “which” to “that”. Page 4, 3rd paragraph: change “RFC8250” to “RFC 8250”. Page 5, 1st paragraph after the 5-tuple definition: insert “the” before “Global Pointer”. Change “type” to “types”. Page 5, 3rd paragraph after the 5-tuple definition: for consistency, change “initialised” to “initialized”. All other usage in the document appears to use American spellings of words with similar endings. Page 6, section 3.3, representation of the encrypted PDMv2 header: The base-10 value line (0 … 1 … 2 … 3) is duplicated. Delete one line. Page 7, Version Number definition: perhaps you might want to explain why it starts at 0x2? I’m assuming it’s because RFC 8250 is considered version 1, but it doesn’t even have a Version Number field. Page 8, section 4, client definition: change “which” to “that”. Page 8, section 4, server definition: change “which” to “that”. Page 9, 2nd bullet point, 2nd sentence: insert “a” before “client-server”. Page 9, 2nd bullet point, 3rd sentence: change “out-of band” to “out-of-band”. Page 9, 4th bullet point: the letter A is duplicated at beginning of the sentence. Delete one. Page 9, section 5.1, 2nd paragraph: insert “the” before “Hybrid”. Change the quoted “SharedSecret” to something like “a shared secret value (SharedSecret)”. Page 10, 1st paragraph: insert “the” before “HPKE”. Page 10, 2nd bullet point: Please use the field names as given in section 3.2. In particular, this matters a little because here you're using "source" (Src) instead of "sender". Page 11, section 5.2.1: change the 1st “which” to “that”. Append a comma after “Option Type”. Change “RFC8250” to “RFC 8250” and then append a comma after that. Change “RFC8200” to “RFC 8200”. Page 11, section 5.2.1, 2nd paragraph: change “identifiers” to “identifier”. Page 11, section 5.2.2., 1st paragraph, 1st sentence: change “which” to “that”. Change “continue” to “continues”. Page 11, section 6, 3rd paragraph, 1st sentence: change “is maintained” to “are maintained”. Page 11, section 6, 3rd paragraph, 2nd sentence: there is no section 5.4, so this pointer needs to be fixed. Page 11, section 6, item a): change “For” to “for”. Append a comma after “example”. Page 12, section 6.4 title: I’m not sure the title is appropriate. This is not a discussion of a single cryptographic algorithm. HPKE uses multiple cryptographic algorithms. RFC 9180 is described as a “scheme”. Given the paragraphs within the section, I’d consider calling this “Cryptographic Mechanism”. Page 12, section 6.4, 1st paragraph: I don’t feel like this paragraph is necessary. It’s sort of informational but gets sort of gets brushed aside in the last sentence. If you want to keep the paragraph, then consider the next several comments. Page 12, section 6.4, 1st paragraph, 1st sentence: change “over” to “than”. Append “key” after “asymmetric”. Page 12, section 6.4, 1st paragraph, 2nd sentence: change “participating” to “deployed”. Page 12, section 6.4, 1st paragraph, 3rd sentence: I’m not even what yields are being discussed here. I might just drop “by joining both yields” and still have a valid sentence. Page 12, section 6.4, 1st paragraph, 4th sentence: change “Hybrid” to “hybrid”. Change “schemes” to “scheme”. (It’s one flexible scheme.) Page 13, 1st paragraph, last sentence: change “default encryption algorithm for HPKE AEAD as AES-128-GCM” to “AES-128-GCM as the default encryption algorithm for HPKE AEAD”. Page 13, section 7, 2nd paragraph: change “RFC3552” to “RFC 3552”. Page 13, section 7.1.1, item a, 2nd sentence: delete “802.3” – that’s what Ethernet is. Also delete “FDDI” – it’s really quite obsolete at this point as far as I can determine. Ethernet seems to have eaten its lunch. Page 13, section 7.1.1, item a, last sentence: change “PDM” to “PDMv2”. This really goes with the issue I raised near the beginning of this review, but I’m too lazy to scroll back there to add it. Page 14, item b, 1st sentence: change “they” to “it”. Page 14, item b, 2nd sentence: change “the victim” to “either victim”. I say that because both legitimate parties using PDM(v2) are victimized. Page 14, item c, 3rd sentence: change “their” to “its”. Page 14, item d, 1st sentence: insert “IEEE” before “802.11”. And if you want to be excruciatingly correct, IEEE would be delighted if you referred to it as “IEEE Std 802.11”. I suppose the year of the standard revision can be left off with any real harm. Page 14, 2nd paragraph after lettered items, 1st sentence: change “RFC3552” to “RFC 3552”. Page 14, section 7.1.2, 2nd sentence: append a comma after “PSNTP”. Insert “the” before “HPKE”. And I don’t suppose the pedantic “is used as a part of the nonce” is truly necessary for those who remembered that the nonce also contains the value of Epoch. Page 15, 2nd paragraph, 2nd sentence: change “IPSec” to “IPsec”. Page 17, section 7.2, 1st sentence: change “applies” to “apply”. Page 18, item a, last sentence: this sentence is mangled and needs to be fixed. Page 18, 1st paragraph after lettered items, 2nd sentence: the use of metadata here is rather unfortunate. We’re already talking about metadata as being what’s conveyed in PDMv2 headers. Now we’re talking about metadata being what’s outside of those headers. This may leave the reader confused. -- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
