Hi Jan, I sincerely appreciate your detailed review and value comments on our draft. We authors will address your comments.
Thanks. Best Regards, Paul On Thu, Jun 27, 2019 at 1:00 AM Jan Lindblad via Datatracker < [email protected]> 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) > > > _______________________________________________ > I2nsf mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/i2nsf > -- =========================== Mr. Jaehoon (Paul) Jeong, Ph.D. Associate Professor Department of Software Sungkyunkwan University Office: +82-31-299-4957 Email: [email protected], [email protected] Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php <http://cpslab.skku.edu/people-jaehoon-jeong.php>
_______________________________________________ I2nsf mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2nsf
