Re: [Lsr] Benjamin Kaduk's Discuss on draft-ietf-ospf-yang-26: (with DISCUSS and COMMENT)

["Acee Lindem (acee)" <[e-mail]>]

Hi Ben, 

On 8/22/19, 9:44 PM, "Benjamin Kaduk" <[e-mail]> wrote:

    On Fri, Aug 23, 2019 at 12:30:27AM +0000, Acee Lindem (acee) wrote:
    > Hi Ben, 
    > 
    > On 8/22/19, 1:46 AM, "Benjamin Kaduk via Datatracker" <[e-mail]> wrote:
    > 
    >     Benjamin Kaduk has entered the following ballot position for
    >     draft-ietf-ospf-yang-26: 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-ospf-yang/
    >     
    >     
    >     
    >     ----------------------------------------------------------------------
    >     DISCUSS:
    >     ----------------------------------------------------------------------
    >     
    >     (1) Can we check whether it's okay to use the yang "string" type for raw
    >     cryptographic keys (e.g., ospfv2-key, ospfv3-key)?  My understanding was
    >     that yang strings were limited to human-readable, but that the crypto
    >     keys could be raw binary values.
    > 
    > It does only include human-readable keys. Here is the RFC 7950 YANG ABNF definition.
    > 
    > 
    >    ;; any Unicode or ISO/IEC 10646 character, including tab, carriage
    >    ;; return, and line feed but excluding the other C0 control
    >    ;; characters, the surrogate blocks, and the noncharacters
    >    yang-char = %x09 / %x0A / %x0D / %x20-D7FF /
    >                                ; exclude surrogate blocks %xD800-DFFF
    >               %xE000-FDCF /    ; exclude noncharacters %xFDD0-FDEF
    >               %xFDF0-FFFD /    ; exclude noncharacters %xFFFE-FFFF
    >               %x10000-1FFFD /  ; exclude noncharacters %x1FFFE-1FFFF
    >               %x20000-2FFFD /  ; exclude noncharacters %x2FFFE-2FFFF
    >               %x30000-3FFFD /  ; exclude noncharacters %x3FFFE-3FFFF
    >               %x40000-4FFFD /  ; exclude noncharacters %x4FFFE-4FFFF
    >               %x50000-5FFFD /  ; exclude noncharacters %x5FFFE-5FFFF
    >               %x60000-6FFFD /  ; exclude noncharacters %x6FFFE-6FFFF
    >               %x70000-7FFFD /  ; exclude noncharacters %x7FFFE-7FFFF
    >               %x80000-8FFFD /  ; exclude noncharacters %x8FFFE-8FFFF
    >               %x90000-9FFFD /  ; exclude noncharacters %x9FFFE-9FFFF
    >               %xA0000-AFFFD /  ; exclude noncharacters %xAFFFE-AFFFF
    >               %xB0000-BFFFD /  ; exclude noncharacters %xBFFFE-BFFFF
    >               %xC0000-CFFFD /  ; exclude noncharacters %xCFFFE-CFFFF
    >               %xD0000-DFFFD /  ; exclude noncharacters %xDFFFE-DFFFF
    >               %xE0000-EFFFD /  ; exclude noncharacters %xEFFFE-EFFFF
    >               %xF0000-FFFFD /  ; exclude noncharacters %xFFFFE-FFFFF
    >               %x100000-10FFFD  ; exclude noncharacters %x10FFFE-10FFFF
    > 
    > However, this is the intent as this is support for implementations that don't support key-chains (RFC 8177). The "Security Considerations" recommend key chains. We'll add the hexadecimal specification as another reason for preferring key-chains. 
    
    Okay, that's enough to resolve the discuss.
    
    >  
    >   
    >     (2) Do we need to say anything about how to indicate when there are
    >     discontinuities for the various "counter" types?
    > 
    > I've never heard any mention of counter discontinuities in the context of OSPF control plane counters. 
    
    Oh, I think this is more of a YANG thing than an OSPF thing (and I'm hardly
    a YANG expert).  That is, the yang:counter32 and similar types are
    constrained to only ever increment ("count events that happened", roughly),
    as opposed to a gauge32 that measures the current level of something and
    can go up or down, or a generic uint32.  Of course, in the real world
    software crashes or restarts, so a YANG consumer might in practice see the
    value of a "counter" type go backwards, even though that's forbidden by the
    spec.  In some cases (and I don't know exactly when it becomes most
    relevant), the YANG model includes a "discontinuity time" (e.g., router
    restart time) to indicate when the counters were last reset to zero, in
    order to give consumers a sense for how long it took the counter to get
    that big.  It's entirely possible that this doesn't make sense for the OSPF
    counters, and if you tell me that I'm happy to clear.
    
    Searching for "RFC YANG discontinuity" brings up several representative
    results, such as RFC 8343, which says:
    
               leaf discontinuity-time {
                 type yang:date-and-time;
                 mandatory true;
                 description
                   "The time on the most recent occasion at which any one or
                    more of this interface's counters suffered a
                    discontinuity.  If no such discontinuities have occurred
                    since the last re-initialization of the local management
                    subsystem, then this node contains the time the local
                    management subsystem re-initialized itself.";

I did some research and the OSPF MIB (RFC 4750) did have a discontinuity time (ospfDiscontinuityTime). I added discontinuity-time for each applicable object in the -28 version. 
    
    
    >     
    >     ----------------------------------------------------------------------
    >     COMMENT:
    >     ----------------------------------------------------------------------
    >     
    >     I'm happy to see the discussion around Roman's Discuss.
    >     
    >     Section 1
    >     
    >        YANG [RFC6020][RFC7950] is a data definition language used to define
    >        the contents of a conceptual data store that allows networked devices
    >        to be managed using NETCONF [RFC6241].  YANG is proving relevant
    >        beyond its initial confines, as bindings to other interfaces (e.g.,
    >        ReST) and encodings other than XML (e.g., JSON) are being defined.
    >     
    >     This text feels a bit stale at this point.
    > 
    > Updated in -27.
    >     
    >     Section 2
    >     
    >        model varies among router vendors.  Differences are observed in terms
    >        of how the protocol instance is tied to the routing domain, how
    >        multiple protocol instances are be instantiated among others.
    >     
    >     nit: the grammar here is a bit odd, with the comma suggesting the start
    >     of a list but no "and" present.
    > 
    > Fixed in -27.
    >     
    >     Section 2.2
    >     
    >        The ospf module is intended to match to the vendor specific OSPF
    >        configuration construct that is identified by the local identifier
    >        'name'.
    >     
    >     I don't really understand what this is intended to mean.
    > 
    > I removed this paragraph and augmented the next one.
    
    Thanks; it makes a lot more sense to me now.
    
    >     Section 2.7
    >     
    >     hello-timer in the module claims to be a rt-types:timer-value-seconds16
    >     but shows up in the tree(s) as a uint32.
    >     Similarly, the wait-itmer is a rt-types:timer-value-seconds32, which
    >     also shows up in the tree(s) as a uint32, which is perhaps more
    >     reasonable but perhaps not entirely accurate.
    >     Other 'timer' leafs seem to have similar issues.
    > 
    > Fixed. 
    >     
    >     Section 3
    >     
    >          feature ospfv2-authentication-trailer {
    >            description
    >              "Use OSPFv2 authentication trailer for OSPFv2
    >               authentication.";
    >     
    >     Is the feature for "use" or "support for"?
    >     (Similarly for the ospfv3 authentication features.)
    > 
    > The latter - fixed. 
    >     
    >          identity ospfv2-lsa-option {
    >            description
    >              "Baes idenity for OSPFv2 LSA option flags.";
    >     
    >     nit: "Base"
    > 
    > Fixed. 
    >     
    >          identity v2-p-bit {
    >            base ospfv2-lsa-option;
    >            description
    >              "Only used in type-7 LSA. When set, an NSSA
    >               border router should translate the type-7 LSA
    >               to a type-5 LSA.";
    >     
    >     There seem to be a few "identity <foo>-bit" stanzas whose description do
    >     not mention the named bit specifically (but many that do).  Do we want
    >     to be consistent about doing so?  (Likewise for <foo>-flag.)
    > 
    > Made these consistent. This was added very late when we realized that YANG type bit is not extendable. 
    
    The YANG type bit is not extendable?!  I'll have to remebmer that for the
    future...
    
    >          grouping tlv {
    >            description
    >              "Type-Length-Value (TLV)";
    >            leaf type {
    >              type uint16;
    >              description "TLV type.";
    >            }
    >            leaf length {
    >              type uint16;
    >              description "TLV length (octets).";
    >            }
    >            leaf value {
    >              type yang:hex-string;
    >              description "TLV value.";
    >     
    >     Is there a way to apply a constraint so the 'length' matches the
    >     hext-string's length?
    > 
    > Since this is returned operational data, it doesn't make a lot of sense to put constraints on it. It is basically what the router returns. If it is wrong, it wouldn't have been parsed correctly in the first place. 
    
    Ah, good point.
    
    >          grouping router-capabilities-tlv {
    >     
    >     The various descriptions hereunder could perhaps benefit from section
    >     references, as, e.g., two nodes named "informational-capabilities" may
    >     otherwise require some effort to distinguish.  Well, aside from the fact
    >     that one is currently listed as "capabilitiess" with two esses, which
    >     seems unlikely to be intended.
    > 
    > I fixed the description on these to differential between the list uint32 flags and the identities.
    
    Thanks (BTW please double-check the description for "leaf functional-flag",
    that currenty looks like "Individual informational capability flag.")

Yes - I've fixed in -28. 
    
    >          grouping ospf-router-lsa-bits {
    >            container rputer-bits {
    >     
    >     s/rputer/router/?
    > 
    > Fixed. 
    >     
    >              container te-opaque {
    >                [...]
    >                container link-tlv {
    >                  description "Describes a singel link, and it is constructed
    >                  of a set of Sub-TLVs.";
    >     
    >     s/singel/single/
    > 
    > Fixed. 
    >     
    >          grouping ospfv3-lsa-external {
    >            [...]
    >            leaf referenced-link-state-id {
    >              type yang:dotted-quad;
    >     
    >     RFC 5340 section 2.2 implies that the Link State ID is going to be a
    >     32-bit identifier that need not be represented as dotted-quad, as it
    >     does not have addressing semantics.  (dotted-quad seems to be used for
    >     Link-State-ID-shaped things elsewhere, too, though the preferred form
    >     may be the union of dotted-quad and uint32.)
    > 
    > Good catch. This should be unit32 for OSPFv3. There were two instances of this. 
    >     
    >          grouping lsa-common {
    >            description
    >                "Common fields for OSPF LSA representation.";
    >            leaf decode-completed {
    >              type boolean;
    >              description
    >                "The OSPF LSA body was successfully decoded other than
    >                 unknown TLVs. Unknown LSAs types and OSPFv2 unknown
    >                 opaque LSA types are not decoded. Additionally,
    >                 malformed LSAs are generally not accepted and are
    >                 not be in the Link State Database.";
    >     
    >     nit: "are not be" is not grammatical.
    > 
    > Fixed - "will not be".
    >     
    >          grouping lsa-key {
    >            description
    >              "OSPF LSA key.";
    >     
    >     This could maybe benefit from a more descriptive description; is this a
    >     sort or lookup key, for example?
    > 
    > Yes - I've improved.
    >     
    >            container database {
    >              description "Container for per AS-scope LSA statistics.";
    >              list as-scope-lsa-type {
    >                [...]
    >                leaf lsa-cksum-sum {
    >                  type uint32;
    >                  description
    >                    "The sum of the LSA checksums of the LSA type.";
    >     
    >     [It's not entirely clear to me why this sum-of-checksums is a useful
    >     thing to track, but it may not be this document's role to do so.
    >     Though, perhaps we do need to say if the sum is computed as integers
    >     modulo 2**32.]
    > 
    > This is from the OSPF MIB (RFC 4750). It is not used functionally as one cannot guarantee differing LSDBs will not yield the same checksum. However, if the checksums are different, you can assure the databases are different. 
    
    Thanks for the extra text -- it helps confirm my guess as to what's going
    on.
    
    >            leaf transmit-delay {
    >              type uint16;
    >              units seconds;
    >              description
    >                "Estimated time needed to transmit Link State Update
    >                 (LSU) packets on the interface (seconds). LSAs have
    >                 their age incremented by this amount on advertised
    >                 on the interface. A sample value would be 1 second.";
    >     
    >     nit: "on advertised on" does not seem grammatical.
    > 
    > Fixed - "when advertised on".
    >     
    >            leaf lls {
    >            [...]
    >            container ttl-security {
    >     
    >     Should these have a default value?
    > 
    > I think 1 would be appropriate since only virtual-links and sham-links require more.
    >     
    >                case ospfv3-auth-ipsec {
    >                  when "derived-from-or-self(../../../../../../rt:type, "
    >                    +  "'ospfv3')" {
    >                    description "Applied to OSPFv3 only.";
    >                  }
    >                  if-feature ospfv3-authentication-ipsec;
    >                  leaf sa {
    >                      type string;
    >     
    >     I don't see RFC 4552 talking about names for SAs; where would this be
    >     discussed (and, are they guaranteed to be human-readable)?
    > 
    > If it is not human readable, how would it be configured? 
    
    I wasn't sure if this was config or operational state, but you make a good
    point.
    
    >                leaf poll-interval {
    >                  type uint16;
    >                  units seconds;
    >                  description
    >                    "Neighbor poll interval (seconds) for sending OSPF
    >                     hello packets to discover the neighbor on NBMA
    >                     networks. This interval dictates the granularity for
    >                     discovery of new neighbors. A sample would be 2 minutes
    >                     for a legacy Packet Data Network (PDN) X.25 network.";
    >     
    >     Maybe "2 minutes (120 seconds)" since the units are seconds?
    > 
    > Fixed.
    >     
    >            container preference {
    >              description
    >                "Route preference configuration In many
    >                 implementations, preference is referred to as
    >                 administrative distance.";
    >     
    >     nit: missing sentence break?
    > 
    > Fixed.
    >     
    >     For the spf- and lsa-logs, do we require that the 'id' is assigned in
    >     any particular order, or do we just rely on the included timestamp(s)
    >     for any time-ordering required by the consumer?
    > 
    > I added ordering to the description of the spf-log and lsa-log. However, the id is a purely internal value to provide a uniqueness key for the YANG list. 
    
    Understood, and thanks.
    
    >          grouping notification-neighbor {
    >            [...]
    >            leaf neighbor-ip-addr {
    >              type yang:dotted-quad;
    >              description "Neighbor address.";
    >     
    >     neighbors can only be identified by IPv4 addresses?
    > 
    > Changed to IP address type.
    >     
    >            leaf packet-source {
    >              type yang:dotted-quad;
    >     
    >     packet sources, too? (multiple times)
    >     
    > Changed to IP address type. 
    > 
    >            description
    >              "This notification is sent when aa neighbor
    >               state change is detected.";
    >     
    >     nit: s/aa/a/
    > 
    > Fixed. 
    >     
    >     Section 4
    >     
    >        Additionally, local specificationn of OSPF authentication keys and
    >        the associated authentication algorithm is supported for legacy
    >        implementations that do not support key-chains [RFC8177] for legacy
    >        implementations that do not support key-chains.  It is RECOMMENDED
    >        that implementations migrate to key-chains due the seamless support
    >        of key and algorithm rollover, as well as, the encryption of key
    >        using the Advanced Encryption Standard (AES) Key Wrap Padding
    >        Algorithm [RFC5649].
    >     
    >     (Roman caught the nits, so I won't duplicate that.)
    >     I expected to see something about keeping the actual key material
    >     secret, as well.
    > 
    > Can you suggest some text?
    
    How's this?
    
    The actual authentication key data (whether locally specified or part of a
    key-chain) is sensitive and needs to be kept secret from unauthorized
    parties; compromise of the key data would allow an attacker to forge OSPF
    traffic that would be accepted as authentic, potentially compromising the
    entirety OSPF domain.

Thanks - this is more prescriptive than what I had added which mirrored the text in RFC 8177. 

Thanks,
Acee
    
    Thanks,
    
    Ben