[Lsr] AD Review of draft-ietf-lsr-isis-invalid-tlv-01

[Alvaro Retana <[e-mail]>]

Dear authors:

First of all, thank you for taking on this work!

I have some comments which I home will be easy to address -- please
see in-line below.  I will wait for a revised ID before starting the
IETF Last Call.

Before getting into the specifics, idnits came up with this warning:

=====
     (Using the creation date from RFC3563, updated by this document, for
     RFC5378 checks: 2002-10-15)

  -- The document seems to lack a disclaimer for pre-RFC5378 work, but may
     have content which was first submitted before 10 November 2008.  If you
     have contacted all the original authors and they are all willing to grant
     the BCP78 rights to the IETF Trust, then this is fine, and you can ignore
     this comment.  If not, you may need to add the pre-RFC5378 disclaimer.
     (See the Legal Provisions document at
     https://trustee.ietf.org/license-info for more information.)
=====

This documents Updates several existing RFCs.  In general, the warning
means that we need to ask the authors of those RFCs, if published
before rfc5378, to grant BCP78 rights to the IETF Trust.  In this case
only rfc3563 is affected.  rfc5305 is not included because Tony Li was
one of the authors.

rfc6233 already Updated rfc3563, and it didn't include the disclaimer
for pre-RFC5378 work, which leads me to conclude that the BCP78 rights
were already granted at that time.  IOW, we don't need the disclaimer
for pre-RFC5378 work.

I'm writing all this here to have a record of this decision and to
give anyone in the WG the opportunity to raise concerns, if any.


Thanks!!

Alvaro.


[Line numbers from idnits.]


...
15	Abstract
...
27	   This document when approved updates RFC3563, RFC5305, RFC6232, and
28	   RFC6233.

[minor] s/This document when approved updates/This document updates


...
90	1.  Introduction

92	   The Intermediate System to Intermediate System (IS-IS) protocol
93	   utilizes Type/Length/Value (TLV) encoding for all content in the body
94	   of Protocol Data Units (PDUs).  New extensions to the protocol are
95	   supported by defining new TLVs.  In order to allow protocol
96	   extensions to be deployed in a backwards compatible way an
97	   implementation is required to ignore TLVs that it does not
98	   understand.  This behavior is also applied to sub-TLVs, which are
99	   contained within TLVs.

[minor] Add references to ISO10589 and rfc5305 in this first
paragraph: ...(IS-IS) protocol [ISO10589]...applied to sub-TLVs
[RFC5305]...


101	   A corollary to ignoring unknown TLVs is having the validation of PDUs
102	   be independent from the validation of the TLVs contained in the PDU.
103	   PDUs which are valid MUST be accepted even if an individual TLV
104	   contained within that PDU is invalid in some way.

[major] "MUST be accepted"   This is the behavior already specified in
ISO10589, right?   If so, then we shouldn't make it Normative here;
instead, s/MUST/must and add a reference to ISO10589.

Including that reference makes the first sentence in the next
paragraph unnecessary.


106	   These behaviors are specified in existing protocol documents -
107	   principally [ISO10589] and [RFC5305].  In addition, the set of TLVs
108	   (and sub-TLVs) which are allowed in each PDU type is documented in
109	   the TLV Codepoints Registry ( https://www.iana.org/assignments/isis-
110	   tlv-codepoints/isis-tlv-codepoints.xhtml ) established by [RFC3563]
111	   and updated by [RFC6233] and [RFC7356].

[minor] s/( https://www.iana.org/assignments/isis-tlv-codepoints/isis-tlv-codepoints.xhtml
)/[TLV_CODEPOINTS]


113	   This document is intended to clarify some aspects of existing
114	   specifications and thereby reduce the occurrence of non-conformant
115	   behavior seen in real world deployments.  Although behaviors
116	   specified in existing protocol specifications are not changed, the
117	   clarifications contained in this document serve as updates to RFC
118	   3563 (see Section 2), RFC 5304, and RFC 6233 (see Section 3).

[minor] You missed RFC6232.

[major] s/5304/5305   Is §3.2 intended to update rfc5304?

[major] §3 has several subsections; can we please be specific above?
For example, §3.3 clearly updates rfc5305...§3.4 rfc6232...  I
suggested some text for §2/§3.3/§4.3; please add other explicit
indications of the updates.

[major] An important clarification, which I don't think is explicitly
made, is that the behavior for unknown is being applied to disallowed.
Not knowing and not expecting (= disallowed) are different things, but
this document is saying that the document treats them the same:
ignore.  I think adding a sentence about that relationship would help
a lot, and it may help us cover any cases that may have been missed (I
don't think there are any, but just in case)...maybe something like
this (I'm sure you can come up with much better words):

   This document extends the concept of unknown to disallowed.


[major] Related to the last comment...  The title of the document
mentions "Invalid" TLVs.  (1) there is no other mention of an invalid
TLV in the text, and (2) there is no connection made between "invalid"
(in the title) and "disallowed" (in the text).


120	2.  TLV Codepoints Registry

122	   [RFC3563] established the IANA managed IS-IS TLV Codepoints Registry
123	   for recording assigned TLV code points [TLV_CODEPOINTS].  The initial
124	   contents of this registry were based on [RFC3359].

[nit] s/IANA managed/IANA-managed


...
142	   If "N" is entered in a column it means the TLV is NOT allowed in the
143	   corresponding PDU type.

[minor] s/NOT/not/g   I know you want the emphasis, but the comment
always comes later about "NOT" not being a key word.

[major] Please indicate somewhere in this section that the update to
rfc3563 is the description of the PDU type columns.


...
151	3.1.  Handling of Disallowed TLVs in Received PDUs other than LSP Purges
...
157	   "Any codes in a received PDU that are not recognised shall be
158	   ignored."

[style nit] Indenting would highlight the quote; there are several
quotes throughout the document.


...
165	3.2.  Special Handling of Disallowed TLVs in Received LSP Purges

167	   When purging LSPs [ISO10589] recommends (but does not require) the
168	   body of the LSP (i.e., all TLVs) be removed before generating the
169	   purge.  LSP purges which have TLVs in the body are accepted though
170	   any TLVs which are present "MUST" be ignored.

[nit] s/When purging LSPs/When purging LSPs,

[major] I'm sure that ISO10589 doesn't have a MUST in it: s/"MUST" be
ignored/are ignored


172	   When cryptographic authentication [RFC5304] was introduced, this
173	   looseness when processing received purges had to be addressed in
174	   order to prevent attackers from being able to initiate a purge
175	   without having access to the authentication key.  [RFC5304] therefore
176	   imposed strict requirements on what TLVs were allowed in a purge
177	   (authentication only) and specified that:

179	   "ISes MUST NOT accept purges that contain TLVs other than the
180	   authentication TLV".

182	   This behavior was extended by [RFC6232] which introduced the Purge
183	   Originator Identification (POI) TLV and [RFC6233] which added the
184	   "Purge" column to the TLV Codepoints registry to identify all the
185	   TLVs which are allowed in purges.

187	   The behavior specified in [RFC5304] is not backwards compatible with
188	   the behavior defined by [ISO10589] and therefore can only be safely
189	   enabled when all nodes support cryptographic authentication.
190	   Similarly, the extensions defined by [RFC6233] are not compatible
191	   with the behavior defined in [RFC5304], therefore can only be safely
192	   enabled when all nodes support the extensions.

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

[major] Is this a new recommendation made in this document?  If so,
should "RECOMMENDED" (aka SHOULD) be used instead?


197	3.3.  Applicability to sub-TLVs

199	   [RFC5305] introduced sub-TLVs, which are TLV tuples advertised within
200	   the body of a parent TLV.  Registries associated with sub-TLVs are
201	   associated with the TLV Codepoints Registry and specify in which TLVs
202	   a given sub-TLV is allowed.  As with TLVs, it is required that sub-
203	   TLVs which are disallowed MUST be ignored on receipt.

[major] The text in rfc5305 reads "Unknown sub-TLVs are to be ignored
and skipped upon receipt."  Please be specific in the last sentence,
something like:

   Section 2 of rfc5305 is updated by adding this sentence: As with TLVs,
   sub-TLVs which are disallowed MUST be ignored on receipt.

You may also want to update the text in rfc5305 about unknown to make
it Normative.


205	3.4.  Correction to POI TLV Registry Entry
...
215	   "The additional values for this TLV should be IIH:n, LSP:y, SNP:n,
216	   and Purge:y. "

[nit] s/y. "/y."


218	   The correct setting for "LSP" is "n".  This document corrects that
219	   error.

[major] s/This document corrects that error./This document updates
rfc6232 to correct that error.


221	4.  TLV Validation and LSP Acceptance

223	   The correct format of a TLV and its associated sub-TLVs if applicable
224	   are defined in the document(s) which introduce each codepoint.  The
225	   definition SHOULD include what action to take when the format/content
226	   of the TLV does not conform to the specification (e.g., "MUST be
227	   ignored on receipt").  When making use of the information encoded in
228	   a given TLV (or sub-TLV) receiving nodes MUST verify that the TLV
229	   conforms to the standard definition.  This includes cases where the
230	   length of a TLV/sub-TLV is incorrect and/or cases where the value
231	   field does not conform to the defined restrictions.

[nit] s/sub-TLVs if applicable/sub-TLVs, if applicable,

[major] "SHOULD include what action to take when the format/content of
the TLV does not conform to the specification"  When is it ok to not
include that information?  It sounds like basic error correction to
me.  IOW, why is "MUST" not used?


...
240	   LSP Acceptance rules are specified in [ISO10589] .  Acceptance rules
241	   for LSP purges are extended by [RFC5304] [RFC5310] and further
242	   extended by [RFC6233].

[nit] s/[RFC5304] [RFC5310]/[RFC5304] and [RFC5310],


...
249	5.  IANA Considerations
...
254	   IANA is also requested to modify the entry for the POI TLV in the TLV
255	   Codepoints Registry to be:

[major] s/POI/Purge Originator Identification
I know POI was expanded before, but let's be explicit in the IANA instructions.


257	   IIH:n, LSP:n, SNP:n, and Purge:y.

[major] A reference to this document should also be added for POI.


259	6.  Security Considerations
...
264	   The clarifications discussed in this document are intended to make it
265	   less likely that implementations will incorrectly process received
266	   LSPs, thereby also making it less likely that a bad actor could
267	   exploit a faulty implementaion.

[nit] s/implementaion/implementation