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")?
[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.
--
last-call mailing list -- last-call@xxxxxxxx
To unsubscribe send an email to last-call-leave@xxxxxxxx
