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

[Robert Raszuk <[e-mail]>]

Hi Sue,

As WG participant I would like to point out that making all specs to
support both v4 and v6 may be a bit short sighted.

At some point in the future (and in some cases already today for example
access points and modems)  only IPv6 is supported - no IPv4. So if we
continue to mandate that all specs must contain and support both address
families it makes it very hard to any modern equipment IPv6 only vendor to
declare support for given RFC.

So IMO it is in best interest of any IETF spec to split IPv4 from IPv6 into
different documents. The push to add IPv6 into all drafts/rfcs perhaps was
a good move initially to a bit force IPv6 extensions, but I do not think it
is good idea long term. Also note that there are some solutions only
proposed to be used over IPv6 networks.

So here specifically with IPv6 we are talking of completely different match
criteria (IPv6 fixed and extension headers) have completely different
format and functionality then IPv4, both use different AFI/SAFIs etc ... so
here you would essentially artificially squeeze two documents into one. I
am not sure if this

Thx,
R.





On Thu, Sep 12, 2019 at 12:38 PM Susan Hares <[e-mail]> wrote:

> Alvaro:
>
>
>
> <WG chair hat on>
>
>
>
> Question:
>
> 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].
>
>
>
> Answer:
>
> The direction from the WG was to limit the draft to RFC5575 fixes.
>
> If you feel strongly, this can be queried to the WG again.
>
>
>
> <WG chair off>
>
> <author hat on>
>
> If WG agrees, this could be added to the draft.
>
> </author hat off>
>
>
>
> Sue Hares
>
>
>
>
>
> *From:* Dongjie (Jimmy) [mailto:[e-mail]]
> *Sent:* Thursday, September 12, 2019 5:14 AM
> *To:* Alvaro Retana; [e-mail]
> *Cc:* [e-mail]; idr@ietf. org
> *Subject:* RE: AD Review of draft-ietf-idr-rfc5575bis-17
>
>
>
> Hi Alvaro,
>
>
>
> Thanks pointing out the IPR issue with this -bis document. I will check
> the third-party disclosure procedure and file related disclosures later
> today.
>
>
>
> Best regards,
>
> Jie
>
>
>
> *From:* Alvaro Retana [mailto:[e-mail]]
> *Sent:* Wednesday, September 11, 2019 12:09 AM
> *To:* [e-mail]
> *Cc:* Dongjie (Jimmy) <[e-mail]>; [e-mail];
> idr@ietf. org <[e-mail]>
> *Subject:* AD Review of draft-ietf-idr-rfc5575bis-17
>
>
>
> 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
>
>
>
>
>