[yang-doctors] Yangdoctors last call review of draft-ietf-i2nsf-consumer-facing-interface-dm-05

[Jan Lindblad via Datatracker <[e-mail]>]

Reviewer: Jan Lindblad
Review result: On the Right Track

This is my YANG Doctor review of
draft-ietf-i2nsf-consumer-facing-interface-dm-05.txt, and the YANG module
ietf-i2nsf-cfi-policy in particular. The document is on the right track, but
several important discussions remain, so is not quite ready for last call.

There are plenty of smaller things to fix in this module, see below for a list.
There are a couple of potentially major issues in this module, however, that I
would like to start with. The first relates to the management access control
for this module.

1) Management access control

This RFC defines a new management access control mechanism, entirely unrelated
to the traditional NACM model (RFC 8341/RFC 6536). It also specifies things
like which auth mechanisms should be used by clients in specific domains when
connecting to the management server. This appears highly disruptive to the
existing base of NETCONF/RESTCONF servers.

To make it worse, the new security model described in this document is rather
heavily unspecified. It may be that this security model is well thought
through, but there is no language or other evidence that shows this in the
text. The YANG model representation of it is broken in many places.

The RFC text refers to a device called "Security Controller". By reading the
parent RFC 8329, which contains an overview of the architecture, I believe the
Security Controller is the management (NETCONF/RESTCONF/...) server with the
ietf-i2nsf-cfi-policy YANG model, and the user is supposed to connect directly
with it. This means, I would think, that the management server would need to be
the component to implement the new management access control model.

Key in this new mechanism is a leaf owner. There isn't much information about
the owner leaf, but the RFC text relating to "owner" above Fig. 3 says:

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

And further, in the YANG module itself, we find:

  leaf owner {
        type string;
          description
           "This field defines the owner of this
           policy. Only the owner is authorized to
           modify the contents of the policy.";

This is all that is mentioned about the owner leaf. That the type of the leaf
is string does not give much clues as to how this is meant to work.

Then there are Policy Domains. A Policy Domain has an (optional) auth method
(which is not correctly modeled in YANG) and a list of Policy Tenants. Each
Policy Tenant represents one department or other part of the organization.

Policy Users are individual users that are allowed to access the management
server to CRUD policies and rules. Either within a tenant (default) or domain.
There is no linkage to which tenant or domain, however. Each Policy User has
exactly one reference to a an access profile in the Policy Role object. The
only information this linkage provides is whether the access is read-only or
read-write.

In summary, I believe significant work is required to get this to a workable
state. In order to invent a new management access control mechanism, a wider
discussion in the NETMOD working group would be needed.

Below I note some details that I came up with while trying to figure this out.
Fixing them will not fix this issue as a whole.

- container policy-mgnt-auth-method should probably be a list
- container policy-role should probably be a list, and the list access-profile
removed - list policy-tenant should probably not have any leaf domain - the
leaf owner description talks about owner of the policy, while the leaf sits on
an individual rule. Either the description or the leaf placement must be wrong.
- I believe there are more issues to look at as part of the "translation from
UML to YANG" that this module has been subject to.

2) container policy

The top level container policy can only exist in a single instance as currently
modeled. Even if not stated explicitly in the document, I get the feeling that
the author's intent is that it should be possible to have more than one policy
in the system. If this is the case, container policy needs to change to list
policy (and probably add a surrounding container policies). Doing so would also
require updating all the leafrefs used throughout the module, to only select
nodes in the current policy.

Another concern is that the name "policy" is very generic. There are many
modules out there which already define a top level object called policy.
Searching for container policy and list policy on yangcatalog.org gives several
hundred pages with hits. Most of them will not be on the top level, but this
shows that "policy" is a popular concept. How about calling the top level
container cfi-policy or i2nsf-cfi-policy?

3) Are rules ordered?

In the RFC section 4, it says regarding list rule:

             This field contains a list of rules.  If the rule does not
             have a user-defined precedence, then any conflict must be
             manually resolved.

I'm not sure what a "user-defined precedence" is, or at least I can't find
anything like that in the YANG. So how are the rules meant to be applied on the
devices? What does "manually resolved" mean, who does that when and how? If
different operators cannot see each other's rules, does that mean that
"manually resolved" just got harder?

Each policy contains a set of rules in a list. This list is keyed by rule name
and modeled as ordered-by system, i.e. rules will be sorted alphabetically. Is
it up to the server to decide in which sequence these rules are applied. I
suppose they could be overlapping, leading to different results based on the
order rules are applied, and there could be performance implications depending
on how this is done. If several policies are allowed, the issue only grows.

4) Restricted to IPv4

The module specifically uses IPv4 in many places. Would it not make sense to
use ip-address instead, to allow the use of IPv6 also, or is there some
particular reason to exclude IPv6? If so, it would be good to mention this
reason.

In a couple of places, an address range is specified, and the start address is
an ipv4-address while the end address is an ip-address. Surely that must be a
mistake.

5) Many optional leafs

The RFC text repeats many times: The XYZ object SHALL have the following
information... This could be understood as the succeeding information items are
mandatory. There are not a single mandatory element (apart from keys) in the
entire module, however. And only three leafs with a default statement.

It would be appropriate to go through the module and add mandatory and defaults
where they make sense. For the leafs that should remain optional, it would be
good if something could be added to the description regarding what the server
behavior is when the leaf has no value, when that is not entirely obvious.
Actually, it is often good to write even when it is obvious, because otherwise
the behavior is technically undefined, and implementations could get away with
bad behavior even when all know it's wrong.

A couple of examples: what happens if the owner leaf is left unset?
primary-action? enforce-type? recur? policy-tenant/domain?

6) Many strings

In the module, the type string is used for names, which is great, but also for
a many cases where some certain format of the content is expected, but not
defined. There is no reason to believe that will lead to interoperable
solutions. It will also leave users frustrated with little information to guess
what type of data in which format to fill in. This needs fixing.

294: leaf-list protocol
322: leaf-list content
388: leaf begin-time
393: leaf end-time
553: leaf primary-action
561: leaf secondary-action
581: leaf owner
697: leaf auth-method
823: leaf threat-feed-description
839: leaf-list signatures

7) leaf date

The grouping meta contains a name and a date leaf. The name is clearly meant to
be a configuration item filled in by the client. I'm not sure about the date
leaf, however. The description says

    description "This is the date when the entity is
    created or modified.";

The date leaf is currently modeled as config true, i.e. is expected to be
filled in by the client. It is not customary to have this sort of meta
information in YANG modules, but it would make some sense if this was config
false, i.e. computed by the server itself.

If that is the intent, the specification should only make the date leaf config
false and clarify how these time stamps should be calculated. Do they pertain
to this particular list entry, or would they be updated if any child entry to
this list entry was modified (similar to etags).

If the intent is that clients should fill this in if they want, for their own
optional housekeeping, I think that needs to be stated clearly. If the intent
is to make it mandatory for clients to keep these timestamps up to date, I
think the model is broken. What implications would it have if they were not
used?

8) container policy-mgnt-auth-method

The container has a description that says

        "This represents the list of authentication methods.";

A container is obviously not a list, so exactly what the author has in mind is
somewhat unclear. At the top of the container, there is a leaf

      leaf auth-method {
        type string;
        description
          "This represents the authentication method name.";

The format and usage of this string is highly unclear, but more importantly, it
seems to speak of a single authentication method, not a list.

Following this leaf is a number of lists; list password-based, list
token-based, list certificate-based, list ipsec-method, list single-sign-on.
Are these lists under container policy-mgnt-auth-method mutually exclusive, or
can several of them be configured at the same time?

This would need some clearing up. As this is an area where we can anticipate
new methods being introduced over time, it would be good if the model advised
how to extend with further auth methods.

9) More on container policy-mgnt-auth-method

RFC text sec 5.1 says

             Authentication method to be used for this
             domain.  It should be a reference to a "Policy-Management-
             Authentication-Method" object

But in fact there is only one instance of container policy-mgnt-auth-method.
Maybe this is meant to be a list?

In the RFC Fig. 8 YANG tree representation authentication-method points like
this

            +--rw authentication-method?   ->
            /policy/multi-tenancy/policy-mgnt-auth-method/name

In the YANG module itself, the path is different:

          path
          "/policy/multi-tenancy/policy-mgnt-auth-method/ipsec-method/method";

Also, the abbreviation of management as "mgnt" is not very common. It is
usually "mgmt". In order to reduce misspellings of policy-mgnt-auth-method I
suggest renaming it to policy-mgmt-auth-method. There is actually already one
such misspelling in this YANG module itself.

10) One cert server per cert type?

List certificate-based allows the configuration of certificate servers. Is
there usually a different server based on certificate type? Max one server can
be configured per certificate type, and max three total as there are three
certificate types. If this is what you want, the YANG says it nicely.

11) Enumerations vs. identities

203: certificate-type is defined as an enumeration. This means the module will
have to be revised if any new certificate types need to be supported one day.
An identity may be more appropriate here, as I expect new certificate types (or
formats) may come out.

366: enforce-type is defined as an enumeration. This means the module will have
to be revised if any new enforcement types need to be supported one day. An
identity or choice may be more appropriate here. Then an additional module
could add new identities or augment the choice.

55: permission-type is defined as an identity. Unless you foresee the need for
other values than read-only and read-write, you could use enumeration here
instead. It could simplify the module a little, but identity is fine too.

168: continent is defined as an identity. Unless you foresee the need for new
continents, you could use enumeration here instead. It could simplify the
module a little, but identity is fine too.

145: identity ransomeware is misspelled. Should be identity ransomware

12) Reference RFC 6087

The RFC text refers to RFC 6087, which is obsoleted by RFC 8407. Update the
reference?

13) YANG trees

The RFC text has many figure descriptions like this

    Figure X show the XML instance

What is shown is actually a YANG tree, so the text should be updated to say

    Figure X show the YANG tree

14) Indentation

The indentation of the YANG module is broken on many, many lines. Make sure to
use a YANG indentation tool before publishing the result.

15) Revision statement

The revision statement at the top of the file needs to be rewritten before
publication.

16) container recursive

The container recursive on line 399 seems to be about how rules recur, not
recurse (which is something completely different). Maybe change to the whole
container recursive to

    leaf frequency {
      type enumeration {
        enum only-once;
        enum daily;
        enum weekly;
        enum monthly;
      }
      default once-only;
    }

17) String passwords

Passwords are modeled as strings in a couple of places. This is not acceptable
from a security point of view. Use one of the cryptographic types for
passwords, such as ianach:crypt-hash RFC 7317.

666: leaf password
710: leaf password

The leaf password on line 710 has the description

            "This should be defined using the
            regular expression.";

What does that mean? Authentication, password, regular expression? I don't get
it.

18) Grouping descriptions

A number of groupings have descriptions like

    This grouping is to remove repetition of
        'name' and 'ip-address' fields.";

This is not exactly true since these groupings are used only once (hence no
repetition). Describe instead what the grouping is used for or what it contains.

231: grouping meta
280: grouping user-group
301: grouping location-group
316: grouping payload-string

19) container time-information

Is container time-information only relevant for enforce-type == time-enforced?
If so, a when expression on the container may be useful. Or, even better, make
the enforce-type a choice and have the time related content inside the choice
case instead.

20) container rate-limit

Is uint8 a good type to use for the rate limiting function? Would it never (now
or in the future) make sense to limit to more than 255 packets per second?

486: container rate-limit

21) container condition

This container seems to hold the configuration of many different triggering
conditions. Would several of these apply at the same time in a given rule?

There are many instances of container source-target and container
destination-target nested underneath. They seem to serve no purpose. Maybe
consider simplifying the model by removing them, unless they have some clever
function for the future.

22) Transactionality

Section 10.1 starts with the words

   In order to create a rule of a security policy, it is essential to
   first register data (those which are used to form such rule) to the
   database.

This could lead some implementors to believe it is acceptable to require that
the endpoints are committed separately from the rest of the configuration. The
actual requirement is that the referenced endpoints exist at the time the
transaction is committed. I.e. they could very well be part of the same
transaction.

23) Incomplete XML examples

The XML snippets in examples in Fig 24-27 use XML namespaces incorrectly. It's
easy to fix. Just make sure the first line with <ietf-i2nsf-cfi-policy:policy>
looks like this instead:

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

The last example, in section 10.4 is talking about <encrypt>. There is no such
in the YANG module (today).

       <encrypt>
         <ipsec-method>ipsec-ike</ipsec-method>
       </encrypt>

The XML loads fine if you just remove the encrypt tag, so maybe you mean:

         <ipsec-method>ipsec-ike</ipsec-method>

(End of list)