Extensions to the Access Control Lists (ACLs) YANG Model
draft-ietf-netmod-acl-extensions-17

Thank you to Sean Turner and Linda Dunbar for their secdir reviews:

Section 5, para 2:  Please replace the second (and last sentence) with "The YANG-based management protocols require the use of a secure transport layer such as SSH [RFC4252], TLS [RFC8446], or QUIC [RFC9000].  The YANG-based management protocols also require mutual authentication."

Section 5, para 4:  Please define 'reasonably sensitive or vulnerable' and 'particular sensitivities/vulnerabilities.  Alternatively, delete the words 'reasonably' and 'particular'. 

Section 5, para 5:  Perhaps the second to last sentence should say 'The former may result in the exposure of sensitive data, or compromise a device.  

Section 5, para 7:  Please delete the word 'particular'.

# Internet AD comments for draft-ietf-netmod-acl-extensions-15
CC @ekline

* comment syntax:
  - https://github.com/mnot/ietf-comments/blob/main/format.md

* "Handling Ballot Positions":
  - https://ietf.org/about/groups/iesg/statements/handling-ballot-positions/

## Comments

### S4

* The `identity layer4` description doesn't address whether IPv6 Extension
  Headers, or other "IP-layer" headers like AH, are to be skipped over or
  not.  I suspect they are, but this description could say explicitly.

  In the spirit of "send text", here's one attempt:

  identity layer4 {
    base offset-type;
    description
      "The offset start right after the IP header and any headers
       pertaining to that IP layer, e.g. IPv6 Extension Headers and the
       Authentication Header (AH). This can be typically the beginning of
       a transport header (e.g., TCP or UDP) or any encapsulation scheme
       over IP such as IP-in-IP.";
  }

  but that's just for your consideration.

* For the `payload` identity and the length in the `payload-match` for
  an `offset` of type `payload`, where is the end of the payload?

  Specifically, does this allow matching into the UDP Options space that
  is beyond the UDP payload but still within the IP payload?

  If the UDP Options space is excluded (or punted until future work), then
  it might be good to have some clarification about that here (we intend
  to include it in the payload match, exclude it, or leave it up to the
  implementer).

* In `payload-match`, the `description` for `operator` reads:

    "How to interpret the prefix match."

  Should that be s/prefix/pattern/?  (this seems like it might be a
  copy-paste error?)

* Not important for this document, but we should probably consider whether
  it should be good practice to include SCTP and maybe DCCP, even if it's
  only for the port set ACL definitions and nothing fancier.

  Just a comment, not a request for any change.

In 3.2, I found this statement to be confusing, perhaps because of my limited familiarity with YANG: "The port numbers can be individual port numbers, a range of port numbers, and an operation." At the least, I would have expected "or", and I didn't know what "an operation" would represent in the context of port numbers.

This seems to be referencing RFC 8519's `port-range-or-operator` grouping, which allows for a single port number, a range of port numbers, or a combination of a single port number with an operator (which in turn can be `eq`, `neq`, `lte`, or `gte`). Clearer wording and an explicit reference might be helpful here, though I assume the intended audience is already familiar with YANG conventions.

I agree with Deb's comments, especially regarding the use of 'reasonably' and 'particular'
and the use of secure transport protocols in the Security Considerations Section.


         In doing so, implementations would optimize the performance of
        matching lists vs multiple rules matching.

I don't believe this is universally true. Making complicated grouping can
actually cause more slowness than having multiple rules. Most DDoSes I
know in this space is from overcomplicated regexps trying to be clever on
matching IPv6.

Thank you to Russ Housley for the GENART review.

Thank you for addressing my DISCUSS and COMMENT feedback.

# Éric Vyncke, INT AD, comments for draft-ietf-netmod-acl-extensions-15
CC @evyncke

Thank you for the work put into this document. It is easy to read and add real value to ACL.

Please find below  some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits.

Special thanks to Lou Berger for the shepherd's write-up including the WG 'limited' interest/consensus and the justification of the intended status.

Other thanks to Tim Wicinski, the Internet directorate reviewer, please consider this int-dir last-call review:
https://datatracker.ietf.org/doc/review-ietf-netmod-acl-extensions-11-intdir-lc-wicinski-2024-11-17/ (status "ready")

I hope that this review helps to improve the document,

Regards,

-éric

## COMMENTS (non-blocking)

### Abstract

s/This document discusses a set of extensions/This document specifies a set of extensions/ after all, its intended status is proposed standard.

### Section 1

Humm... I understand what is meant but this paragraph appears to be self-contradicting `Network operators maintain sets of IP prefixes ... These lists are maintained and manipulated by security expert teams` (suggest adding "of the network operators").

It took me a while to parse `supporting means to easily map to the filtering rules conveyed in messages triggered by these tools is valuable from a network operation standpoint` mainly because the subject of "is valuable" is too long.

### Section 2

In `IP address, IP prefixes,` any reason why the plural form is used for "IP prefixes" ?

### Section 3.2

Where are the names defined in ` A protocol can be identified either by a number (e.g., 17) or a name (e.g., UDP).`

Should the example for aliases be dual-stack ? I.e., having both an IPV6 address and an IPv4 one ? Same comment for section D.1

I was about the ballot a DISCUSS on `beyond just the header information` which header is it ? Layer-2 ? IP ? Based on `identity offset-type` appearing later, I am balloting NoObjection but the clarification should already be in this section.

### Section 3.6

Related to my near-DISCUSS on section 3.2, `data offset` from which start ?

### Section 4

Generic comment: why next-header-set for IPv4 and not protocol-set as in IPv6 as they refer to the same identities ? Or even having protocol subtree to be version agnostic (like TCP), i.e., some operators would probably like to allow protocol == 50 (ESP) on both IPv6 and IPv4.

Like Erik Kline, I think that `identity layer4` for offset is not correct and Erik's suggestion is correct.

`The offset start right after the end of the transport payload.`, I think that the authors mean "transport header".

Rather than defining identities for all TCP flags (e.g., `identity ack`), why not using the same technique as for ICMP type, i.e., rely on the https://www.iana.org/assignments/tcp-parameters/tcp-parameters.xhtml#tcp-header-flags IANA registry?

### Section 6.3

Several IANA instructions are similar to `"enum": Replicates the name from the registry with all spaces striped.`, I am unsure whether the result will be readable and useful, it there a reason why the spaces must be removed ?

The "(deprecated)" and "(obsolete)" status appears only in the ICMPv4 registry, unsure whether they are applicable to ICMPv6 and extension headers registries. I will trust IANA review on this section.

### Section 7.2

As some IANA registries are used as input by the XSLT in appendix A, I wonder whether they should be normative references.

### Section E.3

Should there also be a match on the 'protocol' ? I.e., do not match for TCP packets having "2001:db8::1"

Moreover, I guess that the payload match is a binary comparison so it will never match the ASCII "2001:db8::1", suggest using an hexadecimal string in this example.


## NITS (non-blocking / cosmetic)

s/transpot/transport/ (saw it at least once)