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

["Rob Wilton (rwilton)" <[e-mail]>]

Hi,

Some additional comment on Ben's comments, and also a comment about actions.

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

Both of these would trigger.

I believe that these must statements can be simplified:
 - It is only necessary (and more correct) to only have one of them.
 - No special handling is required for defaults.  At the point in the instance data tree that the must statements is evaluated, it will be known that the other leaf semantically exists because it has a defined default value.

So, these two leaf statements can just be written as:

        leaf holdtime-interval {
          type uint16 {
            range "3..65535";
          }
          units seconds;
          default 75;
          description "The SA hold down period of this MSDP peer.";
        }
        leaf keepalive-interval {
          type uint16 {
              range "1..65535";
          }
          units seconds;
          must '. < ../holdtime-interval' {
            error-message
              "The keepalive interval must be smaller than the
               hold time interval";
          }
          default 60;
          description "The keepalive timer of this MSDP peer.";
        }


In addition to this, I think that it would be helpful for the draft to highlight that it defines action statements, along with a brief description of what they do.  This could probably just be added to the end of section 3.2.

For the action statements in the YANG model, did you consider binding them more directly to the data nodes that they are affecting?

For example, I would suggest splitting the "clear-peer" action into two separate actions:
 1) clear - that could be defined within the "peer" entry (and hence would require no arguments)
 2) clear-all - defined under the "peers" container, but requiring no arguments.

For the "clear-sa-cache" action, this could be defined under "sa-cache", and renamed to "clear", but otherwise the definition would stay as is.

Thanks,
Rob


> -----Original Message-----
> From: iesg <[e-mail]> On Behalf Of Benjamin Kaduk via
> Datatracker
> Sent: 29 February 2020 00:05
> To: The IESG <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Benjamin Kaduk's Discuss on draft-ietf-pim-msdp-yang-15: (with
> DISCUSS and COMMENT)
> 
> 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.
> 
>