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

"Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.com> Fri, 28 August 2020 13:43 UTC

Return-Path: <jaehoon.paul@gmail.com>
X-Original-To: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id D9D8B3A05AC; Fri, 28 Aug 2020 06:43:27 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.096
X-Spam-Level:
X-Spam-Status: No, score=-2.096 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HK_NAME_FM_MR_MRS=0.001, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 6-Cyge4ghZ1D; Fri, 28 Aug 2020 06:43:23 -0700 (PDT)
Received: from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com [IPv6:2a00:1450:4864:20::12a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 64DD93A058F; Fri, 28 Aug 2020 06:43:22 -0700 (PDT)
Received: by mail-lf1-x12a.google.com with SMTP id v12so729001lfo.13; Fri, 28 Aug 2020 06:43:22 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=bjTYOki75nHAqZMhffhkiCJU8lkoYf/p2dvGd+ljP5M=; b=ZV0i1hw0fubGd+bTnZGk8iZ8lGQnb/xmn7R7Tl3yn2BmddaCt/oUZntCaQVFINJsgy PswkKmGokt9d9EltKc6rceRdyxLJdJdix53waTSbO9PcVBwSk/aB8S0SK6zSdrKMKJfF 5mstMHcWsQw9T/AJ6sLSkB66ZbQXhSjAwk4KRloDl6ZYTQh8R+TiAh5Jhc9uHqwBxMpD 9+tDdu8iVcYtre4OLbMnJv54oVdKRaNOXbnPoVVjRLG6aUFBJUHjGu6JmAlPyDz6jzfT u1OI+/EkLDWu87thrmluGmMrFa/IvCyXOhLsPKvbXX/eQ1eejHnX+AOQ0cdzErvg/GXW ehoQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=bjTYOki75nHAqZMhffhkiCJU8lkoYf/p2dvGd+ljP5M=; b=t+WVb+zOZwXIzU+/BctedCtm3yQqVCckSu5x2A30yfXaHMeXgZaqdNizsGX9W6o/Jf CHfr2URrMpxh8nmKEHsWzocUEHad0pFeBiWNEZfcy6sOFe2G3NNouIWClZ0eopiIPVR1 g70lPe+ns3vlO/GKCmoEa69D2WZBBDqfcvkigBUbSx/4VApozrC961oJCl0Hyr1ST5v1 bMWsRCERMUdmqR1IB/7Pv94og2Mozch7VFqzKG+Lyp0W0Zk/1eqA0JN9/17UX4c2gcCQ c/9cdcuLffy/IvWYD8pvC8pd3LCLw0aEQw2Ln9HZVPHxEC08qiU8zSzalCf/l0kRVu+0 oRMQ==
X-Gm-Message-State: AOAM532jzrZdZ4bTGwSfgZGkbSn79xh6jcPh8ORMwwhAQWSLUVEgpFB6 R/KyP/KE5y1VXhs+o67j/RgawLiCwdGXXNaWp7g=
X-Google-Smtp-Source: ABdhPJw3GY3Lo9IffOrboz/6XJneb1Dx9pl7Z51/S/9RDUKWyO2n2uvisCjDPPq4VihbaD4ciG20EAbmgNrREgvA0Vc=
X-Received: by 2002:a19:be0e:: with SMTP id o14mr855088lff.203.1598622200150; Fri, 28 Aug 2020 06:43:20 -0700 (PDT)
MIME-Version: 1.0
References: <157349122063.7571.1978842562243958252@ietfa.amsl.com> <CAPK2Dexgk81Saufei3z67E4XZg=LLra1HdTUWU-kU33Pj_o+eg@mail.gmail.com> <CAPK2DeyWEzR6Qy6HPURnKp481mH=y+3O2xpLBS9kLc1MPbcjBg@mail.gmail.com> <44A4E4A8-AF9A-47AF-A31A-8AAACAF0A6BA@tail-f.com> <CAPK2Dey7GzzAWh8AeKA8e5Ng8skxZBf1SYKGLyuatpZDJ+YPWQ@mail.gmail.com> <5F117489.6060709@btconnect.com>
In-Reply-To: <5F117489.6060709@btconnect.com>
From: "Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.com>
Date: Fri, 28 Aug 2020 22:42:40 +0900
Message-ID: <CAPK2Dezsp-J462GAEYuKO7gKe9wkxiPmyAXX0pjhXFCnJTwqig@mail.gmail.com>
To: tom petch <daedulus@btconnect.com>
Cc: Jan Lindblad <janl@tail-f.com>, YANG Doctors <yang-doctors@ietf.org>, "i2nsf@ietf.org" <i2nsf@ietf.org>, skku-iotlab-members <skku-iotlab-members@googlegroups.com>
Content-Type: multipart/alternative; boundary="0000000000005f151f05adf03c36"
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/tvAJV8qXpjNhU2LpQkEOQcbXuWw>
Subject: Re: [yang-doctors] [I2nsf] Yangdoctors last call review of draft-ietf-i2nsf-consumer-facing-interface-dm-07
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 28 Aug 2020 13:43:28 -0000

Hi Tom,
I have included RFCs used in the YANG module as follows:
----------------------------------------------------------------------------
8.1. YANG Module of Consumer-Facing Interface
 This section describes a YANG module of Consumer-Facing Interface.
 This YANG module imports from [RFC6991] and uses the typedef of time
 in [I-D.ietf-netmod-rfc6991-bis]. It makes references to [RFC0854]
 [RFC0913][RFC0959][RFC1081][RFC1631][RFC2616][RFC2818][RFC4250]
 [RFC5321].
----------------------------------------------------------------------------

Thanks.

Best Regards,
Paul

On Fri, Jul 17, 2020 at 6:51 PM tom petch <daedulus@btconnect.com> wrote:

> On 11/07/2020 08:44, Mr. Jaehoon Paul Jeong wrote:
> > Hi Jan and Tom,
> > I have revised our I2NSF Consumer-Facing Interface (CFI) Data Model Draft
> > according to both your comments.
> >
> > Jan,
> > I attach the revised draft and the revision letter to explain how I have
> > reflected your comments one by one.
> >
> > Tom,
> > the references to RFC inside our YANG module cannot be cited in my I-D
> XML
> > file, so I cannot include them
> > in Normative References.
>
> Yes you can and in fact you must:-)
> You can put anything you want to in Normative References with a
> corresponding [RFC0913] in the text part of the I-D so you add Section
> 8.1 "This YANG module imports from [RFC6991], ... and makes reference to
> [RFC0854], [RFC0913], ......"
>
> Note that all import must have a reference clause and the referenced
> work must appear in Normative References; same technique applies.
>
> Tom Petch
>
> >
> > Also, the choice of the prefix  is i2nsf-cfi.
> >
> > I put "Note: This section is informative" for Sections 7 and 10, which
> > include XML configuration examples.
> >
> > If you have further comments, please let me know by July 12, 2020, in
> EST.
> > If possible, I want to post this revision on July 13, 2020 after
> reflecting
> > your further comments on the revision.
> >
> > Thanks.
> >
> > Paul
> >
> >
> > On Fri, Mar 20, 2020 at 2:25 AM Jan Lindblad <janl@tail-f.com> wrote:
> >
> >> Paul,
> >>
> >> Thank you for all your work with the module, and for the reminder for me
> >> to verify all the changes.
> >>
> >> I am afraid I think the module is still not ready for last call, even if
> >> it is better shape than ever thanks to your efforts. I went through the
> >> module from top to bottom, so this is sorted in order of appearance.
> >>
> >> Line 107-204: The following identities are declared in the module, but
> >> never referenced. They should either have a common base with something,
> or
> >> be referenced somewhere. If not, why are they defined here? They
> currently
> >> serve no purpose in this YANG module.
> >>    identity ddos {
> >>    identity enforce-type {
> >>    identity admin {
> >>    identity time {
> >>
> >> Line 377: Defining a custom date-and-time type seems odd. You should
> >> probably use one that has already been defined
> >>    typedef date-and-time {
> >>
> >> Line 513: The leaf represents the name of a user, but the format is
> >> undefined. What should be the format for the string value? How would a
> user
> >> know what to configure here? Email addresses? If implementation
> dependent,
> >> say so.
> >>      leaf name {
> >>        type string;
> >>        description
> >>          "This represents the name of a user.";
> >>
> >> Line 518: If no IP address information is specified for the user-group,
> >> what happens then? Is the user access accepted, rejected, or something
> else?
> >>      uses ip-address-info;
> >>
> >> Line 658: Key leaf declared mandatory. All keys are mandatory, so
> >> mandatory is not needed on this leaf.
> >>      leaf policy-name {
> >>        type string;
> >>        mandatory true;
> >>
> >> Line 664: Users mentioned in the owners-ref should have full CRUD
> >> privileges to the policy. But what about everyone else? Should they have
> >> R(ead) privileges? Can anyone create new policies? If not, who can? If
> >> someone creates a policy, but does not mention his own name among owners
> >> (e.g. misspells or does not get the format right), he will not be able
> to
> >> modify or remove the policy. If no owner is mentioned, then noone can.
> >>      uses owners-ref;
> >>
> >> Line 673: Key leaf declared mandatory. All keys are mandatory, so
> >> mandatory is not needed on this leaf.
> >>          leaf rule-name {
> >>            type string;
> >>            mandatory true;
> >>
> >> Line 682: Users mentioned in the owners-ref should have full CRUD
> >> privileges to the rule. But what about everyone else? Should they have
> >> R(ead) privileges? Can anyone create new rules, or only those that have
> >> full CRUD privileges for the policy? If someone creates a rule, but does
> >> not mention his own name among owners (e.g. misspells or does not get
> the
> >> format right), he will not be able to modify or remove the rule.
> >>      uses owners-ref;
> >>
> >> Line 697: Choice enforce-type has a description that I can't understand.
> >> What does this mean?
> >>            choice enforce-type {
> >>              description
> >>                "There are two different enforcement types;
> >>                admin, and time.
> >>                It cannot be allowed to configure
> >>                admin=='time' or enforce-time=='admin'.";
> >>
> >> Line 703: In case of enforce-type admin (whatever that means), a string
> >> value needs to be configured. What are the valid values for this leaf?
> >>              case enforce-admin {
> >>                leaf admin {
> >>                  type string;
> >>                  description
> >>                    "This represents the enforcement type
> >>                    based on admin's decision.";
> >>
> >> Line 711: In case of enforce-type time, three times can be configured.
> >> What is the relation between enforce-time, and the other two
> (begin-time,
> >> end-time)?
> >>              case time {
> >>                container time-information {
> >>                  description
> >>                    "The begin-time and end-time information
> >>                    when the security rule should be applied.";
> >>                  leaf enforce-time {
> >>                    type date-and-time;
> >>                    description
> >>                      "The enforcement type is time-enforced.";
> >>                  }
> >>                  leaf begin-time {
> >>                    type date-and-time;
> >>                    description
> >>                      "This is start time for time zone";
> >>                  }
> >>                  leaf end-time {
> >>                    type date-and-time;
> >>                    description
> >>                      "This is end time for time zone";
> >>                  }
> >>
> >> Furthermore, the locally defined date-and-time type used includes both a
> >> date and time, which seems to be at odds with the example
> configurations in
> >> the draft. Example 9.2:
> >>    <rules>
> >>      <rule>
> >>        <rule-name>block_access_to_sns_during_office_hours</rule-name>
> >>        <event>
> >>          <time-information>
> >>            <begin-time>2020-03-11T09:00:00.00Z</begin-time>
> >>            <end-time>2020-03-11T18:00:00.00Z</end-time>
> >>
> >> In the example, the rule-name "block_access_to_sns_during_office_hours "
> >> suggests that the begin-time and end-time should be times of day between
> >> which the policy should be enforced. E.g. every day between 9.00 and
> 18.00.
> >> If that is a valid use case, using a time type with a date doesn't make
> >> much sense. In the context of the policy that repeats "daily", how
> should
> >> the start date-and-time value "2020-03-11T09:00:00.00Z " be interpreted?
> >> What if it was "monthly"?
> >>
> >> Line 736: In the frequency leaf, the enumeration value only-once is for
> >> rules that don't repeat. But how long do they apply? A single packet? A
> >> single time the rule is triggered? How does a user know if the rule is
> >> still in effect, i.e. if the "once" has happened or not?
> >>                enum only-once {
> >>
> >> Line 835: Maybe it's just my limited understanding of how threat-feeds
> >> work, but I wonder i this construct with source and destinations for
> threat
> >> feeds is meaningful?
> >>            container threat-feed-condition {
> >>              description
> >>                "The condition based on the threat-feed information.";
> >>              leaf-list source {
> >>                type leafref {
> >>                  path
> >> "/i2nsf-cfi-policy/threat-preventions/threat-feed-list/name";
> >>                }
> >>              description
> >>                "Describes the threat-feed condition source.";
> >>              }
> >>              leaf dest-target {
> >>                type leafref {
> >>                  path
> >> "/i2nsf-cfi-policy/threat-preventions/threat-feed-list/name";
> >>                }
> >>              description
> >>                "Describes the threat-feed condition destination.";
> >>
> >> Line 920: Location groups can be configured, but there seems to be no
> >> references to them. How are they supposed to be used?
> >>        list location-group{
> >>          key "name";
> >>          uses location-group;
> >>
> >> Line 931: Regarding point 16.1 in your revision letter, you say "We
> think
> >> list type of threat-feed-list can be configured more than one feed of
> the
> >> same type". I'm afraid that is not the case with the current YANG
> model. If
> >> you do wish to allow more than one threat-feed-list for the same
> >> threat-feed-type, you need to add an additional key to your
> >> threat-feed-list.
> >>        list threat-feed-list {
> >>          key "name";
> >>          description
> >>            "There can be a single or multiple number of
> >>            threat-feeds.";
> >>          uses threat-feed-info;
> >>
> >> ...
> >>    grouping threat-feed-info {
> >>      description
> >>        "This is the grouping for the threat-feed-list";
> >>      leaf name {
> >>        type identityref {
> >>          base threat-feed-type;
> >>
> >>
> >> Generally, the indentation in the module is much improved. Some lines
> are
> >> still a bit off, however, so I would recommend using a tool that indents
> >> consistently.
> >>
> >> Generally, I also wonder whether there has been any discussion with
> >> implementors around the admin security model proposed here. As noted
> >> before, it's a bit different from everything else I have seen. Is it
> well
> >> thought through? Do implementors feel this is doable and user friendly?
> >> Currently there are no examples involving owner. Perhaps an example that
> >> sheds some light over how different users create, modify and see the
> >> various rules would shed some light over this.
> >>
> >> Best Regards,
> >> /jan
> >>
> >>
> >>
> >>
> >>
> >> On 18 Mar 2020, at 18:41, Mr. Jaehoon Paul Jeong <
> jaehoon.paul@gmail.com>
> >> wrote:
> >>
> >> 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 <
> >> jaehoon.paul@gmail.com> 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 <
> >>> noreply@ietf.org> 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
> >>>> I2nsf@ietf.org
> >>>> 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: jaehoon.paul@gmail.com, pauljeong@skku.edu
> >>> 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: +82-31-299-4957
> >> Email: jaehoon.paul@gmail.com, pauljeong@skku.edu
> >> 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 Computer Science and Engineering
Sungkyunkwan University
Office: +82-31-299-4957
Email: jaehoon.paul@gmail.com, pauljeong@skku.edu
Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php
<http://cpslab.skku.edu/people-jaehoon-jeong.php>