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

"Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.com> Thu, 27 June 2019 00:30 UTC

Return-Path: <jaehoon.paul@gmail.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 C206D120314; Wed, 26 Jun 2019 17:30:57 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -0.497
X-Spam-Level:
X-Spam-Status: No, score=-0.497 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HK_NAME_FM_MR_MRS=1.5, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=no 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 bU4XPfXWgNUL; Wed, 26 Jun 2019 17:30:54 -0700 (PDT)
Received: from mail-wm1-x32c.google.com (mail-wm1-x32c.google.com [IPv6:2a00:1450:4864:20::32c]) (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 D40C112009C; Wed, 26 Jun 2019 17:30:50 -0700 (PDT)
Received: by mail-wm1-x32c.google.com with SMTP id x15so3884966wmj.3; Wed, 26 Jun 2019 17:30:50 -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=109EzjFZEHX8bZ3TWTk/w7T56aB0IFNr8z45m+KO9Ps=; b=UgXMjFJUanfQUVNbsnsg3vr6TAyzpcGgMFz/+QjZ6XuMiDG9YTEV5hZD91XBHwU9yt 88riC3Hb404m6DrtnebTPp3pO5sF8oHZMSN5jRgJbrrznapoEkD2X0gneNJTLz+GAaf/ vMOxFWljpuJU22UBY0Cm6Y5hQ1qMZzZNLUw6yrUUuhgZRMA5GhKBCvOqEPZ3V2yf7fKp /8M2V2VP4COCiINFbeNVds6pKZSF/ITlI2ykdij+76PY4v6FnBz/gXOjFgC/SW7HOggg SVKaD7HH6MlqUO24TVR0Rxq2sh5o+wk7lbWyBXr1Kt8LQ9GxVEcyetOF8aIvzI4EtOUZ OpIg==
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=109EzjFZEHX8bZ3TWTk/w7T56aB0IFNr8z45m+KO9Ps=; b=rLpdetYtRGehmOusUMU3o866Q59LY/SG0kDFessENZaBEkMKX+Bi5IXawY9Zv/mR4X W+S3pyWid/JOyoOH7g/YoBsinPKk8pxXeBTVXxGLLITiXyR8OWpjkA3EW4VyDZ5xInA7 1Awjt2KuNdO9RWCOD0mVW85lrFTFJ0yxnTwejA0z+p12bwiOMcJIXXnlhv8et3o+66iA isPIGC7iORPw8btUnTsFkT4PhyTAUvfsLMc+HtCzQ06p3AFI83bMYz2svF7s/cdbdKov OCfqbO14zG4nDRvcHvX3+y8YlJDCacXrvjKLYFwrgxDSAJh59eGmqEqQ0PtNbQ8d221l grSA==
X-Gm-Message-State: APjAAAVqW7C+8NAEntmGW60E49VmTJZgAq2TmV0M76euhUMBhegU64rb 78wQ+SWnCN9np5Gzf2gsENATeKqELlA5TuPHOCE=
X-Google-Smtp-Source: APXvYqx+OPin/TgSG8xMTVMft0mr/I4cYg0fPaL2WjbbRhIKSjNRygxEvfTYXVCWEfGVNW81CpnHRSRYhV7fSNTDch0=
X-Received: by 2002:a1c:f512:: with SMTP id t18mr882897wmh.47.1561595448922; Wed, 26 Jun 2019 17:30:48 -0700 (PDT)
MIME-Version: 1.0
References: <156156480691.19914.1926691912558233407@ietfa.amsl.com>
In-Reply-To: <156156480691.19914.1926691912558233407@ietfa.amsl.com>
From: "Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.com>
Date: Thu, 27 Jun 2019 09:35:53 +0900
Message-ID: <CAPK2DezeiPjuyGF8eK68U2vow2uLRdVxafaqoZ8QzxcP+w81jg@mail.gmail.com>
To: Jan Lindblad <janl@tail-f.com>
Cc: YANG Doctors <yang-doctors@ietf.org>, "i2nsf@ietf.org" <i2nsf@ietf.org>, draft-ietf-i2nsf-consumer-facing-interface-dm.all@ietf.org, IETF Discussion <ietf@ietf.org>, skku_secu-brain_all@googlegroups.com, "Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.com>
Content-Type: multipart/alternative; boundary="0000000000000416f8058c4346e5"
Archived-At: <https://mailarchive.ietf.org/arch/msg/i2nsf/wVzi5nP5c7gxbN8diK579AjfHsE>
Subject: Re: [I2nsf] Yangdoctors last call review of draft-ietf-i2nsf-consumer-facing-interface-dm-05
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: Thu, 27 Jun 2019 00:30:58 -0000

Hi Jan,
I sincerely appreciate your detailed review and value comments on our draft.
We authors will address your comments.

Thanks.

Best Regards,
Paul

On Thu, Jun 27, 2019 at 1:00 AM Jan Lindblad via Datatracker <
noreply@ietf.org> wrote:

> Reviewer: Jan Lindblad
> Review result: On the Right Track
>
> This is my YANG Doctor review of
> draft-ietf-i2nsf-consumer-facing-interface-dm-05.txt, and the YANG module
> ietf-i2nsf-cfi-policy in particular. The document is on the right track,
> but
> several important discussions remain, so is not quite ready for last call.
>
> There are plenty of smaller things to fix in this module, see below for a
> list.
> There are a couple of potentially major issues in this module, however,
> that I
> would like to start with. The first relates to the management access
> control
> for this module.
>
> 1) Management access control
>
> This RFC defines a new management access control mechanism, entirely
> unrelated
> to the traditional NACM model (RFC 8341/RFC 6536). It also specifies things
> like which auth mechanisms should be used by clients in specific domains
> when
> connecting to the management server. This appears highly disruptive to the
> existing base of NETCONF/RESTCONF servers.
>
> To make it worse, the new security model described in this document is
> rather
> heavily unspecified. It may be that this security model is well thought
> through, but there is no language or other evidence that shows this in the
> text. The YANG model representation of it is broken in many places.
>
> The RFC text refers to a device called "Security Controller". By reading
> the
> parent RFC 8329, which contains an overview of the architecture, I believe
> the
> Security Controller is the management (NETCONF/RESTCONF/...) server with
> the
> ietf-i2nsf-cfi-policy YANG model, and the user is supposed to connect
> directly
> with it. This means, I would think, that the management server would need
> to be
> the component to implement the new management access control model.
>
> Key in this new mechanism is a leaf owner. There isn't much information
> about
> the owner leaf, but the RFC text relating to "owner" above Fig. 3 says:
>
>    Owner: This field contains the owner of the rule.  For example,
>              the person who created it, and eligible for modifying it.
>
> And further, in the YANG module itself, we find:
>
>   leaf owner {
>         type string;
>           description
>            "This field defines the owner of this
>            policy. Only the owner is authorized to
>            modify the contents of the policy.";
>
> This is all that is mentioned about the owner leaf. That the type of the
> leaf
> is string does not give much clues as to how this is meant to work.
>
> Then there are Policy Domains. A Policy Domain has an (optional) auth
> method
> (which is not correctly modeled in YANG) and a list of Policy Tenants. Each
> Policy Tenant represents one department or other part of the organization.
>
> Policy Users are individual users that are allowed to access the management
> server to CRUD policies and rules. Either within a tenant (default) or
> domain.
> There is no linkage to which tenant or domain, however. Each Policy User
> has
> exactly one reference to a an access profile in the Policy Role object. The
> only information this linkage provides is whether the access is read-only
> or
> read-write.
>
> In summary, I believe significant work is required to get this to a
> workable
> state. In order to invent a new management access control mechanism, a
> wider
> discussion in the NETMOD working group would be needed.
>
> Below I note some details that I came up with while trying to figure this
> out.
> Fixing them will not fix this issue as a whole.
>
> - container policy-mgnt-auth-method should probably be a list
> - container policy-role should probably be a list, and the list
> access-profile
> removed - list policy-tenant should probably not have any leaf domain - the
> leaf owner description talks about owner of the policy, while the leaf
> sits on
> an individual rule. Either the description or the leaf placement must be
> wrong.
> - I believe there are more issues to look at as part of the "translation
> from
> UML to YANG" that this module has been subject to.
>
> 2) container policy
>
> The top level container policy can only exist in a single instance as
> currently
> modeled. Even if not stated explicitly in the document, I get the feeling
> that
> the author's intent is that it should be possible to have more than one
> policy
> in the system. If this is the case, container policy needs to change to
> list
> policy (and probably add a surrounding container policies). Doing so would
> also
> require updating all the leafrefs used throughout the module, to only
> select
> nodes in the current policy.
>
> Another concern is that the name "policy" is very generic. There are many
> modules out there which already define a top level object called policy.
> Searching for container policy and list policy on yangcatalog.org gives
> several
> hundred pages with hits. Most of them will not be on the top level, but
> this
> shows that "policy" is a popular concept. How about calling the top level
> container cfi-policy or i2nsf-cfi-policy?
>
> 3) Are rules ordered?
>
> In the RFC section 4, it says regarding list rule:
>
>              This field contains a list of rules.  If the rule does not
>              have a user-defined precedence, then any conflict must be
>              manually resolved.
>
> I'm not sure what a "user-defined precedence" is, or at least I can't find
> anything like that in the YANG. So how are the rules meant to be applied
> on the
> devices? What does "manually resolved" mean, who does that when and how? If
> different operators cannot see each other's rules, does that mean that
> "manually resolved" just got harder?
>
> Each policy contains a set of rules in a list. This list is keyed by rule
> name
> and modeled as ordered-by system, i.e. rules will be sorted
> alphabetically. Is
> it up to the server to decide in which sequence these rules are applied. I
> suppose they could be overlapping, leading to different results based on
> the
> order rules are applied, and there could be performance implications
> depending
> on how this is done. If several policies are allowed, the issue only grows.
>
> 4) Restricted to IPv4
>
> The module specifically uses IPv4 in many places. Would it not make sense
> to
> use ip-address instead, to allow the use of IPv6 also, or is there some
> particular reason to exclude IPv6? If so, it would be good to mention this
> reason.
>
> In a couple of places, an address range is specified, and the start
> address is
> an ipv4-address while the end address is an ip-address. Surely that must
> be a
> mistake.
>
> 5) Many optional leafs
>
> The RFC text repeats many times: The XYZ object SHALL have the following
> information... This could be understood as the succeeding information
> items are
> mandatory. There are not a single mandatory element (apart from keys) in
> the
> entire module, however. And only three leafs with a default statement.
>
> It would be appropriate to go through the module and add mandatory and
> defaults
> where they make sense. For the leafs that should remain optional, it would
> be
> good if something could be added to the description regarding what the
> server
> behavior is when the leaf has no value, when that is not entirely obvious.
> Actually, it is often good to write even when it is obvious, because
> otherwise
> the behavior is technically undefined, and implementations could get away
> with
> bad behavior even when all know it's wrong.
>
> A couple of examples: what happens if the owner leaf is left unset?
> primary-action? enforce-type? recur? policy-tenant/domain?
>
> 6) Many strings
>
> In the module, the type string is used for names, which is great, but also
> for
> a many cases where some certain format of the content is expected, but not
> defined. There is no reason to believe that will lead to interoperable
> solutions. It will also leave users frustrated with little information to
> guess
> what type of data in which format to fill in. This needs fixing.
>
> 294: leaf-list protocol
> 322: leaf-list content
> 388: leaf begin-time
> 393: leaf end-time
> 553: leaf primary-action
> 561: leaf secondary-action
> 581: leaf owner
> 697: leaf auth-method
> 823: leaf threat-feed-description
> 839: leaf-list signatures
>
> 7) leaf date
>
> The grouping meta contains a name and a date leaf. The name is clearly
> meant to
> be a configuration item filled in by the client. I'm not sure about the
> date
> leaf, however. The description says
>
>     description "This is the date when the entity is
>     created or modified.";
>
> The date leaf is currently modeled as config true, i.e. is expected to be
> filled in by the client. It is not customary to have this sort of meta
> information in YANG modules, but it would make some sense if this was
> config
> false, i.e. computed by the server itself.
>
> If that is the intent, the specification should only make the date leaf
> config
> false and clarify how these time stamps should be calculated. Do they
> pertain
> to this particular list entry, or would they be updated if any child entry
> to
> this list entry was modified (similar to etags).
>
> If the intent is that clients should fill this in if they want, for their
> own
> optional housekeeping, I think that needs to be stated clearly. If the
> intent
> is to make it mandatory for clients to keep these timestamps up to date, I
> think the model is broken. What implications would it have if they were not
> used?
>
> 8) container policy-mgnt-auth-method
>
> The container has a description that says
>
>         "This represents the list of authentication methods.";
>
> A container is obviously not a list, so exactly what the author has in
> mind is
> somewhat unclear. At the top of the container, there is a leaf
>
>       leaf auth-method {
>         type string;
>         description
>           "This represents the authentication method name.";
>
> The format and usage of this string is highly unclear, but more
> importantly, it
> seems to speak of a single authentication method, not a list.
>
> Following this leaf is a number of lists; list password-based, list
> token-based, list certificate-based, list ipsec-method, list
> single-sign-on.
> Are these lists under container policy-mgnt-auth-method mutually
> exclusive, or
> can several of them be configured at the same time?
>
> This would need some clearing up. As this is an area where we can
> anticipate
> new methods being introduced over time, it would be good if the model
> advised
> how to extend with further auth methods.
>
> 9) More on container policy-mgnt-auth-method
>
> RFC text sec 5.1 says
>
>              Authentication method to be used for this
>              domain.  It should be a reference to a "Policy-Management-
>              Authentication-Method" object
>
> But in fact there is only one instance of container
> policy-mgnt-auth-method.
> Maybe this is meant to be a list?
>
> In the RFC Fig. 8 YANG tree representation authentication-method points
> like
> this
>
>             +--rw authentication-method?   ->
>             /policy/multi-tenancy/policy-mgnt-auth-method/name
>
> In the YANG module itself, the path is different:
>
>           path
>
> "/policy/multi-tenancy/policy-mgnt-auth-method/ipsec-method/method";
>
> Also, the abbreviation of management as "mgnt" is not very common. It is
> usually "mgmt". In order to reduce misspellings of policy-mgnt-auth-method
> I
> suggest renaming it to policy-mgmt-auth-method. There is actually already
> one
> such misspelling in this YANG module itself.
>
> 10) One cert server per cert type?
>
> List certificate-based allows the configuration of certificate servers. Is
> there usually a different server based on certificate type? Max one server
> can
> be configured per certificate type, and max three total as there are three
> certificate types. If this is what you want, the YANG says it nicely.
>
> 11) Enumerations vs. identities
>
> 203: certificate-type is defined as an enumeration. This means the module
> will
> have to be revised if any new certificate types need to be supported one
> day.
> An identity may be more appropriate here, as I expect new certificate
> types (or
> formats) may come out.
>
> 366: enforce-type is defined as an enumeration. This means the module will
> have
> to be revised if any new enforcement types need to be supported one day. An
> identity or choice may be more appropriate here. Then an additional module
> could add new identities or augment the choice.
>
> 55: permission-type is defined as an identity. Unless you foresee the need
> for
> other values than read-only and read-write, you could use enumeration here
> instead. It could simplify the module a little, but identity is fine too.
>
> 168: continent is defined as an identity. Unless you foresee the need for
> new
> continents, you could use enumeration here instead. It could simplify the
> module a little, but identity is fine too.
>
> 145: identity ransomeware is misspelled. Should be identity ransomware
>
> 12) Reference RFC 6087
>
> The RFC text refers to RFC 6087, which is obsoleted by RFC 8407. Update the
> reference?
>
> 13) YANG trees
>
> The RFC text has many figure descriptions like this
>
>     Figure X show the XML instance
>
> What is shown is actually a YANG tree, so the text should be updated to say
>
>     Figure X show the YANG tree
>
> 14) Indentation
>
> The indentation of the YANG module is broken on many, many lines. Make
> sure to
> use a YANG indentation tool before publishing the result.
>
> 15) Revision statement
>
> The revision statement at the top of the file needs to be rewritten before
> publication.
>
> 16) container recursive
>
> The container recursive on line 399 seems to be about how rules recur, not
> recurse (which is something completely different). Maybe change to the
> whole
> container recursive to
>
>     leaf frequency {
>       type enumeration {
>         enum only-once;
>         enum daily;
>         enum weekly;
>         enum monthly;
>       }
>       default once-only;
>     }
>
> 17) String passwords
>
> Passwords are modeled as strings in a couple of places. This is not
> acceptable
> from a security point of view. Use one of the cryptographic types for
> passwords, such as ianach:crypt-hash RFC 7317.
>
> 666: leaf password
> 710: leaf password
>
> The leaf password on line 710 has the description
>
>             "This should be defined using the
>             regular expression.";
>
> What does that mean? Authentication, password, regular expression? I don't
> get
> it.
>
> 18) Grouping descriptions
>
> A number of groupings have descriptions like
>
>     This grouping is to remove repetition of
>         'name' and 'ip-address' fields.";
>
> This is not exactly true since these groupings are used only once (hence no
> repetition). Describe instead what the grouping is used for or what it
> contains.
>
> 231: grouping meta
> 280: grouping user-group
> 301: grouping location-group
> 316: grouping payload-string
>
> 19) container time-information
>
> Is container time-information only relevant for enforce-type ==
> time-enforced?
> If so, a when expression on the container may be useful. Or, even better,
> make
> the enforce-type a choice and have the time related content inside the
> choice
> case instead.
>
> 20) container rate-limit
>
> Is uint8 a good type to use for the rate limiting function? Would it never
> (now
> or in the future) make sense to limit to more than 255 packets per second?
>
> 486: container rate-limit
>
> 21) container condition
>
> This container seems to hold the configuration of many different triggering
> conditions. Would several of these apply at the same time in a given rule?
>
> There are many instances of container source-target and container
> destination-target nested underneath. They seem to serve no purpose. Maybe
> consider simplifying the model by removing them, unless they have some
> clever
> function for the future.
>
> 22) Transactionality
>
> Section 10.1 starts with the words
>
>    In order to create a rule of a security policy, it is essential to
>    first register data (those which are used to form such rule) to the
>    database.
>
> This could lead some implementors to believe it is acceptable to require
> that
> the endpoints are committed separately from the rest of the configuration.
> The
> actual requirement is that the referenced endpoints exist at the time the
> transaction is committed. I.e. they could very well be part of the same
> transaction.
>
> 23) Incomplete XML examples
>
> The XML snippets in examples in Fig 24-27 use XML namespaces incorrectly.
> It's
> easy to fix. Just make sure the first line with
> <ietf-i2nsf-cfi-policy:policy>
> looks like this instead:
>
> <policy xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy">
>
> The last example, in section 10.4 is talking about <encrypt>. There is no
> such
> in the YANG module (today).
>
>        <encrypt>
>          <ipsec-method>ipsec-ike</ipsec-method>
>        </encrypt>
>
> The XML loads fine if you just remove the encrypt tag, so maybe you mean:
>
>          <ipsec-method>ipsec-ike</ipsec-method>
>
> (End of list)
>
>
> _______________________________________________
> I2nsf mailing list
> 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>