Last Call Review of draft-ietf-netmod-acl-extensions-11
review-ietf-netmod-acl-extensions-11-tsvart-lc-black-2024-11-04-00
Request | Review of | draft-ietf-netmod-acl-extensions-11 |
---|---|---|
Requested revision | 11 (document currently at 12) | |
Type | Last Call Review | |
Team | Transport Area Review Team (tsvart) | |
Deadline | 2024-11-15 | |
Requested | 2024-10-21 | |
Requested by | Mahesh Jethanandani | |
Authors | Oscar Gonzalez de Dios , Samier Barguil , Mohamed Boucadair , Qin Wu | |
I-D last updated | 2024-11-04 | |
Completed reviews |
Yangdoctors Early review of -04
by Mahesh Jethanandani
(diff)
Tsvart Last Call review of -11 by David L. Black (diff) Intdir Last Call review of -11 by Tim Wicinski (diff) |
|
Comments |
The draft is adding support for IPv6 Extension Headers, TCP Flags, fragment handling and payload filtering. I am hoping that the reviews can zoom in one some of these areas to make sure the document is not missing anything. |
|
Assignment | Reviewer | David L. Black |
State | Completed | |
Request | Last Call review on draft-ietf-netmod-acl-extensions by Transport Area Review Team Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/tsv-art/K47FH6XQH5v5wLw99aa0YSVLGiY | |
Reviewed revision | 11 (document currently at 12) | |
Result | Ready w/issues | |
Completed | 2024-11-04 |
review-ietf-netmod-acl-extensions-11-tsvart-lc-black-2024-11-04-00
This document has been reviewed as part of the transport area review team's ongoing effort to review key IETF documents. These comments were written primarily for the transport area directors, but are copied to the document's authors and WG to allow them to address any issues raised and also to the IETF discussion list for information. When done at the time of IETF Last Call, the authors should consider this review as part of the last-call comments they receive. Please always CC tsv-art@ietf.org if you reply to or forward this review. This draft extends the ACL model defined in RFC 8341 to additional areas of functionality. This transport area review focuses on the TCP flags and payload extensions. I found a minor issue in each of these two areas. [A] Minor technical issue: TCP flags bitmask Under "grouping tcp-flags", I see: case builtin { leaf bitmask { type uint16; description "The bitmask matches the last 4 bits of byte 12 and 13 of the TCP header. For clarity, the 4 bits of byte 12 corresponding to the TCP data offset field are not included in any matching."; reference "RFC 9293: Transmission Control Protocol (TCP), Section 3.1"; } } That's peculiar. Byte 12 in the TCP header is not involved, so why is it included? Is that because uint16 the smallest type that can be used? Also, why is the bitmask limited to 4 bits when there are 8 flag bits that are defined for the other representation case ("case explicit" references "identity tcp-flag")? [B] Minor technical issue: payload encryption. The security considerations ought to point out that payload match won't work on encrypted payloads, even though that's relatively obvious. A sentence or two to that effect ought to suffice, perhaps complemented by referencing the extensive discussion of transport header confidentiality implications in RFC 9065 (whether to add a reference to RFC 9065 is left to the authors' discretion). Editorial - TCP flags: It would be helpful for the description to include a list of which flag is in which position in the bitmask (see above). Editorial - Payload: The use of the word "payload" by itself as a type initially confused me, e.g., in: augment "/acl:acls/acl:acl/acl:aces/acl:ace" + "/acl:matches" { description "Adds a match type based on the payload."; choice payload { description "Matches based upon a prefix pattern."; container prefix-pattern { if-feature "match-on-payload"; description "Indicates the rule to perform the payload-based match."; uses payload; } } I initially read "payload" as an object representing the payload, whereas it actually refers to an object that contains rules for matching on the payload (which is the only possibility in this context). This would be somewhat clearer if "payload" were changed to "payload-match" here and elsewhere in the draft.