[netmod] Re: AD review of draft-ietf-netmod-acl-extensions

[Mahesh Jethanandani <[e-mail]>]

Hi Med,

Thanks for addressing most of my comments as well as other *DIR reviews. I hope the SECDIR review comes through soon. 

See inline with [mj] for any follow-up.

> On Jan 7, 2025, at 9:44 PM, [e-mail] wrote:
> 
> Hi Mahesh,
>  
> Thank you for the review.
>  
> Samier kindly prepared an updated version that addresses most of your comments. The diff 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/Mahesh-comments/draft-ietf-netmod-acl-extensions.txt <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/Mahesh-comments/draft-ietf-netmod-acl-extensions.txt>
>  
> Please see inline.
>  
> Cheers,
> Med
>  
> De : Mahesh Jethanandani <[e-mail] <mailto:[e-mail]>> 
> Envoyé : vendredi 3 janvier 2025 06:32
> À : [e-mail] <mailto:[e-mail]>
> Cc : NETMOD Working Group <[e-mail] <mailto:[e-mail]>>; Per Andersson <[e-mail] <mailto:[e-mail]>>
> Objet : AD review of draft-ietf-netmod-acl-extensions
>  
> 
> Hi Authors of  draft-ietf-netmod-acl-extensions,
>  
> Happy New Year!
>  
> One expert SECDIR review is still pending on the document, which I hope will be coming soon. Thanks to Per for providing the YANG Doctors review, to Tim for providing the INTDIR, and to David for the TSVART review. In the meantime, please find my AD review comments on the document. I hope these comments help to improve the document.
>  
> COMMENT:
>  
> "Abstract", paragraph 2
> >    RFC 8519 defines a YANG data model for Access Control Lists (ACLs).
> >    This document discusses a set of extensions that fix many of the
> >    limitations of the ACL model as initially defined in RFC 8519.
> > 
> >    The document also defines IANA-maintained modules for ICMP types and
> >    IPv6 extension headers.
>  
> This document provided important extensions to the base ACL YANG model defined in RFC8519.
>  
> However, nowhere in the document does it mention that these extensions augment the base model, except in the model itself. Reading the abstract and the introduction, a reader would be left wondering whether this model is replacing RFC 8519 or augmenting it. Can that be made clear?
>  
> [Med] A new sentence was added to the abstract.

[mj] Thanks.

>  
> Section 2, paragraph 4
> >    Defined set:  Refers to reusable description of one or multiple
> >       information elements (e.g., IP address, IP prefix, port number, or
> >       ICMP type).
>  
> Is it just a description? A better way to define it would be to say:
>  
> Elements in a defined set typically share a logical purpose or function, such as IP address, IP prefixes, port number, or ICMP type.
>  
> Similarly definition of 'payload-based filtering' could be added to define the term or referenced if defined somewhere else.
>  
> [Med] Done.

[mj] Thanks.

>  
> Section 3.5, paragraph 1
> >    The augmented ACL structure (Figure 1) includes new leafs
> >    'ipv4-fragment' and 'ipv6-fragment' to better handle fragments.
>  
> The document comments that RFC8519 does not do a good job of handling fragments and that this document addresses that issue. It would help to explain how the model defined in the document allows for better handling of fragments. For example, describe how the combination of operator and fragment type eases fragment handling.
>  
> [Med] We do explain this in https://datatracker.ietf.org/doc/html/draft-ietf-netmod-acl-extensions-13#name-partial-or-lack-of-ipv4-ipv <https://datatracker.ietf.org/doc/html/draft-ietf-netmod-acl-extensions-13#name-partial-or-lack-of-ipv4-ipv> and then illustrate the use of bitmask inhttps://datatracker.ietf.org/doc/html/draft-ietf-netmod-acl-extensions-13#name-fragments-handling-2 <https://datatracker.ietf.org/doc/html/draft-ietf-netmod-acl-extensions-13#name-fragments-handling-2>. Do you think we need to say more?  

[mj] I seem to have missed that somehow. I think it suffices, so no more edits needed.

>  
>  
> Section 4, paragraph 1
> >    This model imports types from [RFC6991], [RFC8519], and [RFC8294].
>  
> Thanks first of all to Per for providing YANG doctor review comments. I also tend to agree with him that as a practice reference to RFC 9293, 4443, 3032, 792, and IEEE 802.1Q, IEEE 802.1ah need to be added here, even if they are referenced or mentioned elsewhere.
>  
> [Med] I’m afraid that saying this module references REF1, REF2, etc. isn’t that useful. These refs are cited in their context.

[mj] I went looking for precedence to see if this has been followed but did not find any, which I think is sad. The interface YANG module and the Routing Policy module follow the practice, but NI or Routing modules do not. Citing RFCs without a square bracket is common in YANG modules, and the idtool does not pick up on it unless the square bracket is present. That means that RFCs referenced in the YANG module can be missed unless the RFC Editor catches them. The practice of referencing all YANG modules in the section that includes the YANG module was supposed to address it and why Per brought it up. The fact that you have made sure to reference them elsewhere in the document addresses the concern, but I wonder how to ensure the practice is followed.

Cheers..

>  
> "Appendix E.", paragraph 1
> >    This section provides a few examples to illustrate the use of the
> >    enhanced ACL module ("ietf-acl-enh").
>  
> Thank you for providing examples along with the module. The added feature in this module is payload-based filtering. An example configuration reflecting that, say for HTTP headers, would greatly enhance the understanding of this module.
> [Med] Will consider adding an example to illustrate this.
>  
> No reference entries found for these items, which were mentioned in the text:
>  [RFC3692].
>  
> [Med] ACK. 
>  
> -------------------------------------------------------------------------------
> NIT
> -------------------------------------------------------------------------------
>  
> All comments below are about very minor potential issues that you may choose to
> address in some way - or ignore - as you see fit. Some were flagged by
> automated tools (via https://github.com/larseggert/ietf-reviewtool <https://github.com/larseggert/ietf-reviewtool>), so there
> will likely be some false positives. There is no need to let me know what you
> did with these suggestions.
>  
> Section 3.6, paragraph 2
> >    A new feature, called 'match-on-payload', is defined in the document.
> >    This can be used, for example, for QUIC [RFC9000] or for tunneling
> >    protocols.  This feature requires configuring a data offset, a
> >    length, and a binary pattern to macth data against using a specified
> >    operator.
>  
> s/macth/match/
> [Med] ACK
>  
> Section 6.3.1, paragraph 15
> >    IANA is requested to updated the "Reference" in the "ICMP Type
> >    Numbers" registry as follows:
>  
> s/updated/update/
> [Med] ACK
>  
> Section 6.3.2, paragraph 15
> >    IANA is requested to updated the "Reference" in the "ICMPv6 "type"
> >    Numbers" registry as follows:
>  
> s/updated/update/
> [Med] ACK
>  
> Section 6.3.3, paragraph 15
> >    IANA is requested to updated the "Reference" in the "IPv6 Extension
> >    Header Types" registry as follows:
>  
> s/updated/update/
> [Med] ACK
>  
> Section 1.1, paragraph 10
> > ixes, port number, or ICMP type. Similarly definition of 'payload-based filt
> >                                  ^^^^^^^^^
> A comma may be missing after the conjunctive/linking adverb "Similarly".
> [Med] Don’t see where this one is used in the doc.
>  
> Section 4, paragraph 3
> >  { base tcp-flag; description "Acknowledgment TCP flag bit."; reference "RFC
> >                                ^^^^^^^^^^^^^^
> Do not mix variants of the same word ("acknowledgment" and "acknowledgement")
> within a single text.
>  
> [Med] ACK
> 
> Thanks.
> 
> Mahesh Jethanandani
> [e-mail] <mailto:[e-mail]>
>  
>  
>  
>  
> 
>  
> ____________________________________________________________________________________________________________
> 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.


Mahesh Jethanandani
[e-mail]