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

["Les Ginsberg (ginsberg)" <[e-mail]>]

Alvaro -



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

Responses inline.



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

> From: Alvaro Retana <[e-mail]>

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

> To: [e-mail]

> Cc: Christian Hopps <[e-mail]>; [e-mail]; [e-mail]

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

>

> Dear authors:

>

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



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



>

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

>



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



>

> 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



[Les:] Done



>

>

> ...

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



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

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



[Les:] Done



>

> Including that reference makes the first sentence in the next

> paragraph unnecessary.



[Les:] Removed.



>

>

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



>

>

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



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



>

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



[Les:] Done



>

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

>



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



>

> 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



[Les:] Done.

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



>

>

> ...

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



[Les:] Done



>

> [major] Please indicate somewhere in this section that the update to

> 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

> ...

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



[Les:] Done



>

>

> ...

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



[Les:] Done



>

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

> ignored/are ignored



[Les:] Done



>

>

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



[Les:] Done



>

>

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



[Les:] Done



>

>

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



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

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



>

>

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



[Les:] Done



>

>

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



[Les:] Done



>

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



[Les:] Done



>

>

> ...

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



[Les:] Done



>

>

> ...

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



[Les:] Done



>

>

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

>

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



[Les:] Done



>

>

> 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



[Les:] Done



   Les