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/ .
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
3.2. Event Clause
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
Except if I missed something, there is 2 times the same figure. I assume this is a copy/paste error.
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.
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.
3.3. Condition Clause
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.
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.
3.4. Action Clause
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.
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.
4.1. YANG Module of NSF-Facing Interface
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";
}
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?
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";
}
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.";
}
You really don’t like IPv6 :)
Why not a ICMPv6 response message with Type 1 Code 4 ;)
identity session-log {
base log-action;
description
"Identity for session log. Log the tasks that is performed
during a session.";
Clarification (e.g., text, reference(s)), please, about the terms:
- session
- tasks
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.";
}
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.";
}
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).
identity redirection {
base egress-action;
description
"Identity for redirection. This action redirects the packet to
another destination.";
}
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?
identity application-protocol {
description
"Base identity for Application protocol";
}
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?
Thanks in advance for you reply.
Best regards,
JMC.