[pim] Benjamin Kaduk's Discuss on draft-ietf-pim-msdp-yang-15: (with DISCUSS and COMMENT)

[Benjamin Kaduk via Datatracker <[e-mail]>]

Benjamin Kaduk has entered the following ballot position for
draft-ietf-pim-msdp-yang-15: 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 IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-pim-msdp-yang/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I don't think there's enough clarity on what authentication schemes are
supported and how the YANG configuration interacts with MSDP operation.
If I'm reading^Wsearching through RFC 3618 correctly, the only supported
authentication mechanism is TCP-MD5 (RFC 2385), and there have been no
updates to RFC 3618 that are indicated in the RFC database.  However,
RFC 2385 is obsoleted by RFC 5925 (TCP-AO).  Can TCP-AO be used with
MSDP?  What protocol elements or operation are controlled by the
"authentication" container?  What algorithms are valid for use with the
"password" case?  RFC 8177 is the sole reference for both the
"key-chain" leaf and the "password" case, but that does not seem a
sufficient reference from which to implement.

Also, there are a couple of elements in the "state-attributes" container
that say they indicate a time when something will/did happen and measure
it in seconds.  I don't see an indication of what the reference point is
for them, though -- "seconds since when?".  Further context in the
COMMENT to avoid too much quoted text here.


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

This document (and the YANG module) lists six authors, which is more
than the typical five and would normally spark some discussion.  I do
see that the shepherd writeup indicates that all six members of the
design team were quite active, and presume that this indicates that it
is appropriate to have the larger author count than is typically seen.

Section 2.1

   This model can be used to configure and manage MSDP protocol.  The

nit: "the MSDP protocol".

Section 3.1

   MSDP configurations require peer configurations.  Several peers may

nit: this sentence on first read sounds like it is instantiating a
dependency loop ("my MDSP configuration depends on my peers' MSDP
configuration, which in turn depends on mine").  I suspect the intent is
to say something like "MSDP operation requires configuration information
that is distributed amongst several peers" or "MSDP operation requires
that all peers active in a given group receive identical configuration
information for that group" or similar.

Section 4

    grouping authentication-container {
          [...]
          case password {
            leaf key {
              type string;
              description
                "This leaf describes the authentication key.";
              reference

I'm not entirely sure what is being "described" -- if this is the
"password" choice (vs. key-chain), is it not the actual password itself
(versus a description thereof)?

            leaf crypto-algorithm {
              type identityref {
                base key-chain:crypto-algorithm;
              }
              description
                "Cryptographic algorithm associated with key.";

This is still in the "password" case, and I'm not entirely sure which
sorts of algorithms you're expecting to see here.  Things like
"hmad-sha-256" don't really apply directly to a password, and bare
hashes like sha-1 are cryptographically bad choices.

    grouping global-config-attributes {
        [...]
          "The default peer accepts all MSDP SA messages.
           A default peer is needed in topologies where MSDP peers
           do not coexist with BGP peers. The reverse path
           forwarding (RPF) check on SA messages can fail, and no
           SA messages are accepted. In these cases, you can configure
           the peer as a default peer and bypass RPF checks.";

nit: I think there could be a better connection between the sentences
here.  Is the need for default peer (in such topologies) due solely to
the potential for RPF failure?

        leaf peer-addr {
          type leafref {
            path "../../../peers/peer/address";

[Does this imply restrictions on where the grouping can be used in the
tree?]

      leaf mesh-group {
        type string;
        description
          "Configure this peer to be a member of a mesh group";
        reference
          "RFC 3618: Multicast Source Discovery Protocol (MSDP),
                     section 10.2.";
      }

This description leaves the reader rather unclear about what the
contents of this string-type leaf are supposed to be.

      leaf peer-as {
         [...]
          "Peer's autonomous system number (ASN). Using peer-as to
           do verification can provide more controlled ability.

nits: (1) more controlled than what?  (2) ability to do what?

           If the AS number is the same as the local AS, then the
           peer is within the same domain; otherwise, this peer is
           external to the domain. Like the definition and usage
           in BGP protocol.";

nit: "BGP protocol" is redundant.

site note: Both holdtime-interval and keepalive-interval have "must"
statements asserting that keepalive < holdtime when both are present.
Is it well-defined which one will trigger if an attempt is made to
violate the condition?  (The error message is the same for both, so this
is basically just for my personal edification.)

      leaf elapsed-time {
        type uint32;
        units seconds;
        config false;
        description "Elapsed time for being in a state.";
      }

Is this more properly a counter type than a uint32?

      leaf is-default-peer {
        type boolean;
        config false;
        description "'true' if this peer is a default peer.";

nit(?): how can there be more than one ("a") default peer?

      leaf reset-count {
        type uint32;
        config false;
        description "The reset count of this peer.";
      }

This one also sounds like it might be a counter.

      container statistics {

Please double-check for whether counter and/or gauge types might be more
appropriate than integer types.

            container state-attributes {
              description "SA cache state attributes for MSDP.";

              leaf up-time {
                type uint32;
                units seconds;
                description
                  "Indicates the time when this SA entry is created
                   in the cache. MSDP is a periodic protocol, the
                   up-time value can be used to check the state of
                   SA cache.";
              }
              leaf expire {
                type uint32;
                units seconds;
                description
                  "Indicates the time when this SA entry in the cache
                   times out. MSDP is a periodic protocol, the expire
                   value can be used to check the state of SA cache.";
              }

If these are "time" entries, that sounds like an absolute time.  If it's
measured in seconds, what is the reference point for that absolute time?

              leaf rpf-peer {
                type inet:ipv4-address;
                description
                  "The address is used to find the SA's
                   originating RP.";

"used to find" the originating RP or *is* the originating RP?  The name
would suggest the latter.

Section 5

      This subtree specifies the configuration for the MSDP attributes
      at the peer level.  The modification configuration will allow the
      unexpected MSDP peer establishment and unexpected SA information
      learning and advertisement.

nits: s/allow the unexpected/allow unexpected/; s/The modification
cofiguration/Modifying the configuration/.

      The "key" field is also a sensitive readable configuration, the
      unauthorized reading function may lead to the password leaking.
      The modification will allow the unexpected peer reconstruction.

I think it would be appropriate to mention that the key-chain choice
from RFC 8177 is designed to avoid this sort of vulnerabilities.
Also, nits: comma splice in the first sentence, and s/The
modification/Modification/.

Also^2, is this note better placed in the "readable data nodes" section
of the template?

   /rt:routing/rt:control-plane-protocols/msdp,

   Unauthorized access to any data node of the above subtree can
   disclose the operational state information of MSDP on this device.

It would be nice to go into a bit more detail about some of the
particularly important leaves under the msdp tree, and how knowing their
contents might aid an attacker.
E.g., the ACL nodes seem of particular interest (whether reading or
writing).

   Unauthorized access to any of the above action operations can
   reconstruct the MSDP peers or delete SA records on this device.

Will the reader know what "reconstruct the peer" means here (and the
operational/security impact of doing so)?

Section 9.1

It's not entirely clear to me that RFC 3688 is referenced in a normative
(as opposed to informative) manner.

Appendix A

Where does the 173.104.116.8 addrss come from?  I don't see it in RFC
5737 and it's not in the multicast range.
Likewise for 101/8.