[Idr] AD Review of draft-ietf-idr-rfc5575bis-17

[Alvaro Retana <[e-mail]>]

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



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

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.

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.

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


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

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

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

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


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

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


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.


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

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

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

...
297       +------------------------------+
298       |    length (0xnn or 0xfn nn)  |
299       +------------------------------+
300       |    NLRI value  (variable)    |
301       +------------------------------+

[minor] s/0xfn nn/0xfnnn


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

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.


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

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?

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.


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


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


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.

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

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?

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


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

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.


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?


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

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

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

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.

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?

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.

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

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.


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

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.

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?

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.

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.

577   Decode for protocol:

[minor] Please show the decodes for all the fields.

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.

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


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

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


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


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.

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

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

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.

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

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


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

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

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.

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.

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.

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.

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?

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?


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.

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.

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

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?

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

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.

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


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

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.

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

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?

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


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


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.

955   Interferes with: All other redirect functions.

[minor] What other redirect functions?  The only ones defined are in this
section.


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

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?

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.

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.


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)

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


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

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?


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


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.

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


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


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

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

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

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.

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


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

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

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.

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?


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


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.

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.


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


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

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.

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.


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

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.

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.

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


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

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,


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?


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


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