Hi Med,
Working bottom-up ...
>> Editorial - Payload: The use of the word "payload" by itself as a type
>> initially confused me,
> However, I changed the name of the grouping to "payload-match" as that is
> indeed better. Thanks.
That's fine, check this one off as resolved.
>> 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 ...
In section 3.4 of the revised draft, I see the added mention of Section 3.1 to
focus the reference to RFC 9293 on that section. That will suffice.
>> [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
We're talking past each other, which is an indication that something does need
to be added. Section 3.6 says:
Some transport protocols use existing protocols (e.g., TCP or UDP) as
substrate.
The match criteria for such protocols may rely upon the 'protocol'
under 'l3',
TCP/UDP match criteria, part of the TCP/UDP payload, or a combination
thereof.
If the match on "part of the TCP/UDP payload" is looking for a specific value
at a specific offset in the payloads (e.g., all packets for the protocol have
the same value at that specific offset), then encryption of the payload that
includes encryption of the value at that specific offset results in different
packets having different and unpredictable values at that offset, rendering it
infeasible to match on the specific value at that specific offset.
Something should be said, but not what I just wrote - it ought to suffice to
point out that payload match only works as intended on unencrypted payloads or
unencrypted portions of partially encrypted payloads. As I noted earlier, this
is relatively obvious, and does not require a reference to RFC 9065.
> > [A] Minor technical issue: TCP flags bitmask
I'm still lost here, as I can't quickly figure out how to map the provisions in
this draft to the specific flags in the TCP header. This should be blindingly
obvious to the reader in order to avoid potential interoperability problems.
Thanks, --David
-----Original Message-----
From: [email protected] <[email protected]>
Sent: Tuesday, November 5, 2024 2:56 AM
To: Black, David <[email protected]>; [email protected]
Cc: [email protected]; [email protected];
[email protected]
Subject: RE: Tsvart last call review of draft-ietf-netmod-acl-extensions-11
[EXTERNAL EMAIL]
Hi David,
Thanks for the review.
The diff to track the changes can be seen here:
https://urldefense.com/v3/__https://author-tools.ietf.org/api/iddiff?url_1=https:**Anetmod-wg.github.io*enhanced-acl-netmod*draft-ietf-netmod-acl-extensions.txt&url_2=https:**Anetmod-wg.github.io*enhanced-acl-netmod*TSV-review*draft-ietf-netmod-acl-extensions.txt__;Ly8vLy8vLy8v!!LpKI!hBXAxRar8NOLzgnyfwEv94Rq2OWRFUp23238NekAf-2L7QMq0xGpNQKSHb1khck0bENVGvIKb3JJTvE0eqvBu_sdm0lx$
[author-tools[.]ietf[.]org].
Please see inline.
Cheers,
Med
> -----Message d'origine-----
> De : David Black via Datatracker <[email protected]>
> Envoyé : lundi 4 novembre 2024 19:01
> À : [email protected]
> Cc : [email protected]; last-
> [email protected]; [email protected]; [email protected]
> 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 [email protected] 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://urldefense.com/v3/__https://datatracker.ietf.org/doc/html/rfc8955*name-type-9-tcp-flags__;Iw!!LpKI!hBXAxRar8NOLzgnyfwEv94Rq2OWRFUp23238NekAf-2L7QMq0xGpNQKSHb1khck0bENVGvIKb3JJTvE0eqvBu3Q9oIXY$
[datatracker[.]ietf[.]org] (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://urldefense.com/v3/__https://datatracker.ietf.org/doc/html/rfc9293*table-7__;Iw!!LpKI!hBXAxRar8NOLzgnyfwEv94Rq2OWRFUp23238NekAf-2L7QMq0xGpNQKSHb1khck0bENVGvIKb3JJTvE0eqvBuyOs3R4A$
[datatracker[.]ietf[.]org], 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.
_______________________________________________
netmod mailing list -- [email protected]
To unsubscribe send an email to [email protected]
