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

["STARK, BARBARA H" <[e-mail]>]

Hi Benjamin,
We haven't seen any reply from you as to whether Mahesh's answers satisfy your DISCUSS and COMMENTs.

For the first DISCUSS item, it would be really helpful to know if the proposed change would be satisfactory, though
I would suggest an additional tweak to what Mahesh had, to make it clear where the enabled extension would
be included. That is:

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 in ClientHello and
             ServerHello packets if the value is 'true'.";

If this is acceptable, I would like to mirror this change in the information model, so the two documents can be kept in synch.
The information model is waiting final AUTH48 approval from me.
Thx,
Barbara

From: babel <[e-mail]> On Behalf Of Mahesh Jethanandani
Sent: Tuesday, June 08, 2021 10:47 AM
To: Benjamin Kaduk <[e-mail]>
Cc: babel-chairs <[e-mail]>; Donald Eastlake <[e-mail]>; [e-mail]; The IESG <[e-mail]>; Babel at IETF <[e-mail]>
Subject: Re: [babel] Benjamin Kaduk's Discuss on draft-ietf-babel-yang-model-10: (with DISCUSS and COMMENT)

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 <[e-mail]<mailto:[e-mail]>> 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://urldefense.com/v3/__https:/www.ietf.org/iesg/statement/discuss-criteria.html__;!!BhdT!xdoik8lnQ06bLOaHORgkxv4-KMDP9ADWMs6HZ4iKk8tYj6xi6En6qiqWPLJ4vQ$>
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://urldefense.com/v3/__https:/datatracker.ietf.org/doc/draft-ietf-babel-yang-model/__;!!BhdT!xdoik8lnQ06bLOaHORgkxv4-KMDP9ADWMs6HZ4iKk8tYj6xi6En6qiotaSuJ9w$>



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



               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://urldefense.com/v3/__https:/datatracker.ietf.org/doc/html/rfc8407.html*section-3.7.1__;Iw!!BhdT!xdoik8lnQ06bLOaHORgkxv4-KMDP9ADWMs6HZ4iKk8tYj6xi6En6qipKTc_nhw$>:

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
[e-mail]<mailto:[e-mail]>