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

tom petch <daedulus@btconnect.com> Fri, 17 July 2020 11:07 UTC

Return-Path: <daedulus@btconnect.com>
X-Original-To: i2nsf@ietfa.amsl.com
Delivered-To: i2nsf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 9ACDB3A09F8; Fri, 17 Jul 2020 04:07:26 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.901
X-Spam-Level:
X-Spam-Status: No, score=-1.901 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, MSGID_FROM_MTA_HEADER=0.001, NICE_REPLY_A=-0.001, RCVD_IN_MSPIKE_H2=-0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=btconnect.onmicrosoft.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 fg4WhBgdWGHE; Fri, 17 Jul 2020 04:07:23 -0700 (PDT)
Received: from EUR05-AM6-obe.outbound.protection.outlook.com (mail-am6eur05on2101.outbound.protection.outlook.com [40.107.22.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id A3ABA3A09F7; Fri, 17 Jul 2020 04:07:22 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jm+xuAvUhu+/QSppIUb3UZwECrP41MduM60iOetuHCw6fGT2HupWE32pbH2pKyrAEOZrBw6rEfVj8pAEos4W/2YcU55EjW3sVD4SIScLWp/7nZFXIOnQeVutftYX3WuvMovwY2tFX6gB1DzL4ESzl0rn047q1cnyVFhxwLnAsZHkwiSTTMQe/w3zzOORrm8kRAUCmU0KtNmbg871wi1xa278iy7YAGnXa4d2eBfv5eKr3gCqjZhRU7INUnWFqHOWLohsCKqIR38b/4XSHPkngBmMIKKI3L7KhCz2wLwCEPT03yT/U8ucEvkqkCC4y4v3/Jec7McoLv8bI7CLioRfJA==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=4M+ZTpE6k8uoiXxcWhp4kjulVrNqc7T/g8Sz0pM7YBU=; b=CI3sL2yjikOBI4pavDme2bcDTffblhR9U9jmGKr0ya8uwHZcpIiF1/lhr8Z1Pwwh85AQbvTwV5k0Gs/lw1lYvz+Fbl7WvffAOvjWnkQFwFqSCssIec6R6KwEy7Iv9xXGFBgER/Q4SVtVPJiFvVjO0VE9YNhAj/vwwXdhxmPhcAKkyoU8vyRhX+LRPIeiUxujgDhsZi2RgG8/09MNyqd1AzqPs6D2jTA1e5sfL3XiJLE7ZNrsUk0m/Mb2pYz7GTTqGDaycM28OvgMXWavTFgkKO9fxpWs7i/iGDx4gcpkzyqm1KEkuuDEYmhAMIZqxvs2iwGRR8ZgdhdI4Qw5Q9pUaQ==
ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=btconnect.com; dmarc=pass action=none header.from=btconnect.com; dkim=pass header.d=btconnect.com; arc=none
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=btconnect.onmicrosoft.com; s=selector2-btconnect-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=4M+ZTpE6k8uoiXxcWhp4kjulVrNqc7T/g8Sz0pM7YBU=; b=fcfcCK4iiN4xDyzR0gi+BjtypF5Y8jDAiYr5542oM5l6fc5Mwtk6o4td+NNpnAbFSiYDcQBe8MRK9K3izKxcwF8z/ycfv4XBIokP1UMJdbD00HfSKuZ9J7fSjOtRjYeFzSsgEdaO6kHRDQ5NkBgZYjt183PQpM3FVE83Bp8IrHs=
Authentication-Results: ietf.org; dkim=none (message not signed) header.d=none;ietf.org; dmarc=none action=none header.from=btconnect.com;
Received: from VI1PR07MB6704.eurprd07.prod.outlook.com (2603:10a6:800:18b::8) by VI1PR07MB6558.eurprd07.prod.outlook.com (2603:10a6:800:18e::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3195.10; Fri, 17 Jul 2020 11:07:19 +0000
Received: from VI1PR07MB6704.eurprd07.prod.outlook.com ([fe80::bc6a:1add:e84e:f19b]) by VI1PR07MB6704.eurprd07.prod.outlook.com ([fe80::bc6a:1add:e84e:f19b%9]) with mapi id 15.20.3195.019; Fri, 17 Jul 2020 11:07:19 +0000
To: "Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.com>, Jan Lindblad <janl@tail-f.com>
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>
Cc: YANG Doctors <yang-doctors@ietf.org>, "i2nsf@ietf.org" <i2nsf@ietf.org>
From: tom petch <daedulus@btconnect.com>
Message-ID: <5F118663.8010707@btconnect.com>
Date: Fri, 17 Jul 2020 12:07:15 +0100
User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:38.0) Gecko/20100101 Thunderbird/38.5.0
In-Reply-To: <5F117489.6060709@btconnect.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
X-ClientProxiedBy: LO2P265CA0229.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:b::25) To VI1PR07MB6704.eurprd07.prod.outlook.com (2603:10a6:800:18b::8)
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from [192.168.1.65] (81.131.229.35) by LO2P265CA0229.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:b::25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.3174.25 via Frontend Transport; Fri, 17 Jul 2020 11:07:18 +0000
X-Originating-IP: [81.131.229.35]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: a1fb39fa-86bf-4902-0441-08d82a41927e
X-MS-TrafficTypeDiagnostic: VI1PR07MB6558:
X-Microsoft-Antispam-PRVS: <VI1PR07MB655817BEB845A39E965DE953C67C0@VI1PR07MB6558.eurprd07.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:6790;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: D19i6aJH9qJ1CNcvlhr/4axPmypC5T+Qdq1k5GKlzQMuDPaNZwaVmQI+88xiwgOTcU003fEQjkp0/QfTHfK1A+293842XFIVE/mecQ2/RJDDHLVs8ip1Ivi7a3bTygeu+SP7OwrZjruubmMuArjWFQFEcaVC5c4P8SRMJYgI9Pobnxc1cm4lTgA2QfnWiqKcx6sRZpfr6QCS9nJFBI//JWfkHF0WsQCjHU4C2Qu4tu+DfZAFOUYgK1K3G8YlySfGaQCDCqh1aA8cQLGqaOVdLsKn3/zQIJNJ7lLYgrLuZuENTeoEacXsL/LuiTIE4+MxZQIm8bbqhRJtzjRkkZyfrhDp86R3TiNHbKFx5WGaeCMNlJTcFM2qtsc/dBRsbbZYCf4tq55g/mmMeg++76ulqQ==
X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:VI1PR07MB6704.eurprd07.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(376002)(366004)(346002)(136003)(39860400002)(396003)(15188155005)(6666004)(966005)(66556008)(52116002)(26005)(16799955002)(478600001)(87266011)(53546011)(66476007)(33656002)(66946007)(30864003)(5660300002)(4326008)(956004)(2616005)(6486002)(83380400001)(36756003)(186003)(2906002)(66574015)(16526019)(86362001)(16576012)(54906003)(8676002)(316002)(8936002)(110136005); DIR:OUT; SFP:1102;
X-MS-Exchange-AntiSpam-MessageData: 5mp8YbsUCFRvF8P9SHxjigxe+CluJdl6l4LD8kWfFnoqZeVbSJSiv5OE3ghFTY8mjp+Asln+fmNq2k2ykBzrkwNyDvQbtat1tbAf6JqnIjWSc1b+fYBPZ4ZhNSFATcp6FDlAUr6dppIa4rbYOeY7lP3B6ucvB9j5/GaCo4StC93xBWy3jpN2BTMeyfU23xBMqD9Nl7e1eOXQo7/4lRVLHk+S1nOPKzMVXAqFTvrLeopwg8kNO34AtTZX6PxyWaTqvcpw+3u8frYb0gFi6UZ3CyqhOmQ6UUI0FxOQZs1H/UEz5TJACAqSJWbCh7qtsssFVA8MzKPRR4yCaToOLIWxzEGpeP8gEyY3a0QQXd0dOVQ01mjpk7i6wtOiHKQWhsvg9UldqF4WJhIWU+CSPVPquoa9PzLFCf7eXnmYmmRMjVNsFiciFLY6Qwhss73yX4Zgyub2QrsDUH+dfOGXgSEjafzeP68h7C4TfRX1myHwX+U=
X-OriginatorOrg: btconnect.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a1fb39fa-86bf-4902-0441-08d82a41927e
X-MS-Exchange-CrossTenant-AuthSource: VI1PR07MB6704.eurprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jul 2020 11:07:19.7025 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: cf8853ed-96e5-465b-9185-806bfe185e30
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: l4Pzz3NqsMDSkPXkceF3Q1QPwBwIGtjQbePFUZNXH7ncXVCrfHghURdIXaw/S15K3B52sXzVdjJHJYFLF1sb2w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR07MB6558
Archived-At: <https://mailarchive.ietf.org/arch/msg/i2nsf/i69zC-zQyV5S_VOXi3K30PF0Wpo>
Subject: Re: [I2nsf] Yangdoctors last call review of draft-ietf-i2nsf-consumer-facing-interface-dm-07
X-BeenThere: i2nsf@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "*I2NSF: Interface to Network Security Functions mailing list*" <i2nsf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/i2nsf>, <mailto:i2nsf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/i2nsf/>
List-Post: <mailto:i2nsf@ietf.org>
List-Help: <mailto:i2nsf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/i2nsf>, <mailto:i2nsf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 17 Jul 2020 11:07:27 -0000

On 17/07/2020 10:51, tom petch 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.

Some more minor tweaks

s.5.1 /gorup/group/

YANG module

WG Chairs are not usually listed in the module - they used to be

descripton is a bit terse - some quote the Abstract

YARA, SNORT, SURICATA would benefit from references; they are not ones I 
see in TLS or SSH!

typedef time I see in RFC6991bis

does the ipv6 addresss ever need the interface?

start/end ipv4/ipv6 could do with a must end > start

geo-ip could do with a reference

s.9.1  221.159 is not a documentation address - see RFC5737

IESG often expect an ipv6 example alongside ipv4

s.12 Registrant should be IESG

prefix is not that of the module


Tom Petch


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