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

["Mr. Jaehoon Paul Jeong" <[e-mail]>]

Hi Jan,
Thanks for your volunteering to improve our Consumer-Facing Interface YANG
Module.
Could you propose a way to redesign ietf-i2nsf-cfi-policy.yang to befit
NACM?

I am not aware of any particular party interested to implement our data
model.

Thanks.

Best Regards,
Paul

On Fri, Jul 26, 2019 at 11:51 PM Jan Lindblad <[e-mail]> wrote:

> 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
>
>
> 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]> 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
>> [e-mail]
>> 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], [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>
>
>
>

-- 
===========================
Mr. Jaehoon (Paul) Jeong, Ph.D.
Associate Professor
Department of Software
Sungkyunkwan University
Office: [phone]
Email: [e-mail], [e-mail]
Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php
<http://cpslab.skku.edu/people-jaehoon-jeong.php>