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

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

Hi Jan,
Could you update the state of YANGDOCTORS Last Call Review on
I2NSF Consumer-Facing Interface YANG Data Model  if the updates are fine to
you?

https://datatracker.ietf.org/doc/draft-ietf-i2nsf-consumer-facing-interface-dm/

I think your comments are all addressed in this version.

Thanks.

Best Regards,
Paul

On Thu, Mar 12, 2020 at 1:15 AM Mr. Jaehoon Paul Jeong <
[e-mail]> wrote:

> Hi Jan,
> We authors have addressed your comments with the revision:
>
> https://tools.ietf.org/html/draft-ietf-i2nsf-consumer-facing-interface-dm-08
>
> I attach a revision letter to explain how to respond to your comments.
>
> If you have further comments, please let me know.
>
> Thanks.
>
> Best Regards,
> Paul
>
> On Tue, Nov 12, 2019 at 1:53 AM Jan Lindblad via Datatracker <
> [e-mail]> wrote:
>
>> Reviewer: Jan Lindblad
>> Review result: Almost Ready
>>
>> This is my YD review of draft-ietf-i2nsf-consumer-facing-interface-dm-07.
>> I
>> have previously reviewed the -05 revision (~end June). I find the new
>> revision
>> much improved, but still with much to discuss. I will call this "almost
>> ready".
>>
>> Generally speaking, I think the YANG module lacks the precision and
>> descriptions needed to foster interoperability. The examples at the end
>> are
>> very enlightening however, and compensate for much of that, but their
>> informal
>> nature can never replace proper YANG. The module usage needs to be mostly
>> clear
>> from the module itself.
>>
>> The management access control model proposed here is, even with its latest
>> adaptation towards NACM, is still quite different from NACM author's
>> original
>> ideas. I will therefore bring this use case up in the NETMOD WG for
>> discussion.
>>
>> 1. Network access control principles
>>
>> Network access control is about which users are able to use the network
>> being
>> managed, for example connect to facebook. The purpose of the NSF module
>> is to
>> control this access. This version of the YANG module is now based on a
>> list of
>> policies.
>>
>> Each policy has a list of rules. Each rule has an event -- condition --
>> action
>> triplet. This resembles traditional firewall management, which is a good
>> thing,
>> because that concept is stable and much tried. This allows operators to
>> create
>> lists of rules in this style:
>>
>> if pkt.x == 1: drop                     // Rule 1
>> elif pkt.y > 2: alert                   // Rule 2
>> elif pkt.z == 10: pass          // Rule 3
>> else: drop                              // Rule 4
>>
>> This pattern relies heavily on the ability to control the order of the
>> rules.
>> The current model relies on the alphabetical sorting of names rules for
>> the
>> ordering. The YANG trick I would recommend to give operators the ability
>> to
>> insert and move rules as they wish is to add ordered-by user on the list:
>>
>>     list rule {
>>       ordered-by user;  // <== Add this line
>>       leaf rule-name {
>>
>> Nothing is said about what the system should do in case policies
>> conflict. What
>> if one policy says pass, the other drop for the same packet? Please
>> clarify.
>> What should happen to packets that do not match any of the policies?
>>
>> This module also assumes that all users in the operator's organization are
>> listed in one or more NACM groups (e.g. "employees"). That wasn't really
>> the
>> NACM authors' original intent. Even if this could be made to work maybe,
>> there
>> is no strong reason to repurpose the NACM group concept for user network
>> access
>> purposes. It could easily be modeled differently. So in the cases where
>> there
>> are leafrefs to NACM groups when dealing with network access rather than
>> management access, don't use NACM groups.
>>
>>                 leaf src-target {
>>                   type leafref {
>>                     path
>> /nacm:nacm/nacm:groups/nacm:group/nacm:user-name;  //
>>                     <== Point to some other list of network users
>>
>> 2. Management access control principles
>>
>> Management access control is about which users are able to configure/run
>> actions/the policies and rules. IMO, the most controversial aspect of this
>> module has always been its new and creative management access control
>> model. In
>> this revision, the management principles have been remodeled greatly to
>> fit in
>> with NACM. I find this redesign very promising, but the result is still
>> not
>> quite ready for publication.
>>
>> The point where integration with NACM concepts is important is when it
>> comes to
>> allow some users to CRUD the NSF policies and rules themselves. There is
>> now a
>> leaf-list "owners" on each policy and rule which point to a list of NACM
>> groups. My understanding is that the idea is that the NSF module should
>> be seen
>> as a service model that translate high level ownership information to
>> specific
>> NACM rules. It would be good to mention these ideas somewhere in the NSF
>> document.
>>
>>   leaf-list owners {
>>     type leafref {
>>       path /nacm:nacm/nacm:groups/nacm:group/nacm:name;
>>
>> I expect the intent is that any user listed in a NACM group mentioned in
>> the
>> owners list would get full CRUD privileges for the contents of the rule
>> the
>> owners leaf sits on. That is never spelled out anywhere, however.
>>
>> It is a little less clear how leaf-list owners on policy objects should be
>> handled. Should owners listed on a policy object get full CRUD powers
>> over the
>> entire policy, including all the rules inside? Or would they need to be
>> listed
>> on the rules as well? Not clear. Is the intent that users not listed on
>> the
>> policy object are unable to create new rules, but to be able to update
>> rules
>> they are listed as owners of, if any?
>>
>> Who is allowed to create new policy objects? Should users that are not
>> owners
>> get read access to all the policies and rules?
>>
>> Finally, there is an "owner" leaf on each rule with an identityref
>> allowing
>> operators to configure a role name like dept-head or sec-admin. It is
>> marked
>> mandatory, but never included in any of the examples at the end of the
>> document. This makes me think this may be a remnant from bygone times and
>> should be removed from the YANG. If not, an explanation of how to use this
>> leaf, and how it interacts with "owners" needs to be added, and the
>> examples
>> updated.
>>
>> 3. leafrefs crosspointing between policy instances
>>
>> There are six leafrefs that point to various objects inside a policy,
>> e.g. a
>> rule condition pointing to a device group name. None of them restrict
>> what can
>> be pointed to so that only names within the current policy are valid. It
>> is
>> therefore possible to configure the name of a device group defined in a
>> different policy. I suspect this is not the intention. In order to
>> restrict the
>> leafrefs to the same policy, the following predicate can be added:
>>
>>                 leaf-list src-target {
>>                   type leafref {
>>                   path
>>
>> "/i2nsf-cfi-policy[policy-name=current()/../../../../../policy-name]/endpoint-group/device-group/name";
>>                    // <== Add predicate
>>
>> 4. Mandatory to implement all events, conditions, actions
>>
>> Each rule is defined with a choice of different events (admin, time),
>> conditions (firewall, ddos, custom, threat-feed) and actions (pass, drop,
>> alert, mirror, ...). Is the intent that all of these options should be
>> mandatory to implement? The current model requires this. Also, would it
>> make
>> sense to allow additional mechanisms here? If so, it may be good to
>> explain to
>> readers how the set of choices and identities can be extended by
>> implementations.
>>
>> 5. Optional and mandatory elements
>>
>> In this revision of the module, 8 leafs have been marked mandatory. A few
>> of
>> them are list keys, which are conventionally not marked mandatory, since
>> list
>> keys are always mandatory. A few others are skipped in the XML examples
>> at the
>> end of the NSF document, which makes me believe they might not really be
>> mandatory after all.
>>
>> Three leafs have a default, but most leafs are left optional without any
>> default. In many cases I think I understand what it means to not set a
>> leaf,
>> but with the ones listed here, I don't think it clear at all. Either add a
>> default to make it clear, make them mandatory if they should be, or
>> explain in
>> the leaf description what happens if not set.
>>
>> 493: leaf-list name
>> 513: leaf-list protocol
>> 531: leaf geo-ip-ipv4
>> 541: leaf continent
>> 562: leaf feed-server-ipv4
>> 585: leaf payload-description
>> 590: leaf-list content
>> 600: leaf-list owners
>> 870: leaf method
>>
>> 6. Indentation
>>
>> The YANG indentation is mostly wrong. Run the YANG text through pyang or
>> some
>> other tool that can indent the content correctly before you put it into a
>> document.
>>
>> 7. YANG element naming
>>
>> The YANG convention is to not have lists on the top level in the YANG
>> module,
>> but to surround lists with a container. The surrounding container often
>> has a
>> name in the plural and the list in singluar, like this
>>
>> container interfaces {
>>     list interface {
>>
>> To better fit into the world of IETF YANG modules, I'd recommend turning
>> the
>> top level list i2nsf-cfi-policy into this instead:
>>
>> container i2nsf-cfi-policies {
>>     list policy {
>>
>> Further down, I would change container rule to rules:
>>
>> container rules {
>>     list rule {
>>
>> Finally, it is customary to not repeat the names of parent object in the
>> names
>> of elements. For example, in the following:
>>
>> list threat-feed-list
>>     leaf feed-name
>>     leaf feed-server-ipv4
>>     leaf feed-server-ipv6
>>     leaf feed-description
>>
>> all the leafs should normally not repeat "feed-". Just leaf name, leaf
>> server-ipv4, leaf server-ipv6, leaf description. There are many more
>> examples
>> of this throughout the module.
>>
>> The condition choice has many containers with a single leaf inside (e.g.
>> ddos-source). Their purpose is rather unclear to me. Remove?
>>
>>               container ddos-source {
>>                 description
>>                 "This represents the source.";
>>                 leaf-list src-target {
>>
>> Also, I find the name "src-target" rather confusing. How about "source"?
>>
>> 8. No date leaf
>>
>> The draft text near fig 2 talks about a date leaf. There is no date
>> object in
>> this revision of te YANG.
>>
>> "Date:  Date when this object was created or last modified"
>>
>> 9. leaf owner
>>
>> Near fig.3 leaf Owner is mentioned. Is this leaf still current?
>>
>> "Owner: This field contains the onwer of the rule.  For example,
>>              the person who created it, and eligible for modifying it."
>>
>> 10. leaf packet-per-second
>>
>> This is now modeled as uint16. Is this future proof? Many packet flows on
>> the
>> internet exceed 64k pps.
>>
>> 11. container custon-source
>>
>> Misspelled. Should be custom-source
>>
>> 12. identity ddos
>>
>> Is ddos a malware file-type? This is not exactly in line with my
>> intuition.
>>
>> 13. identity protocol-type
>>
>> There are other modules that already define protocol-types. Would it be
>> worth
>> reusing one of them?
>>
>> 14. identity palo-alto
>>
>> Is it a good IETF practice to list vendor names in modules? Can we
>> consider
>> this a protocol name? Is there perhaps an RFC/specification name for it
>> that we
>> could reference instead?
>>
>> 15. grouping ipsec-based-method
>>
>> This grouping contains a list which allows listing none of, either of or
>> both
>> of ipsecike and ikeless. Are all valid configurations?
>>
>> 16. leaf feed-name
>>
>> This leaf is the key in a list, which makes it possible to have at most
>> one
>> feed of each type. If it would make sense to configure more than one feed
>> of
>> the same type, the YANG needs to be updated here.
>>
>> 17. leaf-list content
>>
>> This leaflist is of type string. What is the format of this string? Does
>> the
>> name refer to something?
>>
>> 18. Event types
>>
>> container event has a choice between enforce-admin and time alternatives.
>> Each
>> of those choices have a leaf that allows the operator to configure an
>> identityref to an enforce-type value. What does that mean? What would it
>> mean
>> if an operator configured admin == 'time' (or enforce-time == 'admin')?
>>
>> 19. leaf begin-time, end-time
>>
>> >From the examples, it can be seen that these are meant to be a time of
>> day
>> values. Currently they are modeled as yang:date-and-time, which means
>> they are
>> a concrete time a specific day, e.g. 2019-11-11T16:07. This needs to be
>> changed
>> in order to be what the modeler intended. Perhaps like this:
>>
>> typedef time-of-day {
>>     type string {
>>         pattern '(2[0-3]|[01]?[0-9]):[0-5][0-9]';
>>     }
>> }
>>
>> 20. leaf frequency
>>
>> This leaf is now modeled properly from a YANG perspective. But what does
>> it
>> mean? If this leaf is set to 'once-only', what exactly will happen only
>> once?
>> Please write a description that explains this.
>>
>> 21. Example in Fig.17
>>
>> The example contains XML that refers to "endpoint-group/user-group".
>> There is
>> no such element in the YANG.
>>
>> <endpoint-group xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy">
>>   <user-group>
>>
>> Furthermore, there is nothing called range-ip-address, start-ip-address,
>> end-ip-address. They are called range-ipv4-address, start-ipv4-address,
>> end-ipv4-address.
>>
>>     <range-ip-address>
>>       <start-ip-address>221.159.112.1</start-ip-address>
>>       <end-ip-address>221.159.112.90</end-ip-address>
>>     </range-ip-address>
>>
>> Finally, there must not be any xmlns attribute on an closing XML tag. So
>>
>> </endpoint-group
>> xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy">
>>
>> should be
>>
>> </endpoint-group>
>>
>> This happens in several of the examples.
>>
>> 22. Example in Fig.18
>>
>> There is no element called policy any more. It's now i2nsf-cfi-policy.
>>
>>    <policy xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy">
>>      <policy-name>security_policy_for_blocking_sns</policy-name>
>>
>> The rules are modeled in a container and list, both by the name rule. So
>> there
>> needs to be two <rule> tags.
>>
>>      <rule>
>>        <rule-name>block_access_to_sns_during_office_hours</rule-name>
>>
>> The security-event element is marked mandatory in the YANG, but missing
>> in the
>> example. The times given below may be what is intended, but do not match
>> the
>> date format for the type used (which include a date, etc).
>>
>>        <event>
>>          <time-information>
>>            <begin-time>09:00</begin-time>
>>            <end-time>18:00</end-time>
>>          </time-information>
>>        </event>
>>
>> Since the example is not mentioning leaf frequency, it will have the value
>> 'once-only'. Maybe explain what that means in the context of the example?
>>
>> The condition/firewall-condition says the src-target is mandatory and
>> dest-target optional, exactly like below.
>> condition/custom-destination/dest-target is mandatory and src-target is
>> optional, exactly like below. Is this pure luck, or is there a logical
>> explanation why exactly those should be mandatory, and the example use
>> precisely those?
>>
>>        <condition>
>>          <firewall-condition>
>>            <source-target>
>>              <src-target>employees</src-target>
>>            </source-target>
>>          </firewall-condition>
>>          <custom-condition>
>>            <destination-target>
>>              <dest-target>sns-websites</dest-target>
>>            </destination-target>
>>          </custom-condition>
>>
>> The current YANG model does not allow setting both a firewall-condition
>> and
>> custom-condition. If that should be allowed, the model needs to change.
>> Should
>> it be possible to have multiple firewall- or other conditions? That is not
>> currently possible.
>>
>> This example leaves out the mandatory leaf owner. Is that a sign that it
>> should
>> not be mandatory, or perhaps not exist at all?
>>
>> 23. Example in Fig.19
>>
>> This example lists a firewall-condition with no src-target, which is
>> mandatory.
>>
>>       <firewall-condition>
>>         <destination-target>
>>           <dest-target>employees</dest-target>
>>         </destination-target>
>>       </firewall-condition>
>>
>> Under condition, there is a container rate-limit with a leaf
>> packet-per-second.
>> Is this a trigger value for the condition, or is it an actual limit that
>> the
>> system is expected to enforce? If it's a trigger, it may be good to find a
>> clearer name. If it's enforced, it's placement under condition is
>> deceiving.
>>
>> If a rule's action is set to 'rate-limit', to which rate will it be
>> limited?
>>
>> 24. Security Considerations
>>
>> Section 10 in the NSF document under review is the Security
>> Considerations. I
>> think it would make sense to mention something about the management access
>> control mechanism here, and its relation to NACM.
>>
>> (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>
>


-- 
===========================
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>