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

Benjamin Kaduk via Datatracker <noreply@ietf.org> Thu, 20 May 2021 04:09 UTC

Return-Path: <noreply@ietf.org>
X-Original-To: babel@ietf.org
Delivered-To: babel@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 85B1F3A2D93; Wed, 19 May 2021 21:09:54 -0700 (PDT)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: Benjamin Kaduk via Datatracker <noreply@ietf.org>
To: The IESG <iesg@ietf.org>
Cc: draft-ietf-babel-yang-model@ietf.org, babel-chairs@ietf.org, babel@ietf.org, Donald Eastlake <d3e3e3@gmail.com>, d3e3e3@gmail.com
X-Test-IDTracker: no
X-IETF-IDTracker: 7.29.0
Auto-Submitted: auto-generated
Precedence: bulk
Reply-To: Benjamin Kaduk <kaduk@mit.edu>
Message-ID: <162148379451.10850.9563671396439074857@ietfa.amsl.com>
Date: Wed, 19 May 2021 21:09:54 -0700
Archived-At: <https://mailarchive.ietf.org/arch/msg/babel/my6fguik8hoi7f28rEiu6g8ViI0>
Subject: [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
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: Thu, 20 May 2021 04:09:55 -0000

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



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

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.


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

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?

   Transport Layer Security Version 1.2 [RFC6347], The Blake2

(DTLS 1.3 is in the RFC Editor's queue, FWIW.)

         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?

         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?

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

                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.

           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.

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

             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)

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

               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.

               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.

             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)

                  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.

The security (privacy, really) considerations of RFC 7924 are also
arguably relevant, relating to the use of the TLS "cached_info"
extension.

Are there any security and/or privacy considerations relating to the
logging of babel packets?

   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.

   '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.)

   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.

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

Section 6.1

I don't have a great sense for why RFC 4868 needs to be normative.

RFC 8968 seems missing as a listed reference at all (which would of
course need to be normative as 8967 is).

Section 6.2

I think RFC 7693 should be normative, since you need it in order to do
the blake2 stuff.

RFC 8341 might become normative if we add nacm:default-deny-all stanzas.

NITS

The tree diagram and the prose use different orders for the various
child elements, which is a little distracting.

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

   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.

   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.

         "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)?

         "This implementation supports Expected Transmission Count

"the Expected"

         "This implementation supports hmac-sha256 MAC algorithm.";

"the hmac-sha256"

         "This implementation supports x-509 certificate type.";

"the X.509"

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

       base metric-comp-algorithms;
       if-feature "etx-supported";
       description
         "Expected Transmission Count.";

I'd put an "algorithm" in here somewhere

         "BLAKE2s algorithms supported. Specifically, BLAKE2-128 is
          supported.";

I'd just say "BLAKE2-128 algorithm 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/.
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.

                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/