Hi David, Thanks for the review. The diff to track the changes can be seen here: https://author-tools.ietf.org/api/iddiff?url_1=https://netmod-wg.github.io/enhanced-acl-netmod/draft-ietf-netmod-acl-extensions.txt&url_2=https://netmod-wg.github.io/enhanced-acl-netmod/TSV-review/draft-ietf-netmod-acl-extensions.txt. Please see inline. Cheers, Med > -----Message d'origine----- > De : David Black via Datatracker <noreply@xxxxxxxx> > Envoyé : lundi 4 novembre 2024 19:01 > À : tsv-art@xxxxxxxx > Cc : draft-ietf-netmod-acl-extensions.all@xxxxxxxx; last- > call@xxxxxxxx; netmod@xxxxxxxx; david.black@xxxxxxxx > Objet : Tsvart last call review of draft-ietf-netmod-acl- > extensions-11 > > > Reviewer: David Black > Review result: Ready with Issues > > 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@xxxxxxxx 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")? > [Med] We assumed a byte offset of 0 when counting, but I think this may be confusing. Changed the text to refer to bytes 13 and 14. We can restrict the range for uint16. The reason we didn't initially is because we wanted the type to match what is defined in other protocols that would trigger such ACL. See https://datatracker.ietf.org/doc/html/rfc8955#name-type-9-tcp-flags (2-octet bitmask in particular). Let me know if you have a preference here. > [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. [Med] We don't actually need to decrypt the packet. The match is based on the observable pattern (ddos mitigation, typically): +-- prefix? binary 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). [Med] RFC 9065 includes the following "Another method, if the endpoints do not provide this information, is to use an on-path network device that relies on pattern inferences in the traffic..." Which seems to be close to the match we are discussing. I'm not sure if a modification is needed here. > > 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). > [Med] I can add a pointer to https://datatracker.ietf.org/doc/html/rfc9293#table-7, but that information will be stale as (in theory) we can define new flag bits. > 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. > > [Med] We don't use "match" in the name of the choice because the node is under matches augment /acl:acls/acl:acl/acl:aces/acl:ace/acl:matches: +--rw (payload)? RFC8407 says: " Child nodes within a container or list SHOULD NOT replicate the parent identifier." However, I changed the name of the grouping to "payload-match" as that is indeed better. Thanks. ____________________________________________________________________________________________________________ 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. -- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
