I2NSF Consumer-Facing Interface YANG Data Model
draft-ietf-i2nsf-consumer-facing-interface-dm-31

# Internet AD comments for draft-ietf-i2nsf-consumer-facing-interface-dm-27
CC @ekline

## Comments

### S6.1

* It may seem obvious from the context, but I always think it's better to
  be explicit about ranges of things: the ip-address-info and
  range-match-ipv[46] things might say that the ranges are inclusive of the
  endpoints, or that the intervals are closed, or words to that effect.

# John Scudder, RTG AD, comments draft-ietf-i2nsf-consumer-facing-interface-dm-27
CC @jgscudder

Thanks for this work. Since my expertise with YANG is rudimentary at best, my review was necessarily pretty limited. I did note a few things that seem underspecified or ambiguous, although for all I know some of my comments would be trivially answered by reference to the YANG module if I were better at reading it. For that reason, I haven't chosen to make any of these DISCUSSes, although otherwise I might have done so for the first comment.

## COMMENTS

### Section 3, Priority

I don't see any explanation of what the semantics of "priority" are, other than the tautological definition given here (essentially, it says "the priority is the priority"). Is "priority" explained in one of the normative references? I did spend a little while looking and didn't find anything. If not, it seems as though its semantics need to be explained here.

My guess is that rules are matched in priority order, with lower priorities being matched first, and that matching terminates after the first match occurs. I would also guess that order is undefined among rules with equal priorities. But none of this is stated, and I think it needs to be.

There is also no example that uses 'priority' which might otherwise have helped (although the definition should be able to stand without reference to examples in any case).

### Section 3.1, what is a security event?

This is a little bit of a nit perhaps, but

   based on a security event (i.e., system event and system alarm).

Parsing the parenthetical closely, I would conclude that a security event is defined as the concatenation of both a system event and a system alarm, in other words, both must be present, which seems a little surprising. Maybe that's what you mean, or maybe you mean "or" instead of "and"?

I might have been able to work this out for myself if an example were provided that used 'event', but unfortunately, there isn't one.

### Section 5.2, multiple instances

I can't make out what this sentence means: "If multiple instances of content are defined, it should match all contents somewhere in the session stream." One problem is the disagreement in number -- "multiple instances of content" is plural, and "it" is singular. I wouldn't pick on the grammar nit, except the disagreement in number makes it hard for me to work out what the referent of "it" is.

In general, I can't work out exactly what this paragraph is telling me to do. Maybe an implementor working in this area wouldn't have problems, but on the face of it, it seems like it should be made clearer.

### Section 6.1, time typedef

I was a little surprised you had to typedef time and day for yourself, I would have imagined there would be some well-known base types for these you could reference... but see my disclaimer. :-/

But since you did define your own time, I am curious if you think the definition is clear enough. Your description says,

      "The time type represents an instance of time of zero-duration
       in the specified timezone that recurs every day.";

It would seem desirable to provide a reference for the semantics instead of just relying on the reader to guess (through familiar usage, I suppose). Your regex looks close to, but not quite the same as, RFC 3339 §5.6's 'full-time'. The differences I can see offhand are that you don't permit lower-case 'z',  you restrict the range on the time-offset to +/- 14:00 whereas 3339 looks to permit the full +/- 23:59 range, and you make the TZ optional. The latter point means your current description doesn't really cover the possible range of inputs. I presume UTC is the default if TZ isn't specified, but you don't state this, and it's not the only imaginable option, e.g. I suppose an implementor might choose to use the local time zone if you leave it to their imagination.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

I support Lars' DISCUSS about normative references.

I don't understand the shepherd writeup answer to question #18.  This document doesn't create any new registries.

I'm no expert, but shouldn't the YANG Module itself include (as a comment) the BCP 14 boilerplate?  I ask because the only MUST is in the module, and the only SHOULD is in Security Considerations, so in one sense the BCP 14 reference could go away, and in another, it's missing.

Thanks for addressing my DISCUSSes

My views basically mirror John Scudder's...

Thanks for working on this specification. Thanks to Joe for the TSVART review.

I am supporting Lars's 1st and 3rd discuss point. I am also unsure if a github commit can be normative ref and should be a valid ref in any form in a PS specification.

Thank you for the work put into this document. 

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

Special thanks to Linda Dunbar for the shepherd's detailed write-up including the WG consensus *but* it lacks a proper the justification of the intended status. The write-up mentions 2 IPR while there are 4 of them.

Please note that Dirk Von Hugo is the Internet directorate reviewer (at my request) and you may want to consider this int-dir review as well when it will be available (no need to wait for it though):
https://datatracker.ietf.org/doc/draft-ietf-i2nsf-consumer-facing-interface-dm/reviewrequest/17250/

I hope that this review helps to improve the document,

Regards,

-éric

# COMMENTS (non blocking)

## Threat feed

While threat feeds are important, do they have a place in this model ?

## Section 3

Including a YANG tree in a section about an information model is really surprising. It seems that the whole section is more about the YANG data model than about an information model. Suggest to rename the section.

Having a leaf about language that is optional also means that the complete instance will be mono-lingual (i.e., not possible to have an instance with descriptions both in English and in Korean)

## Section 3.2

What about filtering ARP ? or IPv6 extension headers ?

`For more information about the mapping between ICMPv4 and ICMPv6 messages, refer to [IANA-ICMP-Parameters] and [IANA-ICMPv6-Parameters]` but there is no mapping described in the IANA tables. Also, this text is partially redundant with the text in 'case(firewall)'.

## Sections 4.1 & 4.2

What is impact of randomised MAC addresses and RFC 8981 IPv6 temporary addresses on this model ? Is it still worth doing ?

Is a start-end practical in a sparse addressing space as IPv6 ? Should this rather be a prefix notation ?

## Section 4.3

Is the 'city' in the language as indicated by a top leaf or is it English? Please, specify.

Did the author try to find another representation than basically copying/duplicating addresses in 'user/device/location' trees ? Why not having a list of all addresses (assuming that this is useful) and then having a leaf for user / device / location grouping ? This could be faster to identify a rule relevant to a MAC address (even if performance is not really the key point of an information model).

## Section 5

Should the work done in DOTS WG be listed in this section ?

## Section 6.1

I am not a YANG expert but isn't there an easier way to refer to draft-ietf-i2nsf-nsf-monitoring-data-model rather than redefining all identities here ?

Thanks for the document,

I've opted against making this a discuss - though debated doing so, I would like however to see some feedback on the below point.

I think having read this, and then read through the other comments submitted most of what I was going to say is already covered.  I would though like the draw specific reference to John's comment on section 6.1 and the regex for the time typedef contained there in.  Considering the number of time formats in existence (RFC822/RFC822/RFC850/RFC1123/RFC1123/RFC3339) and sub-parts of those RFC's (what I will refer to as RFC822Z, RFC1123Z and RFC3339Nano) it would be nice to see a format that conforms to one of the defined standards unless there is specific reason to deviate.  I also think that if we're conforming to one of the above - this should probably form a reference in the document for clarity.

# GEN AD review of draft-ietf-i2nsf-consumer-facing-interface-dm-27

CC @larseggert

Thanks to Roni Even for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/PrQuAtGM5yKx1cs4Upt2cRel9IA).

## Comments

### DOWNREFs

Possible DOWNREF from this Standards Track doc to `[OPENIOC]`. If so, the IESG
needs to approve it.

Possible DOWNREF from this Standards Track doc to `[MISPCORE]`. If so, the IESG
needs to approve it.

### Inclusive language

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term `traditional`; alternatives might be `classic`, `classical`, `common`,
   `conventional`, `customary`, `fixed`, `habitual`, `historic`,
   `long-established`, `popular`, `prescribed`, `regular`, `rooted`,
   `time-honored`, `universal`, `widely used`, `widespread`

## Nits

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), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Typos

#### Section 6.1, paragraph 99
```
-           for an IP address, such as IPv4 adress and IPv6 address.";
+           for an IP address, such as IPv4 address and IPv6 address.";
+                                             +
```

#### Section 6.1, paragraph 121
```
-                   category such as SNS sites, game sites, ecommerce
+                   category such as SNS sites, game sites, e-commerce
+                                                            +
```

#### Section 6.1, paragraph 135
```
-               gaming sites, ecommerce sites";
+               gaming sites, e-commerce sites";
+                              +
```

### URLs

These URLs in the document can probably be converted to HTTPS:

 * http://www.iso.org/iso/home/standards/country_codes/iso-3166-1_decoding_table.htm
 * http://www.iso.org/iso/home/standards/country_codes.htm#2012_iso3166-2

### Grammar/style

#### Section 3.1, paragraph 1
```
sf-capability-data-model]. Case (anti-virus): This field represents the conf
                                 ^^^^^^^^^^
```
This word is normally spelled as one.

#### Section 3.2, paragraph 1
```
 This information describes a caller id or receiver id in order to prevent an
                                     ^^
```
This abbreviation for "identification" is spelled all-uppercase.

#### Section 3.2, paragraph 1
```
on describes a caller id or receiver id in order to prevent any exploits (or
                                     ^^
```
This abbreviation for "identification" is spelled all-uppercase.

#### Section 3.2, paragraph 3
```
ow-rate-threshold? uint64 | +--rw anti-virus | | +--rw profile* string | | +-
                                  ^^^^^^^^^^
```
This word is normally spelled as one.

#### Section 3.2, paragraph 9
```
he Action object SHALL have following information: Primary-action: This fiel
                            ^^^^^^^^^^^^^^^^^^^^^
```
The article "the" may be missing.

#### Section 4, paragraph 3
```
, e.g., 'Dublin', 'New York', and 'Sao Paulo'. Range-ipv4-address: This repre
                                   ^^^^^^^^^
```
Did you mean "São Paulo" (= city in Brazil)?

#### Section 4.5, paragraph 1
```
is field is not mandatory but recommended to be used as it is helpful for fut
                              ^^^^^^^^^^^^^^^^^
```
The verb "recommended" is used with the gerund form.

#### Section 5.1, paragraph 4
```
er-Facing Interface, this document provide examples for security policy rules
                                   ^^^^^^^
```
The verb "provide" is plural. Did you mean: "provides"? Did you use a verb
instead of a noun?

#### Section 6.1, paragraph 68
```
nclude 'Dublin', 'New York', and 'Sao Paulo'."; } uses ip-address-info{ refin
                                  ^^^^^^^^^
```
Did you mean "São Paulo" (= city in Brazil)?

#### Section 6.1, paragraph 94
```
ck mitigation."; } } } container anti-virus { description "A condition for an
                                 ^^^^^^^^^^
```
This word is normally spelled as one.

#### Section 6.1, paragraph 94
```
us { description "A condition for anti-virus"; leaf-list profile { type strin
                                  ^^^^^^^^^^
```
This word is normally spelled as one.

#### Section 6.1, paragraph 97
```
hs are filenames/paths to be excluded and relative ones are interpreted as gl
                                     ^^^^
```
Use a comma before "and" if it connects two independent clauses (unless they
are closely connected and short).

#### Section 6.1, paragraph 114
```
ed as a binary to accommodate any kind of a payload type such as HTTP, HTTPS,
                                  ^^^^^^^^^
```
If "kind" is a classification term, "a" is not necessary. Use "kind of". (The
phrases "kind of" and "sort of" are informal if they mean "to some extent".).

#### Section 6.1, paragraph 114
```
5 bytes of the payload. This field accept values greater than or equal to th
                                   ^^^^^^
```
The verb "accept" is plural. Did you mean: "accepts"? Did you use a verb
instead of a noun?

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool

Thanks to Joe Touch for the tsvart review, and the authors for responding.

(3) p 38, sec 6.1.  YANG Module of Consumer-Facing Interface

           leaf-list system-alarm {
             type identityref {
               base i2nsfmi:system-alarm;
             }
             description
               "The security policy rule according to
                system alarms.";
           }
         }
         container condition {
           description
           "Conditions for general security policies.";

Please include documentation here for the condition container as to how the different fields are combined (i.e., that all configured conditions must match for a rule to trigger).

(4) p 4, sec 3.  YANG Tree Diagram of Policy

   Resolution-strategy:  This field represents how to resolve conflicts
             that occur between actions of the same or different policy
             rules that are matched and contained in this particular
             NSF.  The resolution strategy is described in Section 3.2
             of [I-D.ietf-i2nsf-capability-data-model] in detail.

Given you document the default for language above, would it name sense to document the default matching rule here as well?


(5) p 7, sec 3.2.  Condition Sub-model

   The Condition object describes the network traffic pattern or fields
   that must be matched against the observed network traffic for the
   rule to trigger.  The fields used to express the required conditions
   to trigger the rule are organized around the class of NSFs expected
   to be able to observe or compute them.  Figure 5 shows the YANG tree
   of the Condition object.  The Condition Sub-model SHALL have the
   following information:

I find the use of "Case" confusing in the descriptions below.  I mistakenly thought that you were referring to the YANG case statements under choice, and hence only one of these conditions can be expressed for a given rule.


(6) p 20, sec 6.1.  YANG Module of Consumer-Facing Interface

     identity fmr {

Using longer identity names for the resolution-strategies may make the module more readable.  E.g. 'first-matching-rule' might be clearer than fmr.  If you change this, then I would suggest changing it for the other resolution-strategies as well (and the any default values).


(7) p 37, sec 6.1.  YANG Module of Consumer-Facing Interface

         leaf priority {
           type uint8;
           description
             "The priority of the rule to indicate the order of the
              rules to be matched. A higher value means a higher priority.
              The packet or flow will be matched with the rule with
              the highest priority value first and continues to a lower
              priority value. Once a rule matches the packet or flow,
              the NSF should execute the rule and terminate the matching
              process. If multiple rules have an equal priority, the
              actual order is undefined. The handling of the selection
              of those rules depends on the implementer, e.g., non-rule
              selection, first rule selection or random rule selection.";
         }

Did you consider using an "order-by-user" list to define the priority instead?  I.e., process the rules in the order that they are specified in the list.


(8) p 39, sec 6.1.  YANG Module of Consumer-Facing Interface

                   error-message
                     "An end port number MUST be equal to or greater than
                      a start port number.";

I would suggest changing this to 'must', or otherwise you need to add the standard RFC 2119 boilerplate to the YANG module (pyang can help with this).


(9) p 43, sec 6.1.  YANG Module of Consumer-Facing Interface

                 description
                   "This represents the repetition time.  In the case
                    where the frequency is weekly, the days can be
                    set.";

This comment is slightly misleading.  I would suggest deleting, or perhaps rewording "In the case where the frequency is weekly, the days can be set.";"


(10) p 43, sec 6.1.  YANG Module of Consumer-Facing Interface

                 leaf-list date {

'date' is a somewhat confusing name for this.  Would 'day-of-month' be better?


(11) p 44, sec 6.1.  YANG Module of Consumer-Facing Interface

                 leaf-list month {

'month' is a confusing name for this.  Would 'month-and-day' be better?


(12) p 44, sec 6.1.  YANG Module of Consumer-Facing Interface

                   description
                     "This represents the repeated date and month of
                      every year.  More than one can be specified.
                      A pattern used here is Month and Date (MM-DD).";

So, if you wanted the policy to apply for a particular 3 weeks per year, then I presume that it would be necessary to list each of those day separately?  Did you consider allowing ranges here, or what that be too much complexity?

Regards,
Rob