[Idr] AD Review of draft-ietf-idr-bgp-flowspec-oid-12

[Alvaro Retana <[e-mail]>]

Dear authors:

Thank you for your work on this document.

In general, I think that this document is simple and straight forward.
However, I have some major concerns (see details inline) that should
be cleared up before proceeding.

Thanks!

Alvaro.



[Lines from idnits.]

...
4	Updates: 5575bis (if approved)                                J. Alcaide

[major] s/5575bis/rfc8955/g


...
15     Abstract

17	   This document describes a modification to the validation procedure
18	   defined for the dissemination of BGP Flow Specifications.  The
19	   dissemination of BGP Flow Specifications requires that the originator
20	   of the Flow Specification matches the originator of the best-match
21	   unicast route for the destination prefix embedded in the Flow
22	   Specification.  This allows only BGP speakers within the data
23	   forwarding path (such as autonomous system border routers) to
24	   originate BGP Flow Specifications.  Though it is possible to
25	   disseminate such Flow Specifications directly from border routers, it
26	   may be operationally cumbersome in an autonomous system with a large
27	   number of border routers having complex BGP policies.  The
28	   modification proposed herein enables Flow Specifications to be
29	   originated from a centralized BGP route controller.

[] rfc8955 and origination from ASBRs

Specifically on that point, the "originator" as defined in rfc8955 is
"reset" at the AS boundary.  Regardless of whether the Flow
Specification was originally generated at the ASBR or a centralized
route controller, the receiving eBGP speaker will consider its eBGP
peer to be the originator.

You would see the problem internal to the originating AS, unless the
controller also originates the routes internally.

The motivation as laid out in the Abstract is making assumptiona that
are not explained, resulting in lack of accuracy.  Please reword.
Note that many times "less is more" -- perhaps focus the Abstract on
what the draft is proposing: updating the validation rules for iBGP
and route server scenarios.


31	   This document updates RFC5575bis.

[major] s/This document updates RFC5575bis./This document updates the
validation procedure in rfc8955.


...
81	1.  Requirements Language

83	   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
84	   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
85	   document are to be interpreted as described in RFC 2119 [RFC2119].

[major] Use the rfc8174 template!


87	2.  Introduction

89	   [I-D.ietf-idr-rfc5575bis] defined a new BGP [RFC4271] capability that
90	   can be used to distribute traffic Flow Specifications amongst BGP
91	   speakers in support of traffic filtering.  The primary intention of
92	   [I-D.ietf-idr-rfc5575bis] is to enable downstream autonomous systems
93	   to signal traffic filtering policies to upstream autonomous systems.
94	   In this way, traffic is filtered closer to the source and the
95	   upstream autonomous system(s) avoid carrying the traffic to the
96	   downstream autonomous system only to be discarded.  [I-D.ietf-idr-
97	   rfc5575bis] also enables more granular traffic filtering based upon
98	   upper layer protocol information (e.g., protocol port numbers) as
99	   opposed to coarse IP destination prefix-based filtering.  Flow
100	   specification NLRIs received from a BGP peer are subject to validity
101	   checks before being considered feasible and subsequently installed
102	   within the respective Adj-RIB-In.

[]
OLD>
   [I-D.ietf-idr-rfc5575bis] defined a new BGP [RFC4271] capability that
   can be used to distribute traffic Flow Specifications amongst BGP
   speakers in support of traffic filtering.

NEW>
   [RFC8955] defines a BGP NLRI [RFC4271] that can be used to distribute
   traffic Flow Specifications amongst BGP speakers in support of traffic
   filtering.


104	   The validation procedure defined within [I-D.ietf-idr-rfc5575bis]
105	   requires that the originator of the Flow Specification NLRI matches
106	   the originator of the best-match unicast route for the destination
107	   prefix embedded in the Flow Specification.  This allows only BGP
108	   speakers within the data forwarding path (such as autonomous system
109	   border routers) to originate BGP Flow Specification NLRIs.  Though it
110	   is possible to disseminate such Flow Specification NLRIs directly
111	   from border routers, it may be operationally cumbersome in an
112	   autonomous system with a large number of border routers having
113	   complex BGP policies.

[] See the related comment in the Abstract.

115	   This document describes a modification to the [I-D.ietf-idr-
116	   rfc5575bis] validation procedure allowing Flow Specification NLRIs to
117	   be originated from a centralized BGP route controller within the
118	   local autonomous system that is not in the data forwarding path.
119	   While the proposed modification cannot be used for inter-domain
120	   coordination of traffic filtering, it greatly simplifies distribution
121	   of intra-domain traffic filtering policies within an autonomous
122	   system which has a large number of border routers having complex BGP
123	   policies.  By relaxing the validation procedure for iBGP, the
124	   proposed modification allows Flow Specifications to be distributed in
125	   a standard and scalable manner throughout an autonomous system.

[] The motivation presented ("allowing Flow Specification NLRIs to be
originated from a centralized BGP route controller") distracts a
little from the general improved validation for iBGP, and the use of
route servers which are not mentioned at all in the Introduction.


127	3.  Motivation
...
136	   In the case of inter-domain traffic filtering, the Flow Specification
137	   originator at the egress border routers of an AS (e.g.  RTR-D and
138	   RTR-E of ASN1 in figure 1) matches the eBGP neighbor that advertised
139	   the longest match destination prefix (see RTR-F and RTR-G
140	   respectively in figure 1).  Similarly, at the ingress border routers
141	   of ASN (see RTR-A and RTR-B of ASN1 in figure 1), the Flow
142	   Specification originator matches the egress iBGP border routers that
143	   had advertised the unicast route for the best-match destination
144	   prefix (see RTR-D and RTR-E respectively in figure 1).  This is true
145	   even when ingress border routers select paths from different egress
146	   border routers as best path based upon IGP distance.  For example, in
147	   figure 1:

149	      RTR-A chooses RTR-D's path as best

151	      RTR-B chooses RTR-E as the best path

[nit] s/figure 1/Figure 1/g


[nit] There are two cases described -- separate them into their own
paragraph.  The second one would start with "Similarly...".


[minor] The concept of "ingress border routers" being associated with
RTR-A/RTR-B doesn't sound right as neither is at a border.  This same
phrase is also used later on.


[minor] s/ASN/AS/g


[major] "best path"  There is no such concept defined in rfc4271.
Please use "route" and "best route" instead.


...
191	   It is highly desirable that the mechanisms exist to protect each ASN
192	   independently from network security attacks using the BGP Flow
193	   Specification NLRI for intra-domain purposes only.  Network operators
194	   often deploy a dedicated Security Operations Center (SOC) within
195	   their ASN to monitor and detect such security attacks.  To mitigate
196	   attacks within a domain (AS or group of ASes), operators require the
197	   ability to originate intra-domain Flow Specification NLRIs from a
198	   central BGP route controller that is not within the data forwarding
199	   plane.  In this way, operators can direct border routers within their
200	   ASN with specific attack mitigation actions (drop the traffic,
201	   forward to a clean-pipe center, etc.).

[nit] s/that the mechanisms exist/that mechanisms exist


[minor] In this paragraph you mix intra-domain to mean "each AS
independently", and an "AS or group".  Even if a domain ("common
administrative control" is more descriptive) is made up of multiple
ASes, the eBGP-based validation (not the iBGP-based enhancement from
this document) would be applied.  Extending the concept of domain to
multiple ASes only adds confusion.  Is there a need to even mention
it?


203	   To originate a Flow Specification NLRI, a central BGP route
204	   controller must set itself as the originator in the Flow
205	   Specification NLRI.  This is necessary given the route controller is
206	   originating the Flow Specification rather than reflecting it, and to
207	   avoid the complexity of having to determine the egress border router
208	   whose path was chosen as the best in each of the ingress border
209	   routers.  Thus, it is necessary to modify step (b) of the [I-D.ietf-
210	   idr-rfc5575bis] validation procedure such that an iBGP peer that is
211	   not within the data forwarding plane may originate Flow Specification
212	   NLRIs.

[major] "a central BGP route controller must set itself as the
originator"  It is not clear to me whether you're trying to specify an
action or simply stating a fact: the originator originates.  In either
case, there is some redundancy in the text.

OLD>
   To originate a Flow Specification NLRI, a central BGP route
   controller must set itself as the originator in the Flow
   Specification NLRI.  This is necessary given the route controller is
   originating the Flow Specification rather than reflecting it, and to
   avoid the complexity of having to determine the egress border router
   whose path was chosen as the best in each of the ingress border
   routers.

NEW>
   A central BGP route controller that originates a Flow Specification
   NLRI should be able to avoid the complexity of having to determine
   the egress border router whose path was chosen as the best for each
   of its neighbors.


214	4.  Revised Validation Procedure

216	4.1.  Revision of Route Feasibility

218	   Step (b) of the validation procedure specified in [I-D.ietf-idr-
219	   rfc5575bis], section 6 is redefined as follows:

221	   b) One of the following conditions MUST hold true:

223	      1.  The originator of the Flow Specification matches the
224	          originator of the best-match unicast route for the destination
225	          prefix embedded in the Flow Specification (This is the unicast
226	          route with the longest possible prefix length covering the
227	          destination prefix embedded in the Flow Specification).

[nit] s/(This/(this


229	      2.  The AS_PATH attribute of the Flow Specification does not
230	          contain AS_SET and/or AS_SEQUENCE segments.

[major] Is an AS_CONFED_SET segment ok?  I'm assuming that it isn't.
Instead of indicating what the AS_PATH doesn't contain, it may be
easier/clearer to indicate what it can.

Suggestion>
   The AS_PATH attribute of the Flow Specification is empty or contains
   a single AS_CONFED_SEQUENCE sequence [rfc5065].


[minor] To match the rfc8955 style, please convert the following
bullets into paragraphs.

232	          1.  This condition SHOULD be enabled by default.  This default
233	              behavior should validate an empty AS_PATH.

[major] "This condition SHOULD be enabled by default."  Given that the
condition is required, and that eBGP routes won't meet it, I don't
know what you mean here.  Please clarify.


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

[minor] Please indicate the reasons/circumstances when an operator
might consider disabling it.  There's some related text below.


238	          3.  As an exception to this rule, a given AS_PATH with AS_SET
239	              and/or AS_SEQUENCE segments MAY be validated by policy.

[minor] Group this text with the related text below.


[minor] s/ a given AS_PATH with AS_SET and/or AS_SEQUENCE segments/ a
given AS_PATH


241	   Explanation:

243	      In this context, an empty AS_PATH means that it does not have
244	      AS_SET and/or AS_SEQUENCE segments, and local domain means the
245	      local AS [RFC4271] or the local confederation of ASes (in the case
246	      that the local AS belongs to a confederation of ASes [RFC5065]).
247	      Thus, receiving a Flow Specification with an empty AS_PATH
248	      indicates that the Flow Specification was originated inside the
249	      local domain.

[major] An empty AS_PATH is one with no segments, so mapping that to a
"local domain" that could map to a confederation is confusing because
the AS_PATH is then not empty.

Suggestion>
   In this context, a local domain includes the local AS or the local
   confederation [RFC5065].  Receiving either an empty AS_PATH or one
   with an AS_CONFED_SEQUENCE indicates that the Flow Specification was
   originated inside the local domain.


...
256	      Disabling the new condition above (b.2.2) may be a good practice
257	      when the operator knows with certainty that there is not a Flow
258	      Specification originated inside the local domain.

[nit] s/there is not a Flow Specification originated inside the local
domain./a Flow Specification will not be originated inside the local
domain.


...
265	4.2.  Revision of AS_PATH Validation

267	   [I-D.ietf-idr-rfc5575bis] states:

269	   o  BGP implementations MUST also enforce that the AS_PATH attribute
270	      of a route received via the External Border Gateway Protocol
271	      (eBGP) contains the neighboring AS in the left-most position of
272	      the AS_PATH attribute.

[nit] Take out the "o" as it is not in rfc8955.


274	   This rule prevents the exchange of BGP Flow Specification NLRIs at
275	   Internet exchanges with BGP route servers.  Therefore, this document
276	   also redefines the [I-D.ietf-idr-rfc5575bis] AS_PATH validation
277	   procedure referenced above as follows:

[minor] Add a reference to rfc7947.


...
298	      Comparing only the last ASes added is sufficient for eBGP learned
299	      Flow Specification NLRIs.  Requiring a full AS_PATH match would
300	      limit origination of inter-domain Flow Specifications to the
301	      origin AS of the best-match unicast route for the destination
302	      prefix embedded in the Flow Specification only.  As such, a full
303	      AS_PATH validity check may prevent transit ASes from originating
304	      inter-domain Flow Specifications, which is not desirable.

[] Yes, a full AS_PATH check may not be practical.  By the same token,
not checking it allows anyone (even unintended/rogue ASes) the ability
to originate Flow Specifications.   This is not a new threat in BGP,
but one that is not called out in rfc8955.  Since you're talking about
this here, it would be good to point out the potential issue.


306	      Redefinition of this AS_PATH validation rule for a Flow
307	      Specification does not mean that the original rule in [I-D.ietf-
308	      idr-rfc5575bis] cannot be enforced as well.  Its enforcement
309	      remains optional per [RFC4271] section 6.3.  That is, we can
310	      enforce the first AS in the AS_PATH to be the same as the neighbor
311	      AS for any address-family route (including a Flow Specification).

[major] Both rfc5575 and rfc8955 mentioned that the enforcement is
required "for security reasons".  I'm sure the question will come up
about the potential security holes that are now opened if the
enforcement is not performed.  Please add (preferably in the Security
Considerations section) text including any recommendations on when the
enforcement can/should be performed.

For the general eBGP case, for example, there is the possibility that
the left-most ASN matches for both the FS/unicast routes, but that it
doesn't correspond to the peer AS.  I believe this is a case where an
additional vulnerability is introduced --- but also one that can be
mitigated by recommending/requiring the enforcement in this type of
cases (with appropriate caveats).


[nit] s/we can enforce/a BGP speaker can enforce


313	      Using the new rule to validate a Flow Specification received from
314	      an Internal Border Gateway Protocol (iBGP) peer is out of the
315	      scope of this document.  Note that in most scenarios such
316	      validation would be redundant.

[minor] The new text explicitly talks about eBGP, so I think this
paragraph is unnecessary.


318	      Using the new rule to validate a Flow Specification route received
319	      from an External Border Gateway Protocol (eBGP) peer belonging to
320	      the same local domain (in the case that the local AS belongs to a
321	      confederation of ASes) is out of the scope of this document.  Note
322	      that although it's possible, its utility is dubious.

[nit] s/(in the case that the local AS belongs to a confederation of
ASes)/(in the case of a confederation)


[minor] Why can't the same check be applied?  The text above doesn't
talk about segments so it should be able to apply.


[minor] As for the value, you seem to be assuming that just because
the confederation is in the same administrative domain than there
won't be configuration errors or rogue routers.  I believe that is a
dangerous assumption.


324	5.  Other RFC5575bis Considerations

[major] This section is unnecessary because rfc6793 Updated rfc4271,
so everything in it is already assumed to be part of the base BGP
spec.  Please delete to avoid redundancy and potential confusion.


...
339	6.  Topology Considerations

341	   [I-D.ietf-idr-rfc5575bis] indicates that the originator may refer to
342	   the originator path attribute (ORIGINATOR_ID) or (if the attribute is
343	   not present) the transport address of the peer from which we received
344	   the update.  If the latter applies, a network should be designed so
345	   it has a congruent topology.

[nit] s/we received the update/the router received the update


[minor] Please explain (ideally here) what a "congruent topology" is.
There is an example of a non-congruent topology below, but no specific
definition.  It may not be clear to everyone that even if the BGP
speaker is the same, the topology may still not be congruent.


...
354	      Consider the following scenarios without the second condition
355	      (b.2) being added to the validation procedure:

357	      1.  Consider a topology with two BGP speakers with two peering
358	          sessions between them, one for unicast and one for Flow
359	          Specification.  This is a non-congruent topology.  Let's
360	          assume that the ORIGINATOR_ID attribute was not received (e.g.
361	          a route reflector receiving routes from its clients).  In this
362	          case, the Flow Specification validation procedure will fail
363	          because of the first condition (b.1).

365	      2.  Consider a topology with a BGP speaker within a confederation
366	          of ASes, inside local AS X.  ORIGINATOR_ID attribute is not
367	          advertised within the local domain.  Let's assume the Flow
368	          Specification route is received from peer A and the best-match
369	          unicast route is received from peer B.  Both peers belong in
370	          local AS Y.  Both AS X and AS Y belong to the same local
371	          domain.  The Flow Specification validation procedure will also
372	          fail because of the first condition (b.1).

[nit] s/ORIGINATOR_ID attribute/The ORIGINATOR_ID attribute


[nit] s/belong in/belong to the


374	      In the examples above, if Flow Specifications are originated in
375	      the same local domain, AS_PATH will not contain AS_SET and/or
376	      AS_SEQUENCE segments.  When the second condition (b.2) in the
377	      validation procedure is used, the validation procedure will pass.
378	      Thus, non-congruent topologies are supported if the Flow
379	      Specification is originated in the same local domain.

[minor] s/ AS_PATH will not contain AS_SET and/or AS_SEQUENCE
segments./ the AS_PATH will be empty or contain just an
AS_CONFED_SEQUENCE segment/


381	      Even when the second condition (b.2) is used in the validation
382	      procedure, a Flow Specification originated in a different local
383	      domain needs a congruent topology.  AS_SEQUENCE is not empty and
384	      the first condition (b.1) in the validation procedure needs to be
385	      evaluated.  Because transport addresses for Flow Specification and
386	      unicast routes are different, the validation procedure will fail.

[minor] s/AS_SEQUENCE is not empty/The AS_PATH is not empty


388	      This is true both across domains and within domains.  Consider
389	      both cases:

[?] "This is true..."   What is this?


391	      *  Consider the first example.  If the Flow Specification route is
392	         originated in another AS, the validation procedure will fail
393	         because the topology is non-congruent within the domain.

[] Maybe because "this" (above) is not clear, it seems to me that this
is just a repetition of #1 above.


...
404	8.  Security Considerations

406	   No new security issues are introduced by relaxing the validation
407	   procedure for IBGP learned Flow Specifications.  With this proposal,
408	   the security characteristics of BGP Flow Specifications remain
409	   equivalent to the existing security properties of BGP unicast
410	   routing.

[nit] s/IBGP/iBGP


[major] Note that the Security Considerations in rfc8955 talk about
having equivalent security properties *only* if the validation
procedures are applied.  If the updated validation procedures are
followed, then a similar claim can be made here.

Suggestion>

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

   The security considerations discussed in [rfc8955] apply to this
   specification as well.


The only caveat is in the elimination of the enforce-first-as
requirement.  Something must be said about that in this section.


412	   BGP updates learned from iBGP peers are trusted so the Traffic Flow
413	   Specifications contained in BGP updates are trusted.  Therefore it is
414	   not required to validate that the originator of an intra-domain
415	   Traffic Flow Specification matches the originator of the best-match
416	   unicast route for the flow destination prefix.  This proposal
417	   continues to enforce the validation Procedure for eBGP learned
418	   Traffic Flow Specifications, as per [I-D.ietf-idr-rfc5575bis] rules.
419	   In this way, the security properties of [I-D.ietf-idr-rfc5575bis] are
420	   maintained such that an EBGP peer cannot cause a denial-of-service
421	   attack by advertising an inter-domain Flow Specification for a
422	   destination prefix that it does not provide reachability information
423	   for.

[major] "iBGP peers are trusted"   This is a flawed assumption because
it doesn't take into account the possibility of a rogue iBGP speaker.
Consider the case where an attacker takes control of a router and
creates (or limits) Flow Specifications -- not checking would open the
network up to any router becoming an attack point.

Yes, I understand that this is not a new vulnerability introduced in
this draft.  However, the type of risk (creating a Flow Specification)
is new to this work and a direct result.

Because the origin check won't happen for iBGP, you should mention the
risk clearly.


[minor] Start a separate paragraph.

OLD>
   This proposal continues to enforce the validation Procedure for
   eBGP learned Traffic Flow Specifications, as per
   [I-D.ietf-idr-rfc5575bis] rules. In this way, the security
   properties of [I-D.ietf-idr-rfc5575bis] are maintained such that
   an EBGP peer cannot cause a denial-of-service attack by advertising
   an inter-domain Flow Specification for a destination prefix that it
   does not provide reachability information for.

NEW>
   The changes in Section 4.1 don't affect the validation procedures
   for eBGP-learned routes.


[End of Review]