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 looks like this instead: The last example, in section 10.4 is talking about . There is no such in the YANG module (today). ipsec-ike The XML loads fine if you just remove the encrypt tag, so maybe you mean: ipsec-ike (End of list)