Last Call Review of draft-ietf-i2nsf-consumer-facing-interface-dm-07
review-ietf-i2nsf-consumer-facing-interface-dm-07-yangdoctors-lc-lindblad-2019-11-11-00

This is my YD review of draft-ietf-i2nsf-consumer-facing-interface-dm-07. I
have previously reviewed the -05 revision (~end June). I find the new revision
much improved, but still with much to discuss. I will call this "almost ready".

Generally speaking, I think the YANG module lacks the precision and
descriptions needed to foster interoperability. The examples at the end are
very enlightening however, and compensate for much of that, but their informal
nature can never replace proper YANG. The module usage needs to be mostly clear
from the module itself.

The management access control model proposed here is, even with its latest
adaptation towards NACM, is still quite different from NACM author's original
ideas. I will therefore bring this use case up in the NETMOD WG for discussion.

1. Network access control principles

Network access control is about which users are able to use the network being
managed, for example connect to facebook. The purpose of the NSF module is to
control this access. This version of the YANG module is now based on a list of
policies.

Each policy has a list of rules. Each rule has an event -- condition -- action
triplet. This resembles traditional firewall management, which is a good thing,
because that concept is stable and much tried. This allows operators to create
lists of rules in this style:

if pkt.x == 1: drop                     // Rule 1
elif pkt.y > 2: alert                   // Rule 2
elif pkt.z == 10: pass          // Rule 3
else: drop                              // Rule 4

This pattern relies heavily on the ability to control the order of the rules.
The current model relies on the alphabetical sorting of names rules for the
ordering. The YANG trick I would recommend to give operators the ability to
insert and move rules as they wish is to add ordered-by user on the list:

    list rule {
      ordered-by user;  // <== Add this line
      leaf rule-name {

Nothing is said about what the system should do in case policies conflict. What
if one policy says pass, the other drop for the same packet? Please clarify.
What should happen to packets that do not match any of the policies?

This module also assumes that all users in the operator's organization are
listed in one or more NACM groups (e.g. "employees"). That wasn't really the
NACM authors' original intent. Even if this could be made to work maybe, there
is no strong reason to repurpose the NACM group concept for user network access
purposes. It could easily be modeled differently. So in the cases where there
are leafrefs to NACM groups when dealing with network access rather than
management access, don't use NACM groups.

                leaf src-target {
                  type leafref {
                    path /nacm:nacm/nacm:groups/nacm:group/nacm:user-name;  //
                    <== Point to some other list of network users

2. Management access control principles

Management access control is about which users are able to configure/run
actions/the policies and rules. IMO, the most controversial aspect of this
module has always been its new and creative management access control model. In
this revision, the management principles have been remodeled greatly to fit in
with NACM. I find this redesign very promising, but the result is still not
quite ready for publication.

The point where integration with NACM concepts is important is when it comes to
allow some users to CRUD the NSF policies and rules themselves. There is now a
leaf-list "owners" on each policy and rule which point to a list of NACM
groups. My understanding is that the idea is that the NSF module should be seen
as a service model that translate high level ownership information to specific
NACM rules. It would be good to mention these ideas somewhere in the NSF
document.

  leaf-list owners {
    type leafref {
      path /nacm:nacm/nacm:groups/nacm:group/nacm:name;

I expect the intent is that any user listed in a NACM group mentioned in the
owners list would get full CRUD privileges for the contents of the rule the
owners leaf sits on. That is never spelled out anywhere, however.

It is a little less clear how leaf-list owners on policy objects should be
handled. Should owners listed on a policy object get full CRUD powers over the
entire policy, including all the rules inside? Or would they need to be listed
on the rules as well? Not clear. Is the intent that users not listed on the
policy object are unable to create new rules, but to be able to update rules
they are listed as owners of, if any?

Who is allowed to create new policy objects? Should users that are not owners
get read access to all the policies and rules?

Finally, there is an "owner" leaf on each rule with an identityref allowing
operators to configure a role name like dept-head or sec-admin. It is marked
mandatory, but never included in any of the examples at the end of the
document. This makes me think this may be a remnant from bygone times and
should be removed from the YANG. If not, an explanation of how to use this
leaf, and how it interacts with "owners" needs to be added, and the examples
updated.

3. leafrefs crosspointing between policy instances

There are six leafrefs that point to various objects inside a policy, e.g. a
rule condition pointing to a device group name. None of them restrict what can
be pointed to so that only names within the current policy are valid. It is
therefore possible to configure the name of a device group defined in a
different policy. I suspect this is not the intention. In order to restrict the
leafrefs to the same policy, the following predicate can be added:

                leaf-list src-target {
                  type leafref {
                  path
                  "/i2nsf-cfi-policy[policy-name=current()/../../../../../policy-name]/endpoint-group/device-group/name";
                   // <== Add predicate

4. Mandatory to implement all events, conditions, actions

Each rule is defined with a choice of different events (admin, time),
conditions (firewall, ddos, custom, threat-feed) and actions (pass, drop,
alert, mirror, ...). Is the intent that all of these options should be
mandatory to implement? The current model requires this. Also, would it make
sense to allow additional mechanisms here? If so, it may be good to explain to
readers how the set of choices and identities can be extended by
implementations.

5. Optional and mandatory elements

In this revision of the module, 8 leafs have been marked mandatory. A few of
them are list keys, which are conventionally not marked mandatory, since list
keys are always mandatory. A few others are skipped in the XML examples at the
end of the NSF document, which makes me believe they might not really be
mandatory after all.

Three leafs have a default, but most leafs are left optional without any
default. In many cases I think I understand what it means to not set a leaf,
but with the ones listed here, I don't think it clear at all. Either add a
default to make it clear, make them mandatory if they should be, or explain in
the leaf description what happens if not set.

493: leaf-list name
513: leaf-list protocol
531: leaf geo-ip-ipv4
541: leaf continent
562: leaf feed-server-ipv4
585: leaf payload-description
590: leaf-list content
600: leaf-list owners
870: leaf method

6. Indentation

The YANG indentation is mostly wrong. Run the YANG text through pyang or some
other tool that can indent the content correctly before you put it into a
document.

7. YANG element naming

The YANG convention is to not have lists on the top level in the YANG module,
but to surround lists with a container. The surrounding container often has a
name in the plural and the list in singluar, like this

container interfaces {
    list interface {

To better fit into the world of IETF YANG modules, I'd recommend turning the
top level list i2nsf-cfi-policy into this instead:

container i2nsf-cfi-policies {
    list policy {

Further down, I would change container rule to rules:

container rules {
    list rule {

Finally, it is customary to not repeat the names of parent object in the names
of elements. For example, in the following:

list threat-feed-list
    leaf feed-name
    leaf feed-server-ipv4
    leaf feed-server-ipv6
    leaf feed-description

all the leafs should normally not repeat "feed-". Just leaf name, leaf
server-ipv4, leaf server-ipv6, leaf description. There are many more examples
of this throughout the module.

The condition choice has many containers with a single leaf inside (e.g.
ddos-source). Their purpose is rather unclear to me. Remove?

              container ddos-source {
                description
                "This represents the source.";
                leaf-list src-target {

Also, I find the name "src-target" rather confusing. How about "source"?

8. No date leaf

The draft text near fig 2 talks about a date leaf. There is no date object in
this revision of te YANG.

"Date:  Date when this object was created or last modified"

9. leaf owner

Near fig.3 leaf Owner is mentioned. Is this leaf still current?

"Owner: This field contains the onwer of the rule.  For example,
             the person who created it, and eligible for modifying it."

10. leaf packet-per-second

This is now modeled as uint16. Is this future proof? Many packet flows on the
internet exceed 64k pps.

11. container custon-source

Misspelled. Should be custom-source

12. identity ddos

Is ddos a malware file-type? This is not exactly in line with my intuition.

13. identity protocol-type

There are other modules that already define protocol-types. Would it be worth
reusing one of them?

14. identity palo-alto

Is it a good IETF practice to list vendor names in modules? Can we consider
this a protocol name? Is there perhaps an RFC/specification name for it that we
could reference instead?

15. grouping ipsec-based-method

This grouping contains a list which allows listing none of, either of or both
of ipsecike and ikeless. Are all valid configurations?

16. leaf feed-name

This leaf is the key in a list, which makes it possible to have at most one
feed of each type. If it would make sense to configure more than one feed of
the same type, the YANG needs to be updated here.

17. leaf-list content

This leaflist is of type string. What is the format of this string? Does the
name refer to something?

18. Event types

container event has a choice between enforce-admin and time alternatives. Each
of those choices have a leaf that allows the operator to configure an
identityref to an enforce-type value. What does that mean? What would it mean
if an operator configured admin == 'time' (or enforce-time == 'admin')?

19. leaf begin-time, end-time

From the examples, it can be seen that these are meant to be a time of day
values. Currently they are modeled as yang:date-and-time, which means they are
a concrete time a specific day, e.g. 2019-11-11T16:07. This needs to be changed
in order to be what the modeler intended. Perhaps like this:

typedef time-of-day {
    type string {
        pattern '(2[0-3]|[01]?[0-9]):[0-5][0-9]';
    }
}

20. leaf frequency

This leaf is now modeled properly from a YANG perspective. But what does it
mean? If this leaf is set to 'once-only', what exactly will happen only once?
Please write a description that explains this.

21. Example in Fig.17

The example contains XML that refers to "endpoint-group/user-group". There is
no such element in the YANG.

<endpoint-group xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy">
  <user-group>

Furthermore, there is nothing called range-ip-address, start-ip-address,
end-ip-address. They are called range-ipv4-address, start-ipv4-address,
end-ipv4-address.

    <range-ip-address>
      <start-ip-address>221.159.112.1</start-ip-address>
      <end-ip-address>221.159.112.90</end-ip-address>
    </range-ip-address>

Finally, there must not be any xmlns attribute on an closing XML tag. So

</endpoint-group xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy">

should be

</endpoint-group>

This happens in several of the examples.

22. Example in Fig.18

There is no element called policy any more. It's now i2nsf-cfi-policy.

   <policy xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy">
     <policy-name>security_policy_for_blocking_sns</policy-name>

The rules are modeled in a container and list, both by the name rule. So there
needs to be two <rule> tags.

     <rule>
       <rule-name>block_access_to_sns_during_office_hours</rule-name>

The security-event element is marked mandatory in the YANG, but missing in the
example. The times given below may be what is intended, but do not match the
date format for the type used (which include a date, etc).

       <event>
         <time-information>
           <begin-time>09:00</begin-time>
           <end-time>18:00</end-time>
         </time-information>
       </event>

Since the example is not mentioning leaf frequency, it will have the value
'once-only'. Maybe explain what that means in the context of the example?

The condition/firewall-condition says the src-target is mandatory and
dest-target optional, exactly like below.
condition/custom-destination/dest-target is mandatory and src-target is
optional, exactly like below. Is this pure luck, or is there a logical
explanation why exactly those should be mandatory, and the example use
precisely those?

       <condition>
         <firewall-condition>
           <source-target>
             <src-target>employees</src-target>
           </source-target>
         </firewall-condition>
         <custom-condition>
           <destination-target>
             <dest-target>sns-websites</dest-target>
           </destination-target>
         </custom-condition>

The current YANG model does not allow setting both a firewall-condition and
custom-condition. If that should be allowed, the model needs to change. Should
it be possible to have multiple firewall- or other conditions? That is not
currently possible.

This example leaves out the mandatory leaf owner. Is that a sign that it should
not be mandatory, or perhaps not exist at all?

23. Example in Fig.19

This example lists a firewall-condition with no src-target, which is mandatory.

      <firewall-condition>
        <destination-target>
          <dest-target>employees</dest-target>
        </destination-target>
      </firewall-condition>

Under condition, there is a container rate-limit with a leaf packet-per-second.
Is this a trigger value for the condition, or is it an actual limit that the
system is expected to enforce? If it's a trigger, it may be good to find a
clearer name. If it's enforced, it's placement under condition is deceiving.

If a rule's action is set to 'rate-limit', to which rate will it be limited?

24. Security Considerations

Section 10 in the NSF document under review is the Security Considerations. I
think it would make sense to mention something about the management access
control mechanism here, and its relation to NACM.

(End of list)