Invalid TLV Handling in IS-IS
draft-ietf-lsr-isis-invalid-tlv-03

Note: This ballot was opened for revision 02 and is now closed.

Benjamin Kaduk Yes

Comment (2020-07-13 for -02)
The shepherd writeup is a bit unclear as to why Proposed Standard is the
right document status (vs., e.g., Informational).  I guess it's desired
to have the Updates: relationship to the indicated documents, which
essentially forces it to be standards-track.  On the other hand, perhaps
the sense that ignoring a TLV that is specifically disallowed (as
opposed to "merely" unrecognized) is itself a newly specified
requirement, in which case the status as Proposed Standard makes sense
for that reason.  It might be worth a little more clarity on which (if
either) of these scenarios are intended.

Section 1

   A corollary to ignoring unknown TLVs is having the validation of PDUs
   be independent from the validation of the TLVs contained in the PDU.

nit: this doesn't exactly seem like a "corollary" specifically, but
rather a choice that [ISO10589] made (and which keeps some overall sense
of consistency between PDU and TLV handling).

Section 3.1

   [ISO10589] defines the behavior required when a PDU is received
   containing a TLV which is "not recognised".  It states (see Sections
   9.3 - 9.13):

This is Sections 9.5 (not 9.3) to 9.13 in the copy I have.

Section 3.2

   Similarly, the extensions defined by [RFC6233] are not compatible
   with the behavior defined in [RFC5304], therefore can only be safely
   enabled when all nodes support the extensions.

nit: I'd argue that technically the extensions are *defined* by 6232, even
though 6233 is what makes their nature (as "allowed in purge") easily
discoverable.

   It is RECOMMENDED that implementations provide controls for the
   enablement of behaviors that are not backward compatible.

We also specifically want the ability to generate but not
process/require at least some of them, right?  Is that worth calling out
in addition to just "controls for the enablement"?

Section 3.3

   a given sub-TLV is allowed.  Section 2 of [RFC5305] is updated by the
   following sentence:

      "As with TLVs, it is required that sub-TLVs which
       are disallowed MUST be ignored on receipt.".

Do we want to say where this logical insertion occurs?

Section 3.4

   The correct setting for "LSP" is "n".  This document updates
   [RFC6232] by correcting that error.

It's slightly interesting that there doesn't seem to be a corresponding
Errata Report (but may not be worth doing anything about given that this
update is about to be approved).

Section 8.1

It's not entirely clear that RFC 7356 is referenced in a normative
manner.

Alvaro Retana Yes

Deborah Brungard No Objection

Roman Danyliw No Objection

Comment (2020-07-13 for -02)
I'm glad to see language clarifying error handling.  Thanks for the work on it.

Section 3.2.  Per “It is RECOMMENDED that implementations provide controls for the enablement of behaviors that are not backward compatible”, I want to double check that I’m understanding this  sentence correctly. RFC5304 provides normative guidance that isn’t backward compatible with ISO10589. RFC6233 provide guidance that isn’t backward compatible with either RFC5304 or ISO10589.  Is the initial sentence effectively saying that implementations should support deployments in configurations that are not backward compatible (i.e., those using the newer TLVs)?  As these changes are covering security matters, I read “controls” in the cyber mitigation sense -- they prevent an action, not enable one.

Martin Duke No Objection

Comment (2020-07-11 for -02)
It might be helpful to define “ignore” as “skip the number of octets indicated by the length field.” An alternate interpretation might skip the number of bytes implied by the type code, if the type is known.

Similarly, I take it that a length value beyond the end of the message ends processing of the PDU, but the PDU as a whole MUST NOT be discarded.

Erik Kline No Objection

Comment (2020-07-10 for -02)
No email
send info
[ section 2 ]

* s/PDUS/PDUs/

Murray Kucherawy No Objection

Comment (2020-07-13 for -02)
In the Abstract:

* "... in order to insure that interoperability is maximized." --  That should be "ensure".

Barry Leiba No Objection

Martin Vigoureux No Objection

Éric Vyncke No Objection

Comment (2020-07-13 for -02)
No email
send info
Minor nits in the abstract:

Suggest using ';' and more commas in 
  "Although there are explicit
   statements in existing specifications, deployment experience has
   shown that there are inconsistencies in the behavior when a TLV which
   is disallowed in a particular Protocol Data Unit (PDU) is received."

Robert Wilton No Objection

Comment (2020-07-14 for -02)
The document is short and easy to read, thanks!  However, I was sure whether I should put a DISCUSS on this document for section 3.4.

I find that the quoted paragraph from RFC6232 to be badly worded:

      "The POI TLV SHOULD be found in all purges and
       MUST NOT be found in LSPs with a non-zero
       Remaining Lifetime."

Taking a strict reading of this, my interpretation is that an implementation is not compliant to RFC 6232 if it happens to receive a POI TLV in an LSP with non-zero remaining lifetime!  Further, this text then arguably conflicts with earlier parts of this document regarding how unrecognized or bad TLVs should be handled.

Hence, given that RFC6232 is being updated, I would prefer it if this document also updated RFC6232 to clarify the above paragraph to something like:

      "The POI TLV SHOULD be sent in all purges and
       MUST NOT be sent in LSPs with a non-zero
       Remaining Lifetime."

One other minor comment:

    It is RECOMMENDED that implementations provide controls for the enablement of behaviors that are not backward compatible.

Is this covered by the existing ISIS YANG model, or included in the latest update to that YANG model?

Regards,
Rob