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

From: Alvaro Retana <[email-masked]>
Date: Thu, 18 Jun 2020 13:05:28 -0700

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

---

From: "Les Ginsberg (ginsberg)" <[email-masked]>
Date: Fri, 19 Jun 2020 15:49:47 +0000

Alvaro -

I will publish an update once we have converged on all your comments.

Responses inline.

> -----Original Message-----

> From: Alvaro Retana <[email-masked]>

> Sent: Thursday, June 18, 2020 1:05 PM

> To: [email-masked]

> Cc: Christian Hopps <[email-masked]>; [email-masked]; [email-masked]

> Subject: AD Review of draft-ietf-lsr-isis-invalid-tlv-01

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

[Les:] And thank you for insisting that the draft be written. 😊

[Les:] Just to be sure I understand, you are documenting that we need do nothing in this regard?

> 27          This document when approved updates RFC3563, RFC5305,

> RFC6232, and

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

[Les:] Done

> 93          utilizes Type/Length/Value (TLV) encoding for all content in the

> body

> [RFC5305]...

[Les:] Done

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

> instead, s/MUST/must and add a reference to ISO10589.

[Les:] Done

> paragraph unnecessary.

[Les:] Removed.

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

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

> codepoints.xhtml<https://www.iana.org/assignments/isis-tlv-codepoints/isis-tlv-codepoints.xhtml>

> )/[TLV_CODEPOINTS]

[Les:] Done

> 114       specifications and thereby reduce the occurrence of non-

> conformant

> [minor] You missed RFC6232.

[Les:] Fixed

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

[Les:] 3.2 updates RFC 5304 to the extent that it recommends controls for behaviors that might not be backwards compatible. (Similar for RFC 6233)

Section 3.3 clearly updates RFC 5305, so I have added that here.

But it is a fair question to ask if RFC 5304 should be added to the list of documents updated? It currently is NOT listed -  which makes me wonder why we include RFC 6233 in the list of updated documents.

I am inclined to not include either in the list of updated documents.

Please comment.

> indications of the updates.

[Les:] Done

[Les:] It seems to me that Section 3.1 is very clear on this point. I prefer not to change/add text as you have suggested.

I did add some examples of "invalid".

> 122       [RFC3563] established the IANA managed IS-IS TLV Codepoints

> Registry

> 123       for recording assigned TLV code points [TLV_CODEPOINTS].  The

> initial

> [nit] s/IANA managed/IANA-managed

[Les:] Done.

I see you and Acee went to grammar school together. 😊

> always comes later about "NOT" not being a key word.

[Les:] Done

> rfc3563 is the description of the PDU type columns.

[Les:] It isn’t clear to me that we have updated RFC3563 - unless you presume that anything that alters the registries in any way is considered an update to RFC 3563.

RFC3563 created the registry based on RFC 3359 - which had the columns in there.

The description of the columns is present in this document to provide context for the rest of the document. I don’t believe this description represents any change to the registry.

Specifically, we are not proposing that the registry be altered to include the description.

If you agree, should we then remove RFC 3563 from the list of RFCs updated by this document??

> 151     3.1.  Handling of Disallowed TLVs in Received PDUs other than LSP

> Purges

> quotes throughout the document.

[Les:] Done

> 167       When purging LSPs [ISO10589] recommends (but does not require)

> the

> 169       purge.  LSP purges which have TLVs in the body are accepted

> though

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

[Les:] Done

> ignored/are ignored

[Les:] Done

> 175       without having access to the authentication key.  [RFC5304]

> therefore

> 182       This behavior was extended by [RFC6232] which introduced the

> Purge

> 187       The behavior specified in [RFC5304] is not backwards compatible

> with

> 188       the behavior defined by [ISO10589] and therefore can only be

> safely

> 191       with the behavior defined in [RFC5304], therefore can only be

> safely

> should "RECOMMENDED" (aka SHOULD) be used instead?

[Les:] Done

> 199       [RFC5305] introduced sub-TLVs, which are TLV tuples advertised

> within

> 201       associated with the TLV Codepoints Registry and specify in which

> TLVs

> it Normative.

[Les:] Done

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

[Les:] This is a direct quote from RFC6232.

Sorry, you don’t get to edit it. 😊

> rfc6232 to correct that error.

[Les:] Done

> 224       are defined in the document(s) which introduce each

> codepoint.  The

> 225       definition SHOULD include what action to take when the

> format/content

> 227       ignored on receipt").  When making use of the information

> encoded in

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

[Les:] Done

> me.  IOW, why is "MUST" not used?

[Les:] Done

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

[Les:] Done

> 254       IANA is also requested to modify the entry for the POI TLV in the

> TLV

> I know POI was expanded before, but let's be explicit in the IANA

> instructions.

[Les:] Done

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

[Les:] Done

> 264       The clarifications discussed in this document are intended to make

> it

> [nit] s/implementaion/implementation

[Les:] Done

   Les

---

From: Alvaro Retana <[email-masked]>
Date: Mon, 22 Jun 2020 15:05:14 -0700

On June 19, 2020 at 11:49:51 AM, Les Ginsberg wrote:

Les:

Hi!

Just a couple of in-line answers.

We should be ready to move forward once you submit the update.

Thanks!!

Alvaro.

...
> [Les:] Just to be sure I understand, you are documenting that we need do

Correct.

...
> [Les:] 3.2 updates RFC 5304 to the extent that it recommends controls for
> behaviors that might not be backwards compatible. (Similar for RFC 6233)
>
> But it is a fair question to ask if RFC 5304 should be added to the list of
> documents updated? It currently is NOT listed - which makes me wonder why
> we include RFC 6233 in the list of updated documents.
>

I agree that it is not necessary to formally Update either.

...
> [Les:] It seems to me that Section 3.1 is very clear on this point. I
> prefer not to change/add text as you have suggested.
>

As long as it is clear to other reviewers that invalid, disallowed and
unknown have the same behavior...we can go ahead with no changes.

...
> [Les:] It isn’t clear to me that we have updated RFC3563 - unless you
> presume that anything that alters the registries in any way is considered
> an update to RFC 3563.
>
> The description of the columns is present in this document to provide
> context for the rest of the document. I don’t believe this description
> represents any change to the registry.
>

Yes, I agree.

BTW, this request..

   "IANA is requested to update the TLV Codepoints Registry to reference
   this document."

...is to add a reference to this document, and not to remove the others, right?

If so, then maybe "IANA is requested to add this document as a
reference for the TLV Codepoints Registry." might be better.

...
> Sorry, you don’t get to edit it. 😊

Not editing the quote...just the extra space after the period. ;-)

---

From: "Les Ginsberg (ginsberg)" <[email-masked]>
Date: Mon, 22 Jun 2020 22:45:46 +0000

Alvaro -

Thanx.
V2 of the draft has been posted. I believe it addresses all of your comments.

   Les

> Sent: Monday, June 22, 2020 3:05 PM
> To: Les Ginsberg (ginsberg) <[email-masked]>; draft-ietf-lsr-isis-invalid-
> [email-masked]
> Cc: [email-masked]; [email-masked]; Christian Hopps <[email-masked]>
> Subject: RE: AD Review of draft-ietf-lsr-isis-invalid-tlv-01
> 
> > If you agree, should we then remove RFC 3563 from the list of RFCs
> updated
> > by this document??
> 
> 
> Yes, I agree.
> 
> BTW, this request..
>