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 _______________________________________________ Lsr mailing list Lsr@ietf.org https://www.ietf.org/mailman/listinfo/lsr
