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

[limited dist]

This is a really comprehensive review.

My nominee for best line in a YD review for 2019:

     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.

Ouch!

Andy

On Wed, Jun 26, 2019 at 9:00 AM Jan Lindblad via Datatracker <
noreply@ietf.org> wrote:

> 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)
>
>
> _______________________________________________
> yang-doctors mailing list
> yang-doctors@ietf.org
> https://www.ietf.org/mailman/listinfo/yang-doctors
>