Return-Path: X-Original-To: idr@ietfa.amsl.com Delivered-To: idr@ietfa.amsl.com Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 5C6F3120923; Mon, 4 Nov 2019 11:24:01 -0800 (PST) X-Virus-Scanned: amavisd-new at amsl.com X-Spam-Flag: NO X-Spam-Score: -1.899 X-Spam-Level: X-Spam-Status: No, score=-1.899 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id EePpiYUR8HDm; Mon, 4 Nov 2019 11:23:47 -0800 (PST) Received: from mail.hated.at (mail.hated.at [IPv6:2001:858:2:8::235]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 49EDC120897; Mon, 4 Nov 2019 11:23:42 -0800 (PST) Received: from 80-110-104-164.cgn.dynamic.surfer.at ([80.110.104.164] helo=[192.168.66.207]) by mail.hated.at with esmtpsa (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.69) (envelope-from ) id 1iRhnx-0007re-3H; Mon, 04 Nov 2019 20:14:25 +0100 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) From: Christoph Loibl In-Reply-To: Date: Mon, 4 Nov 2019 20:23:34 +0100 Cc: draft-ietf-idr-rfc5575bis@ietf.org, "idr@ietf. org" , idr-chairs@ietf.org Content-Transfer-Encoding: quoted-printable Message-Id: References: To: Alvaro Retana X-Mailer: Apple Mail (2.3445.104.11) Archived-At: Subject: Re: [Idr] AD Review of draft-ietf-idr-rfc5575bis-17 -> Updated Version -18 and Flowspec v6 X-BeenThere: idr@ietf.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Inter-Domain Routing List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Nov 2019 19:24:07 -0000 Hi Alvaro and IDR, Alvaro, thank you very much for your detailed review and comments! On behalf of the draft-ietf-idr-rfc5575bis authors I uploaded a new = version (-18) of the draft: https://datatracker.ietf.org/doc/draft-ietf-idr-rfc5575bis/ This is the result of the improvements that we worked on after your = review. Some of the structural changes lead to big changes if you run = the -17/-18 thru rfcdiff. However, we documented all the changes and the = entire process on Github. All issues and the related changes can be = reviewed there (hoping to move it to an IETF/IDR Org. account soon): https://github.com/stoffi92/rfc5575bis=20 Additionally, please see the below comments that we added to your review = text (for documentation on the IDR list). For all issues you raised we = added links to the issues tracked on Github. For the major issues we = also added a comment how those have been resolved. We think that the = document has very much improved and benefited from your review and the = process that followed. Because of the discussion about the Flow Specification v6 (IDR concluded = that the v6 draft should *not* be merged into rfc5575bis - as pointed = out previously) Sue, Robert and me also spent some time to update the = Flow Specification v6 draft and align it with the current = draft-ietf-idr-rfc5575bis: https://datatracker.ietf.org/doc/draft-ietf-idr-flow-spec-v6/ I encourage the WG to review the resulting document (-10), we want to = proceed if there are no objections. I also ask implementors of the v6 = draft (we know that there are quite a few) to contribute to the = implementation report for draft-ietf-idr-flow-spec-v6 on the IDR wiki: = https://trac.ietf.org/trac/idr/wiki/draft-ietf-idr-flow-spec-v6%20implemen= tations Cheers Christoph [Line numbers from idnits.] ... 17 Abstract [nit] It is interesting to me that the Abstract was significantly = rewritten while the Introduction was mostly left unchanged. I assume = this was done to reflect the changes in the document upfront...but it = then results in, what I think, is an Abstract that is too long, and an = incomplete Introduction. <-- *** Authors *** Tracked via issue #1: https://github.com/stoffi92/rfc5575bis/issues/1 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/117 Abstract and Introduction have been updated in = https://github.com/stoffi92/rfc5575bis/issues/117 --> 19 This document defines a Border Gateway Protocol Network Layer 20 Reachability Information (BGP NLRI) encoding format that can = be used 21 to distribute traffic Flow Specifications. This allows the = routing 22 system to propagate information regarding more specific = components of 23 the traffic aggregate defined by an IP destination prefix. 25 It specifies IPv4 traffic Flow Specifications via a BGP NLRI = which 26 carries traffic Flow Specification filter, and an Extended = community 27 value which encodes actions a routing system can take if the = packet 28 matches the traffic flow filters. The flow filters and the = actions 29 are processed in a fixed order. Other drafts specify IPv6, = MPLS 30 addresses, L2VPN addresses, and NV03 encapsulation of IP = addresses. [nit] s/carries traffic Flow Specification filter/carries a traffic Flow = Specification filter [minor] I think that this paragraph, or something like it, belongs in = the Introduction (and not the Abstract), because it provides information = that could benefit from references: - the two parts of the NLRI; BTW, the community is not even mentioned in = the Introduction. - other drafts... The Introduction only mentions and provides a = reference to the IPv6 work. <-- *** Authors *** Tracked via issue #2: https://github.com/stoffi92/rfc5575bis/issues/2 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/117 Abstract and Introduction have been updated in #117 --> 32 This document obsoletes RFC5575 and RFC7674 to correct unclear 33 specifications in the flow filters. [major] Please add a similar statement in the Introduction, with = references to both RFCs. There should be an Informative reference to = both. [minor] Appendix A talks about the difference of this document with = respect to rfc5575. What about rfc7674? It looks like any updates from = rfc7674 have been incorporated in this document. It would be very nice, = even if just for completion, if there was an Appendix that talked about = rfc7674 -- I even think that a sub-section of Appendix A would be = enough. <-- *** Authors *** Tracked via issue #3: https://github.com/stoffi92/rfc5575bis/issues/3 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/8a4098ac8153d55d2a277bc26879= 5969696214d0 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/117 Abstract and Introduction have been updated in #117 Both now mention the obsoleted RFCs=20 --> 35 Applications which use the bgp Flow Specification are: 1) = application 36 which automate inter-domain coordination of traffic filtering, = such 37 as what is required in order to mitigate (distributed) = denial-of- 38 service attacks; 2) applications which control traffic = filtering in 39 the context of a BGP/MPLS VPN service, and 3) applications = with 40 centralized control of traffic in a SDN or NFV context. Some 41 deployments of these three applications can be handled by the = strict 42 ordering of the BGP NLRI traffic flow filters, and the strict = actions 43 encoded in the extended community Flow Specification actions. [minor] Please move this paragraph to the Introduction. [nit] s/extended community/Extended Community/g <-- *** Authors *** Tracked via issue #4: https://github.com/stoffi92/rfc5575bis/issues/4 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/117 Abstract and Introduction have been updated in #117 The mentioned paragraph has been shortened but not completely removed. --> ... 133 1. Introduction ... 149 This document defines a general procedure to encode flow 150 specification rules for aggregated traffic flows so that they = can be 151 distributed as a BGP [RFC4271] NLRI. Additionally, we define = the 152 required mechanisms to utilize this definition to the problem = of 153 immediate concern to the authors: intra- and inter-provider 154 distribution of traffic filtering rules to filter = (distributed) 155 denial-of-service (DoS) attacks. [minor] The document uses "Flow Specification" and "flow specification" = to refer to the same thing...right? Or are there differences due to the = capitalization? Please be consistent. [style nit] Using "we" is not the best for a consensus document. s/we = define/it defines [nit] "problem of immediate concern to the authors" Only the authors? = This piece of text was also present in rfc5575 -- having a different set = of authors, I would assume we can safely say that the = concern/application goes beyond the authors...right? Please reword. [minor] Given that this is a bis, is the motivation still the same? I = think in part it is, but in part there may be other drivers. Just = asking... [minor] This seems to be a good place to move the text from the Abstract = that describes applications... <-- *** Authors *** Tracked via issue #5: https://github.com/stoffi92/rfc5575bis/issues/5 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/117 Abstract and Introduction have been updated in #117 --> ... 164 A Flow Specification received from an external autonomous = system will 165 need to be validated against unicast routing before being = accepted. 166 If the aggregate traffic flow defined by the unicast = destination 167 prefix is forwarded to a given BGP peer, then the local system = can 168 install more specific flow rules that may result in different 169 forwarding behavior, as requested by this system. [major] "A Flow Specification received from an external autonomous = system will need to be validated against unicast routing before being = accepted." What about if received internally? <-- *** Authors *** Tracked via issue #6: https://github.com/stoffi92/rfc5575bis/issues/6 Commits mentions: = https://github.com/stoffi92/rfc5575bis/commit/f380d82028084a17b800e3b34f2f= 8d6b51f8dbcf = https://github.com/stoffi92/rfc5575bis/commit/ef68aec39a87c04aa729c4793f57= a6c87adccbe2 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/117 Abstract and Introduction have been updated in #117. The flow specification received from an internal BGP peer within the = same autonomous system (per RFC4271) is assumed to have been validated = prior to transmission within the iBGP mesh of an autonomous system. --> 171 The key technology components required to address the class of 172 problems targeted by this document are: 174 1. Efficient point-to-multipoint distribution of control = plane 175 information. 177 2. Inter-domain capabilities and routing policy support. 179 3. Tight integration with unicast routing, for verification 180 purposes. 182 Items 1 and 2 have already been addressed using BGP for other = types 183 of control plane information. Close integration with BGP also = makes 184 it feasible to specify a mechanism to automatically verify = flow 185 information against unicast routing. These factors are behind = the 186 choice of BGP as the carrier of Flow Specification = information. [nit] I don't think that we need to keep justifying... Just a nit... <-- *** Authors *** Tracked via issue #7: https://github.com/stoffi92/rfc5575bis/issues/7 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/fa7a74fd1d61cbaee7ef4f68c920= 6ac972f7b9d0 --> 188 As with previous extensions to BGP, this specification makes = it 189 possible to add additional information to Internet routers. = These 190 are limited in terms of the maximum number of data elements = they can 191 hold as well as the number of events they are able to process = in a 192 given unit of time. The authors believe that, as with = previous 193 extensions, service providers will be careful to keep = information 194 levels below the maximum capacity of their devices. 196 Experience with previous BGP extensions has also shown that = the 197 maximum capacity of BGP speakers has been gradually increased 198 according to expected loads. For example Internet unicast = routing as 199 well as other BGP applications increased their maximum = capacity as 200 they gain popularity. [minor] This is the same text from 10 years ago. Many things, including = hardware processing/storage, has changed. Is this text still necessary? = If so, then I would like to see explicit operational considerations on = what an operator should look for when being "careful". <-- *** Authors *** Tracked via issue #8: https://github.com/stoffi92/rfc5575bis/issues/8 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/bd4c8b7e1ee1b05a4729adc481de= d3aa6f0a9fca Finally this has been moved to the security considerations. --> ... 214 In current deployments, the information distributed by the = flow-spec 215 extension is originated both manually as well as = automatically. The 216 latter by systems that are able to detect malicious flows. = When 217 automated systems are used, care should be taken to ensure = their 218 correctness as well as to limit the number and advertisement = rate of 219 flow routes. [major] An automated system that is not "correct", because it may not be = properly programmed, the algorithms used are not performing as expected, = or simply because it is rogue, are all vulnerabilities that should be = called out in the Security Considerations section. <-- *** Authors *** Tracked via issue #9: https://github.com/stoffi92/rfc5575bis/issues/9 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/17634fafd1bcd3607966ce507649= 13d07e07f152 Has been added to security considerations. --> 221 This specification defines required protocol extensions to = address 222 most common applications of IPv4 unicast and VPNv4 unicast = filtering. 223 The same mechanism can be reused and new match criteria added = to 224 address similar filtering needs for other BGP address families = such 225 as IPv6 families [I-D.ietf-idr-flow-spec-v6], [nit] s/[I-D.ietf-idr-flow-spec-v6],/[I-D.ietf-idr-flow-spec-v6]. <-- *** Authors *** Tracked via issue #10: https://github.com/stoffi92/rfc5575bis/issues/10 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/f35e22ea6f94340a7fb24b96ec64= 278d8ec7ed82 --> 227 2. Definitions of Terms Used in This Memo ... 233 Loc-RIB - Local RIB. [major] This simple definition doesn't match the one in =C2=A71.1/rfc4271.= <-- *** Authors *** Tracked via issue #11: https://github.com/stoffi92/rfc5575bis/issues/11 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/b4950ef76c347a67a8e31e0eff4d= 1433a00e34b8 Added the definition to to match RFC4271 --> .. 247 3. Flow Specifications ... 266 BGP itself treats the NLRI as an key to an entry in its = databases. 267 Entries that are placed in the Loc-RIB are then associated = with a 268 given set of semantics, which is application dependent. This = is 269 consistent with existing BGP applications. For instance, IP = unicast 270 routing (AFI=3D1, SAFI=3D1) and IP multicast reverse-path = information 271 (AFI=3D1, SAFI=3D2) are handled by BGP without any particular = semantics 272 being associated with them until installed in the Loc-RIB. [nit] s/an key/a key <-- *** Authors *** Tracked via issue #12: https://github.com/stoffi92/rfc5575bis/issues/12 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/8bcf213fccebec306606500f2292= 07d495f4fbdf --> 274 Standard BGP policy mechanisms, such as UPDATE filtering by = NLRI 275 prefix as well as community matching and manipulation, MUST = apply to 276 the Flow Specification defined NLRI-type, especially in an = inter- 277 domain environment. Network operators can also control = propagation 278 of such routing updates by enabling or disabling the exchange = of a 279 particular (AFI, SAFI) pair on a given BGP peering session. [major] The point of NLRIs all being treated the same is made above, to = reinforce the default BGP behavior...and this paragraph tries to bring = home the point by Normatively enforcing it (MUST). However, because the = behavior is what BGP specifies by default, then this document cannot be = Normative in it (unless it specified an exception). s/MUST/must <-- *** Authors *** Tracked via issue #13: https://github.com/stoffi92/rfc5575bis/issues/13 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/d94c3588a4c232bd217ce554393c= 1a2def0eb190 solved as suggested --> 281 4. Dissemination of IPv4 FLow Specification Information ... 287 This NLRI information is encoded using MP_REACH_NLRI and 288 MP_UNREACH_NLRI attributes as defined in [RFC4760]. Whenever = the 289 corresponding application does not require Next-Hop = information, this 290 shall be encoded as a 0-octet length Next Hop in the = MP_REACH_NLRI 291 attribute and ignored on receipt. [minor] s/Next-Hop/Next Hop rfc4760 uses "Next Hop" [nit] "...shall be encoded as a 0-octet length Next Hop in the = MP_REACH_NLRI attribute and ignored on receipt." What is ignored? The = Next Hop? If it doesn't exist (length =3D 0), then it can't be = ignored... Perhaps delete " and ignored on receipt". <-- *** Authors *** Tracked via issue #14: https://github.com/stoffi92/rfc5575bis/issues/14 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/a89200d70c7e661c519bf4379ef4= 3fee817f1d71 --> ... 297 +------------------------------+ 298 | length (0xnn or 0xfn nn) | 299 +------------------------------+ 300 | NLRI value (variable) | 301 +------------------------------+ [minor] s/0xfn nn/0xfnnn <-- *** Authors *** Tracked via issue #15: https://github.com/stoffi92/rfc5575bis/issues/15 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/2fe26e058f1428a739e154ebfef7= bdbea647f894 --> ... 312 4.1. Length Encoding 314 o If the NLRI length value is smaller than 240 (0xf0 hex), = the 315 length field can be encoded as a single octet. [nit] s/240/240 octets <-- *** Authors *** Tracked via issue #16: https://github.com/stoffi92/rfc5575bis/issues/16 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/a86083b7217af1e3b7670ebbdad8= e9632bd72911 --> 317 o Otherwise, it is encoded as an extended-length 2-octet = value in 318 which the most significant nibble of the first byte is all = ones. 320 In figure 1 above, values less-than 240 are encoded using two = hex 321 digits (0xnn). Values above 239 are encoded using 3 hex = digits 322 (0xfnnn). The highest value that can be represented with this 323 encoding is 4095. The value 241 is encoded as 0xf0f1. [nit] It may make more sense to show the encoding for 240. <-- *** Authors *** Tracked via issue #17: https://github.com/stoffi92/rfc5575bis/issues/17 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/b62e4c85de2a731855cf9e0e0ed4= a1157075171a --> 325 4.2. NLRI Value Encoding ... 332 The encoding of each of the NLRI components begins with a type = field 333 (1 octet) followed by a variable length parameter. Section = 4.2.1 to 334 Section 4.2.12 define component types and parameter encodings = for the 335 IPv4 IP layer and transport layer headers. IPv6 NLRI = component types 336 are described in [I-D.ietf-idr-flow-spec-v6]. [minor] "followed by a variable length parameter" Only the first two = types have a variable length parameter... <-- *** Authors *** Tracked via issue #18: https://github.com/stoffi92/rfc5575bis/issues/18 --> 338 Flow Specification components must follow strict type ordering = by 339 increasing numerical order. A given component type may = (exactly 340 once) or may not be present in the specification. If present, = it 341 MUST precede any component of higher numeric type value. [major] What should happen if a component appears more than once? [major] What should happen if the order is not maintained? <-- *** Authors *** Tracked via issue #19: https://github.com/stoffi92/rfc5575bis/issues/19 Basically if the NLRI is not encoded to specification we have a = malformed NLRI (most probably the BGP NLRI parser will throw an = exception on the implementation). I am not sure if this needs to be made = explicit. - But I can make this explicit. --> 343 All combinations of component types within a single NLRI are = allowed, 344 even if the combination makes no sense from a semantical = perspective. 345 If a given component type within a prefix in unknown, the = prefix in 346 question cannot be used for traffic filtering purposes by the 347 receiver. Since a Flow Specification has the semantics of a = logical 348 AND of all components, if a component is FALSE, by definition = it 349 cannot be applied. However, for the purposes of BGP route 350 propagation, this prefix should still be transmitted since BGP = route 351 distribution is independent on NLRI semantics. [nit] s/prefix in unknown/prefix is unknown [nit] s/independent on NLRI/independent of NLRI [major] "...for the purposes of BGP route propagation, this prefix = should still be transmitted since BGP route distribution is independent = on NLRI semantics." I think this is a vulnerability: a (large) set of = meaningless Flow Specifications may be injected in the routing system... [major] Also, propagating these unknown components may result in a = router down the line, which understands them, reacting. While the = reaction shouldn't result in reset adjacencies, it may result in = inconsistent forwarding or other unexpected outcomes... [major] This treatment of unknown extensions is in conflict with the = text in =C2=A711. See my comments there. <-- *** Authors *** Tracked via issue #20: https://github.com/stoffi92/rfc5575bis/issues/20 Related issues mentioned: https://github.com/stoffi92/rfc5575bis/issues/20 https://github.com/stoffi92/rfc5575bis/issues/93 Only filters that actually can match packets should be propagated, = unknown components will not be propagated see the updated paragraph = about FS extensions. --> 353 4.2.1. Type 1 - Destination Prefix 355 Encoding: 357 Defines: the destination prefix to match. Prefixes are = encoded as 358 in BGP UPDATE messages, a length in bits is followed by = enough 359 octets to contain the prefix information. [nit] s/Defines: the destination prefix/Defines the destination prefix [major] rfc4271: "The Prefix field contains an IP address prefix, = followed by the minimum number of trailing bits needed to make the end = of the field fall on an octet boundary." The text above makes it sound = as if the prefix field may not end in an octet boundary, which is what = rfc4271 specifies. NEW (suggestion)> Defines the destination prefix to match. The length and prefix fields = are encoded as in BGP UPDATE messages [rfc4271]. <-- *** Authors *** Tracked via issue #21: https://github.com/stoffi92/rfc5575bis/issues/21 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/f8fe075c0a1bf3d9ceab95152aa2= 911f70f7fd31 solved as suggested. --> 361 4.2.2. Type 2 - Source Prefix 363 Encoding: 365 Defines the source prefix to match. [minor] "... The length and prefix fields are encoded as in BGP UPDATE = messages [rfc4271]." <-- *** Authors *** Tracked via issue #22: https://github.com/stoffi92/rfc5575bis/issues/22 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/f8fe075c0a1bf3d9ceab95152aa2= 911f70f7fd31 --> 367 4.2.3. Type 3 - IP Protocol 369 Encoding: 371 Contains a set of {operator, value} pairs that are used to = match 372 the IP protocol value byte in IP packets. [minor] Include a reference to the protocol numbers.. [major] Are all protocol numbers valid? I guess that in theory anything = is -- what should a receiver do with Flow Specifications that cover = protocols that are not supported? I'm wondering if sending Flow = Specifications for every protocol under the sun is a vulnerability -- = knowing that only a few will ever be present in the Internet. Is there = any guidance that you can provide in =C2=A714 (or a separate Operational = Considerations section)? I also point this out because the rest of the = types focus on TCP/UDP...what about other transport layer protocols? [major] Related question: even for "valid" protocols, should all be = accepted from eBGP peers? I think that it is probably ok...asking for = completeness. <-- *** Authors *** Tracked via issue #23: https://github.com/stoffi92/rfc5575bis/issues/23 *all* protocol numbers are valid even from eBGP peers. There is no value restricting FS to a set of predefined protocol = numbers. It is simply a match against the IP protocol octet in the IP = header. We see packets flowing with all kinds of odd packets used in = DDoS attacks (including some odd protocol numbers). BGP update filtering based on components may be implemented (as it is = today for ipv4 prefixes) by vendors. There are big frameworks build = around import/export policies for other NLRIs already.=20 FS extensions may add new components matching on different properties of = the packet. This draft does not introduce any changed components as = requested by the WG to RFC5575 --> 374 The operator byte is encoded as: 376 0 1 2 3 4 5 6 7 377 +---+---+---+---+---+---+---+---+ 378 | e | a | len | 0 |lt |gt |eq | 379 +---+---+---+---+---+---+---+---+ 381 Numeric operator [nit] Center the figure... [clarity] Please describe the operators independent of one of the Types. = As defined, it looks like they only apply to one type...it is much = later that the reader realizes that there is a reason for the = "complexity". Along the same lines, I think that the "set of {operator, = value} pairs" phrase could use some more text to explain that the = operator is the whole octet, with a corresponding value... <-- *** Authors *** Tracked via issue #24: https://github.com/stoffi92/rfc5575bis/issues/24 --> 383 e - end-of-list bit. Set in the last {op, value} pair in = the 384 list. [major] What action should be taken if a received flow spec has this bit = not set anywhere, or is set somewhere other than the last pair? <-- *** Authors *** Tracked via issue #25: https://github.com/stoffi92/rfc5575bis/issues/25 If this is the case the parser will not be able to parse the FS NLRI and = will abort. This is where Error Handling kicks in -> malformed update --> 386 a - AND bit. If unset, the previous term is logically ORed = with 387 the current one. If set, the operation is a logical AND. = In the 388 first operator byte of a sequence it SHOULD be encoded as = unset 389 and and MUST be treated as always unset on decoding. The = AND 390 operator has higher priority than OR for the purposes of 391 evaluating logical expressions. 393 len - length of the value field for this operator given as = (1 << 394 len). This encodes 1 (00) - 8 (11) bytes. Type 3 flow = component 395 values SHOULD be encoded as single byte (len =3D 00). [major] Please expand on the meaning of "1 << len". <-- *** Authors *** Tracked via issue #26: https://github.com/stoffi92/rfc5575bis/issues/26 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/30613b6c8bdf0f25734c825d96e0= e21a08c2fa3f Explicitly expanding the bit-shift operator in the doc. --> ... 406 The bits lt, gt, and eq can be combined to produce common = relational 407 operators such as "less or equal", "greater or equal", and = "not equal 408 to". [minor] "...as shown in Table 1." <-- *** Authors *** Tracked via issue #27: https://github.com/stoffi92/rfc5575bis/issues/27 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/1a0f5300a4e80d7dc7d49b3fa03b= 511a1032ffa0 --> 410 +----+----+----+----------------------------------+ 411 | lt | gt | eq | Resulting operation | 412 +----+----+----+----------------------------------+ 413 | 0 | 0 | 0 | false (independent of the value) | 414 | 0 | 0 | 1 | =3D=3D (equal) = | 415 | 0 | 1 | 0 | > (greater than) | 416 | 0 | 1 | 1 | >=3D (greater than or equal) | 417 | 1 | 0 | 0 | < (less than) | 418 | 1 | 0 | 1 | <=3D (less than or equal) | 419 | 1 | 1 | 0 | !=3D (not equal value) | 420 | 1 | 1 | 1 | true (independent of the value) | 421 +----+----+----+----------------------------------+ 423 Table 1: Comparison operation combinations 425 4.2.4. Type 4 - Port 427 Encoding: 429 Defines a list of {operator, value} pairs that matches = source OR 430 destination TCP/UDP ports. This list is encoded using the = numeric 431 operator format defined in Section 4.2.3. Values SHOULD be 432 encoded as 1- or 2-byte quantities. [minor] A reference to TCP/UDP header/ports would be nice. [major] "matches source OR destination TCP/UDP ports" Which one? Both? = Either? How does the receiver know which one? [minor] What is the interaction/relationship between this type and Types = 5 and 6? The text in =C2=A74.2 allows for all 3 types to be present, = and have an influence in the action taken...they seem redundant. <-- *** Authors *** Tracked via issue #28: https://github.com/stoffi92/rfc5575bis/issues/28 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/c5b7f37ef7f2a68531de7f8ff088= 638b51739064 ad-major1: It matches src-port OR destination-port as logical "OR", for this = component to result in "true" either the src OR the destination port = must match the value.=20 In contrary to the dst-port (Type 5) or src-port (Type 6) component = which matches only against dst-port or src-port exclusively. ad-minor2: [minor] What is the interaction/relationship between this type and Types = 5 and 6? The text in =C2=A74.2 allows for all 3 types to be present, and = have an influence in the action taken...they seem redundant. See above. Using all 3 at the same time may seem redundant. However = _all_ components of a flowspec must result in True for a flow to match = (logical AND between components). Example 1: Type 4 port=3D=3D8080 Type 5 dst-port=3D=3D80 Type 6 src-port=3D=3D443 -> will not match any flow because there is no flow with=20 Expression: (dst-port=3D=3D8080 OR src-port=3D=3D8080) AND dst-port=3D=3D8= 0 AND src-port=3D=3D443 Example 2: Type 4 port=3D=3D8080 Type 5 dst-port=3D=3D80 -> matches flow dst-port 80 and src-port 8080 Expression: (dst-port=3D=3D8080 OR src-port=3D=3D8080) AND dst-port=3D=3D8= 0 Example 3: Type 4 port=3D=3D8080 -> matches flow dst-port=3D8080 src-port=3D* -> matches flow dst-port any src-port=3D8080 Expression: (dst-port=3D=3D8080 OR src-port=3D=3D8080) Example 4: Type 6 src-port=3D=3D80 -> matches flow src-port=3D80 dst-port=3D* Expression: src-port=3D=3D80 --> 434 Port, source port, and destination port components evaluate = to 435 FALSE if the IP protocol field of the packet has a value = other 436 than TCP or UDP, if the packet is fragmented and this is = not the 437 first fragment, or if the system in unable to locate the = transport 438 header. Different implementations may or may not be able = to 439 decode the transport header in the presence of IP options = or 440 Encapsulating Security Payload (ESP) NULL [RFC4303] = encryption. [minor] "Port, source port, and destination port components..." This = section only talks about the port; please duplicate this text in the = other sections, or put a reference to it there, or put a forward = reference here... [major] "...evaluate to FALSE if the IP protocol field of the packet has = a value other than TCP or UDP, if the packet is fragmented and this is = not the first fragment, or if the system in unable to locate the = transport header." This sentence seems to mix the applicability of the = Flow Specification (FALSE is first introduced in =C2=A74.2 to describe = the effect of a component on the rule), and the application to a = specific packet. Please separate the two aspects. I do have some = specific questions/comments. (1) The text starts by talking about the "protocol field of the packet" = (not the protocol value in the Type 3 parameter)... I assume that a = Flow Specification would only apply to a packet if the protocol matches = the Type 3 parameter...but the statement seems to say that it wouldn't = apply regardless of the Type 3 (see my question there about valid = protocols)...or maybe even if a Type 3 is not present.... (2) "...evaluate to FALSE...if the packet is fragmented and this is not = the first fragment..." Type 12 specifically includes values for other = cases. How is the interaction expected? <-- *** Authors *** Tracked via issue #29: https://github.com/stoffi92/rfc5575bis/issues/29 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/73760db087e3f91d2e781c4e4537= 853f4de384f4 Made it clearer in the document, that this can only ever match if the = UDP/TCP header can be identified. Non TCP or UDP packets will never = match that component. Adding a match against the protocol number is not = needed because it is implicit. --> ... 460 4.2.7. Type 7 - ICMP type 462 Encoding: 464 Defines a list of {operator, value} pairs used to match the = type 465 field of an ICMP packet. This list is encoded using the = numeric 466 operator format defined in Section 4.2.3. Values SHOULD be 467 encoded using a single byte. [minor] A reference to ICMP would be nice. <-- *** Authors *** Tracked via issue #30: https://github.com/stoffi92/rfc5575bis/issues/30 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/a4ca42e137974ba03fed58b50b17= 1e05f7b2eb18 --> 469 The ICMP type specifiers evaluate to FALSE whenever the = protocol 470 value is not ICMP. 472 4.2.8. Type 8 - ICMP code 474 Encoding: 476 Defines a list of {operator, value} pairs used to match the = code 477 field of an ICMP packet. This list is encoded using the = numeric 478 operator format defined in Section 4.2.3. Values SHOULD be 479 encoded using a single byte. 481 The ICMP code specifiers evaluate to FALSE whenever the = protocol 482 value is not ICMP. [minor] I guess that it should also evaluate FALSE if the ICMP code is = not relevant for the Type. ?? <-- *** Authors *** Tracked via issue #31: https://github.com/stoffi92/rfc5575bis/issues/31 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/34 --> 484 4.2.9. Type 9 - TCP flags 486 Encoding: [minor] The operator (described below) is called "bitmask", which is a = little confusing with the bitmask itself... <-- *** Authors *** Tracked via issue #32: https://github.com/stoffi92/rfc5575bis/issues/32 --> 488 Bitmask values can be encoded as a 1- or 2-byte bitmask. = When a 489 single byte is specified, it matches byte 13 of the TCP = header 490 [RFC0793], which contains bits 8 though 15 of the 4th = 32-bit word. 491 When a 2-byte encoding is used, it matches bytes 12 and 13 = of the 492 TCP header with the data offset field having a "don't care" = value. [minor] Identifying the right octets is more important than counting the = number of bytes... The interesting bytes are identified above as "bytes = 12 and 13"; however, work from the Transport Area talks about "bytes 13 = and 14": https://tools.ietf.org/html/rfc3168#section-6.1 It would be = nice if this was aligned or if any ambiguity could be avoided. [minor] "...with the data offset field having a "don't care" value." = What does that mean? To me, it sounds as if the bitmask values can't be = used to match on the offset....is that the right interpretation? Some = clarity would avoid guessing.. <-- *** Authors *** Tracked via issue #33: https://github.com/stoffi92/rfc5575bis/issues/33 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/f53ce5dfe0185b292b4e95033df0= f332b9d72100 --> 494 This component evaluates to FALSE for packets that are not = TCP 495 packets. [major] As mentioned before, this sentence also seems to mix/confuse the = applicability of the component (whether it can be used at all) and the = application of it to match a specific packet. In this case, the text = seems to simply say that a Flow Specification which uses Type 9 can only = be used to match TCP packets... [major] Should the Flow Specification evaluate to FALSE if this Type is = used *and* Type 3 doesn't include TCP *only* in it's description? <-- *** Authors *** Tracked via issue #34: https://github.com/stoffi92/rfc5575bis/issues/34 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/eaa56214468d3a7326df46a55d9a= b7a291210c89 Same solution as with UDP/TCP ports --> 497 This type uses the bitmask operator format, which differs = from the 498 numeric operator format in the lower nibble. [minor] As with the numeric operator, I think it would be clearer if it = was introduced before the types. <-- *** Authors *** Tracked via issue #35: https://github.com/stoffi92/rfc5575bis/issues/35 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/115 --> 500 0 1 2 3 4 5 6 7 501 +---+---+---+---+---+---+---+---+ 502 | e | a | len | 0 | 0 |not| m | 503 +---+---+---+---+---+---+---+---+ 505 Bitmask operator [nit] Center the figure... <-- *** Authors *** Tracked via issue #36: https://github.com/stoffi92/rfc5575bis/issues/36 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/e72952a2842f7ce96aa049a3271e= ec7fc8ac3cf3 --> 507 e, a, len - Most significant nibble: (end-of-list bit, AND = bit, and 508 length field), as defined for in the numeric operator = format in 509 Section 4.2.3. [] See the questions about the e bit above. <-- *** Authors *** Tracked via issue #37: https://github.com/stoffi92/rfc5575bis/issues/37 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/25 --> ... 542 4.2.12. Type 12 - Fragment 544 Encoding: 546 Uses bitmask operator format defined in Section 4.2.9. [major] No, it doesn't. The new one is defined below. [clarity] Again, please introduce the operators before the types. In = this case, this operator seems to also carry the bitmask name, which can = be confusing with the one introduced in =C2=A74.2.9 and the name of the = value field... <-- *** Authors *** Tracked via issue #38: https://github.com/stoffi92/rfc5575bis/issues/38 In fact it uses the same operator (op) from 4.2.9 - but a different = bitmask. I think with the definition of the operators prior to the = components and proper references it is now clearer. --> 548 0 1 2 3 4 5 6 7 549 +---+---+---+---+---+---+---+---+ 550 | 0 | 0 | 0 | 0 |LF |FF |IsF|DF | 551 +---+---+---+---+---+---+---+---+ [nit] Center the figure... [nit] Please add Figure numbers. <-- *** Authors *** Tracked via issue #39: https://github.com/stoffi92/rfc5575bis/issues/39 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/bdd2e69858ea6a0b34f7ff4cd3bf= ffeed507535e Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/118 --> 553 Bitmask values: 555 Bit 7 - Don't fragment (DF) 557 Bit 6 - Is a fragment (IsF) 559 Bit 5 - First fragment (FF) 561 Bit 4 - Last fragment (LF) 563 Bit 0-3 - SHOULD be set to 0 on NLRI encoding, and MUST = be 564 ignored during decoding [major] The operation is not specified. Is this also an = (operator,bitmask) pair, or just 8 bits indicating the values? Can = multiple bits be set at the same time? What fields in the IP header do = these map to? <-- *** Authors *** Tracked via issue #40: https://github.com/stoffi92/rfc5575bis/issues/40 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/f078e919842330d670bc195276ea= d7bdc05a3351 Added the text to the bit-descriptions and IP-header fields - And an = example using the bitmask operator. --> 566 4.3. Examples of Encodings 568 An example of a Flow Specification encoding for: "all packets = to 569 10.0.1/24 and TCP port 25". [nit] For clarity, include the whole subnet: s/ 10.0.1/24 / 10.0.1.0/24 [major] Use IP addresses from the documentation pool [rfc5737] in all = examples. <-- *** Authors *** Tracked via issue #41: https://github.com/stoffi92/rfc5575bis/issues/41 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/574e2641bee448a1137eca011489= fa988e0e9a99 Changed the example accordingly (including example addresses) --> 571 +------------------+----------+----------+ 572 | destination | proto | port | 573 +------------------+----------+----------+ 574 | 0x01 18 0a 00 01 | 03 81 06 | 04 81 19 | 575 +------------------+----------+----------+ [minor] It would be nice if the examples show the the whole Flow-spec = NLRI, and not just the NLRI value. Also, it would be great if one of = the examples required more than 240 bytes. <-- *** Authors *** Tracked via issue #42: https://github.com/stoffi92/rfc5575bis/issues/42 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/9652cafb08a23d4d5997e60bbd9e= 6c20db569a96 --> 577 Decode for protocol: [minor] Please show the decodes for all the fields. <-- *** Authors *** Tracked via issue #43: https://github.com/stoffi92/rfc5575bis/issues/43 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/82a6bc647cdfbd53a6fa27c3def8= 9383bcb2d209 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/41 --> 579 +-------+----------+------------------------------+ 580 | Value | | | 581 +-------+----------+------------------------------+ 582 | 0x03 | type | | 583 | 0x81 | operator | end-of-list, value size=3D1, =3D | 584 | 0x06 | value | | 585 +-------+----------+------------------------------+ [minor] For completion, indicate that Protocol 6 is TCP. <-- *** Authors *** Tracked via issue #44: https://github.com/stoffi92/rfc5575bis/issues/44 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/ed33739428da09560b8bfc1c03cf= 53c404c261ee --> 587 An example of a Flow Specification encoding for: "all packets = to 588 10.1.1/24 from 192/8 and port {range [137, 139] or 8080}". [] Ah...NETBIOS... [nit] It might be a good idea to number the examples... <-- *** Authors *** Tracked via issue #45: https://github.com/stoffi92/rfc5575bis/issues/45 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/fc6328a224add74e0cb5d2048a70= 5dd99a17f562 --> ... 612 5. Traffic Filtering 614 Traffic filtering policies have been traditionally considered = to be 615 relatively static. Limitations of the static mechanisms = caused this 616 mechanism to be designed for the three new applications of = traffic 617 filtering (prevention of traffic-based, denial-of-service = (DOS) 618 attacks, traffic filtering in the context of BGP/MPLS VPN = service, 619 and centralized traffic control for SDN/NFV networks) requires 620 coordination among service providers and/or coordination among = the AS 621 within a service provider. Section 9 has details on the = limitation 622 of previous mechanisms and why BGP Flow Specification provides = a 623 solution for to prevent DOS and aid BGP/MPLS VPN filtering = rules. [minor] This sentence, without the parenthesis, doesn't seem to make = sense: "Limitations of the static mechanisms caused this mechanism to be = designed for the three new applications of traffic filtering requires = coordination among service providers and/or coordination among the AS = within a service provider." [nit] s/solution for to prevent/solution to prevent <-- *** Authors *** Tracked via issue #46: https://github.com/stoffi92/rfc5575bis/issues/46 Commits mentions: = https://github.com/stoffi92/rfc5575bis/commit/4ccfaa5fecbb85322dc2544d1b76= d68eb16d4133 = https://github.com/stoffi92/rfc5575bis/commit/e4a8c18242478cdd6a2b86a65332= 1cd204fe2f7c --> 625 This Flow Specification NLRI defined above to convey = information 626 about traffic filtering rules for traffic that should be = discarded or 627 handled in manner specified by a set of pre-defined actions = (which 628 are defined in BGP Extended Communities). This mechanism is 629 primarily designed to allow an upstream autonomous system to = perform 630 inbound filtering in their ingress routers of traffic that a = given 631 downstream AS wishes to drop. [nit] s/This Flow Specification NLRI/The Flow Specification NLRI <-- *** Authors *** Tracked via issue #47: https://github.com/stoffi92/rfc5575bis/issues/47 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/dfcb18cf034a82bea15707e8b914= 50b4ee65ee3e --> ... 645 Distribution of the IPv4 Flow Specification is described in 646 Section 6, and distibution of BGP/MPLS traffic Flow = Specification is 647 described in Section 8. The traffic filtering actions are = described 648 in Section 7. [minor] Section 6 talks about validation, not distribution. [nit] s/distibution/distribution <-- *** Authors *** Tracked via issue #48: https://github.com/stoffi92/rfc5575bis/issues/48 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/09c6c5c346c9595d2d4e76274f35= 1b9f1aafe0df --> 650 5.1. Ordering of Traffic Filtering Rules 652 With traffic filtering rules, more than one rule may match a 653 particular traffic flow. Thus, it is necessary to define the = order 654 at which rules get matched and applied to a particular traffic = flow. 655 This ordering function must be such that it must not depend on = the 656 arrival order of the Flow Specification's rules and must be 657 consistent in the network. [clarification] Are "traffic filtering rules" the same thing as "traffic = filtering actions", or are they more like "Flow Specification's rules"? = You also mention (below) "Flow Specification rules" in the context of = ordering, so my guess is that "traffic filtering rules" and "Flow = Specification rules" are equivalent...are they? In my opinion, there = are too many ways to refer to the same, or very similar things. Please = take advantage of =C2=A72 to help the reader, or at least simplify the = terminology. <-- *** Authors *** Tracked via issue #49: https://github.com/stoffi92/rfc5575bis/issues/49 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/53d904bc5a5458cc277e0f87608e= 69b28f5a768c --> 659 The relative order of two Flow Specification rules is = determined by 660 comparing their respective components. The algorithm starts = by 661 comparing the left-most components of the rules. If the types 662 differ, the rule with lowest numeric type value has higher = precedence 663 (and thus will match before) than the rule that doesn't = contain that 664 component type. If the component types are the same, then a = type- 665 specific comparison is performed (see below) if the types are = equal 666 the algorithm continues with the next component. [minor] To be clear: the comparison is done between the component types = defined in =C2=A74.2...and "left-most" means "first"... <-- *** Authors *** Tracked via issue #50: https://github.com/stoffi92/rfc5575bis/issues/50 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/620d5da6ff87d70dec5ffad1ba97= d0b0915a16ac --> 668 For IP prefix values (IP destination or source prefix): If the 669 prefixes overlap, the one with the longer prefix-length has = higher 670 precedence. If they do not overlap the one with the lowest IP = value 671 has higher precedence. [minor] I need you to be more specific when talking about "overlap". = Clearly 10.1..0.0/16 and 10.1.1.0/24 overlap, then the higher precedence = would be for the /24, right? Do 130.0.0.0/16 and 150..1.1.0/24 overlap = (they have the first 3 bits in common)? rfc5575 talks about a "common = prefix", which is not completely clear either, but it could mean at = least what is covered by the shortest mask (which would be my guess)... [minor] "prefix-length" is used here, but "prefix length" is used in = =C2=A74.2.1. Please be consistent. [minor] The "-" confused me a little. By "For IP prefix values...the = longer prefix-length" do you mean the value of the prefix length, or the = length of the prefix field? rfc5575 talks about "more specific", which = may be easier to understand in this case... <-- *** Authors *** Tracked via issue #51: https://github.com/stoffi92/rfc5575bis/issues/51 Commits mentions: = https://github.com/stoffi92/rfc5575bis/commit/182e01124f64c36878bab046bb7d= baafb77462d3 = https://github.com/stoffi92/rfc5575bis/commit/c6f3c54a8e5f1731bb8e1516fb96= 9d18cce16f34 = https://github.com/stoffi92/rfc5575bis/commit/a2fc9c257da7460184c20695cc13= dec6484bf73f --> 673 For all other component types, unless otherwise specified, the 674 comparison is performed by comparing the component data as a = binary 675 string using the memcmp() function as defined by the ISO C = standard. 676 For strings with equal lengths the lowest string (memcmp) has = higher 677 precedence. For strings of different lengths, the common = prefix is 678 compared. If the common prefix is not equal the string with = the 679 lowest prefix has higher precedence. If the common prefix is = equal, 680 the longest string is considered to have higher precedence = than the 681 shorter one. [major] Please add a Normative reference for "the memcmp() function as = defined by the ISO C standard". [minor] What is the "common prefix"? Is it the bits that correspond to = the shorter length? In this case I think that using "prefix" may be = confusing. [minor] If my interpretation is correct, given a common set of rules, = the longer the Flow Specification the most preferred, right? Using one = of the examples in =C2=A74.3, "all packets to 10.1.1/24 from 192/8 and = port {range [137, 139] or 8080}" would be preferred over "all packets to = 10.1.1/24 from 192/8 and port range [137, 139]"...because when comparing = the common prefix for the port, the second rule would have the e bit = set, resulting in a higher prefix, right? [major] I would like to see some discussion about the management of Flow = Specifications and their advertisement order from an operational point = of view. In the case above, if an operator uses the first rule (only), = but later decides to allow web traffic and the system advertises the = second rule, it won't take effect until the first one is withdrawn. = This type of operational consideration is not explained in this = document. <-- *** Authors *** Tracked via issue #52: https://github.com/stoffi92/rfc5575bis/issues/52 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/d6481bb62fccbb0cfa52610a3b24= acf96aefaa89 Related issue mentioned: https://github.com/stoffi92/rfc5575bis/issues/2 Ad Major1: Added a reference to ISO C Ad Minor1: I agree that prefix may confuse a little. However, since we decided to = use python code here we actually have something to run and do testing = (the pseudo-code was less clear). Ad Minor 2: Yes the e bit results in a higher prefix, and since the lower prefix = wins, the second rule (the one without port 8080) is also #2 after = sorting. Ad Major2: The point on the sorting algorithm is, that the order of the = announcement is irrelevant to the outcome of the filter. Whenever a new = filter is received the filters need to be sorted and may then be applied = to the traffic-flows. See =C2=A75.1 first paragraph. =20 --> 683 The code below shows a Python3 implementation of the = comparison 684 algorithm. The full code was tested with Python 3.6.3 and can = be 685 obtained at https://github.com/stoffi92/flowspec-cmp [1]. [minor] I would prefer to see the code in an Appendix. [major] We need to include template text about the licensing in the Code = Component below. Please take a look at the IETF Trust Legal Provisions = and add the appropriate text: = https://trustee.ietf.org/license-info/IETF-TLP-5.pdf <-- *** Authors *** Tracked via issue #53: https://github.com/stoffi92/rfc5575bis/issues/53 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/c2bdc4ea0f4da80c48f664a3d19d= c24210830b78 Moved the Python source to the Appendix and added a the licence in the = code component. --> 687 688 import itertools 689 import ipaddress 691 def flow_rule_cmp(a, b): 692 for comp_a, comp_b in itertools.zip_longest(a.components, 693 b.components): 694 # If a component type does not exist in one rule 695 # this rule has lower precedence 696 if not comp_a: 697 return B_HAS_PRECEDENCE 698 if not comp_b: 699 return A_HAS_PRECEDENCE [] What if the component is not in either? The lines above look like = the wrong outcome could be obtained. Disclaimer: I don't know Python... <-- *** Authors *** Tracked via issue #54: https://github.com/stoffi92/rfc5575bis/issues/54 --> ... 742 6. Validation Procedure ... 757 The concept can be extended, in the case of Flow Specification = NLRI, 758 to allow other validation procedures. [nit] s/of Flow Specification NLRI/of the Flow Specification NLRI <-- *** Authors *** Tracked via issue #55: https://github.com/stoffi92/rfc5575bis/issues/55 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/ec1988d1bfa2ff8c0dc76c9dfb33= 8bc217e7d43d --> 760 A Flow Specification NLRI must be validated such that it is 761 considered feasible if and only if all of the below is true: [major] There is no Normative language above, but I think there is a = contradiction of sorts with the new text below ("Rule a) MAY be = relaxed..."). The introductory text to the rules is "must = be...considered feasible if and only if all of the below is true", which = sounds very strict and specific...but then the Normative exception comes = in ("MAY be relaxed...rules b) and c)...MUST be disregarded") saying = that it doesn't matter. Please reword...perhaps something like: "If a = destination is present...a Flow Specification MUST be validated this = way...otherwise..." <-- *** Authors *** Tracked via issue #56: https://github.com/stoffi92/rfc5575bis/issues/56 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/d96a496295021f2aab0332bd2120= 920547f71c56 Changed the text to refer to the default behaviour of the implementation = (MUST validate). However this MAY be relaxed by configuration. --> 763 a) A destination prefix component is embedded in the Flow 764 Specification. 766 b) The originator of the Flow Specification matches the = originator 767 of the best-match unicast route for the destination prefix 768 embedded in the Flow Specification. [major] What is the "best-match unicast route"? Please be specific. <-- *** Authors *** Tracked via issue #57: https://github.com/stoffi92/rfc5575bis/issues/57 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/e81d7de9262c59caba7c07012fb3= 3de923816156 Added: " (this is the unicast route with the longest possible prefix length covering the destination prefix embedded in the Flow Specification)" --> 770 c) There are no more specific unicast routes, when compared = with 771 the flow destination prefix, that has been received from a 772 different neighboring AS than the best-match unicast route, = which 773 has been determined in rule b). 775 Rule a) MAY be relaxed by configuration, permitting Flow 776 Specifications that include no destination prefix component. = If such 777 is the case, rules b) and c) are moot and MUST be disregarded. [major] This action opens the door to all sorts of things. I note that = the Security Considerations section simply mentions it without going = into more details. <-- *** Authors *** Tracked via issue #58: https://github.com/stoffi92/rfc5575bis/issues/58 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/3883234a5a12e25d75f9c00f0bd1= 5b9d32535072 Referenced this issue in the security considerations section. --> 779 By originator of a BGP route, we mean either the BGP = originator path 780 attribute, as used by route reflection, or the transport = address of 781 the BGP peer, if this path attribute is not present. [major] s/BGP originator path attribute, as used by route = reflection/address of the originator in the ORIGINATOR_ID Attribute = [RFC4456] The reference to rfc4456 should be Normative. [minor] rfc4271 doesn't talk about a "transport addresses". Instead, it = talks about the "source IP address". I know it is the same thing, but = please be consistent. <-- *** Authors *** Tracked via issue #59: https://github.com/stoffi92/rfc5575bis/issues/59 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/7ab66f3e4db38036ab3ce8c29596= 353cfbadb2e5 Added a normative reference and changed the term to source IP address to = be consistent. --> 783 BGP implementations MUST also enforce that the AS_PATH = attribute of a 784 route received via the External Border Gateway Protocol (eBGP) 785 contains the neighboring AS in the left-most position of the = AS_PATH 786 attribute. While this rule is optional in the BGP = specification, it 787 becomes necessary to enforce it for security reasons. [major] Is this requirement only for the Flow Specification AFI/SAFI = pairs, or for all address families (IPv4 in the case of this document)? = Why? [major] [Assuming that the answer to the last question is: "Yes, for all = AFs"...] Should all the border routers in the AS enforce the first ASN, = or is the requirement only for routers receiving Flow Specifications? [major] In the case of receiving Flow Specifications from a neighbor in = an IXP, it may not be possible to enforce the rule above if a = "transparent ASN" is being used. Please include some text/guidance = about that type of case. Include it either here or in the Security = Considerations. [nit] The mention of security above makes me want to see related = considerations in =C2=A713/14. <-- *** Authors *** Tracked via issue #60: https://github.com/stoffi92/rfc5575bis/issues/60 This paragraph is about all AFI/SAFIs which flow spec can act on so 1/1 = and 1/128 + of course the flow spec itself 1/133 & 1/134. That = recommendation is for all routers in the domain as we must make sure = that some other AS did not injected more specific unicast route.=20 In the case of IXP indeed this sometime may break in the cases where RS = is used. If you even in the IX (which is L2 typically) use direct EBGP = session it all works ok. If you use session to RS and such RS by design = is transparent in order to not extend the AS_PATH length that enforce = first AS should be disabled. I think we could say that use of flow spec = when peering with RS in IXP env is much less secure as ASN spoofing in = the AS-PATH can occur. It is advised to apply strict inbound route = policy in such scenarios.=20 --> 789 The best-match unicast route may change over the time = independently 790 of the Flow Specification NLRI. Therefore, a revalidation of = the 791 Flow Specification NLRI MUST be performed whenever unicast = routes 792 change. Revalidation is defined as retesting that clause a = and 793 clause b above are true. [major] What about the case where a destination prefix is not included? = Besides enforcing the first AS, there isn't any verification specified. = What are the consideration about using that option? <-- *** Authors *** Tracked via issue #61: https://github.com/stoffi92/rfc5575bis/issues/61 Validation fails when no destination-prefix is embedded in the NLRI as = described in the validation process. --> 795 Explanation: 797 The underlying concept is that the neighboring AS that = advertises the 798 best unicast route for a destination is allowed to advertise = flow- 799 spec information that conveys a more or equally specific = destination 800 prefix. Thus, as long as there are no more specific unicast = routes, 801 received from a different neighboring AS, which would be = affected by 802 that filtering rule. 804 The neighboring AS is the immediate destination of the traffic 805 described by the Flow Specification. If it requests these = flows to 806 be dropped, that request can be honored without concern that = it 807 represents a denial of service in itself. Supposedly, the = traffic is 808 being dropped by the downstream autonomous system, and there = is no 809 added value in carrying the traffic to it. [major] A rogue router may request the traffic to be dropped. While the = local AS is simply reacting to the neighbor's request, the action can = still result in a DoS. I would like to see rogue router scenarios = reflected in the Security Considerations. [major] All this section seems to assume that flows are controlled = (dropped/redirected) between ASes...but the actions can also be = triggered from inside an AS. What are the considerations in that case? = Why isn't iBGP explicitly considered? <-- *** Authors *** Tracked via issue #62: https://github.com/stoffi92/rfc5575bis/issues/62 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/b4fe9935a8c0486d215b100a8059= 4039a46fbce5 Related issue mentioned: https://github.com/stoffi92/rfc5575bis/issues/9 Major1: Has been addressed by #9 Major2: In the iBGP case we are basically validating ourselves (see the = introduction). Compared to iBGP for ipv4 unicast: Our iBGP neighbors or = RRs are usually part of my own administrative/trust domain. We usually = care about what we receive from outside our administrative domain (ig. = eBGP). Furthermore in the iBGP case we do not have any origin AS to = verify against. However, the rough system case - that you pointed out, = is now addressed in the security considerations. --> 811 7. Traffic Filtering Actions ... 820 Implementations SHOULD provide mechanisms that map an = arbitrary BGP 821 community value (normal or extended) to filtering actions that 822 require different mappings in different systems in the = network. For 823 instance, providing packets with a worse-than-best-effort, = per-hop 824 behavior is a functionality that is likely to be implemented 825 differently in different systems and for which no standard = behavior 826 is currently known. Rather than attempting to define it here, = this 827 can be accomplished by mapping a user-defined community value = to 828 platform-/network-specific behavior via user configuration. [major] While this paragraph sounds technically correct, I think it = doesn't belong in this document because it (randomly) talks about a = different, yet tangentially related, topic. Also, it basically says = "SHOULD provide a mechanism to take arbitrary actions...which are not = defined here", so it is not complete from a Normative point of view. I = would prefer if we took this paragraph out. <-- *** Authors *** Tracked via issue #63: https://github.com/stoffi92/rfc5575bis/issues/63 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/f06933b47053dbf53f425ac19b37= fb8e361ee5d4 We need this to keep interoperability but added a new section on = "Interaction with other Filtering Mechanisms in Routers" --> 830 The default action for a traffic filtering Flow Specification = is to 831 accept IP traffic that matches that particular rule. 833 This document defines the following extended communities = values shown 834 in Table 2 in the form 0x8xnn where nn indicates the sub-type. 835 Encodings for these extended communities are described below. [minor] The "0x8xnn" format doesn't explain what x indicates. Perhaps = it would be better for the format to match the IANA section and include, = for example, 0xttss for type and sub-type...with the corresponding = change in Table 2. <-- *** Authors *** Tracked via issue #64: https://github.com/stoffi92/rfc5575bis/issues/64 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/6ed245f371c9a274dd8bf58e6cf4= 7dc252ee4b64 --> 837 = +-----------+----------------------+--------------------------------+ 838 | community | action | encoding = | 839 = +-----------+----------------------+--------------------------------+ 840 | 0x8006 | traffic-rate-bytes | 2-byte ASN, 4-byte float = | 841 | TBD | traffic-rate-packets | 2-byte ASN, 4-byte float = | 842 | 0x8007 | traffic-action | bitmask = | 843 | 0x8008 | rt-redirect AS-2byte | 2-octet AS, 4-octet value = | 844 | 0x8108 | rt-redirect IPv4 | 4-octet IPv4 addres, = 2-octet | 845 | | | value = | 846 | 0x8208 | rt-redirect AS-4byte | 4-octet AS, 2-octet value = | 847 | 0x8009 | traffic-marking | DSCP value = | 848 = +-----------+----------------------+--------------------------------+ 850 Table 2: Traffic Action Extended Communities [minor] The Table contains terms that have not been defined... It would = be ideal if the Table contained a forward reference to the section where = each action is discussed....or at least a general statement about the = details in the upcoming sub-sections... <-- *** Authors *** Tracked via issue #65: https://github.com/stoffi92/rfc5575bis/issues/65 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/ff76c0439d16dc5a338eda7843e0= 2b2daec37950 --> 852 Some traffic action communities may interfere with each other. 853 Section 7.6 of this specification provides general = considerations on 854 such traffic action interference. Any additional definition = of a 855 traffic actions specified by additional standards documents or = vendor 856 documents MUST specify if the traffic action interacts with an 857 existing traffic actions, and provide error handling per = [RFC7606]. [nit] s/definition of a traffic actions/definition of traffic actions [major] "Any additional definition of a traffic actions specified by = additional standards documents or vendor documents MUST specify..." We = really can't mandate what vendor documents say. s/additional = definition of a traffic actions specified by additional standards = documents or vendor documents MUST specify/additional definition of a = traffic action MUST specify [major] "MUST specify if the traffic action interacts with an existing = traffic actions" I think you meant something like: "MUST specify the = action to take if..." [major] "Any additional definition of a traffic actions...MUST...provide = error handling per [RFC7606]." rfc7606 already indicates what to do = about a malformed Extended Community attribute, which is how other = actions would presumably be specified. rfc7606 only mandates error = specifications for new attributes. What are your expectations here? <-- *** Authors *** Tracked via issue #66: https://github.com/stoffi92/rfc5575bis/issues/66 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/52baf1d9ae96cb4c761aa741b3b9= cff899c81233 Major1: Removed the vendor documents and standard documents as suggested Major2: Changed as suggested Major3:=20 Due to the changes in the "error handling" section this can be removed, = it seems to sufficiently defined in that other section. --> 859 Multiple traffic actions may be present for a single NLRI. = The 860 traffic actions are processed in ascending order of the = sub-type 861 found in the BGP Extended Communities. If not all of them can = be 862 processed the filter SHALL NOT be applied at all (for example: = if for 863 a given flow there are the action communities rate-limit-bytes = and 864 traffic-marking attached, and the plattform does not support = one of 865 them also the other shall not be applied for that flow). [minor] This paragraph is related to =C2=A77.6 (Considerations on = Traffic Action Interference). Consider putting all the related = information together. [major] "traffic actions are processed in ascending order of the = sub-type" Several of the communities have the same sub-type; if more = than one is present, which one should be processed first? [major] What should a receiver do if multiple of the same community = (type and sub-type) are included in the UPDATE? Would that be also = considered interference? [major] What does "processed" mean? Let me explain... The example is = about not being able to support an action. What about not being able to = apply the action because, for example, the next hop is not reachable? = Would that qualify as not being able to "process" the action? If other = redirect traffic rules are included (with perhaps an alternate next = hop), would the answer be different? [nit] Make the example a sentence on it's own: eliminate the = parenthesis. [minor] s/rate-limit-bytes/traffic-rate-bytes (0x8006) [minor] s/traffic-marking/traffic-marking (0x8009) [nit] s/plattform/platform [major] "If not all of them can be processed the filter SHALL NOT be = applied..." Should they be forwarded? Is this an example of = "interfering flow actions" (=C2=A77.6)? <-- *** Authors *** Tracked via issue #67: https://github.com/stoffi92/rfc5575bis/issues/67 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/b81db958595eee30df25e0c69f03= 6f7065117d63 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/119 major1: The document at some point had very strict rules on traffic = action interference and needed a strict ordering of traffic actions. = However, this idea did not find consensus in the WG. Without this the = ordering of traffic action is irrelevant and thus has been removed. major2: This is considered an interference and is explained in the = "interfering flow actions" section. major3: This was in the document for the same reason as major1. The = change in #119 makes it consistent with the major implementations and = gives advice how to treat FS in cases where not all of the actions can = be applied to the flow. --> 867 All traffic actions are specified as transitive BGP Extended 868 Communities. 870 7.1. Traffic Rate in Bytes (traffic-rate-bytes) sub-type 0x06 ... 888 Interferes with: No other BGP Flow Specification traffic = action in 889 this document. [minor] The definition of interference (=C2=A77.6) uses "more than one = conflicting traffic-rate action" as part of it. So it seems that = traffic-rate-bytes and traffic-rate-packets may interfere with each = other. <-- *** Authors *** Tracked via issue #68: https://github.com/stoffi92/rfc5575bis/issues/68 --> 891 7.2. Traffic Rate in Packets (traffic-rate-packets) sub-type = TBD [major] Because the "traffic actions are processed in ascending order of = the sub-type" (=C2=A77), what is the intent for this action? How should = IANA assign it? I assume that the intent might be to process it instead = of traffic-rate-bytes (assuming only one might be present)... Please be = clear in the instructions to IANA (in =C2=A712.3). Note that Table 7 = requests the assignment from the "Generic Transitive Experimental Use = Extended Community Sub-Types" registry, which seems to limit the = assignment choices. Having said all that, I would have assumed that = this action would be a variation of the 0x06 sub-type, but with a = different type... <-- *** Authors *** Tracked via issue #69: https://github.com/stoffi92/rfc5575bis/issues/69 No, this is not either traffic-rate-bytes or traffic-rate-packets a = router can maintain a counters for bytes/time AND packets/time at the = same time and thus drop packets that exceed a certain rate (whatever = limit is exceeded first). The traffic-rate-packets action should be = assigned alongside the traffic-rate-packets action.=20 --> ... 901 Interferes with: No other BGP Flow Specification traffic = action in 902 this document. [minor] The definition of interference (=C2=A77.6) uses "more than one = conflicting traffic-rate action" as part of it. So it seems that = traffic-rate-bytes and traffic-rate-packets may interfere with each = other. <-- *** Authors *** Tracked via issue #70: https://github.com/stoffi92/rfc5575bis/issues/70 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/f3fc28d0b3b9f32ba47a789031a2= 905b1be9f236 --> 904 7.3. Traffic-action (traffic-action) sub-type 0x07 906 The traffic-action extended community consists of 6 bytes of = which 907 only the 2 least significant bits of the 6th byte (from left = to 908 right) are currently defined. 910 40 41 42 43 44 45 46 47 911 +---+---+---+---+---+---+---+---+ 912 | reserved | S | T | 913 +---+---+---+---+---+---+---+---+ [minor] s/reserved/Traffic Action Fields It would be nice if the = Figure showed that all the bits (not just the ones in the last octet) = are part of the same field. [nit] Please add a Figure number.. <-- *** Authors *** Tracked via issue #71: https://github.com/stoffi92/rfc5575bis/issues/71 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/44e6c9a5d4e6b3ad0d0d0b8aad8c= a31b501091bf Related issues mentioned: https://github.com/stoffi92/rfc5575bis/issues/74 https://github.com/stoffi92/rfc5575bis/issues/73 --> 915 where S and T are defined as: 917 o T: Terminal Action (bit 47): When this bit is set, the = traffic 918 filtering engine will apply any subsequent filtering rules = (as 919 defined by the ordering procedure). If not set, the = evaluation of 920 the traffic filter stops when this rule is applied. [minor] According to the processing order and the values from Table 2, = not setting the bit would effectively cause only the traffic-rate-bytes = (0x8006) to ever be applied. Is that the correct interpretation? [minor] If the T bit is not set, can a router drop the communities that = are not going to be applied...or should they all be propagated? [major] Clearly, a rogue router could unset the bit before = propagating... <-- *** Authors *** Tracked via issue #72: https://github.com/stoffi92/rfc5575bis/issues/72 Minor1: The terminal action applies to filter rules only, not to the filter = actions (extended communties). Given we have 3 FS filters that match the = traffic (they are sorted first):=20 FS1) The first has T=3D0 and a traffic-rate-bytes action FS2) The second has only a redirect-action FS3) The third has only a traffic-rate-packets action Evaluation takes place according to the sorted filters FS1 first, then = FS2(because of T=3D0 in FS1). However, because FS2 has no T=3D0 set = evaluation stops here and the resulting action is: traffic-rate-bytes = and redirect. =20 Minor2: No it should propagate the communities - I think it is clearer with the = example above if a packet would not match FS1 but match FS2 only the = filter for FS2 is applied regardless of FS1 this is only the redirect = action. Major1: Yes, it is addressed in the Sec.considerations section, that community = screening is needed addtionally to the FS NLRI validation. --> 922 o S: Sample (bit 46): Enables traffic sampling and logging = for this 923 Flow Specification. [major] If the bit is not set, would sampling/logging be disabled? IOW, = is this an on/off switch, or is just the on action valid? <-- *** Authors *** Tracked via issue #73: https://github.com/stoffi92/rfc5575bis/issues/73 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/c0df11d084bbb686dfd21693c7be= 1c273eeed9a9 This bit is only effective when set. Updated the document. --> 925 o reserved: should always be set to 0 by the originator and = not be 926 evaluated by the receiving BGP speaker. [major] There is a registry for these bits. s/reserved/Traffic Action = Fields <-- *** Authors *** Tracked via issue #74: https://github.com/stoffi92/rfc5575bis/issues/74 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/c0df11d084bbb686dfd21693c7be= 1c273eeed9a9 Updated the document to name the bits according to the registry. --> ... 934 Interferes with: No other BGP Flow Specification traffic = action in 935 this document. [minor] Based on the definition in =C2=A77.6, I would have thought that = this action, with the T bit unset, would interfere with other actions = that will now not be applied. <-- *** Authors *** Tracked via issue #75: https://github.com/stoffi92/rfc5575bis/issues/75 --> 937 7.4. RT Redirect (rt-redirect) sub-type 0x08 ... 948 It should be noted that the low-order nibble of the Redirect's = Type 949 field corresponds to the Route Target Extended Community = format field 950 (Type). (See Sections 3.1, 3.2, and 4 of [RFC4360] plus = Section 2 of 951 [RFC5668].) The low-order octet (Sub-Type) of the Redirect = Extended 952 Community remains 0x08 for all three encodings of the BGP = Extended 953 Communities (AS 2-byte, AS 4-byte, and IPv4 address). [nit] I think that this whole paragraph is not needed....and it actually = may confuse people. I recommend deleting it. <-- *** Authors *** Tracked via issue #76: https://github.com/stoffi92/rfc5575bis/issues/76 Commits mentions: = https://github.com/stoffi92/rfc5575bis/commit/dfa0e6f65d2eb2ca3d898779a15e= 7e31645f4451 = https://github.com/stoffi92/rfc5575bis/commit/3440782681232e788e22a3e237b8= 6574ca12a8ba --> 955 Interferes with: All other redirect functions. [minor] What other redirect functions? The only ones defined are in = this section. <-- *** Authors *** Tracked via issue #77: https://github.com/stoffi92/rfc5575bis/issues/77 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/4cf4114ec4d09470a08146a46c31= f131290a048d --> 957 7.5. Traffic Marking (traffic-marking) sub-type 0x09 959 The traffic marking extended community instructs a system to = modify 960 the DSCP bits of a transiting IP packet to the corresponding = value. 961 This extended community is encoded as a sequence of 5 zero = bytes 962 followed by the DSCP value encoded in the 6 least significant = bits of 963 6th byte. [major] What action (if any) should a receiver take if the "5 zero = bytes" are not (all) set to 0? Maybe include something like: "MUST be = ignored when received...". <-- *** Authors *** Tracked via issue #78: https://github.com/stoffi92/rfc5575bis/issues/78 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/e3cc62728bf17f247340153f9e0f= c8f91ffec62c Updated the document as suggested.=20 --> 965 Interferes with: No other BGP Flow Specification traffic = action in 966 this document. 968 7.6. Considerations on Traffic Action Interference 970 Since traffic actions are represented as BGP extended = community 971 values, traffic actions may interfere with each other (ie. = there may 972 be more than one conflicting traffic-rate action associated = with a 973 single flow-filter). Traffic action interference has no = impact on 974 BGP propagation of flow filters (all communities are = propagated 975 according to policies). [nit] s/ie./e.g. I'm assuming it is an example and not the only case. [minor] Is "Traffic action interference" only the case when actions = describe conflicting actions? For example, different traffic rates. = Specifically, are actions that can't be applied (as described on =C2=A77),= also considered as interference? <-- *** Authors *** Tracked via issue #79: https://github.com/stoffi92/rfc5575bis/issues/79 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/19067fcd2526619d14273f2617cf= 082e39e2b4e1 --> 977 If a flow filter associated with interfering flow actions is = selected 978 for packet forwarding, it is a implementation decision which = of the 979 interfering traffic actions are selected. Implementors of = this 980 specification SHOULD document the behaviour of their = implementation 981 in such cases. [major] IOW, deployment of a set of "interfering flow actions" could = result in inconsistent behavior in the network. Could a rogue BGP = speaker advertise (or even add/delete) actions to a Flow Specification = and cause unexpected results? I guess that depending on what the action = is, there could be a significant effect. I think this is a = vulnerability that should be called out explicitly. Thinking a little = bit more...there are two vulnerabilities: (1) add/delete in the normal = case (even with consistent behavior), and (2) add/delete to exploit a = specific behavior of a node in the network. <-- *** Authors *** Tracked via issue #80: https://github.com/stoffi92/rfc5575bis/issues/80 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/009164aff026bde1e485986b7ea4= d6ed330ecdf5 The possible inconsistent behaviour within an AS is not so much of an = problem, because this should be avoided by the operator: ``` If required, operators are encouraged to make use of the BGP policy framework supported by their implementation in order to achieve a predictable behaviour (ie. match - replace - delete communities on administrative boundaries). ``` I think that Security Considerations addresses the fact that in addition = to the NLRI validation filtering/screening of the flow filtering action = extended community is needed: ``` This can be avoided by proper filtering of action communities at network borders and by mapping user-defined communities (see Section 7) to expose certain forwarding properties to third parties. ``` --> 983 If required, operators are encouraged to make use of the BGP = policy 984 framework supported by their implementation in order to = achieve a 985 predictable behaviour (ie. match - replace - delete = communities on 986 administrative boundaries). [minor] "If required..." When it is not required? IOW, I think that = those two words are not needed. <-- *** Authors *** Tracked via issue #81: https://github.com/stoffi92/rfc5575bis/issues/81 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/a66efedfde770579c0c1838b02b5= 1b71be16478c --> 988 8. Dissemination of Traffic Filtering in BGP/MPLS VPN Networks 990 Provider-based Layer 3 VPN networks, such as the ones using a = BGP/ 991 MPLS IP VPN [RFC4364] control plane, may have different = traffic 992 filtering requirements than Internet service providers. But = also 993 Internet service providers may use those VPNs for scenarios = like 994 having the Internet routing table in a VRF, resulting in the = same 995 traffic filtering requirements as defined for the global = routing 996 table environment within this document. This document = proposes an 997 additional BGP NLRI type (AFI=3D1, SAFI=3D134) value, which = can be used 998 to propagate traffic filtering information in a BGP/MPLS VPN 999 environment. [nit] s/proposes/defines (or maybe specifies) <-- *** Authors *** Tracked via issue #82: https://github.com/stoffi92/rfc5575bis/issues/82 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/d66835f7ba5422f68d853c059499= 7ae1fe5f3588 --> 1001 The NLRI format for this address family consists of a = fixed-length 1002 Route Distinguisher field (8 bytes) followed by a Flow = Specification, 1003 following the encoding defined above in Section 4.2 of this = document. 1004 The NLRI length field shall include both the 8 bytes of the = Route 1005 Distinguisher as well as the subsequent Flow Specification. [minor] s/Flow Specification, following the encoding defined above in = Section 4.2 of this document./Flow Specification (Section 4.2). <-- *** Authors *** Tracked via issue #83: https://github.com/stoffi92/rfc5575bis/issues/83 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/e72cf7124b42cb581f63e9cab336= 9098cccd4303 --> ... 1017 Propagation of this NLRI is controlled by matching Route = Target 1018 extended communities associated with the BGP path = advertisement with 1019 the VRF import policy, using the same mechanism as described = in "BGP/ 1020 MPLS IP VPNs" [RFC4364].. [nit] s/"BGP/MPLS IP VPNs"/BGP/MPLS IP VPNs <-- *** Authors *** Tracked via issue #84: https://github.com/stoffi92/rfc5575bis/issues/84 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/afd37b10067b4d9c08751d76368e= d44ebbc266cd --> 1022 Flow Specification rules received via this NLRI apply only to = traffic 1023 that belongs to the VRF(s) in which it is imported. By = default, 1024 traffic received from a remote PE is switched via an MPLS = forwarding 1025 decision and is not subject to filtering. 1027 Contrary to the behavior specified for the non-VPN NLRI, flow = rules 1028 are accepted by default, when received from remote PE routers. [major] The only other mention of "flow rule" is in the Introduction = when referring to the validation of external Flow Specifications, which = seems to then map to =C2=A76...but the next sub-section says that those = procedures apply. What am I missing? <-- *** Authors *** Tracked via issue #85: https://github.com/stoffi92/rfc5575bis/issues/85 The term "flow rule" has been changed and Flow Specification is used in = the entire document. This paragraph also mentions that in an MPLS = network traffic from other PEs (L3-VPN) receive MPLS traffic and will = not filter the traffic. Traffic needs to be filtered ingress between = CE-PE not in the MPLS forwarding domain. --> 1030 8.1. Validation Procedures for BGP/MPLS VPNs 1032 The validation procedures are the same as for IPv4. 1034 8.2. Traffic Actions Rules 1036 The traffic action rules are the same as for IPv4. [nit] These 2 sub-sections could simply be covered by a couple of = sentences... <-- *** Authors *** Tracked via issue #86: https://github.com/stoffi92/rfc5575bis/issues/86 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/36c8ea33d709d80b7b535749abd9= 0b366c3f3a86 --> 1038 9. Limitations of Previous Traffic Filtering Efforts [major] This section reads like a justification... I think it would be = a better fit as a subsection of the Introduction. <-- *** Authors *** Tracked via issue #87: https://github.com/stoffi92/rfc5575bis/issues/87 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/4ae3212aaa9b885ef17599ba015f= c41ad3319fe9 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/128 Removed the entire "justification" as of #128=20 --> 1040 9.1. Limitations in Previous DDoS Traffic Filtering Efforts ... 1052 Several techniques are currently used to control traffic = filtering of 1053 DoS attacks. Among those, one of the most common is to inject 1054 unicast route advertisements corresponding to a destination = prefix 1055 being attacked (commonly known as remote triggered blackhole = RTBH). 1056 One variant of this technique marks such route advertisements = with a 1057 community that gets translated into a discard Next-Hop by the 1058 receiving router. Other variants attract traffic to a = particular 1059 node that serves as a deterministic drop point. [minor] Please add Informative references to rfc3882, rfc5635, = rfc7999... <-- *** Authors *** Tracked via issue #88: https://github.com/stoffi92/rfc5575bis/issues/88 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/2de6ee4f037df43941965637c676= f0b0ad544d51 --> ... 1103 10. Traffic Monitoring 1105 Traffic filtering applications require monitoring and traffic 1106 statistics facilities. While this is an = implementation-specific 1107 choice, implementations SHOULD provide: 1109 o A mechanism to log the packet header of filtered traffic. 1111 o A mechanism to count the number of matches for a given flow 1112 specification rule. [minor] Is there any relationship between this section and the S bit in = =C2=A77.3? <-- *** Authors *** Tracked via issue #89: https://github.com/stoffi92/rfc5575bis/issues/89 --> 1114 11. Error-Handling and Future NLRI Extensions [major] Suggestion: this section should be limited to describing what a = malformed traffic action extended community is, and then simply point to = rfc7606, which already covers the rest. See more comments below. [nit] The two topics covered here seem unrelated... <-- *** Authors *** Tracked via issue #90: https://github.com/stoffi92/rfc5575bis/issues/90 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/7d844cc29fa37fe8afa7f3e0d08f= 5f362904fdea Related issues mentioned: https://github.com/stoffi92/rfc5575bis/issues/90 https://github.com/stoffi92/rfc5575bis/issues/91 https://github.com/stoffi92/rfc5575bis/issues/92 #90, #91, #92 have been addressed by splitting the Error handling and = Future extensions Sections. The change also makes it clear that what a = malformed Traffic Action community is and how a unknown FS component = should be treated.=20 --> 1116 In case BGP encounters an error in a Flow Specification UPDATE 1117 message it SHOULD treat this message as Treat-as-withdraw = according 1118 to [RFC7606] Section 2. [major] The SHOULD above with the communities-related errors described = below are in conflict with rfc7606, which says this: "An UPDATE message = with a malformed Extended Community attribute SHALL be handled using the = approach of "treat-as-withdraw"." <-- *** Authors *** Tracked via issue #91: https://github.com/stoffi92/rfc5575bis/issues/91 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/7d844cc29fa37fe8afa7f3e0d08f= 5f362904fdea Related issues mentioned: https://github.com/stoffi92/rfc5575bis/issues/90 https://github.com/stoffi92/rfc5575bis/issues/91 https://github.com/stoffi92/rfc5575bis/issues/92 #90, #91, #92 have been addressed by splitting the Error handling and = Future extensions Sections. The change also makes it clear that what a = malformed Traffic Action community is and how a unknown FS component = should be treated.=20 --> 1120 Possible reasons for an error are (for more reasons see also 1121 [RFC7606]): 1123 o Incorrect implementation of this specification - the = encoding/ 1124 decoding of the NLRI or traffic action extended-communities = do not 1125 comply with this specification. [major] Related to the NLRI, rfc7606 says that "in order to use the = approach of "treat-as-withdraw", the entire NLRI field and/or the = MP_REACH_NLRI and MP_UNREACH_NLRI attributes need to be successfully = parsed... If this is not possible...that the "session reset" approach = (or the "AFI/SAFI disable" approach) MUST be followed." [major] For the Extended Communities... The "incorrect implementation" = basically means that the encoding is wrong, right? But is the part = about "comply with this specification" necessary? Other traffic action = extended communities (defined elsewhere) might be received. I would = rather if the text above talked about malformed (to match the language = in rfc7606) traffic action extended communities in general (not just the = ones in this specification). <-- *** Authors *** Tracked via issue #92: https://github.com/stoffi92/rfc5575bis/issues/92 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/7d844cc29fa37fe8afa7f3e0d08f= 5f362904fdea Related issues mentioned: https://github.com/stoffi92/rfc5575bis/issues/90 https://github.com/stoffi92/rfc5575bis/issues/91 https://github.com/stoffi92/rfc5575bis/issues/92 #90, #91, #92 have been addressed by splitting the Error handling and = Future extensions Sections. The change also makes it clear that what a = malformed Traffic Action community is and how a unknown FS component = should be treated.=20 --> 1127 o Unknown Flow Specification extensions - The sending party = has 1128 implemented a Flow Specification NLRI extension unknown to = the 1129 receiving party. [major] This treatment of unknown extensions is in conflict with the = text in =C2=A74.2: "If a given component type within a prefix in = unknown, the prefix in question cannot be used for traffic filtering = purposes by the receiver... However, for the purposes of BGP route = propagation, this prefix should still be transmitted since BGP route = distribution is independent on NLRI semantics." IOW, = "treat-as-withdraw" is not compatible with forwarding UPDATES. <-- *** Authors *** Tracked via issue #93: https://github.com/stoffi92/rfc5575bis/issues/93 Commits mentions: = https://github.com/stoffi92/rfc5575bis/commit/a2709e9cc85fd67b5b498c8c41e3= 2f20c62598b8 = https://github.com/stoffi92/rfc5575bis/commit/c5ef244318446ce2d5b4e9f8de44= a9c1db6532d7 Related issues mentioned: https://github.com/stoffi92/rfc5575bis/issues/20 https://github.com/stoffi92/rfc5575bis/issues/93 You are completely right, this seems to be a remain from days where we = expected FS to be opaque to BGP. This has been solved with #20. --> 1131 In order to facilitate future extensions of the Flow = Specification 1132 NLRI, such extensions SHOULD specify a way to encode a = "always-true" 1133 match condition within the newly introduced components.. This = match 1134 condition can be used to propagate (and apply) certain filters = only 1135 if a specific extension is known to the implemenation. [nit] s/a "always-true"/an "always-true" [minor] What does "always-true" mean? [major] How come this document doesn't follow the advice about the = "always-true" match condition? [nit] s/implemenation/implementation <-- *** Authors *** Tracked via issue #94: https://github.com/stoffi92/rfc5575bis/issues/94 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/32664ea319323d544ba13e337e43= 31d21840a2cc The always-true match condition is nothing that is needed by the base = spec only by the extensions, but it can be achieved in multiple ways = even with this (the most obvious to mention: match ip protocol <=3D 0)=20= --> ... 1141 12.1. AFI/SAFI Definitions 1143 IANA maintains a registry entitled "SAFI Values". For the = purpose of 1144 this work, IANA updated the registry and allocated two = additional 1145 SAFIs: [nit] Even though the text will probably end up as written, it doesn't = ask IANA for anything: it assumes that the work is done. I would prefer = it if the text was worded as a request. It may not be an issue for = IANA, so there's no need to change anything, unless they say so. 1147 = +-------+------------------------------------------+----------------+ 1148 | Value | Name | Reference = | 1149 = +-------+------------------------------------------+----------------+ 1150 | 133 | IPv4 dissemination of Flow Specification | [this = | 1151 | | rules | document] = | 1152 | 134 | VPNv4 dissemination of Flow | [this = | 1153 | | Specification rules | document] = | 1154 = +-------+------------------------------------------+----------------+ [major] It's not clear to me (because there's no explicit request) if = the intent is to add this document as a reference, or to replace the one = to rfc5575. I would like you to be explicit. <-- *** Authors *** Tracked via issue #95: https://github.com/stoffi92/rfc5575bis/issues/95 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/4701d6e6bed7977e11871c3418bf= e9e9ddff0601 The IANA considerations section has been modified to address all the = issues. However we agreed not to change the registry names with this = document.=20 --> 1156 Table 3: Registry: SAFI Values 1158 12.2. Flow Component Definitions ... 1184 In order to manage the limited number space and accommodate = several 1185 usages, the following policies defined by [RFC8126] used: [nit] s/[RFC8126] used/[RFC8126] are used <-- *** Authors *** Tracked via issue #96: https://github.com/stoffi92/rfc5575bis/issues/96 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/9509ec46830f125d1289d8d2a9c4= 8fbf8cfca521 --> 1187 +--------------+-------------------------------+ 1188 | Range | Policy | 1189 +--------------+-------------------------------+ 1190 | 0 | Invalid value | 1191 | [1 .. 12] | Defined by this specification | 1192 | [13 .. 127] | Specification required | 1193 | [128 .. 255] | First Come First Served | 1194 +--------------+-------------------------------+ [major] 0 is not really a range...and it's Invalid, so it shouldn't be = part of the Table detailing the registration policies. BTW, I couldn't = find the text where 0 is declared Invalid -- please add some text to = =C2=A74.2. Move 0 to Table 4. [minor] Besides the fact that "Defined by this specification" is not a = Policy, this table doesn't change anything in the current registry; it = is not needed. <-- *** Authors *** Tracked via issue #97: https://github.com/stoffi92/rfc5575bis/issues/97 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/889fb220d67c5c4d44e9faad79a4= f3969137a7db Remove the invalid value. Furthermore the IANA considerations section = has been modified to address all the issues. However we agreed not to = change the registry names with this document.=20 --> 1196 Table 5: Flow Spec Component Types Policies 1198 The specification of a particular "Flow Spec Component Type" = must 1199 clearly identify what the criteria used to match packets = forwarded by 1200 the router is. This criteria should be meaningful across = router hops 1201 and not depend on values that change hop-by-hop such as TTL or = Layer 1202 2 encapsulation. [minor] This paragraph doesn't belong in the IANA section. It seems to = be laying out the groundwork for new components...so it belongs = somewhere else. Should any of the language be Normative? <-- *** Authors *** Tracked via issue #98: https://github.com/stoffi92/rfc5575bis/issues/98 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/6084af89831a9cdc7c4d7c83f07d= 8d599d87aa43 --> 1204 12.3. Extended Community Flow Specification Actions 1206 The Extended Community Flow Specification Action types defined = in 1207 this document consist of two parts: 1209 Type (BGP Transitive Extended Community Type) 1211 Sub-Type 1213 For the type-part, IANA maintains a registry entitled "BGP = Transitive 1214 Extended Community Types". For the purpose of this work = (Section 7), 1215 IANA updated the registry to contain the values listed below: [major] The range is defined in the registry as "0x80-0x8f Reserved = for Experimental Use". According to rfc8126, "IANA does not record = assignments from registries or ranges with this policy". I don't know why 0x80 was the first value chosen; it looks like it was = first used in draft-marques-idr-flow-spec-01 (2004), while the = corresponding Extended Communities draft = (draft-ietf-idr-bgp-ext-communities-07) already indicated that the range = was for Experimental Use. I guess just lack of sync... But then I also = don't understand how/why IANA ended up with the information in the = Registry....maybe because the sub-types are not for Experimental Use -- = hmmm, which sounds contradictory to me. The reason/history doesn't matter anymore, but the current use does. = The mechanism described in this document is clearly not experimental. = Given that changing the Type values is not an option because of the = deployed base, etc.., then I think we should clean up the Registry and = move 0x80-0x82 from the Experimental Use range to the FCFS range. This = change would mean an Update to rfc7153. To simplify the process, the Update can be done in this document. = However, I think that there's some confusion with these types apparently = being associated only with Flow Specifications, when they are labeled as = Generic. IOW, ideally the issue would be corrected independently... <-- *** Authors *** Tracked via issue #99: https://github.com/stoffi92/rfc5575bis/issues/99 This is not to be solved within this document. I note that I should = propose a update to RFC7153 to cleanup the registry independently. - As = noticed moving the values around is not an option. --> 1217 = +-------+-----------------------------------------------+-----------+ 1218 | Type | Name | = Reference | 1219 | Value | | = | 1220 = +-------+-----------------------------------------------+-----------+ 1221 | 0x80 | Generic Transitive Experimental Use Extended | = [RFC7153] | 1222 | | Community (Sub-Types are defined in the | = | 1223 | | "Generic Transitive Experimental Use Extended | = | 1224 | | Community Sub-Types" registry) | = | 1225 | 0x81 | Generic Transitive Experimental Use Extended | = [this | 1226 | | Community Part 2 (Sub-Types are defined in | = document] | 1227 | | the "Generic Transitive Experimental Use | [See = | 1228 | | Extended Community Part 2 Sub-Types" | = Note-1] | 1229 | | Registry) | = | 1230 | 0x82 | Generic Transitive Experimental Use Extended | = [this | 1231 | | Community Part 3 (Sub-Types are defined in | = document] | 1232 | | the "Generic Transitive Experimental Use | [See = | 1233 | | Extended Community Part 3 Sub-Types" | = Note-1] | 1234 | | Registry) | = | 1235 = +-------+-----------------------------------------------+-----------+ 1237 Table 6: Registry: Generic Transitive Experimental Use = Extended 1238 Community Types [major] In line with Updating the registry and the intent, the names of = the Types/Registries should not include the word "experimental" to avoid = any further confusion. <-- *** Authors *** Tracked via issue #100: = https://github.com/stoffi92/rfc5575bis/issues/100 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/4abe0685205f8ce112b5843ccb36= 5a2e1df5c415 The IANA considerations section has been modified to address all the = issues. However we agreed not to change the registry names with this = document.=20 --> 1240 Note-1: This document obsoletes RFC7674. [minor] Putting the reference to this note in the Table seems to be = asking IANA to add a note there too...which I would think is not the = case. This goes back to the intent of whether the reference to this = document should replace what is there or simply be added. <-- *** Authors *** Tracked via issue #101: = https://github.com/stoffi92/rfc5575bis/issues/101 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/d7614ac98d01b92a934ebc42f08a= 6fac1f9b6649 --> ... 1292 The "traffic-action" extended community (Section 7.3) defined = in this 1293 document has 46 unused bits, which can be used to convey = additional 1294 meaning. IANA created and maintains a new registry entitled: 1295 "Traffic Action Fields". These values should be assigned via = IETF 1296 Review rules only. The following traffic-action fields have = been 1297 allocated: [major] It needs to be mentioned somewhere that the reference for the = whole registry (not just the values below) should be moved to this = document. <-- *** Authors *** Tracked via issue #102: = https://github.com/stoffi92/rfc5575bis/issues/102 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/1a72e87cfd399b99a650b18b4953= 7ef7899255e7 The IANA considerations section has been modified to address this issue.=20= --> ... 1308 13. Security Considerations 1310 Inter-provider routing is based on a web of trust. = Neighboring 1311 autonomous systems are trusted to advertise valid reachability 1312 information. If this trust model is violated, a neighboring 1313 autonomous system may cause a denial-of-service attack by = advertising 1314 reachability information for a given prefix for which it does = not 1315 provide service. [major] References to Origin Validation (rfc6811) and BGPSec (rfc8205) = should be mentioned as possible mitigation...with maybe a comment about = the current deployment status. <-- *** Authors *** Tracked via issue #103: = https://github.com/stoffi92/rfc5575bis/issues/103 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/96ed1025fbf4dc22debd33bceb9f= 416212855ae5 Added those references --> 1317 As long as traffic filtering rules are restricted to match the 1318 corresponding unicast routing paths for the relevant prefixes, = the 1319 security characteristics of this proposal are equivalent to = the 1320 existing security properties of BGP unicast routing. However, = this 1321 document also specifies traffic filtering actions that may = need 1322 custom additional verification on the receiver side. See = Section 14. [major] In general, Flow Specifications have a one-AS-hop propagation = model, right? This means that the security properties are different = because (1) unicast routing propagates multiple hops, and (2) the intent = of the "Route Origin ASN" (rfc6811) is not reflected in the request to = rate-limit, or even drop (!) traffic to a destination. Yes, it is all = based on trust...but different. For example, Origin Validation wouldn't = be available for Flow Specifications. <-- *** Authors *** Tracked via issue #104: = https://github.com/stoffi92/rfc5575bis/issues/104 FS is never of one-AS-hop propagation model. To the very question of the Origin Validation it is all integrated by = original design. That means that if I must validate received FS routes = against unicast prefix and that unicast prefix was already Origin = Validated that implies that FS is also Origin Validated without adding = any new ROAs. The problem starts when validation is getting disabled as = this has avalanche effect for entire security model. --> 1324 Where it is not the case, this would open the door to further = denial- 1325 of-service attacks. [major] Like what? What are possible mitigations? Just saying that the = door is open is not enough. <-- *** Authors *** Tracked via issue #105: = https://github.com/stoffi92/rfc5575bis/issues/105 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/6ac9e480f9c8fa4865e5c7271b58= 3a5be4fd129b ... such as unwanted traffic filtering, remarking or redirection. Edited = document accordingly --> ... 1337 14. Operational Security Considerations [minor] If you ask me, this section should be rolled into the last one: = I think all the considerations (in both sections) are really = operational... <-- *** Authors *** Tracked via issue #106: = https://github.com/stoffi92/rfc5575bis/issues/106 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/59f663b6b36ca22f3ff6a818f1a5= 8287037373e1 --> 1339 While the general verification of the traffic filter NLRI is 1340 specified in this document (Section 6) the traffic filtering = actions 1341 received by a third party may need custom verification or = filtering. 1342 In particular all non traffic-rate actions may allow a third = party to 1343 modify packet forwarding properties and potentially gain = access to 1344 other routing-tables/VPNs or undesired queues. This can be = avoided 1345 by proper filtering of action communities at network borders = and by 1346 mapping user-defined communities (see Section 7) to expose = certain 1347 forwarding properties to third parties. [minor] I didn't get this last part... I understand filtering, but = didn't quite understand how the mapping of communities would help. <-- *** Authors *** Tracked via issue #107: = https://github.com/stoffi92/rfc5575bis/issues/107 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/9a0b71856eaf5c53d81766998062= 69aadcf0390e --> 1349 Since verfication of the traffic filtering NLRI is tied to the 1350 announcement of the best unicast route, a unfiltered address = space 1351 hijack (e.g. advertisement of a more specific route) may cause = this 1352 verification to fail and consequently prevent Flow = Specification 1353 filters from being accepted by a peer. [nit] s/verfication/verification [nit] s/a unfiltered/an unfiltered [minor] Again, mention Origin Validation as possible mitigation. <-- *** Authors *** Tracked via issue #108: = https://github.com/stoffi92/rfc5575bis/issues/108 Related issue mentioned: = https://github.com/stoffi92/rfc5575bis/issues/106 --> 1355 15. Original authors 1357 Barry Greene, Pedro Marques, Jared Mauch, Danny McPherson, and 1358 Nischal Sheth were authors on RFC5575, and therefore are = contributing 1359 authors on this document. [minor] To be in line with rfc7322, this section should be renamed to = "Contributors". <-- *** Authors *** Tracked via issue #109: = https://github.com/stoffi92/rfc5575bis/issues/109 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/7c98253a73b430ff78917114f70f= 1d0ad16f0852 --> 1361 16. Acknowledgements ... 1370 A packet rate flowspec action was also discribed in a flowspec 1371 extention draft and the authors like to thank Wesley Eddy, = Justin 1372 Dailey and Gilbert Clark for their work. [nit] This is the first time that "flowspec" is used. Not a bad = thing...just an observation that we went through the whole document = without using the colloquial name flowspec. [nit] s/discribed/described [nit] s/extention/extension <-- *** Authors *** Tracked via issue #110: = https://github.com/stoffi92/rfc5575bis/issues/110 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/0e7c49c736128f030315487bef1f= ed4b1b91e8a2 --> 1374 Additional the authors would like to thank Alexander = Mayrhofer, 1375 Nicolas Fevrier, Job Snijders, Jeffrey Haas and Adam Chappell = for 1376 their comments and review. [nit] s/Additional/Additionally, <-- *** Authors *** Tracked via issue #111: = https://github.com/stoffi92/rfc5575bis/issues/111 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/a2309120ed55ae39408213a18443= 91b7d5de3e01 --> 1378 17. References 1380 17.1. Normative References 1382 [IEEE.754.1985] 1383 IEEE, "Standard for Binary Floating-Point = Arithmetic", 1384 IEEE 754-1985, August 1985. [minor] IEEE has revised this spec twice, the most current revision was = published earlier this year. Should the reference to the 1985 version = be kept? Is there a reason not to point generically to IEEE 754, = instead of to a specific version? <-- *** Authors *** Tracked via issue #112: = https://github.com/stoffi92/rfc5575bis/issues/112 --> ... 1419 [RFC5668] Rekhter, Y., Sangli, S., and D. Tappan, "4-Octet AS 1420 Specific BGP Extended Community", RFC 5668, 1421 DOI 10.17487/RFC5668, October 2009, 1422 . [minor] I don't think this needs to be a Normative reference. <-- *** Authors *** Tracked via issue #113: = https://github.com/stoffi92/rfc5575bis/issues/113 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/b8b903c42e3751b4fdd49f23f954= f92d8a5b84f2 --> ... 1458 Appendix A. Comparison with RFC 5575 ... 1464 Section 1 introduces the Flow Specification NLRI. In = RFC5575 this 1465 NLRI was defined as an opaque-key in BGPs database. This 1466 specification has removed all references to a opaque-key = property. 1467 BGP is able understand the NLRI encoding. This change also 1468 resulted in a new section regarding error-handling and 1469 extensibility (Section 11). [nit] s/able understand/able to understand <-- *** Authors *** Tracked via issue #114: = https://github.com/stoffi92/rfc5575bis/issues/114 Commit mention: = https://github.com/stoffi92/rfc5575bis/commit/37d55a470b3258fb2bb7bc4471b4= eddaaa7f16b5 --> =20 --=20 Christoph Loibl c@tix.at | CL8-RIPE | PGP-Key-ID: 0x4B2C0055 | http://www.nextlayer.at > On 10.09.2019, at 18:09, Alvaro Retana wrote: >=20 > Dear authors: >=20 > I just finished reading this document. Thank you for the work in = clarifying and updating rfc5575! Many of my comments (see below) are = related to what I think is still missing clarity, or lack of it in some = of the new text. >=20 > Besides the specific comments, I have some larger issues that I want = to detail here. The first 2 are directed at the Shepherd and Chairs. >=20 > (A) IPR >=20 > The Shepherd report, the datatracker and the WGLC thread [1] all point = at no existing IPR. However, several declarations do exist...for = rfc5575 [2]. IMO, the changes between rfc5575 and this document are not = that significant to assume that the declarations don't apply.. I also = note that none of the original authors mentioned as "contributing = authors" (=C2=A715) replied to the IPR call during the WGLC.. >=20 > Jie: As Shepherd, can you please file a third-party disclosure [3] = pointing at the rfc5575 disclosures? Once that is done I will send a = message to the WG to consider the information -- I don't expect any = issues, but it has to be done. I'll need you to also update the Shepherd = writeup. Thanks! >=20 >=20 > (B) Support for IPv6 >=20 > I understand why this document only focuses on IPv4. While the text = points at draft-ietf-idr-flow-spec-v6, that draft has been expired for = over a year! What is the plan to move that work forward? It looks like = there may already be implementations in place [4]. >=20 > We all know this question will come up during IESG Evaluation, = specially in light of the IAB Statement on IPv6 [5] and the fact that = there was a related DISCUSS when rfc5575 was first processed [6] -- at = that time (2009!) the objection was cleared with the promise that an = IPv6 document would be forthcoming. >=20 > We should have a plan in place by the time this document makes it to = the IESG Telechat. It would have been ideal to publish both at the same = time, but I'll settle for the ability to (at least) point at the WGLC = (which has been brought up before [7]). >=20 >=20 > (C) IANA Considerations >=20 > (C1) traffic-rate-packets >=20 > The instructions to IANA for the assignment of the = traffic-rate-packets sub-type are not clear. The existing assignments = and the requirement that "traffic actions are processed in ascending = order of the sub-type" (=C2=A77) seem to imply that a specific order for = this new action may be intended. Unless explicitly instructed, IANA may = not assign a value that aligns with that intent. [See related comments = in =C2=A77.2.] >=20 > (C2) Experimental Use Ranges >=20 > This document uses ranges from the "BGP Transitive Extended Community = Types" registry which are reserved for Experimental Use. While the = history of this use is not clear, we should take the opportunity to = clean the registry. [See more in =C2=A712.3.] >=20 >=20 > (D) Document organization >=20 > This document kept most of the Introduction text, but then added = related and, in some cases, overlapping and redundant text in =C2=A75 = (not =C2=A75.1) and =C2=A79. Please combine the information from =C2=A71 = and =C2=A75, and the background from =C2=A79 into an updated = Introduction. =C2=A76 seems to belong right after the definition of the = NLRI (=C2=A74), and before the next part of the specification = (filtering) starts with =C2=A75.1, then =C2=A77... >=20 > Most of the old text is about justification, some from the specific = point of view of the then-authors. Please reconsider whether that still = applies. >=20 >=20 > I will wait for the major issues/comments to be addressed before = starting the IETF Last Call. >=20 > Thanks! >=20 > Alvaro. >=20 >=20 > [1] = https://mailarchive.ietf.org/arch/msg/idr/0WQW0pdqq1ae31GYZ7-dk3_Wqv8 > [2] https://datatracker.ietf.org/ipr/search/?rfc=3D5575&submit=3Drfc > [3] https://datatracker.ietf.org/ipr/new-third-party/ > [4] = https://mailarchive.ietf.org/arch/msg/idr/VH0mYVgT39ueJapb0axMgfgcAN8 > [5] https://www.iab.org/2016/11/07/iab-statement-on-ipv6/ > [6] https://datatracker.ietf.org/doc/rfc5575/history/ > [7] = https://mailarchive.ietf.org/arch/msg/idr/0J6gWHgBx33u8WpTa0B73mI6rIM >=20 >=20