Re: [Idr] AD Review of draft-ietf-idr-rfc5575bis-17 -> Updated Version -18 and Flowspec v6

Christoph Loibl <c@tix.at> Mon, 04 November 2019 19:24 UTC

Return-Path: <c@tix.at>
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 <c@tix.at>) 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 <c@tix.at>
In-Reply-To: <CAMMESsxHXUB_jQk7E9FkeNef2C7DDcbiEvnROFdbEjAVtMqcFA@mail.gmail.com>
Date: Mon, 4 Nov 2019 20:23:34 +0100
Cc: draft-ietf-idr-rfc5575bis@ietf.org, "idr@ietf. org" <idr@ietf.org>, idr-chairs@ietf.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <F9C8F51F-FB7C-4530-93EA-BF188D007C98@tix.at>
References: <CAMMESsxHXUB_jQk7E9FkeNef2C7DDcbiEvnROFdbEjAVtMqcFA@mail.gmail.com>
To: Alvaro Retana <aretana.ietf@gmail.com>
X-Mailer: Apple Mail (2.3445.104.11)
Archived-At: <https://mailarchive.ietf.org/arch/msg/idr/VmBXR-iEt6N-N8jXwHITng1oAEo>
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 <idr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/idr>, <mailto:idr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/idr/>
List-Post: <mailto:idr@ietf.org>
List-Help: <mailto:idr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/idr>, <mailto:idr-request@ietf.org?subject=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 

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%20implementations

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/8a4098ac8153d55d2a277bc268795969696214d0
Related issue mentioned: https://github.com/stoffi92/rfc5575bis/issues/117

Abstract and Introduction have been updated in #117
Both now mention the obsoleted RFCs 
-->

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/f380d82028084a17b800e3b34f2f8d6b51f8dbcf
    https://github.com/stoffi92/rfc5575bis/commit/ef68aec39a87c04aa729c4793f57a6c87adccbe2
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/fa7a74fd1d61cbaee7ef4f68c9206ac972f7b9d0
-->


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

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/17634fafd1bcd3607966ce50764913d07e07f152

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

227	2.  Definitions of Terms Used in This Memo
...
233	  Loc-RIB -   Local RIB.

[major] This simple definition doesn't match the one in §1.1/rfc4271.

<-- *** Authors ***
Tracked via issue #11: https://github.com/stoffi92/rfc5575bis/issues/11
Commit mention: https://github.com/stoffi92/rfc5575bis/commit/b4950ef76c347a67a8e31e0eff4d1433a00e34b8

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=1, SAFI=1) and IP multicast reverse-path information
271	  (AFI=1, SAFI=2) 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/8bcf213fccebec306606500f229207d495f4fbdf
-->

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

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 = 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/a89200d70c7e661c519bf4379ef43fee817f1d71
-->

...
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/2fe26e058f1428a739e154ebfef7bdbea647f894
-->

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

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

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 §11.  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: <type (1 octet), prefix length (1 octet), prefix>

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

solved as suggested.
-->

361	4.2.2.  Type 2 - Source Prefix

363	     Encoding: <type (1 octet), prefix-length (1 octet), prefix>

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

367	4.2.3.  Type 3 - IP Protocol

369	     Encoding:<type (1 octet), [op, value]+>

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 §14 (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. 

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 = 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/30613b6c8bdf0f25734c825d96e0e21a08c2fa3f

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/1a0f5300a4e80d7dc7d49b3fa03b511a1032ffa0
-->

410	           +----+----+----+----------------------------------+
411	           | lt | gt | eq | Resulting operation              |
412	           +----+----+----+----------------------------------+
413	           | 0  | 0  | 0  | false (independent of the value) |
414	           | 0  | 0  | 1  | == (equal)                       |
415	           | 0  | 1  | 0  | > (greater than)                 |
416	           | 0  | 1  | 1  | >= (greater than or equal)       |
417	           | 1  | 0  | 0  | < (less than)                    |
418	           | 1  | 0  | 1  | <= (less than or equal)          |
419	           | 1  | 1  | 0  | != (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:<type (1 octet), [op, value]+>

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 §4.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/c5b7f37ef7f2a68531de7f8ff088638b51739064

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. 

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 §4.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==8080
Type 5 dst-port==80
Type 6 src-port==443
-> will not match any flow because there is no flow with 
Expression: (dst-port==8080 OR src-port==8080) AND dst-port==80 AND src-port==443

Example 2:
Type 4 port==8080
Type 5 dst-port==80
-> matches flow dst-port 80 and src-port 8080
Expression: (dst-port==8080 OR src-port==8080) AND dst-port==80

Example 3:
Type 4 port==8080
-> matches flow dst-port=8080 src-port=*
-> matches flow dst-port any src-port=8080
Expression: (dst-port==8080 OR src-port==8080)

Example 4:
Type 6 src-port==80
-> matches flow src-port=80 dst-port=*
Expression: src-port==80
-->

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 §4.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/73760db087e3f91d2e781c4e4537853f4de384f4

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:<type (1 octet), [op, value]+>

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

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:<type (1 octet), [op, value]+>

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:<type (1 octet), [op, bitmask]+>

[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/f53ce5dfe0185b292b4e95033df0f332b9d72100
-->

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

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

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:<type (1 octet), [op, bitmask]+>

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 §4.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/bdd2e69858ea6a0b34f7ff4cd3bfffeed507535e
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/f078e919842330d670bc195276ead7bdc05a3351

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/574e2641bee448a1137eca011489fa988e0e9a99

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/9652cafb08a23d4d5997e60bbd9e6c20db569a96
-->

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/82a6bc647cdfbd53a6fa27c3def89383bcb2d209
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=1, = |
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/ed33739428da09560b8bfc1c03cf53c404c261ee
-->

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

...
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/4ccfaa5fecbb85322dc2544d1b76d68eb16d4133
    https://github.com/stoffi92/rfc5575bis/commit/e4a8c18242478cdd6a2b86a653321cd204fe2f7c
-->

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

...
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/09c6c5c346c9595d2d4e76274f351b9f1aafe0df
-->


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 §2 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/53d904bc5a5458cc277e0f87608e69b28f5a768c
-->

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 §4.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/620d5da6ff87d70dec5ffad1ba97d0b0915a16ac
-->

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 §4.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/182e01124f64c36878bab046bb7dbaafb77462d3
    https://github.com/stoffi92/rfc5575bis/commit/c6f3c54a8e5f1731bb8e1516fb969d18cce16f34
    https://github.com/stoffi92/rfc5575bis/commit/a2fc9c257da7460184c20695cc13dec6484bf73f
-->

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 §4.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/d6481bb62fccbb0cfa52610a3b24acf96aefaa89
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 §5.1 first paragraph.
  
-->

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

Moved the Python source to the Appendix and added a the licence in the code component.
-->

687	  <CODE BEGINS>
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/ec1988d1bfa2ff8c0dc76c9dfb338bc217e7d43d
-->

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

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

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/3883234a5a12e25d75f9c00f0bd15b9d32535072

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

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 §13/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. 

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

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/b4fe9935a8c0486d215b100a80594039a46fbce5
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/f06933b47053dbf53f425ac19b37fb8e361ee5d4

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/6ed245f371c9a274dd8bf58e6cf47dc252ee4b64
-->

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

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/52baf1d9ae96cb4c761aa741b3b9cff899c81233

Major1:
Removed the vendor documents and standard documents as suggested

Major2:
Changed as suggested

Major3: 
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 §7.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" (§7.6)?

<-- *** Authors ***
Tracked via issue #67: https://github.com/stoffi92/rfc5575bis/issues/67
Commit mention: https://github.com/stoffi92/rfc5575bis/commit/b81db958595eee30df25e0c69f036f7065117d63
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 (§7.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" (§7), 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 §12.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. 
-->

...
901	  Interferes with: No other BGP Flow Specification traffic action in
902	  this document.

[minor] The definition of interference (§7.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/f3fc28d0b3b9f32ba47a789031a2905b1be9f236
-->

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/44e6c9a5d4e6b3ad0d0d0b8aad8ca31b501091bf
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): 
FS1) The first has T=0 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=0 in FS1). However, because FS2 has no T=0 set evaluation stops here and the resulting action is: traffic-rate-bytes and redirect.  

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

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

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 §7.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/dfa0e6f65d2eb2ca3d898779a15e7e31645f4451
    https://github.com/stoffi92/rfc5575bis/commit/3440782681232e788e22a3e237b86574ca12a8ba
-->

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/4cf4114ec4d09470a08146a46c31f131290a048d
-->

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

Updated the document as suggested. 
-->

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 §7), also considered as interference?

<-- *** Authors ***
Tracked via issue #79: https://github.com/stoffi92/rfc5575bis/issues/79
Commit mention: https://github.com/stoffi92/rfc5575bis/commit/19067fcd2526619d14273f2617cf082e39e2b4e1
-->

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/009164aff026bde1e485986b7ea4d6ed330ecdf5

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

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=1, SAFI=134) 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/d66835f7ba5422f68d853c0594997ae1fe5f3588
-->

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

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

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 §6...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/36c8ea33d709d80b7b535749abd90b366c3f3a86
-->

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/4ae3212aaa9b885ef17599ba015fc41ad3319fe9
Related issue mentioned: https://github.com/stoffi92/rfc5575bis/issues/128

Removed the entire "justification" as of #128 
-->

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/2de6ee4f037df43941965637c676f0b0ad544d51
-->

...
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 §7.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/7d844cc29fa37fe8afa7f3e0d08f5f362904fdea
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. 
-->

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

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

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 §4.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/a2709e9cc85fd67b5b498c8c41e32f20c62598b8
    https://github.com/stoffi92/rfc5575bis/commit/c5ef244318446ce2d5b4e9f8de44a9c1db6532d7
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/32664ea319323d544ba13e337e4331d21840a2cc

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 <= 0) 
-->

...
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/4701d6e6bed7977e11871c3418bfe9e9ddff0601

The IANA considerations section has been modified to address all the issues. However we agreed not to change the registry names with this document. 
-->

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/9509ec46830f125d1289d8d2a9c48fbf8cfca521
-->

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 §4.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/889fb220d67c5c4d44e9faad79a4f3969137a7db

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

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/6084af89831a9cdc7c4d7c83f07d8d599d87aa43
-->

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/4abe0685205f8ce112b5843ccb365a2e1df5c415

The IANA considerations section has been modified to address all the issues. However we agreed not to change the registry names with this document. 
-->

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

...
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/1a72e87cfd399b99a650b18b49537ef7899255e7

The IANA considerations section has been modified to address this issue. 
-->

...
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/96ed1025fbf4dc22debd33bceb9f416212855ae5

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/6ac9e480f9c8fa4865e5c7271b583a5be4fd129b

... 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/59f663b6b36ca22f3ff6a818f1a58287037373e1
-->

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/9a0b71856eaf5c53d8176699806269aadcf0390e
-->

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

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/0e7c49c736128f030315487bef1fed4b1b91e8a2
-->

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

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	             <https://www.rfc-editor.org/info/rfc5668>;.

[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/b8b903c42e3751b4fdd49f23f954f92d8a5b84f2
-->

...
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/37d55a470b3258fb2bb7bc4471b4eddaaa7f16b5
-->


   

-- 
Christoph Loibl
c@tix.at | CL8-RIPE | PGP-Key-ID: 0x4B2C0055 | http://www.nextlayer.at



> On 10.09.2019, at 18:09, Alvaro Retana <aretana.ietf@gmail.com>; wrote:
> 
> Dear authors:
> 
> 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.
> 
> 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.
> 
> (A) IPR
> 
> 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" (§15) replied to the IPR call during the WGLC..
> 
> 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!
> 
> 
> (B) Support for IPv6
> 
> 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].
> 
> 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.
> 
> 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]).
> 
> 
> (C) IANA Considerations
> 
> (C1) traffic-rate-packets
> 
> 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" (§7) 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 §7.2.]
> 
> (C2) Experimental Use Ranges
> 
> 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 §12.3.]
> 
> 
> (D) Document organization
> 
> This document kept most of the Introduction text, but then added related and, in some cases, overlapping and redundant text in §5 (not §5.1) and §9.  Please combine the information from §1 and §5, and the background from §9 into an updated Introduction.  §6 seems to belong right after the definition of the NLRI (§4), and before the next part of the specification (filtering) starts with §5.1, then §7...
> 
> 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.
> 
> 
> I will wait for the major issues/comments to be addressed before starting the IETF Last Call.
> 
> Thanks!
> 
> Alvaro.
> 
> 
> [1] https://mailarchive.ietf.org/arch/msg/idr/0WQW0pdqq1ae31GYZ7-dk3_Wqv8
> [2] https://datatracker.ietf.org/ipr/search/?rfc=5575&submit=rfc
> [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
> 
>