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

[Benjamin Kaduk <[e-mail]>]

Hi Mahesh,

My apologies again for the delayed response.  Thank you for submitting the
updates in the -11 and -12 (which also serves to generate a reminder to me
that I have an action item).  I'm happy to report that my DISCUSS points
are addressed, and will update my ballot position after I send this mail.

A few things left to comment on, inline...

On Wed, Aug 04, 2021 at 11:34:52AM -0700, Mahesh Jethanandani wrote:
> Hi Ben,
> 
> No worries. See inline, and for brevity reasons I will also skip “ok”, and other responses that would have acknowledged but do not require a specific response.
> 
> > On Jul 20, 2021, at 4:55 PM, Benjamin Kaduk <[e-mail]> wrote:
> > 
> > Hi Mahesh, Barbara,
> > 
> > My humble apologies for not responding earlier; this really was an
> > unreasonable amount of time for you to wait.
> > 
> > Responses inline, though for brevity I will omit remarks of the form
> > "sounds good", "thank you", etc.
> > 
> > On Tue, Jun 08, 2021 at 08:47:14AM -0700, Mahesh Jethanandani 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 <[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
> >>> 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?
> >> 
> >> 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'.";
> > 
> > Both this form and Barbara's revision will address my concern.  Barbara's
> > version removes any ambiguity about what the "inclusion" would be in, so
> > that seems preferred to me.
> 
> Ok. Will use Barbara’s version.
> 
> > 
> >> 
> >>> 
> >>> 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?
> > 
> > I think binary is probably fine; a PEM encoding is just base64 encoding and
> > sometimes a little bit of a hint as to what the contained structure is.
> > But my recollection is that the PEM type indication for certificate private
> > keys is not always precise enough to indicate what is to be used, so an
> > implementation would need to consult the "type" sibling leaf anyway in
> > order to process the "private-key"...but it's not entirely clear that just
> > "type" is enough information on its own.
> > 
> > It is possible that we can find the necessary references without pulling in
> > an external expert, but if we get stuck I would probably ask Russ Housley
> > or Sean Turner for help.
> > 
> > As a starting point, I note that RFC 4211 lists several Private Key
> > structures in Section 4.2.2 in ASN.1 syntax.  We would presumably require
> > that the binary value exposed by the YANG module be the DER-encoded version
> > of that ASN.1 structure.  RFC 5915 has a private-key structure for ECC
> > keys.  The complications then come in specifying how the other YANG
> > elements determine which (DER-encoded) ASN.1 structure is used for the
> > "private-key" contents.  In particular, "dtls-cert-type" indicates only a
> > "certificate type" in the TLS sense -- X.509, raw public key, and the like.
> > Both X.509 certificates and raw public keys can use keys from different
> > cryptosystems or cryptographic algorithms.  So I find it pretty
> > inescapable that we'll need some in-band signaling about whether the
> > private-key is a basic RSA key, or perhaps an RSA-PSS key that has some
> > specific associated parameters, vs a DH or DSA key, or ECDH, ECDSA, EdDSA,
> > etc.  With in-band signaling comes the need for a table or enumeration of
> > what the different (currently supported) key structures are.  It seems that
> > draft-ietf-netconf-crypto-types may have a start at private-key structures
> > of this sort, though I have not been following that work closely (it seems
> > to be in WGLC at the moment).
> > 
> 
> We will need to go back to the WG to discuss this. Let us get back to you on this.

I see the follow-up message, and using ‘private-key-formats’ from
ietf-crypto-types seems like a really good option.  Thank you for finding
that!  Of particular note (to me) is that it's extensible via identityref,
so if someone does have cause to use one of the more exotic TLS certificate
types, there's a straightforward path to doing so.

> >>> 
> >>> 
> >>> ----------------------------------------------------------------------
> >>> 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.
> > 
> > My understanding from
> > https://datatracker.ietf.org/doc/html/rfc7950#section-9.8.1 is that "length
> > 8" indicates an 8-octet value.
> 
> Ok. Added it.
> 
> > 
> >>> 
> >>>        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.”;
> > 
> > I think so, yes.
> > 
> >>> 
> >>>              "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 see there was an additional follow-up; that text looks good modulo the
> > duplicate word "in in" across the line break.
> > 
> >>> 
> >>>               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??
> > 
> > Sorry for my rambly comment, which is really more of a side note than
> > something intended to change the document.  The OLD text is better than the
> > NEW.  My question (to the extent there was a question) here was along the
> > lines of whether we should be more precise about whether "used to populate"
> > includes retaining the preference order.  I don't think it's currently
> > possible to actually do that, though, so my recommendation is to leave this
> > text unchanged.
> > 
> >> 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.
> > 
> > My understanding is that it is not typically treated as an implementation
> > detail, but my understanding is just an inference based on seeing a number
> > of YANG modules go by during my time as an AD.  Rob Wilton might have a
> > better sense, or we could ask a YANG doctor for help (or both).
> 
> Ok. I have added a ‘discontinuity-counter’ from other YANG models, and is pretty much a cut-and-paste :-(. I imagine it is not controversial addition, but do want to inform the WG, here, about the addition.
> 
> > 
> >>> 
> >>>            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?
> > 
> > I think you are correct that the model basically says that already.  My
> > concern was that since we use the quoted forms "'1' bit" and "'0' bit",
> > someone might think that we are using only binary digits (which are, after
> > all, a subset of hex digits) and write out a binary representation rather
> > than a hex one.  This is, admittedly, a little farfetched, but since I
> > already opened the topic, my attempt at rewording would be "[...]
> > represented as a string of utf-encoded hex digits.  A bit that is set
> > indicates that the corresponding Hello was received, and a bit that is
> > cleared indicates that the corresponding Hello was not received".
> 
> Ok. I have edited the text and it now reads as:
> 
>                “... represented as a string of
>                utf-8 encoded hex digits. A bit that is set indicates
>                that the corresponding Hello was received, and a bit
>                that is cleared indicates that the corresponding Hello
>                was not received."
> 
> > 
> >>> 
> >>>            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."
> > 
> > That looks wonderful; thank you!
> > 
> >>> 
> >>> 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?
> > 
> > okay.  My "arguably" was intended to imply "and also arguably not"
> > 
> >>> 
> >>> 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.
> > 
> > Hmm, my understanding was that the routes used by a given device are a
> > function of the received packets but that the packets themselves can
> > contain more information than just the routes currently used.  If that's
> > the case, then enabling a full packet log would disclose that "more
> > information", but I don't know offhand exactly what that information is, I
> > can't write good text to use here.
> 
> Ok. How about if we add this in the config true section of the Security Considerations section?
> 
> ‘packet-log-enable’ and ‘packet-log’: Enabling the logging of packets and unintended access to the log files gives an attacker detailed knowledge of the network, and allow it to launch an attack on the traffic traversing the network device.

That sounds good to me!  I think that my delay in responding caused this to
not be included in the -12, so hopefully it can still make it in.

> 
> > 
> >>> 
> >>>  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
> > 
> > My mistake, apologies for that.
> > I think I typically use
> > https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines as the
> > reference, but the phrase in question appears there as well.
> > 
> >>> 
> >>>  '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.
> > 
> > That's an improvement, certainly.  I would propose
> > 
> > NEW 2:
> > 
> >  This model defines two actions.  Resetting the statistics within an
> >  interface container would be visible to any monitoring processes, which
> >  should be designed to account for the possibility of such a reset.
> >  The "test" action allows for validation that a MAC key and MAC algorithm
> >  have been properly configured.  The MAC key is a sensitive piece of
> >  information, and it is important to prevent an attacker that does not know
> >  the MAC key from being able to determine the MAC value of an
> >  attacker-influenced plaintext.  The "test" action has been designed to
> >  not reveal such information directly.  Such information might also be
> >  revealed indirectly, due to side channels such as the time it takes to
> >  produce a response to the action.  Implementations SHOULD use a
> >  constant-time comparison between the input mac and the locally generated
> >  MAC value for comparison, in order to avoid such side channel leakage.
> 
> I am fine with this proposed text.
> 
> > 
> >>> 
> >>> 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.
> > 
> > When I was reading it, my expectation was something like
> > 
> > NEW 2:
> > 
> >   The 'interfaces' subtree describes attributes such as the '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.
> > 
> > but I trust your understanding of how the technology works more than my
> > understanding!
> 
> As I understand it, the interface can be described as being either ‘wired', ‘wireless' or ‘tunnel'. It can be further described by setting values of ‘metric-algorithm’ and ’split-horizon’. Finally, the interface can be enabled or disabled. So I would suggest one small change to your NEW 2.
> 
> NEW 3:
> 
>   The 'interfaces' subtree describes attributes such as the 'interface'
>   object that is being referenced; the type of link, e.g., wired,
>   wireless or tunnel; it is enumerated by 'metric-algorithm' and 'split-
>   horizon’; and whether the interface is enabled or not.

Ah, that makes perfect sense.  I expect the RFC Editor will suggest some
additional wording tweaks (to make the structure of the elements of the
list "parallel"), but am happy to leave the polishing to them.

Thanks again,

Ben

> > 
> >> 
> >>> 
> >>>  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?
> > 
> > My understanding of the nit (if any) here was about "algorithms" (plural) vs
> > "algorithm" (singular), since this is a single YANG identity that can only
> > have a single semantic.  So, we could choose to make it be "indicates that
> > BLAKE2-128 and BLAKE2-256 are both supported" or just "indicates that
> > BLAKE2-128 is supported".  Given the second sentence, my understanding is
> > that this particular YANG identity indicates that BLAKE2-128 is supported,
> > even if the feature "blake2s-supported" might in the future indicate
> > whether some other blake2s variant is supported in addition to this one.
> > If this YANG identity only indicates that a single algorithm is supported,
> > I don't think the plural "algorithms" adds much value, and once that part
> > is gone then the value of the first sentence comes into question as well.
> 
> My intent of naming the feature ‘blake2-supported’ was to include *all* BLAKE2 algorithms, even if there is support only for BLAKE2-128. I can drop the second sentence in the description to just say:
> 
>  "This implementation supports all blake2s MAC algorithms.”;
> 
> 
> > 
> >>> 
> >>>        "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!
> > 
> > And thank you for the detailed responses and updates; as I noted in the
> > preface, the ones I did not reply to all look good!
> > 
> > My apologies once more for the slow response time,
> > 
> > Ben
> 
> Mahesh Jethanandani
> [e-mail]
> 
> 
> 
> 
>