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

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Wed, 21 Aug 2019 22:45:46 -0700

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.

(2) Do we need to say anything about how to indicate when there are
discontinuities for the various "counter" types?

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

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.

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.

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.

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

     identity ospfv2-lsa-option {
       description
         "Baes idenity for OSPFv2 LSA option flags.";

nit: "Base"

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

     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?

     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.

     grouping ospf-router-lsa-bits {
       container rputer-bits {

s/rputer/router/?

         container te-opaque {
           [...]
           container link-tlv {
             description "Describes a singel link, and it is constructed
             of a set of Sub-TLVs.";

s/singel/single/

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

     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.

     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?

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

       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.

       leaf lls {
       [...]
       container ttl-security {

Should these have a default value?

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

           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?

       container preference {
         description
           "Route preference configuration In many
            implementations, preference is referred to as
            administrative distance.";

nit: missing sentence break?

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?

     grouping notification-neighbor {
       [...]
       leaf neighbor-ip-addr {
         type yang:dotted-quad;
         description "Neighbor address.";

neighbors can only be identified by IPv4 addresses?

       leaf packet-source {
         type yang:dotted-quad;

packet sources, too? (multiple times)

       description
         "This notification is sent when aa neighbor
          state change is detected.";

nit: s/aa/a/

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.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Fri, 23 Aug 2019 00:30:27 +0000

Hi Ben, 

On 8/22/19, 1:46 AM, "Benjamin Kaduk via Datatracker" <[email-masked]> wrote:

    

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. 

I've never heard any mention of counter discontinuities in the context of OSPF control plane counters. 

    
    Section 1

Updated in -27.
    
    Section 2

Fixed in -27.
    
    Section 2.2

I removed this paragraph and augmented the next one.
    
    Section 2.7

Fixed. 
    
    Section 3

The latter - fixed. 

    nit: "Base"

Fixed. 
    
         identity v2-p-bit {
           base ospfv2-lsa-option;
           description

Made these consistent. This was added very late when we realized that YANG type bit is not extendable. 
    
         grouping tlv {
           description

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. 

I fixed the description on these to differential between the list uint32 flags and the identities.

    s/rputer/router/?

Fixed. 
    
             container te-opaque {
               [...]
               container link-tlv {
    
    s/singel/single/

Fixed. 

Good catch. This should be unit32 for OSPFv3. There were two instances of this. 
    
         grouping lsa-common {
           description

Fixed - "will not be".
    
         grouping lsa-key {
           description
             "OSPF LSA key.";

Yes - I've improved.
    
           container database {

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. 
    
           leaf transmit-delay {
             type uint16;
             units seconds;
             description

Fixed - "when advertised on".
    
           leaf lls {
           [...]
           container ttl-security {

I think 1 would be appropriate since only virtual-links and sham-links require more.
    
               case ospfv3-auth-ipsec {

If it is not human readable, how would it be configured? 
    
               leaf poll-interval {
                 type uint16;
                 units seconds;
                 description

Fixed.
    
           container preference {
             description

Fixed.

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. 

Changed to IP address type.
    
           leaf packet-source {
             type yang:dotted-quad;

Changed to IP address type. 

           description
    
    nit: s/aa/a/

Fixed. 
    
    Section 4

Can you suggest some text?

Thanks,
Acee

---

From: Benjamin Kaduk <[email-masked]>
Date: Thu, 22 Aug 2019 20:44:02 -0500

>     https://datatracker.ietf.org/doc/draft-ietf-ospf-yang/

Okay, that's enough to resolve the discuss.

> 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.";

Thanks; it makes a lot more sense to me now.

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

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

Ah, good point.

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

> 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 t…

Thanks for the extra text -- it helps confirm my guess as to what's going
on.

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

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

> 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
entirey OSPF domain.

Thanks,

Ben

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 26 Aug 2019 15:02:18 +0000

Hi Ben, 

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

    

    

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. 

    

    
    Ah, good point.

Yes - I've fixed in -28. 

    

    
    Understood, and thanks.

    How's this?
    
    entirety OSPF domain.

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

Thanks,
Acee
    
    Thanks,
    
    Ben

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 26 Aug 2019 10:09:47 -0500

Hi Acee,

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

Thank you for doing the research -- I had forgotten to even check for an
analogous MIB!

The updates in the -28 look good, and I've cleared in the datatracker.

-Ben

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 26 Aug 2019 21:54:06 +0000

Hi Ben, 

On 8/26/19, 11:11 AM, "Lsr on behalf of Benjamin Kaduk" <[email-masked] on behalf of [email-masked]> wrote:

    Hi Acee,

    

Thanks much for the review - especially the detailed review of the YANG model.
Acee
    
    -Ben
    
    _______________________________________________
    Lsr mailing list
    [email-masked]
    https://www.ietf.org/mailman/listinfo/lsr

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 27 Aug 2019 14:54:56 +0200

> spec.  In some cases (and I don't know exactly when it becomes most

In fact, such occassions are taken into account in the definition of the yang:counter32 type [RFC6991]. The ietf-ospf also properly describes, for each use of that type, the situations when a discontinuity may occur. 

> counters, and if you tell me that I'm happy to clear.

Another means for learning about discontinuities may be notifications - for example, a discontinuity in nbr-event-count can probably be inferred from the nbr-state-change notification.

Lada

-- 
Ladislav Lhotka 
Head, CZ.NIC Labs
PGP Key ID: 0xB8F92B08A9F76C67