Hi Paul, Thank you very much for the detailed review.
There are some points that are common to the udp spec. Will discuss those in separate threads. Please see inline. Cheers, Med De : tsvwg <tsvwg-boun...@ietf.org> De la part de Aitken, Paul Envoyé : vendredi 19 janvier 2024 10:52 À : Joe Clarke (jclarke) <jclarke=40cisco....@dmarc.ietf.org>; opsawg@ietf.org Cc : t...@ietf.org; ts...@ietf.org; 6...@ietf.org; ip...@ietf.org Objet : Re: [tsvwg] [IPFIX] WG LC: IPFIX documents https://datatracker.ietf.org/doc/draft-ietf-opsawg-ipfix-tcpo-v6eh Essentially the middle of this document is missing: a summary of issues is given and new IEs are proposed as a solution. But the issues are not developed or explained. 1.1. Issues with ipv6ExtensionHeaders Information Element * Specify a means to export the order and the number of occurences of a given extension header. Typo, "occurrences" [Med] Fixed. The specification of ipv6ExtensionHeaders IPFIX IE does not: Consider including the IE number for clarity, eg "The specification of ipv6ExtensionHeaders IPFIX IE (#64) does not:" Same for 1.2 (tcpOptions, #209). [Med] Sure. Went for (64) and (209). * Describe how any observed TCP option in a Flow can be exported using IPFIX. Only TCP options having a kind =< 63 can be exported in a tcpOptions IPFIX IE. Conventionally "<=" is used. [Med] OK 3. Information Elements for IPv6 Extension Headers The definition of the ipv6ExtensionHeaders IE is updated in Section 4.1 of [I-D.ietf-opsawg-ipfix-fixes] to address some of the issues listed in Section 1.1. For clarity say, "Section 1.1 above", else it seems to refer to 1.1 of ietf-opsawg-ipfix-fixes. [Med] Added "of this document". The definition of the ipv6ExtensionHeaders IE is updated in Section 4.1 of [I-D.ietf-opsawg-ipfix-fixes] to address some of the issues listed in Section 1.1. Because some of these limitations cannot be addressed by simple updates to ipv6ExtensionHeaders, this section specifies a set of new IEs to address all the ipv6ExtensionHeaders IE limitations. Please expand on this and explain "some of": which issues are addressed and which are not? [Med] This is already covered by this pointer: "Refer also to {{Section 4.1.1 of ?I-D.ietf-opsawg-ipfix-fixes}} for more details.". We do already have two paragraphs there with these details. No need to be redundant here. Why does the document identify issues without proposing solutions to them all? How and when will those other issues be fixed? [Med] The new IEs in this I-D fix all the issues. 3.1 - 3.4 These definitions should be in the IANA section. Section 3 should explain the problems and how they are solved, rather than jumping straight to the IEs as a fait-accompli. [Med] The issues and rationale are already provided in previous sections, hence the current structure. 3.1. ipv6ExtensionHeadersFull Information Element Please include some discussion and justification for creating this new element rather than updating the existing ipv6ExtensionHeaders IE (#64). If the existing IE cannot be updated then explain backwards compatibility between the proposed and existing IEs and deprecate the existing IE. [Med] This is explained in the other spec and have a clear statement that the new IE MUST be used when the range is exceeded, etc. The information is encoded in a set of bit fields. It sounds like a singular bit field rather than a set of bit fields. [Med] Hmm, this is a well used terminology in IPFIX IE description. Please check the IANA registry. In doing so, few octets will be needed to encode common IPv6 extension headers when observed in a Flow. This justification should be part of the discussion I mentioned above, and not part of the definition. The "No Next Header" (59) value is used Add an xref for the 59. [Med] Added a pointer to {{Section 4.7 of !RFC8200}}. This Information Element SHOULD NOT be exported if ipv6ExtensionHeaderCount Information Element is also present. Explain why not. This explanation should be in the text rather than in the definition. This should possibly be in section 5.1 where the 3rd paragraph discusses similar limitations. [Med] Moved to 5.1. The reason is that the content of the IEs is redundant. Will see how to convey that in the text. Abstract Data Type: unsigned256 No, it's a bitfield. unsigned256 represents an integer, which this is not. [Med] Will reply to this one in a separate message as this is also discussed in the udp spec. 3.2. ipv6ExtensionHeaderCount Information Element Description: As per Section 4.1 of [RFC8200], IPv6 nodes must accept and attempt to process extension headers in occurring any number Typo, "in". [Med] Fixed. MSB LSB 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 ... +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | EH Type#1 | Count |...| EH Type#n | Count | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ Abstract Data Type: unsigned64 This is neither a simple IE, nor an unsigned64. It's a structured data type, ie a variable-length array of { type, count } tuples. [Med] Do we need to define a new type for this? 3.3. ipv6ExtensionHeadersLimit Information Element What is to be understood when this IE is not included in the IPFIX data? Is it the same as "true"; the same as "false"; or something else? [Med] No specific meaning is assumed, especially that we leave it to the implem to decide its presence: = The ipv6ExtensionHeadersLimit IE ({{sec-v6limit}}) may or may not be present when the ipv6ExtensionHeadersChainLength IE ({{sec-v6aggr}}) is also present as these IEs are targeting distinct properties of extension headers handling. == 3.4. ipv6ExtensionHeadersChainLength Information Element See [RFC9098] for an overview of operational implications of IPv6 packets with extension headerss. Typo, "headers" [Med] OK 4. Information Elements for TCP Options Again this jumps directly from the introduction to the solution without any discussion or explanation. [Med] Idem as per the EH justification provided in my reply above. The definition of the tcpOptions IE is updated in [I-D.ietf-opsawg-ipfix-fixes] to address some of the issues listed in Section 1.2. Because some of these limitations cannot be addressed by simple updates to tcpOptions, this section specifies a set of new IEs to address all the tcpOptions IE limitations. Please expand on this and explain "some of": which issues are addressed and which are not? Why does the document identify issues without proposing solutions to them all? How and when will those other issues be fixed? [Med] This is already listed in the issues. The encoding cannot be optimized in a backward compatible manner. 4.1. tcpOptionsFull Information Element Please include some discussion and justification for creating this new element rather than updating the existing tcpOptions IE (#209). If the existing IE cannot be updated then explain backwards compatibility between the proposed and existing IEs and deprecate the existing IE. [Med] I think this is already explained in the simple-fix I-D. The information is encoded in a set of bit fields. Again, this sounds like a singular bit field rather than a set of bit fields. [Med] Idem as above. Abstract Data Type: unsigned256 No, it's a bitfield. [Med] Idem as per the UDP spec. Will discuss that one in a separate note. 4.2. tcpSharedOptionExID16 Information Element Description: Any observed 2-byte Experiments IDs (ExIDs) in a shared TCP option (Kind=253 or 254) in a Flow. The information is encoded in a set of 16-bit fields. Each 16-bit field carries an observed 2-byte ExID in a shared option. I did not understand what this measures, nor how to encode it, until I reviewed section 6.2. As this is an array of 16-bit values, the "octetArray" type is somewhat misleading as it's really a hextetArray. [Med] Will discuss this in a separate note as this is also applicable to the udp spec. 4.3. tcpSharedOptionExID32 Information Element Description: Any observed 4-byte Experiments IDs (ExIDs) in a shared TCP option (Kind=253 or 254) in a Flow. The information is encoded in a set of 32-bit fields. Each 32-bit field carries an observed 4-byte ExID in a shared option. I do not understand what this measures, nor how to encode it, until later. Again, the "octetArray" type is somewhat misleading as it's really a 32tetArray. [Med] I'm not sure this is problematic given that the definition of octetArray indicates explicitly the following: The octetArray data type has no encoding rules; it represents a raw array of zero or more octets, with the interpretation of the octets defined in the Information Element definition. 5.1. IPv6 Extension Headers If an implementation determines that it includes an extension header that it does not support, Consider "that the observed packet stream includes". [Med] Updated to « an observed packet of a Flow". 5.2. TCP Options If a TCP Flow contains packets with a mix of 2-byte and 4-byte Experiment IDs, the same Template Record is used with both tcpSharedOptionExID16 and tcpSharedOptionExID32 IEs. Consider: If a TCP Flow contains packets with a mix of 2-byte and 4-byte Experiment IDs, then a single Template Record SHOULD be used with both tcpSharedOptionExID16 and tcpSharedOptionExID32 IEs. [Med] This is more about usage than interop. I still prefer to not use the normative language here. 6. Examples This section provides few examples to illustrate the use of some IEs defined in the document. Typos: "provides a few"; "in this document". [Med] OK 6.1. IPv6 Extension Headers Concretely, the ipv6ExtensionHeadersFull IE will be set to 35. It would be more intuitive to say "0x23" as that matches the bit pattern. Ideally all 8 LS bits would be shown in the figure. [Med] OK 6.2. TCP Options Given TCP kind allocation practices and the option mapping defined in Section 4.1, fewer octers are likely to be used for Flows with common TCP options. Typo, "octets" [Med] Fixed. Concretely, the tcpOptionsFull IE will be set to 13. It would be more intuitive to say "0x0D" as that matches the bit pattern. Ideally all 8 LS bits would be shown in the figure. [Med] OK 1. The tcpSharedOptionExID16 IE set to 55067982 (i.e., 0x348454E) to report observed 2-byte ExIDs: HOST_ID and TCP-ENO ExIDs. 2. The tcpSharedOptionExID32 IE set to 3805594585 (i.e., 0xE2D4C3D9) to report the only observed 4-byte ExID. Remove the base 10 numbers. They're irrelevant and confusing. [Med] Will see how to make all the examples consistent. 7. Security Considerations The ipv6ExtensionHeadersChainLength could be used to determine hardware capabilities and limitations, and possibly even to identify the hardware through which the traffic is flowing. [Med] Not sure how this can be used to identify the hardware. It is to be hoped that anyone capable of enabling IPFIX export on a device would already know this information. I can't think of any other IEs whose specific purpose is to identify hardware limitations, so this should be called out in this section. [Med] That's not new per se as this is a variation of this text of Section 8 of 7012: == The IPFIX information model itself does not directly introduce security issues. Rather, it defines a set of attributes that may, for privacy or business issues, be considered sensitive information. For example, exporting values of header fields may make attacks possible for the receiver of this information; this would otherwise only be possible for direct observers of the reported Flows along the data path. == 8.2. New IPFIX Information Element Data Type The type "unsigned256" represents a non-negative integer value in the range of '0' to '2^256 - 1'. The IEs which use this new type are not exporting integers, but flags - so it would make more sense to create a new bitfield type. [Med] Covered above. P. On 18/12/23 19:22, Joe Clarke (jclarke) wrote: We'd like to kick off a [rather extended] WG LC on the three IPFIX-related "fixes" documents we have in the hopper. We've already requested some directorate reviews for these, and the authors feel they have stabilized well. Please review: 1. https://datatracker.ietf.org/doc/draft-ietf-opsawg-ipfix-tcpo-v6eh/ [datatracker.ietf.org]<https://urldefense.com/v3/__https://datatracker.ietf.org/doc/draft-ietf-opsawg-ipfix-tcpo-v6eh/__;!!OSsGDw!I6YR3K0-150A1C0ElDbVuvpjkK-Ylkh69dILZCWJl3lPScov6t5X2FzyrjqruiynmXjCnHZBzRn1Gy8IthVfCMEo_jvU$> 2. https://datatracker.ietf.org/doc/draft-ietf-opsawg-tsvwg-udp-ipfix/ [datatracker.ietf.org]<https://urldefense.com/v3/__https://datatracker.ietf.org/doc/draft-ietf-opsawg-tsvwg-udp-ipfix/__;!!OSsGDw!I6YR3K0-150A1C0ElDbVuvpjkK-Ylkh69dILZCWJl3lPScov6t5X2FzyrjqruiynmXjCnHZBzRn1Gy8IthVfCG_d35-j$> 3. https://datatracker.ietf.org/doc/draft-ietf-opsawg-ipfix-fixes/ [datatracker.ietf.org]<https://urldefense.com/v3/__https://datatracker.ietf.org/doc/draft-ietf-opsawg-ipfix-fixes/__;!!OSsGDw!I6YR3K0-150A1C0ElDbVuvpjkK-Ylkh69dILZCWJl3lPScov6t5X2FzyrjqruiynmXjCnHZBzRn1Gy8IthVfCAzlAEMT$> And comment as to whether or not you feel they are in the right shape to progress to the IESG. We will run this LC until Jan 8 given that the holidays means some people will not be around. Also note that an IPR poll was conducted prior to adoption and no known IPR has been disclosed. Thanks. Joe _______________________________________________ IPFIX mailing list ip...@ietf.org<mailto:ip...@ietf.org> https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/ipfix__;!!OSsGDw!I6YR3K0-150A1C0ElDbVuvpjkK-Ylkh69dILZCWJl3lPScov6t5X2FzyrjqruiynmXjCnHZBzRn1Gy8IthVfCO14NBgv$ [ietf[.]org] ____________________________________________________________________________________________________________ Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration, Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci. This message and its attachments may contain confidential or privileged information that may be protected by law; they should not be distributed, used or copied without authorisation. If you have received this email in error, please notify the sender and delete this message and its attachments. As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified. Thank you.
_______________________________________________ OPSAWG mailing list OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg
