Reviewer: Jean-Michel Combes Review result: On the Right Track Hi, I am an assigned INT directorate reviewer for draft-ietf-i2nsf-nsf-facing-interface-dm-21. These comments were written primarily for the benefit of the Internet Area Directors. Document editors and shepherd(s) should treat these comments just like they would treat comments from any other IETF contributors and resolve them along with any other Last Call comments that have been received. For more details on the INT Directorate, see https://datatracker.ietf.org/group/intdir/about/ <https://datatracker.ietf.org/group/intdir/about/>. Based on my review, if I was on the IESG I would ballot this document as DISCUSS. I have the following DISCUSS level issues: - IPv6 (section 3.3) The following are other issues I found with this document that SHOULD be corrected before publication: - Event clause definition (section 3.2) - Action clause definition (section 3.4) - identity session-log description (section 4.1) - identity transformation (section 4.1) - identity redirection (section 4.1) The following are minor issues (typos, misspelling, minor text improvements) with the document: - Figure 2 (section 3.2) - identity hardware-alarm description (section 4.1) - identity reject description (section 4.1) I have also a question, for my curiosity, about identity application-protocol (section 4.1). Please, find below, my deep review. I2NSF Network Security Function-Facing Interface YANG Data Model draft-ietf-i2nsf-nsf-facing-interface-dm-21 <snip> 3.2. Event Clause <snip> module: ietf-i2nsf-policy-rule-for-nsf +--rw i2nsf-security-policy* [name] ... +--rw rules* [name] | ... | +--rw event | | +--rw description? string | | +--rw time | | | +--rw start-date-time? yang:date-and-time | | | +--rw end-date-time? yang:date-and-time | | | +--rw period | | | | +--rw start-time? time | | | | +--rw end-time? time | | | | +--rw day* day | | | | +--rw date* int8 | | | | +--rw month* string | | | +--rw frequency? enumeration | | +--rw event-clauses | | +--rw system-event* identityref | | +--rw system-alarm* identityref | +--rw condition | ... | +--rw action | ... +--rw rule-group ... module: ietf-i2nsf-policy-rule-for-nsf +--rw i2nsf-security-policy* [name] ... +--rw rules* [name] | ... | +--rw event | | +--rw description? string | | +--rw time | | | +--rw start-date-time? yang:date-and-time | | | +--rw end-date-time? yang:date-and-time | | | +--rw period | | | | +--rw start-time? time | | | | +--rw end-time? time | | | | +--rw day* day | | | | +--rw date* int8 | | | | +--rw month* string | | | +--rw frequency? enumeration | | +--rw event-clauses | | +--rw system-event* identityref | | +--rw system-alarm* identityref | +--rw condition | | ... | +--rw action | ... +--rw rule-group ... Figure 2: YANG Tree Diagram for an Event Clause <JMC> Except if I missed something, there is 2 times the same figure. I assume this is a copy/paste error. </JMC> An event clause is any important occurrence at a specific time of a change in the system being managed, and/or in the environment of the system being managed. <JMC> I am not English native but, IMHO, your definition is maybe too restrictive. Indeed, for me (and my English :)) your definition doesn’t apply to your “Example Security Requirement 1: Block Social Networking Service” because there is no change in the system being managed, and/or in the environment of the system being managed. IMHO, your definition only fits with the use cases where there is either a system event (called also alert) or a system alarm. </JMC> <snip> 3.3. Condition Clause <snip> module: ietf-i2nsf-policy-rule-for-nsf +--rw i2nsf-security-policy* [name] ... +--rw rules* [name] | ... | +--rw event | | ... | +--rw condition | | +--rw description? string | | +--rw ethernet | | | +--rw description? string | | | +--rw destination-mac-address? yang:mac-address | | | +--rw destination-mac-address-mask? yang:mac-address | | | +--rw source-mac-address? yang:mac-address | | | +--rw source-mac-address-mask? yang:mac-address | | | +--rw ethertype? eth:ethertype | | +--rw ipv4 | | | +--rw description? string | | | +--rw dscp? inet:dscp | | | +--rw ecn? uint8 | | | +--rw length? uint16 | | | +--rw ttl? uint8 | | | +--rw protocol? uint8 | | | +--rw ihl? uint8 | | | +--rw flags? bits | | | +--rw offset? uint16 | | | +--rw identification? uint16 | | | +--rw (destination-network)? | | | | +--:(destination-ipv4-network) | | | | | +--rw destination-ipv4-network? inet:ipv4-prefix | | | | +--:(destination-ipv4-range) | | | | +--rw destination-ipv4-range* [start end] | | | | +--rw start inet:ipv4-address-no-zone | | | | +--rw end inet:ipv4-address-no-zone | | | +--rw (source-network)? | | | +--:(source-ipv4-network) | | | | +--rw source-ipv4-network? inet:ipv4-prefix | | | +--:(source-ipv4-range) | | | +--rw source-ipv4-range* [start end] | | | +--rw start inet:ipv4-address-no-zone | | | +--rw end inet:ipv4-address-no-zone | | +--rw ipv6 | | | +--rw description? string | | | +--rw dscp? inet:dscp | | | +--rw ecn? uint8 | | | +--rw length? uint16 | | | +--rw ttl? uint8 | | | +--rw protocol? uint8 | | | +--rw (destination-network)? | | | | +--:(destination-ipv6-network) | | | | | +--rw destination-ipv6-network? inet:ipv6-prefix | | | | +--:(destination-ipv6-range) | | | | +--rw destination-ipv6-range* [start end] | | | | +--rw start inet:ipv6-address-no-zone | | | | +--rw end inet:ipv6-address-no-zone | | | +--rw (source-network)? | | | | +--:(source-ipv6-network) | | | | | +--rw source-ipv6-network? inet:ipv6-prefix | | | | +--:(source-ipv6-range) | | | | +--rw source-ipv6-range* [start end] | | | | +--rw start inet:ipv6-address-no-zone | | | | +--rw end inet:ipv6-address-no-zone | | | +--rw flow-label? inet:ipv6-flow-label | | +--rw tcp | | | +--rw description? string | | | +--rw source-port-number | | | | +--rw (source-port)? | | | | +--:(range-or-operator) | | | | | +--rw (port-range-or-operator)? | | | | | +--:(range) | | | | | | +--rw lower-port inet:port-number | | | | | | +--rw upper-port inet:port-number | | | | | +--:(operator) | | | | | +--rw operator? operator | | | | | +--rw port inet:port-number | | | | +--:(port-list) | | | | +--rw port-numbers* [start end] | | | | +--rw start inet:port-number | | | | +--rw end inet:port-number | | | +--rw destination-port-number | | | | +--rw (destination-port)? | | | | +--:(range-or-operator) | | | | | +--rw (port-range-or-operator)? | | | | | +--:(range) | | | | | | +--rw lower-port inet:port-number | | | | | | +--rw upper-port inet:port-number | | | | | +--:(operator) | | | | | +--rw operator? operator | | | | | +--rw port inet:port-number | | | | +--:(port-list) | | | | +--rw port-numbers* [start end] | | | | +--rw start inet:port-number | | | | +--rw end inet:port-number | | | +--rw sequence-number? uint32 | | | +--rw acknowledgement-number? uint32 | | | +--rw data-offset? uint8 | | | +--rw reserved? uint8 | | | +--rw flags? bits | | | +--rw window-size? uint16 | | | +--rw urgent-pointer? uint16 | | | +--rw options? binary | | +--rw udp | | | +--rw description? string | | | +--rw source-port-number | | | | +--rw (source-port)? | | | | +--:(range-or-operator) | | | | | +--rw (port-range-or-operator)? | | | | | +--:(range) | | | | | | +--rw lower-port inet:port-number | | | | | | +--rw upper-port inet:port-number | | | | | +--:(operator) | | | | | +--rw operator? operator | | | | | +--rw port inet:port-number | | | | +--:(port-list) | | | | +--rw port-numbers* [start end] | | | | +--rw start inet:port-number | | | | +--rw end inet:port-number | | | +--rw destination-port-number | | | | +--rw (destination-port)? | | | | +--:(range-or-operator) | | | | | +--rw (port-range-or-operator)? | | | | | +--:(range) | | | | | | +--rw lower-port inet:port-number | | | | | | +--rw upper-port inet:port-number | | | | | +--:(operator) | | | | | +--rw operator? operator | | | | | +--rw port inet:port-number | | | | +--:(port-list) | | | | +--rw port-numbers* [start end] | | | | +--rw start inet:port-number | | | | +--rw end inet:port-number | | | +--rw length? uint16 | | +--rw sctp | | | +--rw description? string | | | +--rw source-port-number | | | | +--rw (source-port)? | | | | +--:(range-or-operator) | | | | | +--rw (port-range-or-operator)? | | | | | +--:(range) | | | | | | +--rw lower-port inet:port-number | | | | | | +--rw upper-port inet:port-number | | | | | +--:(operator) | | | | | +--rw operator? operator | | | | | +--rw port inet:port-number | | | | +--:(port-list) | | | | +--rw port-numbers* [start end] | | | | +--rw start inet:port-number | | | | +--rw end inet:port-number | | | +--rw destination-port-number | | | | +--rw (destination-port)? | | | | +--:(range-or-operator) | | | | | +--rw (port-range-or-operator)? | | | | | +--:(range) | | | | | | +--rw lower-port inet:port-number | | | | | | +--rw upper-port inet:port-number | | | | | +--:(operator) | | | | | +--rw operator? operator | | | | | +--rw port inet:port-number | | | | +--:(port-list) | | | | +--rw port-numbers* [start end] | | | | +--rw start inet:port-number | | | | +--rw end inet:port-number | | | +--rw chunk-type* uint8 | | | +--rw chunk-length? uint16 | | +--rw dccp | | | +--rw description? string | | | +--rw source-port-number | | | | +--rw (source-port)? | | | | +--:(range-or-operator) | | | | | +--rw (port-range-or-operator)? | | | | | +--:(range) | | | | | | +--rw lower-port inet:port-number | | | | | | +--rw upper-port inet:port-number | | | | | +--:(operator) | | | | | +--rw operator? operator | | | | | +--rw port inet:port-number | | | | +--:(port-list) | | | | +--rw port-numbers* [start end] | | | | +--rw start inet:port-number | | | | +--rw end inet:port-number | | | +--rw destination-port-number | | | | +--rw (destination-port)? | | | | +--:(range-or-operator) | | | | | +--rw (port-range-or-operator)? | | | | | +--:(range) | | | | | | +--rw lower-port inet:port-number | | | | | | +--rw upper-port inet:port-number | | | | | +--:(operator) | | | | | +--rw operator? operator | | | | | +--rw port inet:port-number | | | | +--:(port-list) | | | | +--rw port-numbers* [start end] | | | | +--rw start inet:port-number | | | | +--rw end inet:port-number | | | +--rw service-code* uint32 | | | +--rw type* uint8 | | | +--rw data-offset? uint8 | | +--rw icmp* [version] | | | +--rw description? string | | | +--rw version enumeration | | | +--rw type? uint8 | | | +--rw code? uint8 | | | +--rw rest-of-header? binary | | +--rw url-category | | | +--rw description? string | | | +--rw pre-defined* string | | | +--rw user-defined* string | | +--rw voice | | | +--rw description? string | | | +--rw source-voice-id* string | | | +--rw destination-voice-id* string | | | +--rw user-agent* string | | +--rw ddos | | | +--rw description? string | | | +--rw alert-packet-rate? uint32 | | | +--rw alert-flow-rate? uint32 | | | +--rw alert-byte-rate? uint32 | | +--rw anti-virus | | | +--rw profile* string | | | +--rw exception-files* string | | +--rw payload | | | +--rw description? string | | | +--rw content* string | | +--rw context | | +--rw description? string | | +--rw application | | | +--rw description? string | | | +--rw protocol* identityref | | +--rw device-type | | | +--rw description? string | | | +--rw device* identityref | | +--rw users | | | +--rw description? string | | | +--rw user* [id] | | | | +--rw id uint32 | | | | +--rw name? string | | | +--rw group* [id] | | | | +--rw id uint32 | | | | +--rw name? string | | | +--rw security-group? string | | +--rw geographic-location | | +--rw description? string | | +--rw source* string | | +--rw destination* string | +--rw action | ... +--rw rule-group ... Figure 3: YANG Tree Diagram for a Condition Clause A condition clause is defined as a set of attributes, features, and/ or values that are to be compared with a set of known attributes, features, and/or values in order to determine whether the set of actions in that (imperative) I2NSF policy rule can be executed or not. A condition clause is classified as a condition of generic network security functions, advanced network security functions, or context. A condition clause of generic network security functions is defined as IPv4 condition, IPv6 condition, TCP condition, UDP condition, SCTP condition, DCCP condition, or ICMP (ICMPv4 and ICMPv6) condition. Note that the data model in this document does not focus on only IP addresses, but focuses on all the fields of IPv4 and IPv6 headers. The IPv4 and IPv6 headers have similarity with some different fields. In this case, it is better to handle separately the IPv4 and IPv6 headers such that the different fields can be used to handle IPv4 and IPv6 packets. <JMC> What about IPv6 options headers? FWIW, even basic FWs provide the granularity to set-up a policy regarding them. Seems you missed something important (i.e., a large part of the IPv6 world), IMHO ... Currently, your data model would be unusable to set-up an IPv6 filtering policy inside my company. </JMC> <snip> 3.4. Action Clause <snip> module: ietf-i2nsf-policy-rule-for-nsf +--rw i2nsf-security-policy* [name] ... +--rw rules* [name] | ... | +--rw event | ... | +--rw condition | ... | +--rw action | +--rw description? string | +--rw packet-action | | +--rw ingress-action? identityref | | +--rw egress-action? identityref | | +--rw log-action? identityref | +--rw flow-action | | +--rw ingress-action? identityref | | +--rw egress-action? identityref | | +--rw log-action? identityref | +--rw advanced-action | +--rw content-security-control* identityref | +--rw attack-mitigation-control* identityref +--rw rule-group ... Figure 4: YANG Tree Diagram for an Action Clause An action is used to control and monitor aspects of flow-based NSFs when the policy rule event and condition clauses are satisfied. <JMC> What happens when there is either no event clause (e.g., Example Security Requirement 2: Block Malicious VoIP/VoCN Packets Coming to a Company) or no condition clause? What I mean is: is it mandatory to have the both to trigger an action? IMHO, a text clarification is needed here. </JMC> 4.1. YANG Module of NSF-Facing Interface <snip> identity memory-alarm { base system-alarm; description "Identity for memory alarm. Memory is the hardware to store information temporarily or for a short period, i.e., Random Access Memory (RAM). A memory-alarm is emitted when the memory usage is exceeding the threshold."; reference "draft-ietf-i2nsf-nsf-monitoring-data-model-14: I2NSF NSF Monitoring Interface YANG Data Model - System alarm for memory"; } identity cpu-alarm { base system-alarm; description "Identity for CPU alarm. CPU is the Central Processing Unit that executes basic operations of the system. A cpu-alarm is emitted when the CPU usage is exceeding a threshold."; reference "draft-ietf-i2nsf-nsf-monitoring-data-model-14: I2NSF NSF Monitoring Interface YANG Data Model - System alarm for CPU"; } identity disk-alarm { base system-alarm; description "Identity for disk alarm. Disk is the hardware to store information for a long period, i.e., Hard Disk and Solid-State Drive. A disk-alarm is emitted when the disk usage is exceeding a threshold."; reference "draft-ietf-i2nsf-nsf-monitoring-data-model-14: I2NSF NSF Monitoring Interface YANG Data Model - System alarm for disk"; } identity hardware-alarm { base system-alarm; description "Identity for hardware alarm. A hardware alarm is emitted when a problem of hardware (e.g., CPU, memory, disk, or interface) is detected."; reference "draft-ietf-i2nsf-nsf-monitoring-data-model-14: I2NSF NSF Monitoring Interface YANG Data Model - System alarm for hardware"; } <JMC> I don’t understand the last identity here. If I refer to the identities defined here (i.e., previous ones and the one after, I would say that the description should be: “Identity for hardware” alarm. A hardware alarm is emitted when a problem of hardware – other than CPU/memory/disk/interface, is detected.”; Indeed, identities already exist for CPU, memory, disk and interface alarms. Or did I misunderstand something? </JMC> identity interface-alarm { base system-alarm; description "Identity for interface alarm. Interface is the network interface for connecting a device with the network. The interface-alarm is emitted when the state of the interface is changed."; reference "draft-ietf-i2nsf-nsf-monitoring-data-model-14: I2NSF NSF Monitoring Interface YANG Data Model - System alarm for interface"; } <snip> identity reject { base ingress-action; base egress-action; base default-action; description "Identity for reject action capability. The reject action denies a packet to go through the NSF entering or exiting the internal network and sends a response back to the source. The response depends on the packet and implementation. For example, a TCP packet is rejected with TCP RST response or a UDP packet may be rejected with an ICMP response message with Type 3 Code 3, i.e., Destination Unreachable: Destination port unreachable."; } <JMC> You really don’t like IPv6 :) Why not a ICMPv6 response message with Type 1 Code 4 ;) </JMC> <snip> identity session-log { base log-action; description "Identity for session log. Log the tasks that is performed during a session."; <JMC> Clarification (e.g., text, reference(s)), please, about the terms: - session - tasks </JMC> identity tunnel-encapsulation { base egress-action; description "Identity for tunnel encapsulation. The tunnel encapsulation action is used to encapsulate the packet to be tunneled across the network to enable a secure connection."; } <snip> identity transformation { base egress-action; description "Identity for transformation. The transformation action is used to transform the packet by modifying its protocol header such as HTTP-to-CoAP translation."; reference "RFC 8075: Guidelines for Mapping Implementations: HTTP to the Constrained Application Protocol (CoAP) - Translation between HTTP and CoAP."; } <JMC> IMHO, I would do the following change: s/by modifying its protocol header/by modifying it That would permit a more global scope (e.g., SFC, SRHv6). </JMC> identity redirection { base egress-action; description "Identity for redirection. This action redirects the packet to another destination."; } <JMC> I only know two ways to redirect a packet: either tunneling it or modifying it – what you call “transformation”. May you describe any other way you have in mind to do the job, and so to have the need to specify the identity redirection, please? </JMC> <snip> identity application-protocol { description "Base identity for Application protocol"; } <JMC> This question is just for my curiosity: why not to define a generic application-protocol, including the associated well-known port to identify the protocol you need? </JMC> <snip> Thanks in advance for you reply. Best regards, JMC. -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
