Re: [babel] Benjamin Kaduk's Discuss on draft-ietf-babel-yang-model-10: (with DISCUSS and COMMENT)

Mahesh Jethanandani <mjethanandani@gmail.com> Fri, 25 June 2021 18:44 UTC

Return-Path: <mjethanandani@gmail.com>
X-Original-To: babel@ietfa.amsl.com
Delivered-To: babel@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 193103A0763; Fri, 25 Jun 2021 11:44:37 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.097
X-Spam-Level:
X-Spam-Status: No, score=-2.097 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, 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 a-7ogl53b85D; Fri, 25 Jun 2021 11:44:31 -0700 (PDT)
Received: from mail-pg1-x534.google.com (mail-pg1-x534.google.com [IPv6:2607:f8b0:4864:20::534]) (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 6E2C63A0744; Fri, 25 Jun 2021 11:44:31 -0700 (PDT)
Received: by mail-pg1-x534.google.com with SMTP id a2so8581901pgi.6; Fri, 25 Jun 2021 11:44:31 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=9lNsbdaWnc++hjDv3WEQgYJRhLb0P9jhOdRICZLv1Oo=; b=AyysU45ZGLS1M9mA7ZkukHie7ZSwcvT0i+cdrZ6Z04IcG3Bw9C+WS9ix3niqUt0WMa 9xMBFxUA6UA7w7GoUPk5hafC4++2/Oak5x9C39xJXnxGcGxxDhhAhvTxBKcNGywJdwKu br08BCsLSmrUBdylRfC6ZgpnBgeHtOfnD6kolPMOypriJiJos9ESErrBIvBIhEY5A2Cc k/jpndsVWcx4oCA/oBNr3aetJCu7qa7/QGnI0x+8r+NHqF1ixGoPn+K5QqRl0pFLX/w0 9apS1tQQTq37f8BXnxC7CrmXmYpPfFvGhbgAOZ+NVCphTvZiXc5Z9uoHnqw0Apa5X8rZ c6Sg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=9lNsbdaWnc++hjDv3WEQgYJRhLb0P9jhOdRICZLv1Oo=; b=UNhGtls4sqB11GxB6HSrxAWt/c5tDoqfZsCMzSgYmGa9ffeLzgeb9mDAcck/UbSOND UOn+r1n6iAdfyPUEoUNacFd3MKgjli5gjjvwPYju8mBmTcJNAzXOBFTrOQmI1sq5V/DI qRUjmK3sgho9XcnwCgq5EKrr141WTJZIJAzaKZMzkoYil5pQgpdYI56XurNySH8DaSKo G6f4t47bGJIasQr6zBcbNzU2su/N57cEESfqT2pBW1Ov2jiKrztfx9IGLy0rhkKM7+b+ R+w7+HvI5iZDTqFKMrOYGE/KYyvwtdbKU3G0a+a+BfwE0Xgi8BKWbNRzTC8+tedippJO 3NqQ==
X-Gm-Message-State: AOAM531YLiPcG6EZDG+wk9s+tPIvJ5kr5J/HuQsv/Ukwug4mIFMyG3dc pXtsTCPBeXkqjd6ILWeyt24=
X-Google-Smtp-Source: ABdhPJysbrNZccPtJICl49II5CFeHKasfpVeIBWf6oOGo1+FzBnjckXkxPIMxK724PPTLAYLY/JXkQ==
X-Received: by 2002:a62:7616:0:b029:305:f420:49cc with SMTP id r22-20020a6276160000b0290305f42049ccmr11931778pfc.51.1624646669882; Fri, 25 Jun 2021 11:44:29 -0700 (PDT)
Received: from ?IPv6:2601:647:5600:5020:317c:95dc:4f68:92b2? ([2601:647:5600:5020:317c:95dc:4f68:92b2]) by smtp.gmail.com with ESMTPSA id mj17sm11730273pjb.12.2021.06.25.11.44.28 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 25 Jun 2021 11:44:28 -0700 (PDT)
From: Mahesh Jethanandani <mjethanandani@gmail.com>
Message-Id: <BB6805E0-A90C-4801-9683-4900A483AF4A@gmail.com>
Content-Type: multipart/alternative; boundary="Apple-Mail=_0872F95C-C8C5-4051-B7C3-702AFE11FFA5"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Date: Fri, 25 Jun 2021 11:44:27 -0700
In-Reply-To: <99514803-6C88-40F8-B924-315945B052C1@gmail.com>
Cc: The IESG <iesg@ietf.org>, draft-ietf-babel-yang-model@ietf.org, babel-chairs <babel-chairs@ietf.org>, Babel at IETF <babel@ietf.org>, Donald Eastlake <d3e3e3@gmail.com>
To: Benjamin Kaduk <kaduk@mit.edu>
References: <162148379451.10850.9563671396439074857@ietfa.amsl.com> <99514803-6C88-40F8-B924-315945B052C1@gmail.com>
X-Mailer: Apple Mail (2.3608.120.23.2.7)
Archived-At: <https://mailarchive.ietf.org/arch/msg/babel/X4Lp8H29kNtJIJdPRQ8PjThaWLo>
Subject: Re: [babel] Benjamin Kaduk's Discuss on draft-ietf-babel-yang-model-10: (with DISCUSS and COMMENT)
X-BeenThere: babel@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "A list for discussion of the Babel Routing Protocol." <babel.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/babel>, <mailto:babel-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/babel/>
List-Post: <mailto:babel@ietf.org>
List-Help: <mailto:babel-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/babel>, <mailto:babel-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 25 Jun 2021 18:44:37 -0000

Hi Ben,

On re-reading one of your comments, I realized my response to that comment was not quite right. See inline.

> On Jun 8, 2021, at 8:47 AM, Mahesh Jethanandani <mjethanandani@gmail.com> wrote:
> 
> Hi Benjamin,
> 
> Thank you first of all for a detailed review. Some of the comments/changes suggested here may impact the information model that this draft depends on. The information model is in AUTH48 state at this time.
> 
> This response includes feedback from my co-author Barbara.
> 
>> On May 19, 2021, at 9:09 PM, Benjamin Kaduk via Datatracker <noreply@ietf.org <mailto:noreply@ietf.org>> wrote:
>> 
>> Benjamin Kaduk has entered the following ballot position for
>> draft-ietf-babel-yang-model-10: Discuss
>> 
>> When responding, please keep the subject line intact and reply to all
>> email addresses included in the To and CC lines. (Feel free to cut this
>> introductory paragraph, however.)
>> 
>> 
>> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html <https://www.ietf.org/iesg/statement/discuss-criteria.html>
>> for more information about DISCUSS and COMMENT positions.
>> 
>> 
>> The document, along with other ballot positions, can be found here:
>> https://datatracker.ietf.org/doc/draft-ietf-babel-yang-model/ <https://datatracker.ietf.org/doc/draft-ietf-babel-yang-model/>
>> 
>> 
>> 
>> ----------------------------------------------------------------------
>> DISCUSS:
>> ----------------------------------------------------------------------
>> 
>> This is kind of nitpicky, and I'm sorry to pull out the heavy hammer of
>> DISCUSS for it, but for the "dtls-cached-info" leaf with description:
>> 
>>               "Indicates whether the cached_info extension is included
>>                in ClientHello and ServerHello packets. The extension
>>                is included if the value is 'true'.";
>> 
>> It is factually false to just say that "the extension is included if the
>> value is true", and it contradicts the DTLS specification to say so.  In
>> particular, the extension can only be included in the ServerHello
>> message if it was present in the ClientHello message being responded to.
>> So maybe we can say "enabled for inclusion" or append "when permitted by
>> the protocol", or something similar?
> 
> How about this?
> 
> OLD:
>             "Indicates whether the cached_info extension is included
>              in ClientHello and ServerHello packets. The extension
>              is included if the value is 'true'.";
> 
> NEW:
>             "Indicates whether use of cached_info extension is enabled.
>              The extension is enabled for inclusion if the value is 'true'.";
> 
> 
>> 
>> There's also a few places where we didn't quite clean up all the fallout
>> from switching from enumerations to identities (for MAC and
>> DTLS-adjacent algorithms), that really ought to get fixed before
>> publication.  I try to note them in the COMMENT.
>> 
>> I also think we need to be a bit more specific about the structure of
>> the (binary) private-key leaf/values provided when a certificate entry
>> is created.  Ideally this would just be by reference to some other spec,
>> but the situation is unfortunately messy.  (It doesn't help that we
>> allow DTLS 1.2, which allows certificates that are used for key-exchange
>> as opposed to signing, and those are not terribly mainstream.)  We may
>> need to pull in some other experts to figure out the right way to write
>> about this.
> 
> Ok. The way I am reading this is that we need to wait to hear more from the experts that you cite before making any changes.
> 
> Note, in WG discussion the ask was for the format of the private-key to be binary. I am not an expert, but would it help if the description said something about using PEM format for the private-key?
> 
>> 
>> 
>> ----------------------------------------------------------------------
>> COMMENT:
>> ----------------------------------------------------------------------
>> 
>> Section 2.2
>> 
>>   In addition to information like the version number implemented by
>>   this device, the model contains subtrees on 'constants',
>>   'interfaces', 'routes' and 'security'.
>> 
>> I don't see a 'security' node, but rather separate 'mac-key-set' and
>> 'dtls' nodes, both of which contain subtrees.
> 
> Fixed.
> 
>> 
>> Section 2.3
>> 
>>   A router running Babel routing protocol can determine the parameters
>>   it needs to use for an interface based on the interface name.  [...]
>> 
>> Is this always true, or only sometimes?
> 
> How about this?
> 
> OLD:
>    A router running Babel routing protocol can determine the parameters
>    it needs to use for an interface based on the interface name.
> 
> NEW:
>    A router running Babel routing protocol can sometimes determine the parameters
>    it needs to use for an interface based on the interface name.
> 
>> 
>>   Transport Layer Security Version 1.2 [RFC6347], The Blake2
>> 
>> (DTLS 1.3 is in the RFC Editor's queue, FWIW.)
> 
> Replaced it with reference to draft-ietf-tls-dtls13.
> 
>> 
>>         leaf router-id {
>>           type binary;
>>           description
>>             "router-id of the source router for which this route is
>>              advertised.";
> 
>> 
>> IIRC we can do length constraints in YANG, and in Babel router-ids are 8
>> octets.  Should we enshrine that in the YANG?
> 
> What would the constraint statement look like? For 'type uint32' I see 
> 
> length “1..4294967295”;
> 
> but I am not sure for 'type binary’ what it would look like.
> 
>> 
>>         leaf next-hop {
>>           type inet:ip-address;
>>           description
>>             "The next-hop address of this route. This will be empty if
>>              this route has no next-hop address.";
>> 
>> What does it mean for an inet:ip-address to be "empty"?  That the node
>> is absent?
> 
> Yes, that the node is absent. Would it help if it said the following?
> 
> OLD:
>           "The next-hop address of this route. This will be empty if
>            this route has no next-hop address.”;
> 
> NEW:
>           "The next-hop address of this route. This will be NULL if
>            this route has no next-hop address.”;
> 
>> 
>>               "List of supported certificate types, in order of
>>                preference. The values MUST be among those listed in
>>                dtls-cert-types. This list is used to populate the
>> 
>> Since the element is of type leafref to the actual list of certs, it
>> seems like this MUST is achieved by construction (and thus need not be
>> listed specifically).  Though, "dtls-cert-types" appears in this
>> document only as the base identity for Babel DTLS certificate types, so
>> this phrasing seems a bit odd...perhaps it is fallout from an
>> enumeration/identity conversion.
> 
> You are right. There is nothing to say that the leaf-list ‘dtls-cert-prefer’ could not be just 
> 
>             type identityref {
>               base dtls-cert-types;
>             }
> 
> which would a list of certificate types this particular implementation supports. The assumption is that the list of certificate types that the implementation can support is not different from what it does support. @Barbara, would it be ok for the data model to just keep one list of certificate types?

I think the confusion is stemming from the fact that ‘dtls-cert-types’ is a base identity, and all the identities, e.g. x-509, and raw-public-key are derived from that base identity. However, ‘dtls-cert-prefer’ is not a leaf-list of ‘dtls-cert-types’ (the base identity) but rather the types listed in ../../dtls/certs/type. Rather than the change I suggested earlier, we need to reword the description statement to say:

OLD:

            "List of supported certificate types, in order of
             preference. The values MUST be among those listed in
             dtls-cert-types.

NEW:

            "List of supported certificate types, in order of
             preference. The values MUST be the ‘type' attribute in
             in the list ‘certs’ of the list ‘dlls’ (../../dtls/certs/type).

Does this help? Thanks.

> 
>> 
>>                server_certificate_type extension in a Client Hello.
>>                Values that are present in at least one instance in the
>>                certs object under dtls of a referenced dtls instance
>>                and that have a non-empty private-key will be used to
>>                populate the client_certificate_type extension in a
>>                Client Hello.";
>> 
>> Since the DTLS server picks which certificate types are actually used,
>> it is conceivable that the preference order in this list could also be
>> used as input to the server's choice of type.  That said, I don't think
>> there is universal API support for DTLS servers accepting a preference
>> list directly, so we would not want to imply that this list would always
>> be used in such a fashion.  But it could be used in such a fashion.
> 
> Based on what I understood of your comment, what do you think of this change? Or did I completely misunderstand you??
> 
> OLD:
> 
>             This list is used to populate the
>              server_certificate_type extension in a Client Hello.
>              Values that are present in at least one instance in the
>              certs object under dtls of a referenced dtls instance
>              and that have a non-empty private-key will be used to
>              populate the client_certificate_type extension in a
>              Client Hello.
> 
> NEW:
> 
>             This list could be used to populate the
>              server_certificate_type extension in a Client Hello.
>              Values that are present in at least one instance in the
>              certs object under dtls of a referenced dtls instance
>              and that have a non-empty private-key will be used to
>              populate the client_certificate_type extension in a
>              Client Hello.
> 
>> 
>>           container stats {
>>             config false;
>>             description
>>               "Statistics collection object for this interface.";
>> 
>> Often, YANG models with such statistics containers will also include a
>> leaf to indicate the "discontinuity time" at which the counters were
>> last reset.
> 
> Hmm. I wonder if that is not an implementation level detail.
> 
>> 
>>             leaf hello-mcast-history {
>>               type string;
>>               description
>>                 "The multicast Hello history of whether or not the
>>                  multicast Hello packets prior to exp-mcast-
>>                  hello-seqno were received, with a '1' for the most
>>                  recent Hello placed in the most significant bit and
>>                  prior Hellos shifted right (with '0' bits placed
>>                  between prior Hellos and most recent Hello for any
>>                  not-received Hellos); represented as a string using
>>                  utf-8 encoded hex digits where a '1' bit = Hello
>>                  received and a '0' bit = Hello not received.";
>> 
>> I'd consider adding a few more words to confirm that this is the hex
>> representation of a bitstring (so, all hex digits are possible), not a
>> string consisting of only '1' and '0' characters.
>> (Likewise for hello-ucast-history.)
> 
> But doesn’t the model already say that in the last sentence? If not, can you suggest text?
> 
>> 
>>             leaf exp-mcast-hello-seqno {
>>               type uint16;
>>               default "0";
>>               description
>>                 "Expected multicast Hello sequence number of next Hello
>>                  to be received from this neighbor; if multicast Hello
>>                  packets are not expected, or processing of multicast
>>                  packets is not enabled, this MUST be NULL.";
>> 
>> It's not really clear to me that assigning semantics to a NULL leaf
>> makes sense when there is a default value for it (that has already
>> assigned semantics to an absent leaf).
>> (et seq)
> 
> Ok. Will remove the default value.
> 
>> 
>>             leaf value {
>>               type binary;
>>               mandatory true;
>>               description
>>                 "The value of the MAC key. An implementation MUST NOT
>>                  allow this parameter to be read. This can be done by
>> 
>> I believe that we can incorporate this restriction in the YANG itself
>> with something like "nacm:default-deny-all".
> 
> Ok. Will add 'nacm:default-deny-all' and drop the text “An implementation .. subsequently writeable”.
> 
>> 
>>               description
>>                 "The name of the MAC algorithm used with this key. The
>>                  value MUST be the same as one of the enumerations
>>                  listed in the mac-algorithms parameter.";
>> 
>> It's now an identityref, not a name.  Accordingly, there are also not
>> any enumerations listed in the mac-algorithms parameter.
> 
> How about this?
> 
> OLD:
> 
>               "The name of the MAC algorithm used with this key. The
>                value MUST be the same as one of the enumerations
>                listed in the mac-algorithms parameter.”;
> 
> NEW:
> 
>               "The MAC algorithm used with this key. The
>                value MUST be one of the identities
>                listed with the base of 'mac-algorithms'.”;
> 
>> 
>>               description
>>                 "The name of the certificate type of this object
>>                  instance. The value MUST be the same as one of the
>>                  enumerations listed in the dtls-cert-types
>>                  parameter. This value can only be provided when this
>> 
>> Similarly, this is also identityref, and also has no enumerations listed
>> in the dtls-cert-types parameter.
> 
> How about this?
> 
> OLD:
> 
>               "The name of the certificate type of this object
>                instance. The value MUST be the same as one of the
>                enumerations listed in the dtls-cert-types
>                parameter. 
> 
> NEW:
> 
>               "The certificate type of this object
>                instance. The value MUST be the same as one of the
>                identities listed with the base of 'dtls-cert-types'. 
> 
>> 
>>             leaf private-key {
>>               type binary;
>>               mandatory true;
>>               description
>>                 "The value of the private key. If this is non-empty,
>>                  this certificate can be used by this implementation to
>>                  provide a certificate during DTLS handshaking. An
>>                  implementation MUST NOT allow this parameter to be
>>                  read. This can be done by always providing an empty
>> 
>> (nacm:default-deny-all could be useful here as well)
> 
> Ok. Will add it and drop the appropriate text around it.
> 
>> 
>>                  string, or through permissions, or other means. This
>>                  value can only be provided when this instance is
>>                  created, and is not subsequently writable.";
>> 
>> It is perhaps a bit limiting to require the actual private key value to
>> be supplied, since that would preclude the use of (e.g.) HSM-based
>> private keys.  But since we are basically inheriting from the
>> information model here, I won't press it too hard.
>> 
>> Section 4
>> 
>> It might be worth referencing the security considerations of
>> 8966/8967/8968 as being applicable.
> 
> I understand that the Babel protocol itself is a insecure protocol. But I am not clear on how that is applicable to the YANG model that is used to configure or read the *state* of the protocol (over a secure interface). How about we add this? Thanks to Barbara for providing the text.
> 
> "The security considerations outlined here are specific to the YANG data model, and do not cover security considerations of the Babel protocol or its security mechanisms [RFC8966][RFC8967][RFC8968]. Each of these have their own Security Considerations section for considerations that are specific to it."
> 
>> 
>> The security (privacy, really) considerations of RFC 7924 are also
>> arguably relevant, relating to the use of the TLS "cached_info"
>> extension.
> 
> Isn’t this something that is relevant to RFC8968? The YANG model is merely allowing for the configuration of the capability specified in RFC8968. Shouldn’t any security considerations for “cached_info” therefore be mentioned there?
> 
>> 
>> Are there any security and/or privacy considerations relating to the
>> logging of babel packets?
> 
> We say the following in the security considerations section.
> 
>    'babel': Access to the information in the various nodes can disclose
>    the network topology.  Additionally, the routes used by a network
>    device may be used to mount a subsequent attack on traffic traversing
>    the network device.
> 
>> 
>>   There are a number of data nodes defined in the YANG module which are
>>   writable/created/deleted (i.e., config true, which is the default).
>> 
>> I don't think "writable/created/deleted" is part of the template.
> 
> Thanks to Barbara for digging this out:
> 
> https://datatracker.ietf.org/doc/html/rfc8407.html#section-3.7.1 <https://datatracker.ietf.org/doc/html/rfc8407.html#section-3.7.1>:
> 
> There are a number of data nodes defined in this YANG module that are
> writable/creatable/deletable
> 
>> 
>>   'babel/hmac' and 'babel/dtls': These contain security credentials
>>   that influence whether packets are trusted.
>> 
>> I'd consider making two separate (but related) points about controlling
>> whether incoming packets are trusted, and whether outgoing packets are
>> produced in a way such that the receiver will treat them as trusted.
>> (Changing just the use-send/use-verify values can allow for a valid key
>> to be misused, without changing the actual keys being used, which could
>> present an interesting attack in some scenarios.)
> 
> Ok. How about this? Thanks again to Barbara for providing the text.
> 
>   “These contain security credentials
>   that influence whether incoming packets are trusted, and whether outgoing packets are
>   produced in a way such that the receiver will treat them as trusted.”
> 
>> 
>>   Some of the readable data or config false nodes in this YANG module
>>   may be considered sensitive or vulnerable in some network
>>   [...]
>>   'babel/hmac' and 'babel/dtls': These contain security credentials,
>>   include private credentials of the router.
>> 
>> We do require the actual secret keys to not be readable, so we might
>> consider phrasing this more as a reiteration of that requirement than a
>> statement of risk if they are read out.
> 
> How about this? Thanks Barbara.
> 
>    'babel/hmac' and 'babel/dtls': These contain security credentials,
>    including private credentials of the router; however it is
>    required that these values not be readable.
>  
>> 
>>   Some of the RPC operations in this YANG module may be considered
>>   sensitive or vulnerable in some network environments.  It is thus
>>   important to control access to these operations.  These are the
>>   operations and their sensitivity/vulnerability from a RPC operation
>>   perspective:
>> 
>>   This model does not define any RPC operations.
>> 
>> We do define a couple of actions, though, which are like RPCs except
>> "[t]he difference between an action and an rpc is that an action is tied
>> to a node in the datastore, whereas an rpc is not".  This is perhaps an
>> omission in the YANG security considerations template, but I don't think
>> we should let that stop us from noting the actions we define are
>> carefully designed to have minimal security impact and minimal
>> side-channel leakage.  (Well, I guess that's not hard for the
>> stats-reset one, but for the MAC key test one it's quite important.)
> 
> How about this?
> 
> OLD:
> 
>    This model does not define any RPC operations.
> 
> NEW:
> 
>    This model defines two actions, one of which
>    allows for MAC key and MAC algorithm to be tested. 
>    It does not allows for them to be changed.
> 
>> 
>> Section 6.1
>> 
>> I don't have a great sense for why RFC 4868 needs to be normative.
> 
> RFC 4868 is cited as a reference in the YANG model, which is normative.
> 
>> 
>> RFC 8968 seems missing as a listed reference at all (which would of
>> course need to be normative as 8967 is).
> 
> Agreed.
> 
>> 
>> Section 6.2
>> 
>> I think RFC 7693 should be normative, since you need it in order to do
>> the blake2 stuff.
> 
> Ok. Will move it to normative section.
> 
>> 
>> RFC 8341 might become normative if we add nacm:default-deny-all stanzas.
> 
> Agreed.
> 
>> 
>> NITS
>> 
>> The tree diagram and the prose use different orders for the various
>> child elements, which is a little distracting.
> 
> Ok. Will fix the order.
> 
>> 
>> Section 1
>> 
>>   [RFC8966].  The data model is defined using YANG 1.1 [RFC7950] data
>>   modeling language and is Network Management Datastore Architecture
>> 
>> "the YANG [data modeling language]"
> 
>> 
>> Section 2.1
>> 
>>   Information Model and this data module.  The information model
>>   mandates the definition of some of the attributes, e.g.  'babel-
>> 
>> comma after "e.g." (as well as before).  (I'll try to only note it once,
>> though it appears many times.)  Likewise for "i.e."
> 
>> 
>> Section 2.2
>> 
>>   In addition to information like the version number implemented by
>>   this device, the model contains subtrees on 'constants',
>> 
>> The actual data model (and the information model, for that matter) show
>> that the 'version' leaf contains "the name and version of this
>> implementation of the Babel protocol".  The text here suggests that it
>> contains the protocol version number implemented by the device, which is
>> different.  So I'd suggest "the implementation and version used by this
>> device" instead.
> 
>> 
>>   The 'interfaces' subtree describes attributes such as 'interface'
>>   object that is being referenced, the type of link, e.g. wired,
>> 
>> "the 'interface'"
> 
> Ok to the above suggested changes.
> 
>> 
>>   wireless or tunnel, as enumerated by 'metric-algorithm' and 'split-
>>   horizon' and whether the interface is enabled or not.
>> 
>> I suggest using semicolons for the outer layer of grouping, to avoid
>> overloading commas for different hierarchies of separation.
> 
> Something like this?
> 
> OLD:
> 
>    The 'interfaces' subtree describes attributes such as 'interface'
>    object that is being referenced, the type of link, e.g. wired,
>    wireless or tunnel, as enumerated by 'metric-algorithm' and 'split-
>    horizon' and whether the interface is enabled or not.
> 
> NEW:
> 
>    The 'interfaces' subtree describes attributes such as 'interface'
>    object that is being referenced, the type of link, e.g., wired,
>    wireless or tunnel; as enumerated by 'metric-algorithm' and 'split-
>    horizon’; and whether the interface is enabled or not.
> 
> 
>> 
>>   Finally, for security two subtree are defined to contain MAC keys and
>>   DTLS certificates.  The 'mac-key-set' subtree contains keys used with
>> 
>> "subtrees"
> 
>> 
>>   interfaces.  The dtls subtree contains certificates used with DTLS
>> 
>> single quotes for 'dtls'.
>> "the DTLS"
> 
>> 
>> Section 2.3
>> 
>>   Similarly, an interface that is a metered 3G link, and used for
>>   fallback connectivity needs much higher default time constants, e.g.
>> 
>> Comma after "connectivity".
> 
>> 
>>   HMAC: Keyed-Hashing for Message Authentication [RFC2104], Using
>>   HMAC-SHA-256, HMAC-SHA-384, and HMAC-SHA-512 [RFC4868], Datagram
>> 
>> The "with IPsec" part of RFC 4868's title seems to have been dropped.
> 
> Ok to the above suggested changes.
> 
>> 
>>         "This implementation supports two-out-of-three metric
>>          comp algorithm.";
>> 
>> "the two-out-of-three".
>> Also, do we really need to (silently) abbreviate "metric computation"
>> (multiple occurrences)?
> 
> Should be:
> 
> “the ’two-out-of-three’”. 
> 
> Will fix metric comp occurrences.
> 
>> 
>>         "This implementation supports Expected Transmission Count
>> 
>> "the Expected"
> 
> Ok.
> 
>> 
>>         "This implementation supports hmac-sha256 MAC algorithm.";
>> 
>> "the hmac-sha256"
>> 
>>         "This implementation supports x-509 certificate type.";
>> 
>> "the X.509"
> 
> Ok to both.
> 
>> 
>>         "This implementation supports raw-public-key certificate
>>          type.";
>> 
>> Missing article, per the theme, but I think it would also be more
>> conventional to spell the name as "RawPublicKey".
> 
> Ok. That was done to be consistent with the IM.
> 
>> 
>>       base metric-comp-algorithms;
>>       if-feature "etx-supported";
>>       description
>>         "Expected Transmission Count.";
> 
>> 
>> I'd put an "algorithm" in here somewhere
> 
> 
> How about:
> 
> “Expected Transmission Count (ETX) metric compilation algorithm.”
> 
>> 
>>         "BLAKE2s algorithms supported. Specifically, BLAKE2-128 is
>>          supported.";
>> 
>> I'd just say "BLAKE2-128 algorithm supported."
> 
> Then shouldn’t the feature say
> 
> feature blake2-128-supported?
> 
>> 
>>         "Raw Public Key type.";
>> 
>> "certificate type"
> 
>> 
>>         leaf router-id {
>>           type binary;
>> 
>> (If length constraint is applied earlier, it should apply here as well.)
>> 
>>             "Indicates whether statistics collection is enabled (true)
>>              or disabled (false) on all interfaces. When enabled,
>>              existing statistics values are not cleared and will be
>>              incremented as new packets are counted.";
>> 
>> I suggest "on transition to enabled".
>> (Also, thank you for clearly spelling out which behaviors true/false map
>> to!)
> 
>> 
>>                A value of true indicates split horizon optimization
>>                is used.";
>> "the split"
>> 
>>               "List of references to the mac entries that apply
>>                to this interface. When an interface instance is
>>                created, all mac instances with default-apply 'true'
>> 
>> s/mac/MAC/ (twice)
>> 
>>               "Indicates whether the cached_info extension is included
>>                in ClientHello and ServerHello packets. The extension
>> 
>> s/packets/messages/.
> 
> Ok to the above suggested changes.
> 
>> Also, it's conventional to refer to TLS extension names enclosed by
>> double quotes, but I guess in the context of a YANG leaf description
>> that's not really feasible.
> 
> Correct.
> 
>> 
>>                dtls-cert-types. This list is used to populate the
>>                server_certificate_type extension in a Client Hello.
>> 
>> s/Client Hello/ClientHello/.
>> 
>>         list mac-key-set {
>>           key "name";
>> 
>>           description
>>             "A mac key set object. If this object is implemented, it
>>              provides access to parameters related to the MAC security
>>              mechanism.";
>> 
>> s/mac/MAC/
>> 
>>               "A string that uniquely identifies the mac object.";
>> 
>> s/mac/MAC key/
>> 
>>                If 'true', this instance is applied to new babel-
>>                interfaces instances at the time they are created,
>>                by including it in the mac-key-sets list under
>>                interfaces. If 'false', this instance is not applied
>> 
>> s/under interfaces/under the interface/
>> 
>>                to new interfaces instances when they are created.";
>> 
>> s/interfaces/interface/
>> 
>>                If 'true', this instance is applied to new interfaces
>>                instances at the time they are created, by including it
>>                in the dtls-certs list under interfaces. If 'false',
>> 
>> s/under interfaces/under the interface/
>> 
>>                this instance is not applied to new interfaces
>>                instances when they are created.";
>> 
>> s/interfaces/interface/
> 
> Ok to make the above suggested changes.
> 
> Thanks again for a detailed review!
> 
> Mahesh Jethanandani
> mjethanandani@gmail.com <mailto:mjethanandani@gmail.com>
> 
> 
> 
> 
> 

Mahesh Jethanandani
mjethanandani@gmail.com