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

[Jan Lindblad <[e-mail]>]

Paul, I2NSF WG,

Thank you for going through all my comments. I can see you have worked hard to fix up the module. I still see ample opportunities to improve it, but let's come back to those details later. What I miss most of all is a discussion within the NETMOD WG around this new proposed management access control model. 

The proposed model, as far as I understand it, would have a high impact on all existing NETCONF/RESTCONF server implementations and break some architectural principles, so I think we should expect a lot of skepticism there if we propose the current module.

Having read RFC 8192, I can see the vision you are painting. I think it's definitely worthwhile looking at ways to realize that idea. I believe the vision there can be realized in a much less disruptive fashion if we reorganized ietf-i2nsf-cfi-policy.yang to rely on the existing RFC 8341 Network Configuration Access Control Model.

I volunteer to provide a sketch of how the current module could be restructured to fit in with NACM, if you are interested. If you ask me to do this, I will bring the result back to the I2NSF WG for further discussion and possibly further iterations.

In the end, any management access control functionality changes would have to be discussed in the NETMOD WG.

Also, if you already know of any particular party that is interested to implement this, it would be great to have implementor involvement in the design process from this point onward.

Let me know if you want me to propose a way to redesign ietf-i2nsf-cfi-policy.yang to fit better with NACM.

Best Regards,
/jan




> On 25 Jul 2019, at 16:25, Mr. Jaehoon Paul Jeong <[e-mail]> wrote:
> 
> Hi Jan,
> Here is the revision letter for the revised draft, reflecting your comments along with the revised draft:
> https://tools.ietf.org/html/draft-ietf-i2nsf-consumer-facing-interface-dm-06 <https://tools.ietf.org/html/draft-ietf-i2nsf-consumer-facing-interface-dm-06> 
>  
> If you have further comments and questions, please let me know.
> 
> Thanks.
> 
> Best Regards,
> Paul
> 
> On Wed, Jun 26, 2019 at 12:00 PM Jan Lindblad via Datatracker <[e-mail] <mailto:[e-mail]>> 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 <http://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)
> 
> 
> _______________________________________________
> I2nsf mailing list
> [e-mail] <mailto:[e-mail]>
> https://www.ietf.org/mailman/listinfo/i2nsf <https://www.ietf.org/mailman/listinfo/i2nsf>
> 
> 
> -- 
> ===========================
> Mr. Jaehoon (Paul) Jeong, Ph.D.
> Associate Professor
> Department of Software
> Sungkyunkwan University
> Office: [phone]
> Email: [e-mail] <mailto:[e-mail]>, [e-mail] <mailto:[e-mail]>
> Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php <http://cpslab.skku.edu/people-jaehoon-jeong.php>
> <Revision-Letter-for-I2NSF-Consumer-Facing-Interface-20190725.pdf>