Subject: [I2nsf] Intdir telechat review of draft-ietf-i2nsf-nsf-facing-interface-dm-21

From: Jean-Michel Combes via Datatracker <[email-masked]>
Date: Tue, 15 Feb 2022 10:48:15 -0800

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.