Revised Validation Procedure for BGP Flow Specifications
draft-ietf-idr-bgp-flowspec-oid-15

Thanks for this document, I'm glad to see it finally close to publication. I have a few comments I hope will help improve it, below.

1. Section 2

   [RFC8955] defines a BGP NLRI [RFC4271] that can be used to distribute

RFC 4271 is the right citation for “BGP” but for “BGP NLRI” other than IPv4 unicast you want RFC 4760. 

   the Flow Specification.  The aim is making sure that only speakers on

making -> to make

   originated in any location within the same autonomous system than the
  
than -> as

   Flow Specification received from an iBGP peer, it could be possible
   to disseminate such Flow Specification NLRIs directly from the

“It could be possible”? Or “it would be necessary”?

   validated when received by RR and R1.  Sometimes the Flow
   Specification needs to be originated on AS1.  ASBR1 could originate

on -> within


2. Section 4.1

          1.  This condition SHOULD be enabled by default (it may be
              disabled by explicit configuration as described on the
              next list item (b.2.1))..  an empty AS_PATH.

Looks as though “an empty AS_PATH” is a cut-n-paste error that should be deleted?

          3.  As an extension to this rule, a given non-empty AS_PATHs

AS_PATHs -> AS_PATH (agreement in number)

              AS_CONFED_SET segments), MAY be validated by policy.  A

I think “permitted” would be clearer than “validated”. 

      Also, policy may be useful to validate a specific set of non-empty
      AS_PATHs (b.2.3).  For example, it could validate a Flow
      Specification whose AS_PATH contains only an AS_SEQUENCE with ASes
      that are all known to belong to the same administrative domain.

Same point, I suggest “permit”. 


3. Section 4.2

   This rule prevents the exchange of BGP Flow Specification NLRIs at
   Internet exchanges with BGP route servers [RFC7947]. 

Since it may not be common knowledge that route servers don’t insert their own AS number, I suggest a change something like this, for clarity:

   This rule prevents the exchange of BGP Flow Specification NLRIs at
   Internet exchanges with BGP route servers, which by design don’t insert 
   their own AS number into the AS_PATH  ([RFC7947] Section 2.2.2.1).


4. Section 4.2

      For clarity, the AS in the left-most position of the AS_PATH means
      the AS that was last added to the AS_SEQUENCE.

I suggest changing “last” to “most recently”. This is just personal preference, though. I also suggest changing “AS_SEQUENCE” somehow, probably to “AS_PATH”. The reason for this is there could be more than one AS_SEQUENCE in the AS_PATH so the definite article isn’t quite correct. There is however only one AS_PATH, and you can make the change without loss of generality or correctness. So, the full suggested rewrite is:

      For clarity, the AS in the left-most position of the AS_PATH means
      the AS that was most recently added to the AS_PATH.


5. Section 4.2

      Specifications.  This is a security BGP threat, but out of the

“Security BGP threat” -> “security threat” (or “BGP security threat” if you prefer)


6. Section 4.2

      Using the new rule to validate a Flow Specification route received
      from an External Border Gateway Protocol (eBGP) peer belonging to
      the same local domain (in the case of a confederation) is out of
      the scope of this document.  Note that although it's possible, its
      utility is dubious.  Although it is conceivable that an router in
      the same local domain (both iBGP and eBGP within the same local
      domain) could send a rogue update, only eBGP (outside the local
      domain) risk is considered within this document (in the same
      spirit of the mentioned beforehand AS_PATH validation in
      [RFC4271]).

I’m having a hard time understanding this paragraph. This is at least partly due to the use of “local domain“, which isn’t a well-defined term of art. You could try something like “under the same administration“? But, even if I make that change in my head as I read the paragraph, I still don’t get your point. :-( 

Is the whole paragraph strictly about confederations, and what you’re talking about is a BGP session between a router in one member-AS and a router in another member-AS of the confederation? If that’s it, I can probably propose some new text.

Also, “an router” -> “a router”. 

Also, you should cite RFC 5065 when you mention confederations.


7. Section 5

   its best-match unicast route) are learned from the same peer accross

accross-> across


8. Section 5

      Consider the validation procedure preceding this document.  The

I think what you mean here is “consider the validation procedure defined in RFC 8955“. If that’s correct, I suggest saying it that way. (If that’s not correct, what did you mean?)


9. Section 7

   thus maintain security characteristics equivalent to the existing

Maintain -> maintains


10. Section 7

   The original AS_PATH validation rule ([RFC4271] section 6.3) becomes
   hereby optional (section 4.2).  

I don’t think you’re actually updating RFC 4271, are you? The way you’ve written this makes it sound as though you are. You’re relaxing the rule in RFC 8955, but as you point out in section 4.2, RFC 4271 makes AS_PATH neighbor AS insertion enforcement optional to begin with:

   If the UPDATE message is received from an external peer, the local
   system MAY check whether the leftmost (with respect to the position
   of octets in the protocol message) AS in the AS_PATH attribute is
   equal to the autonomous system number of the peer that sent the
   message.  If the check determines this is not the case, the Error
   Subcode MUST be set to Malformed AS_PATH.

Maybe you could rewrite something like this: “As per section 4.2, it is becomes optional to enforce that the AS_PATH attribute of a route received via the External Border Gateway Protocol (eBGP) contains the neighboring AS in the leftmost position of the AS_PATH attribute. Then we continue:

             If that original rule is actually not
   enforced it may introduce some security risks.  A peer (or a client
   of a route server peer) in AS X could advertise a rogue Flow
   Specification route whose first AS in AS_PATH was Y (assume Y is the
   first AS in the AS_PATH of the best-match unicast route).  This risk
   is impossible to prevent if the Flow Specification route is received
   from a route server peer.  

I think you’ve missed out on the opportunity to point out that the risk can also be mitigated if the route server itself enforces the optional rule of RFC 4271 section 6.3.. I think it’s fair to say that at an exchange point, the administrator of the route server enjoys more trust then the administrators of other routers at the exchange point.

               If that peer is known for a fact not to be
   a route server, that optional rule SHOULD be enforced for Flow
   Specification routes.

It may be necessary to say a little more about how you would get this knowledge about the peer not being a route server. Possibly a rewrite could be something like: "Unless configuration (or other means beyond the scope of this document) indicates that the peer is a route server, that optional rule SHOULD be enforced for Flow Specification routes from EBGP peers."


11. Section 7

   absolute and the new possibility than an iBGP speaker could send a

Than -> that

Section 6 refers to this draft as "this memo", while all other self-references are "this document".  I suggest changing this section to match, or, in the alternative, leave a note to the RFC Editor to remove the section.

In Section 7, the "SHOULD not" probably ought to be "SHOULD NOT".

Thank you Magnus Nystrom for the SECDIR review and for the subsequent updates by the authors.

** Section 2.  Editorial. s/same autonomous system than/same autonomous system as/

** Section 2.

   While the
   proposed modification cannot be used for inter-domain coordination of
   traffic filtering, it greatly simplifies distribution of intra-domain
   traffic filtering policies within an autonomous system which has a
   large number of border routers having complex BGP policies.  

Should the above key detail be explicitly framed in an applicability statement?

** Section 4.1.  Editorial + normative guidance which frames the text on the intent of the designed network, not the operator “knowing something”.

OLD
Disabling the new condition above (b.2.2) could be a good practice
      if the operator knew with certainty that a Flow Specification
      would not be originated inside the local domain.

An additional
      case would be if it was known for a fact that only the right
      egress border routers (i.e. those that were also egress border
      routers for the best routes) were originating a Flow Specification
      NLRI.


NEW
Disabling the new condition above (b.2.2) is RECOMMENDED in networks where policy prohibits Flow Specification from originating inside the local domain or where configuration dictates that only the egress border routers (i.e. those that were also egress border routers for the best routes) will originating a Flow  Specification NLRI.

Thank you for the work put into this document.

Special thanks to Susan Hares for her recent and detailed shepherd's write up.

Please find below some non-blocking COMMENT points (but replies would be appreciated).

I hope that this helps to improve the document,

Regards,

-éric

== COMMENTS ==

Is there any reason why the shepherd is not acknowledged in the document ?

-- Section 2 --
Strongly suggest s/protocol port numbers/protocol or port numbers/

Please expand "RR" at first use.

-- Section 4.2 --
Suggest to add also a reference to section 4 of RFC 8955 rather than "[RFC8955] states:"

John had some good editorial comments; thank you to him for spotting
them, and to the authors for pulling in the fixes.

It seems to me that the revision to the validation procedures that we
describe here may be broader than needed to satisfy the stated use case,
and that such broadness has a corresponding weakening of the security
properties of the system as a whole.  My understanding is that when
central controllers or route-reflectors are used, they tend to be in
some sense "more trusted" than peer routers in the domain (and,
correspondingly, more tightly secured).  So it seems like the system as
a whole would be less fragile/more secure if "off-path" flow specs were
allowed only from the trusted source, versus from any peer in the same
AS [confederation].  Is there a reason why such a procedure is
infeasible?  (It seems like all peers in the AS could be marked as
"trusted" in this way, if appropriate, which allows for recovering the
current semantics of this draft if needed.)  This is, in essence, the
same topic raised by the secdir reviewer and relates to "fail-closed" vs
"fail-open".  I think it gives a clearer picture on what the safe and
recommended behavior is, if we say to only accept these routes from
"trusted peers in the local domain" (but allow all peers in the local
domain to be trusted if appropriate) than to say to always accept these
routes from peers in the local domain and optionally do strict
enforcement if we know that the peer advertising the route is not a
route server.

Abstract

   This document describes a modification to the validation procedure
   defined for the dissemination of BGP Flow Specifications.  The
   dissemination of BGP Flow Specifications requires that the originator
   of the Flow Specification matches the originator of the best-match
   unicast route for the destination prefix embedded in the Flow
   Specification.  [...]

I'd suggest adding a couple words about "prior to this document
requires" or "as specified in RFC 8955 requires", since that's exactly
what we're changing with this document.

                However, the validation procedure will fail in this
   scenario.  [...]

Similarly, this could also say "the RFC 8955 validation procedure".

Section 4.1

      2.  The AS_PATH attribute of the Flow Specification is empty or
          contains only an AS_CONFED_SEQUENCE segment [RFC5065].

Would there ever be cause to have a knob that allows this condition only
for the empty AS_PATH (and denies this condition when AS_CONFED_SEQUENCE
is present)?  From skimming RFC 5065 I mostly assume not, but have to
ask...

Section 4.2

      For clarity, the AS in the left-most position of the AS_PATH means
      the AS that was last added to the AS_SEQUENCE.

Thank you for including this; it is a big help for readability.

      Using the new rule to validate a Flow Specification route received
      from an External Border Gateway Protocol (eBGP) peer belonging to
      the same local domain (in the case of a confederation) is out of
      the scope of this document.  Note that although it's possible, its
      utility is dubious.  Although it is conceivable that a router in
      the same local domain (both iBGP and eBGP within the same local
      domain) could send a rogue update, only eBGP (outside the local
      domain) risk is considered within this document (in the same
      spirit of the mentioned beforehand AS_PATH validation in
      [RFC4271]).

I'm having (I think) the same trouble John did reading this paragraph.
I'll watch his ballot thread and chime in there if needed; no need to
specifically reply here.

Section 7

We could mention again (per §3) that having this mechanism allows
central SOC or controller systems to be able to propagate flowspecs more
readily during attacks and improves the stability/security of the
network.

   This document updates the route feasibility validation procedures for
   Flow Specifications learned from iBGP peers and through route
   servers.  This change is in line with the procedures described in
   [RFC8955] and, thus, the security characteristics equivalent to the
   existing security properties of BGP unicast routing are maintained.

I would argue that the security characteristics are not (strictly)
"equivalent", but only "roughly equivalent".  With the new procedures, a
malicious or compromised node in the local AS can (e.g.) cause traffic
to be discarded without steering that traffic to/through itself.  There
are plenty of other ways that such a malicious node can cause harm, not
least announcing that prefix and dropping the traffic as it passes
through, so the overall properties of the system are roughly equivalent,
but there are clear differences of the particulars.

   route servers may suppose an operational challenge.  If the condition
   of the peer is unknown, the rule SHOULD not be enforced.

In light of my top-level comment (and the secdir review), I'm
uncomfortable with a SHOULD-level "fail-open" behavior.  Could the
default behavior when the condition of the peer is unkonwn be subject to
a configuration knob?

   BGP updates learned from iBGP peers are considered trusted, so the
   Traffic Flow Specifications contained in BGP updates are also
   considered trusted.  Therefore, it is not required to validate that
   the originator of an intra-domain Traffic Flow Specification matches
   the originator of the best-match unicast route for the destination
   prefix embedded in that Flow Specification.  Note that this
   trustworthy consideration is not absolute and the new possibility
   than an iBGP speaker could send a rogue Flow Specification is
   introduced.

Also in light of my toplevel comment, it's not clear that this paragraph
adds much value.  Specifically, as we note in the last sentence, the
level of trust in a route server is not (in the general case) the same
level of trust in a generic peer in the iBGP domain, even if both peers
are trusted in a general sense.  So it seems that the core sentiment
here is that when we get the flowspec from a trusted source, it's
trusted, and we don't need to be as strict about validating that it's
the same source that we would send the traffic to in the absence of a
flowspec.

Section 9.1

It seems that RFC 7947 is reference in only one location, and not in a
manner that strongly implies a normative relationship.

NITS

Abstract

                For an iBGP received route, the originator is

"IBGP" (with that capitalization) appears on the RFC Editor's list
at https://www.rfc-editor.org/materials/abbrev.expansion.txt but is not
marked as "well-known".  So we should probably use the expanded version
in the abstract and define it on first use in the body.

Section 2

   the same forwarding path as the dest-route.  For the case where AS1
   has thousands of ASBRs, it becomes impractical to originate different
   Flow Specification rules on each ASBR in AS1 based on which ASBR each
   dest-route is learned from.  The objective is to advertise all the
   Flow Specifications from the same route-controller.

"it becomes impractical" does not flow directly to "the objective is";
I'd put some transition phrase in, like "to make the situation more
tenable".

Section 3

   can direct border routers within their AS with specific attack
   mitigation actions (drop the traffic, forward to a clean-pipe center,
   etc.).

Maybe "clean-pipe center" is a term of art I'm not familiar with, but
the natural reading would be that a clean pipe is something that is the
output of a scrubbing center, and that pipe-cleaning is the operation
performed there.  This would suggest rewording to something like
"forward to a scrubbing center", "forward to a pipe-cleaning location",
etc.

   In addition, an operator may extend the requirements above for a
   group of ASes via policy.  This is described in section (b.2.3) of
   the validation procedure.

I suggest a forward reference ("below" or "in Section 4.1") to contrast
to the RFC 8955 procedure.

Section 5

                If the latter applies, a network should be designed so
   it has a congruent topology amongst unicast routes and Flow
   Specification routes.  [...]

This "should" does not give any indication of why the network should be
designed in this manner (e.g., that failing to do so would result in
(b.1) failing).

      Flow Specifications originated in a different local domain sill
      need a congruent topology.  The reason is that the second
      condition (b.2) evaluates to false and only the first condition
      (b.1) is evaluated.

I suggest s/different local domain/different domain/ -- my first reading
of "different local domain" was "a different AS but the same
confederation", and only by contrasting this statement to the previous
paragraph did I realize the intended meaning.
Alternately (or additionally?), this could be clarified by noting that
this scenario will result in an AS_PATH that contains
non-AS_CONFED_SEQUENCE segments.

Section 7, paragraph 4, comment:
>    from a route server peer.  If that peer is known for a fact not to be
>    a route server, that optional rule SHOULD be enforced for Flow
>    Specification routes.

The phrasing here seems complicated. Would it be easier to say that the rule
MUST NOT be enforced if the peer is known to be a route server?

-------------------------------------------------------------------------------
All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools, so there will likely be some false positives. There is no need
to let me know what you did with these suggestions.

Section 5, paragraph 2, nit:
-    it has a congruent topology amongst ipv4 unicast routes and Flow
-                                        ^^
+    it has a congruent topology amongst IPv4 unicast routes and Flow
+                                        ^^

Section 5, paragraph 2, nit:
-    for the two equivalent routes (i.e. the Flow Specification route and
-    its best-match unicast route) are learned from the same peer accross
-                                                                   -
+    for the two equivalent routes (i.e., the Flow Specification route and
+                                       +
+    its best-match unicast route) are learned from the same peer across

Section 7, paragraph 2, nit:
-    servers.  This change is in line with the procedures in [rfc8955] and
-                                                             ^^^
+    servers.  This change is in line with the procedures in [RFC8955] and
+                                                             ^^^

Section 7, paragraph 4, nit:
-    hereby optional (section 4.2).  If that original rule is actually not
-           ^^^^^^^^
-    enforced it may introduce some security risks.  A peer (or a client
+    hereby OPTIONAL (section 4.2).  If that original rule is actually not
+           ^^^^^^^^
+    enforced, it may introduce some security risks.  A peer (or a client
+            +

Section 2, paragraph 3, nit:
> e in an autonomous system with a large number of border routers having compl
>                                ^^^^^^^^^^^^^^^^^
Specify a number, remove phrase, or simply use "many" or "numerous"

Section 2, paragraph 7, nit:
> n autonomous system which has a large number of border routers having complex
>                               ^^^^^^^^^^^^^^^^^^^^
Specify a number, remove phrase, or simply use "many" or "numerous"

Section 3, paragraph 8, nit:
> ms exist to protect each AS independently from network security attacks using
>                             ^^^^^^^^^^^^^^^^^^
The usual collocation for "independently" is "of", not "from". Did you mean
"independently of"?

Section 4.1, paragraph 2, nit:
> tion 6 is redefined as follows: b) One of the following conditions MUST ho
>                                  ^
Unpaired symbol: '(' seems to be missing

Section 4.1, paragraph 6, nit:
> he next list item (b.2.1)).. an empty AS_PATH. 2. This
>                           ^^
Two consecutive dots

Section 4.2, paragraph 12, nit:
> bious. Although it is conceivable that an router in the same local domain (
>                                        ^^
Use "a" instead of 'an' if the following word doesn't start with a vowel sound,
e.g. 'a sentence', 'a university'.

Section 5, paragraph 2, nit:
> tter applies, a network should be designed so it has a congruent topology am
>                                   ^^^^^^^^^^^
Use a comma before 'so' if it connects two independent clauses (unless they are
closely connected and short).

"MUST", paragraph 2, nit:
> tes are also considered trusted. Therefore it is not required to validate th
>                                  ^^^^^^^^^
Did you forget a comma after a conjunctive/linking adverb?

Uncited references: [RFC6793].

Hi,

Thanks for this document.  My nits have already been pointed out by other ADs, hence my only non-blocking comment is on this text in section 4.1.

          1.  This condition SHOULD be enabled by default (it may be
              disabled by explicit configuration as described on the
              next list item (b.2.2)).

          2.  This condition MAY be disabled by explicit configuration
              on a BGP speaker.

I felt that the text "(it may be disabled by explicit configuration as described on the next list item (b.2.2))." is repetitive and unnecessary, and obvious by the next list item.  However, I'm happy to leave the handling of this comment to the authors discretion.

Regards,
Rob