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

From: Alvaro Retana <[email-masked]>
Date: Thu, 12 Dec 2019 11:38:57 -0800

On November 4, 2019 at 2:23:41 PM, Christoph Loibl
([email-masked](mailto:[email-masked])) wrote:

Christoph:

Hi!

> On behalf of the draft-ietf-idr-rfc5575bis authors I uploaded a new version
> (-18) of the draft:
>
> https://datatracker.ietf.org/doc/draft-ietf-idr-rfc5575bis/
>
> This is the result of the improvements that we worked on after your review.
> Some of the structural changes lead to big changes if you run the -17/-18
> thru rfcdiff. However, we documented all the changes and the entire process
> on Github. All issues and the related changes can be reviewed there (hoping
> to move it to an IETF/IDR Org. account soon):
>
> https://github.com/stoffi92/rfc5575bis
>
> Additionally, please see the below comments that we added to your review text
> (for documentation on the IDR list). For all issues you raised we added links
> to the issues tracked on Github. For the major issues we also added a comment
> how those have been resolved. We think that the document has very much
> improved and benefited from your review and the process that followed.

I went through the comments, the explanations, the GitHub issues, the
diffs... :-)  Thank you for being so detailed!! The document is
looking a lot better!

I responded to some of your comments below; I'm also including in-line
comments on the -18 version.  In some cases it seemed easier/clearer
to comment on the resulting text.

There is only one significant item remaining that we should resolve
before moving the document forward: the handling of unknown
components; take a look at the comments in §11 (of -18).

Thanks!

Alvaro.

[Part I: Comments on your review responses.]

...
> 227 2. Definitions of Terms Used in This Memo
> ...
> 233 Loc-RIB - Local RIB.
>
> [major] This simple definition doesn't match the one in §1.1/rfc4271.
>
> <-- *** Authors ***
> Tracked via issue #11: https://github.com/stoffi92/rfc5575bis/issues/11
> Commit mention: https://github.com/stoffi92/rfc5575bis/commit/b4950ef76c347a67a8e31e0eff4d1433a00e34b8
>
> Added the definition to to match RFC4271
> -->

[minor] Please add a reference to rfc4271 there too.

...
> 281 4. Dissemination of IPv4 FLow Specification Information
> ...
> 287 This NLRI information is encoded using MP_REACH_NLRI and
> 288 MP_UNREACH_NLRI attributes as defined in [RFC4760]. Whenever the
> 289 corresponding application does not require Next-Hop information, this
> 290 shall be encoded as a 0-octet length Next Hop in the MP_REACH_NLRI
> 291 attribute and ignored on receipt.
>
> [minor] s/Next-Hop/Next Hop rfc4760 uses "Next Hop"
>
> [nit] "...shall be encoded as a 0-octet length Next Hop in the MP_REACH_NLRI attribute and ignored on receipt." What is ignored? The Next Hop? If it doesn't exist (length = 0), then it can't be ignored... Perhaps delete " and ignored on receipt".
>
> <-- *** Authors ***
> Tracked via issue #14: https://github.com/stoffi92/rfc5575bis/issues/14
> Commit mention: https://github.com/stoffi92/rfc5575bis/commit/a89200d70c7e661c519bf4379ef43fee817f1d71
> -->

[major] This is the new text from -18:

255	   This NLRI information is encoded using MP_REACH_NLRI and
256	   MP_UNREACH_NLRI attributes as defined in [RFC4760].  Whenever the
257	   corresponding application does not require Next Hop information, this
258	   shall be encoded as a 0-octet length Next Hop in the MP_REACH_NLRI
259	   attribute (if a non 0-octet Next Hop is present it should be ignored
260	   on receipt).

I think I get it: the Next Hop is not needed -- so it is either not
present (0-octet length) or ignored.  Is that it?

What is confusing me is the part about "Whenever the corresponding
application does not require Next Hop information", because it sounds
as if there are cases when the Next Hop is needed...but then it is
ignored anyway.

I looked in rfc4760/rfc4271, but they both talk about forwarding to
the Next Hop, and not about cases where it is not needed (or at least
I didn't find that).  IOW, the use defined here is specific to the FS
AFI/SAFI pair.  The specification of the use (or not) or the Next Hop
should then be a lot clearer, and Normative.

SUGGESTION>
   This NLRI information is encoded using MP_REACH_NLRI and MP_UNREACH_NLRI
   attributes as defined in [RFC4760].  When advertising Flow Specifications,
   the Length of Next Hop Network Address SHOULD be set to 0.  The Network
   Address of Next Hop field MUST be ignored.

...
> 325 4.2. NLRI Value Encoding
...
> 338 Flow Specification components must follow strict type ordering by
> 339 increasing numerical order. A given component type may (exactly
> 340 once) or may not be present in the specification. If present, it
> 341 MUST precede any component of higher numeric type value.
>
> [major] What should happen if a component appears more than once?
>
> [major] What should happen if the order is not maintained?
>
> <-- *** Authors ***
> Tracked via issue #19: https://github.com/stoffi92/rfc5575bis/issues/19
>
> Basically if the NLRI is not encoded to specification we have a malformed
> NLRI (most probably the BGP NLRI parser will throw an exception on the
> implementation). I am not sure if this needs to be made explicit. - But I can
> make this explicit.
> -->

The question is: what should the implementation do in response to the
exception?  Should it reset the session?  Should it ignore the Update?
 Should it disable the AFI/SAFI?  Should it only consider the first
component (if it appears more than once)?  Something else?

I looked at rfc4760/rfc7606, but didn't find anything that looked to
me explicitly as corresponding to a Malformed NLRI...but rfc4760 does
specify "AFI/SAFI disable":

   If a BGP speaker receives from a neighbor an UPDATE message that
   contains the MP_REACH_NLRI or MP_UNREACH_NLRI attribute, and if the
   speaker determines that the attribute is incorrect, the speaker MUST
   delete all the BGP routes received from that neighbor whose AFI/SAFI
   is the same as the one carried in the incorrect MP_REACH_NLRI or
   MP_UNREACH_NLRI attribute. For the duration of the BGP session over
   which the UPDATE message was received, the speaker then SHOULD ignore
   all the subsequent routes with that AFI/SAFI received over that
   session.

The result is then that FS (including the rules/actions already
received) would now not be used.  Personal opinion: Disabling the
AFI/SAFI can be worse than resetting the session because there's no
indication to the sender that anything is wrong.

If AFI/SAFI disable is the expected behavior for both out-of-order and
duplicate components, then nothing else is needed.  A note in §10
would be nice but not necessary.

...
> 542 4.2.12. Type 12 - Fragment
...
> 548 0 1 2 3 4 5 6 7
> 549 +---+---+---+---+---+---+---+---+
> 550 | 0 | 0 | 0 | 0 |LF |FF |IsF|DF |
> 551 +---+---+---+---+---+---+---+---+
...
> [major] The operation is not specified. Is this also an (operator,bitmask)
> pair, or just 8 bits indicating the values? Can multiple bits be set at the
> same time? What fields in the IP header do these map to?
>
> <-- *** Authors ***
> Tracked via issue #40: https://github.com/stoffi92/rfc5575bis/issues/40
> Commit mention: https://github.com/stoffi92/rfc5575bis/commit/f078e919842330d670bc195276ead7bdc05a3351
>
> Added the text to the bit-descriptions and IP-header fields - And an example
> using the bitmask operator.
> -->

The example is for a "matching packet with DF bit set or First
Fragments", which answers the question above about whether multiple
bits can be set -- yes.  What is still not specified is that in the
case of multiple bits set, an OR operation is expected.

...
> 742 6. Validation Procedure
...
> 760 A Flow Specification NLRI must be validated such that it is
> 761 considered feasible if and only if all of the below is true:
>
> [major] There is no Normative language above, but I think there is a
> contradiction of sorts with the new text below ("Rule a) MAY be relaxed...").
> The introductory text to the rules is "must be...considered feasible if and
> only if all of the below is true", which sounds very strict and
> specific...but then the Normative exception comes in ("MAY be relaxed...rules
> b) and c)...MUST be disregarded") saying that it doesn't matter. Please
> reword...perhaps something like: "If a destination is present...a Flow
> Specification MUST be validated this way...otherwise..."
>
> <-- *** Authors ***
> Tracked via issue #56: https://github.com/stoffi92/rfc5575bis/issues/56
> Commit mention: https://github.com/stoffi92/rfc5575bis/commit/d96a496295021f2aab0332bd2120920547f71c56
>
> Changed the text to refer to the default behaviour of the implementation
> (MUST validate). However this MAY be relaxed by configuration.
> -->

I'm still not happy with the contradiction: MUST means "an absolute
requirement" (no exceptions!), but then we say MAY, which makes the
MUST optional...even with the "by default"...   Please s/MUST/SHOULD
 The MAY will then explain the "valid reason" to not...

...
> 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] 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.
...
> <-- *** Authors ***
> Tracked via issue #60: https://github.com/stoffi92/rfc5575bis/issues/60
>
...
> In the case of IXP indeed this sometime may break in the cases where RS is
> used. If you even in the IX (which is L2 typically) use direct EBGP session
> it all works ok. If you use session to RS and such RS by design is
> transparent in order to not extend the AS_PATH length that enforce first AS
> should be disabled. I think we could say that use of flow spec when peering
> with RS in IXP env is much less secure as ASN spoofing in the AS-PATH can
> occur. It is advised to apply strict inbound route policy in such scenarios.
> -->

Please add some text in §13 about it -- something similar to the
explanation above would be great.

...
> 904 7.3. Traffic-action (traffic-action) sub-type 0x07
...
> 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?
...
> <-- *** Authors ***
> Tracked via issue #72: https://github.com/stoffi92/rfc5575bis/issues/72
>
> Minor1:
> The terminal action applies to filter rules only, not to the filter actions
> (extended communties). Given we have 3 FS filters that match the traffic
> (they are sorted first):
> FS1) The first has T=0 and a traffic-rate-bytes action
> FS2) The second has only a redirect-action
> FS3) The third has only a traffic-rate-packets action
>
> Evaluation takes place according to the sorted filters FS1 first, then
> FS2(because of T=0 in FS1). However, because FS2 has no T=0 set evaluation
> stops here and the resulting action is: traffic-rate-bytes and redirect.

Right...rules vs actions...   Please add a reference to §5.1 (Ordering
of Flow Specifications) to make it clearer for others.

The example helps, but it is wrong.  The definition says that when the
"bit is set, the traffic filtering engine will apply any subsequent
filtering rules"...so the action should result in only
traffic-rate-bytes.

...
> [major] The range is defined in the registry as "0x80-0x8f Reserved for
> Experimental Use". According to rfc8126, "IANA does not record assignments
> from registries or ranges with this policy".
>
> I don't know why 0x80 was the first value chosen; it looks like it was first
> used in draft-marques-idr-flow-spec-01 (2004), while the corresponding
> Extended Communities draft (draft-ietf-idr-bgp-ext-communities-07) already
> indicated that the range was for Experimental Use. I guess just lack of
> sync... But then I also don't understand how/why IANA ended up with the
> information in the Registry....maybe because the sub-types are not for
> Experimental Use -- hmmm, which sounds contradictory to me.
>
> The reason/history doesn't matter anymore, but the current use does. The
> mechanism described in this document is clearly not experimental. Given that
> changing the Type values is not an option because of the deployed base,
> etc.., then I think we should clean up the Registry and move 0x80-0x82 from
> the Experimental Use range to the FCFS range. This change would mean an
> Update to rfc7153.
>
> To simplify the process, the Update can be done in this document. However, I
> think that there's some confusion with these types apparently being
> associated only with Flow Specifications, when they are labeled as Generic.
> IOW, ideally the issue would be corrected independently...
>
> <-- *** Authors ***
> Tracked via issue #99: https://github.com/stoffi92/rfc5575bis/issues/99
>
> This is not to be solved within this document. I note that I should propose a
> update to RFC7153 to cleanup the registry independently. - As noticed moving
> the values around is not an option.
> -->

So, are you working on this already?  ;-)

...
> 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.
>
> 1317 As long as traffic filtering rules are restricted to match the
> 1318 corresponding unicast routing paths for the relevant prefixes, the
> 1319 security characteristics of this proposal are equivalent to the
> 1320 existing security properties of BGP unicast routing. However, this
> 1321 document also specifies traffic filtering actions that may need
> 1322 custom additional verification on the receiver side. See Section 14.
>
> [major] In general, Flow Specifications have a one-AS-hop propagation model,
> right? This means that the security properties are different because (1)
> unicast routing propagates multiple hops, and (2) the intent of the "Route
> Origin ASN" (rfc6811) is not reflected in the request to rate-limit, or even
> drop (!) traffic to a destination. Yes, it is all based on trust...but
> different. For example, Origin Validation wouldn't be available for Flow
> Specifications.
>
> <-- *** Authors ***
> Tracked via issue #104: https://github.com/stoffi92/rfc5575bis/issues/104
>
> FS is never of one-AS-hop propagation model.
> To the very question of the Origin Validation it is all integrated by
> original design. That means that if I must validate received FS routes
> against unicast prefix and that unicast prefix was already Origin Validated
> that implies that FS is also Origin Validated without adding any new ROAs.
> The problem starts when validation is getting disabled as this has avalanche
> effect for entire security model.
> -->

No, the FS cannot be validated without adding ROAs.  For example,
consider prefix-x advertised by AS A to AS B and then to AS C; both B
and C can validate that AS A is the valid origin using a ROA.  AS B
then sends a FS to C asking to drop the traffic to prefix-x.  Note
that the FS has an Origin AS of B, while the ROA is validating an
origin AS of A for prefix-x.

While I agree that the FS could traverse multiple AS hops, it is not
always the case that it would.

All this is moot anyway because ROAs are only defined for the
IPv4/IPv6 AFs (rfc6482).

In any case, if I look really close, the text says that "the security
characteristics of this proposal are equivalent to the existing
security properties of BGP unicast routing" -- equivalent, not the
same.  I'll let this go. :-)

[Part II: Review of -18.]

[Line numbers from id-nits.]

...
17	Abstract

19	   This document obsoletes both RFC5575 and RFC7674.

[style nit] Usually this statement is placed at the end of the
Abstract/Introduction.  Personally, I think it makes it easier to
read...

[minor] The Abstract shouldn't have references.  In the xml: s/<xref
target="RFC5575" /> and <xref target="RFC7674" />/RFC 5575 and RFC
7674

21	   This document defines a Border Gateway Protocol Network Layer
22	   Reachability Information (BGP NLRI) encoding format, that can be used
23	   to distribute traffic Flow Specifications.  This allows the routing
24	   system to propagate information regarding more specific components of
25	   the traffic aggregate defined by an IP destination prefix.

[nit] No comma before "that".   s/encoding format, that/encoding format that

...
32	   Additionally, it defines two applications of that encoding format:
33	   one that can be used to automate inter-domain coordination of traffic
34	   filtering, such as what is required in order to mitigate
35	   (distributed) denial-of-service attacks, and a second application to
36	   provide traffic filtering in the context of a BGP/MPLS VPN service.
37	   Other applications (ie. centralized control of traffic in a SDN or
38	   NFV context) are also possible.  Other drafts specify IPv6, MPLS
39	   addresses, L2VPN addresses, and NV03 encapsulation of IP addresses as
40	   Flow Specification extensions.

[minor] (Related to the last sentence.) As I mentioned before, the
Introduction only points at the IPv6 draft, but it could benefit from
pointers to the other drafts mentioned here.  Given the recent
discussion about extensibility, it may be better to simply not be
specific about what other drafts may specify:

SUGGESTION>
   Other documents may specify Flow Specification extensions.

42	   The information is carried via the BGP, thereby reusing protocol
43	   algorithms, operational experience, and administrative processes such
44	   as inter-provider peering agreements.

[nit] s/carried via the BGP/carried via BGP

...
120	1.  Introduction
...
154	   A Flow Specification received from an external autonomous system will
155	   need to be validated against unicast routing before being accepted
156	   (Section 6).  The flow specification received from an internal BGP
157	   peer within the same autonomous system (per [RFC4271]) is assumed to
158	   have been validated prior to transmission within the iBGP mesh of an
159	   autonomous system.  If the aggregate traffic flow defined by the
160	   unicast destination prefix is forwarded to a given BGP peer, then the
161	   local system can install more specific Flow Specifications that may
162	   result in different forwarding behavior, as requested by this system.

[nit] s/flow specification (and other case variations)/Flow Specification/g

[nit] s/(per [RFC4271])/[RFC4271]

...
249	4.  Dissemination of IPv4 FLow Specification Information

[nit] s/FLow/Flow

...
295	4.2.  NLRI Value Encoding
...
311	   All combinations of components within a single Flow Specification are
312	   allowed.  However, some combinations cannot match any packets (ie.
313	   "ICMP Type AND Port" will never match any packets), and thus SHOULD
314	   NOT be propagated by BGP.

[nit] s/ie./e.g.

...
607	4.3.1.  Example 1
...
620	          +-------+------------+------------------------------+
621	          | Value |            |                              |
622	          +-------+------------+------------------------------+
623	          |  0x0b | length     | 11 octets (len<240 1-octet)  |
624	          |  0x01 | type       | Type 1 - Destination Prefix  |
625	          |  0x18 | length     | 24 bit                       |
626	          |  0xc0 | prefix     | 192                          |
627	          |  0x00 | prefix     | 0                            |
628	          |  0x02 | prefix     | 2                            |
629	          |  0x03 | type       | Type 3 - IP Protocol         |
630	          |  0x81 | numeric_op | end-of-list, value size=1, = |
631	          |  0x06 | value      | IP Protocol 6 = TCP          |
632	          |  0x04 | type       | Type 4 - Port                |
633	          |  0x81 | numeric_op | end-of-list, value size=1, = |
634	          |  0x19 | value      | 25                           |
635	          +-------+------------+------------------------------+

[minor] Table 1 defines equal as "== (equal)" -- just to be
consistent.  s/=/==/g

...
959	7.3.  Traffic-action (traffic-action) sub-type 0x07
...
986	   o  Traffic Action Field: Other Traffic Action Field (see Section 12)
987	      bits unused in this specification.

[major] How should unknown bits be treated?  In the last version, the
text read: "should always be set to 0 by the originator and not be
evaluated by the receiving BGP speaker."  Please add something like
that.

...
1047	7.6.  Interaction with other Filtering Mechanisms in Routers

[minor] Perhaps: s/other Filtering/Custom Filtering

1049	   Implementations SHOULD provide mechanisms that map an arbitrary BGP
1050	   community value (normal or extended) to Traffic Filtering Actions
1051	   that require different mappings in different systems in the network.
1052	   For instance, providing packets with a worse-than-best-effort, per-
1053	   hop behavior is a functionality that is likely to be implemented
1054	   differently in different systems and for which no standard behavior
1055	   is currently known.  Rather than attempting to define it here, this
1056	   can be accomplished by mapping a user-defined community value to
1057	   platform-/network-specific behavior via user configuration.

[] Moving this paragraph to its own section is an improvement.

[major] However, the Normative specification still bothers me because
the text is not specific enough for implementations to
interoperate...which I know is precisely the point (no standard
behavior), so Normative language should not be used.   Please
s/SHOULD/should  (to take the text back to what rfc5575 says)

...
1080	8.  Dissemination of Traffic Filtering in BGP/MPLS VPN Networks
...
1092	   The NLRI format for this address family consists of a fixed-length
1093	   Route Distinguisher field (8 bytes) followed by the Flow
1094	   Specification NLRI value Section 4.2.  The NLRI length field shall
1095	   include both the 8 bytes of the Route Distinguisher as well as the
1096	   subsequent Flow Specification NLRI value.  The resulting encoding is
1097	   shown in Figure 7.

[minor] s/Flow Specification NLRI value Section 4.2./Flow
Specification NLRI value (Section 4.2).

...
1137	10.  Error-Handling

[nit] s/Error-Handling/Error Handling

1139	   Error handling according to [RFC7606] SHOULD apply to this
1140	   specification.

[major] rfc7606 already applies...there's no need to Normatively say
it here.  And, BTW, then wound it not apply?  IOW, why is that
"SHOULD" not a "MUST"?

NEW (suggestion): Error handling according to [RFC7606] and [RFC4760]
applies to this specification.

1142	   This document introduces Traffic Filtering Action Extended
1143	   Communities.  Malformed Traffic Filtering Action Extended Communities
1144	   in the sense of [RFC7606] Section 7.14. are Extended Community values
1145	   that cannot be decoded according to Section 7 of this document.

[] Please see my comment (above) in reply to your comments related to
§4.2 (NLRI Value Encoding).

1147	11.  Future NLRI Extensions

1149	   Future Flow Specification extensions may introduce new Flow
1150	   Specification components.  In order to facilitate such extensions of
1151	   the Flow Specification NLRI, in addition to the cases described in
1152	   [RFC7606], if BGP encounters an unknown Flow Specification component
1153	   in an UPDATE message, it SHOULD also treat this message as Treat-as-
1154	   withdraw as specified in [RFC7606] Section 2.

[minor] This paragraph describes an error condition, so maybe it belongs in §10.

[major] rfc7606/§3 reads:

  j.  Finally, we observe 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 -- what
      this entails is discussed in more detail in Section 5.  If this
      is not possible, the procedures of [RFC4271] and/or [RFC4760]
      continue to apply, meaning that the "session reset" approach (or
      the "AFI/SAFI disable" approach) MUST be followed.

If an unknown Flow Specification component exists, then the entire
NLRI cannot be "successfully parsed"...which results in not being able
to use treat-as-withdraw.  The text above leaves us with AFI/SAFI
disable, which is not extension-friendly.

rfc7606/§5.4 talks about Typed NLRIs, such as in mVPN/EVPN, and
concludes by saying:

   A BGP speaker advertising support for such a typed address family
   MUST handle routes with unrecognized NLRI types within that address
   family by discarding them, unless the relevant specification for that
   address family specifies otherwise.

The FS NLRI is not a Typed NLRI in the same sense that the examples
from rfc7606 are: the NLRI encoding starts with "Route Type".  But,
because the NLRI is basically a set of components, *with Types*, I
think we could make the case that the FS NLRI is a Typed NLRI and that
the paragraph above applies to it...which would result in being able
to ignore/discard an UPDATE with an unknown component (vs AFI/SAFI
disable).  Note that treat-as-withdraw is still not an option.

To be clear: all this is my personal interpretation/proposal.  If you
(authors) want to go with this approach (resulting in the ability to
discard), then we should explicitly bounce it by the WG.  If not, then
we can only reset the session or disable FS...

NEW (suggestion)>
   An advertisement containing an unknown Flow Specification component
should be discarded as specified in Section 5.4 of [RFC7606].

Additional text would be needed in §4.2 identifying the FS NLRI as Typed.

1156	   The specification of a new Flow Specification Component Type SHOULD
1157	   clearly identify what the criteria used to match packets forwarded by
1158	   the router is.  This criteria should be meaningful across router hops
1159	   and not depend on values that change hop-by-hop such as TTL or Layer
1160	   2 encapsulation.

[major] Only "SHOULD"?  I can't imagine a case where it would be ok to
not identify the criteria clearly.  s/SHOULD/MUST

...
1192	12.2.  Flow Component Definitions
...
1222	   In order to manage the limited number space and accommodate several
1223	   usages, the following policies defined by [RFC8126] are used:

1225	             +--------------+-------------------------------+
1226	             | Type Values  | Policy                        |
1227	             +--------------+-------------------------------+
1228	             | 0            | Specification required        |
1229	             | [1 .. 12]    | Defined by this specification |
1230	             | [13 .. 127]  | Specification required        |
1231	             | [128 .. 255] | First Come First Served       |
1232	             +--------------+-------------------------------+

[minor] Consider simply reserving 0.

...
1343	13.  Security Considerations
...
1357	   Deployment of specific relaxations of the validation within an
1358	   administrative boundary of a network, defined by an AS or an AS-
1359	   Confederation boundary, may be useful in some networks for quickly
1360	   distributing filters to prevent denial-of-service attacks.  For a
1361	   network to utilize this relaxation, the BGP policies must support
1362	   additional filtering since the origin AS field is empty.
1363	   Specifications relaxing the validation restrictions SHOULD contain
1364	   security considerations that provide details on the required
1365	   additional filtering.  For example, the use of [RFC6811] to enhance
1366	   filtering within an AS confederation.

[major] When would it be ok for a specification to not provide
details?  IOW, why is "SHOULD" used and not "MUST"?

...
1418	14.  Contributors

1420	   Barry Greene, Pedro Marques, Jared Mauch, Danny McPherson, and
1421	   Nischal Sheth were authors on [RFC5575], and therefore are
1422	   contributing authors on this document.

[minor] Danny is listed both as a Contributor and as an author of this document.

---

From: Jeffrey Haas <[email-masked]>
Date: Tue, 17 Dec 2019 17:09:36 -0500

[Speaking as an implementor rather than one of the document authors.]

Alvaro,

> should then be a lot clearer, and Normative.

The missing detail here, and I'm unclear what should be done about it, is
the following:
https://tools.ietf.org/html/draft-ietf-idr-flowspec-redirect-ip-00

Early versions of the redirect-ip draft encoded the destination to forward
the traffic to in the BGP MP-nexthop field.  (Remember, flowspec is sent as
a MP_REACH/UNREACH_NLRI.)  Later versions of the draft move to BGP extended
communities to encode that.  However, there's shipping code from at least
one vendor (maybe two) that does redirect-ip in that flavor.

>    Address of Next Hop field MUST be ignored.

I think that's safe for the above.

> > [major] What should happen if a component appears more than once?

IMO, it should be treated as a semantic error and treat-as-withdraw should
be exercised per RFC 7606.

> > [major] What should happen if the order is not maintained?

There are one of two lines we could draw:
1. BGP-LS treats such misordering, IIRC, as an error.
2. If the NLRI is otherwise valid, the receiver MUST sort it appropriately.

The underlying issue is that comparability of NLRI (is this the same route?)
is compromised if you don't send it the same way from one BGP speaker to
another consistently.  And even then, you have issues in circumstances where
the same semantic filter is sent with two distinct encodings.  E.g.:

R1 sends to R2 and R3 some flowspec NLRI N1.
R2 sends N1 to R4.
R3 sends N1' to R4.

R4 now has N1 and N1', each the same filtering semantics, but different
encodings.  It now has two different routes.  It also route selects them
differently based on the route selection rules.

> indication to the sender that anything is wrong.

FWIW, I'm not actually aware of an implementation that does the "soft-down"
behavior mentioned above.  The intent was that if you can still parse the
location of BGP frames within the TCP stream, that you could take out one
AFI/SAFI without necessarily impacting other AFI/SAFI over the same BGP
session.  However, the only fix is to eventually bounce the session as a
reset if you implemented such a thing.

> > 542 4.2.12. Type 12 - Fragment

RFC 791 (IP) section 3.1, "Flags".

It's operator, mask, as is clearly in that section. (If you're not seeing
it, you have a mismatch vs. draft-18)

> case of multiple bits set, an OR operation is expected.

I think your point here is the "Christmas tree" attack.

In the above bit sets, LF is (I believe) the only option that doesn't make
sense when the other 3 may be set.  

With respect to its behavior, one option is "this doesn't make sense, and
thus cannot match a packet".  And thus it's a very odd way to encode a NOP.

> So, are you working on this already?  ;-)

I'd suggest sanctioning the AD that let this go through in the first place.
:-)

More seriously, my suggestion is to remove any commentary about the registry
code point intents.  It's a number.  It got used in the wrong way for
reasons that are no longer important.  Let the chairs request the cleanup.

> disable, which is not extension-friendly.

This is exactly my largest concern about flowspec right now.  There is no
safe way to extend the spec at this point.

There are two options available to us, IMO:
1. Flowspec v2, which we stalled out until 5575bis was done.
2. We decide that for 5575-bis that all future extensions MUST be encoded
with a TLV format (length being mandatory).  And perhaps even protect a
compliant extension with a capability.  This is a major change, but perhaps
provides an upgrade path without waiting on v2.
> should be discarded as specified in Section 5.4 of [RFC7606].

You can't parse it, therefore it's malformed.

-- Jeff

---

From: Christoph Loibl <[email-masked]>
Date: Wed, 18 Dec 2019 08:11:37 +0100

Hi Jeffrey, Alvaro!

I only pick out this single issue from the complete list, because I do not 100% agree with this statement:

> On 17.12.2019, at 23:09, Jeffrey Haas <[email-masked]> wrote:
> 

In case of an unknown flowspec component the parser can run until decoding the component type (always the first byte of the component). When it reads a unknown type-value it can easily jump over the remaining bytes of the entire NLRI (the length of the the complete NLRI is encoded in the first bytes of the NLRI) without needing to understand what is in there, because it should treat it as withdraw. I do not see the problem. It is supposed to “ignore” the entire NLRI not only a single component of it. 

Cheers
Christoph

-- 
Christoph Loibl
[email-masked] | CL8-RIPE | PGP-Key-ID: 0x4B2C0055 | http://www.nextlayer.at

---

From: Jeffrey Haas <[email-masked]>
Date: Wed, 18 Dec 2019 07:12:35 -0500

> In case of an unknown flowspec component the parser can run until decoding the component type (always the first byte of the component). When it reads a unknown type-value it can easily jump over th…

Sigh.  Into example-land we go:

You receive the following byte-stream as NLRI in a single BGP PDU:
NLRI-1, length X components 1,2,3
NLRI-2, length Y components 1,2,3,?,?,?
NLRI-3, length Z components 1,2,3

In the case of NLRI-1,3 if you had received these individually, you'd know
they were correct because the implementation understands them.

In NLRI-2, you don't know what some of the components are starting at first ?.

The _best_ you can do in these circumstances is walk the list of NLRI by
length and see if you can arrive at NLRI boundaries and have a valid BGP
Update PDU.

However, you also don't know if you actually have an NLRI-3 or not.  It
might actually be a random byte stream that just happens to parse as an NLRI
and accidentally lands on a packet boundary.  NLRI-2 may be corrupt and the
length mis-calculated.  

We have seen these bugs before, even in simple RFC 4271 PDUs.

-- Jeff

---

From: Alvaro Retana <[email-masked]>
Date: Wed, 18 Dec 2019 08:17:05 -0800

On December 18, 2019 at 7:08:06 AM, Jeffrey Haas wrote:

Jeff/Christoph:

Hi!

As I read the responses it sounded to me as if we were all going in
the same direction: discard if an unknown component is found.  But
reading more into it, I think we're talking about different cases...

...
> > > On 17.12.2019, at 23:09, Jeffrey Haas wrote:
> > >
...
> > > You can't parse it, therefore it's malformed.

Jeff: I interpreted this response as agreeing that if the NLRI can't
be parsed then it should be discarded (even maybe buying the Typed
NLRI argument).  It sounds like we're agreeing.  Is that true?

> > In case of an unknown flowspec component the parser can run until decoding
> > the component type (always the first byte of the component). When it reads
> > a unknown type-value it can easily jump over the remaining bytes of the
> > entire NLRI (the length of the the complete NLRI is encoded in the first
> > bytes of the NLRI) without needing to understand what is in there, because
> > it should treat it as withdraw. I do not see the problem. It is supposed to
> > “ignore” the entire NLRI not only a single component of it.

Christoph: It seems to me that we're getting to the same behavior: I
call it "discard", you call it "treat it as withdraw...“ignore”".
While discarding, ignoring and treat-as-withdraw are different things,
the effect seems to be the same: we won't use the NLRI.  We can find
the right words...  Do you also think we're going in the same
direction?

> We have seen these bugs before, even in simple RFC 4271 PDUs.

In theory, if the length is set correctly, and the unknown components
are "well behaved" (i.e. they are actually correct, the receiver just
doesn't understand them), then it seems to me that we could safely
just discard NLRI-2.  In theory...

I think that we are talking about two different actions when bumping
into an unknown component: discarding an NLRI in the list (e.g. just
NLRI-2 above), vs discarding the whole advertisement (i.e. all NLRIs
to protect against potential bugs).

Going back to rfc7606/§3, it talks about parsing the "entire NLRI
field" (not just a single entry).  But the Typed NLRI behavior
(rfc7606/§5.4) talks about discarding "routes with unrecognized NLRI
types" (not the entire advertisement).

Personal opinion:  If we (WG) don't buy into the Typed NLRI-like
argument, then we deal with the "entire NLRI field", but we're also
stuck with AFI/SAFI disable (or session reset).  If we buy into the
Typed NLRI-like argument, then it seems like we have the option to
specify either discard behavior.

???

Alvaro.

---

From: Jeffrey Haas <[email-masked]>
Date: Wed, 18 Dec 2019 11:54:35 -0500

Alvaro,

[choosing the most terse response]

> NLRI argument).  It sounds like we're agreeing.  Is that true?

[...]

> specify either discard behavior.

IMO, the typed NLRI argument was poorly thought through.  It effectively
devolves into a form of the issues above.  The issue in both cases is that
it's impossible to tell that you're dealing with garbage or not.  The choice
made in that section was discard with exactly the same risks I mentioned.

If the WG converges on discard, so be it.  If an implementation and a
network roll the dice in a lucky fashion, they only encounter such issues
for well-formed unknowns that are discarded.  If not, they get to manually
bounce their peering session.

The fix for flowspec really should be flowspec v2 or capabilities based fix
for later extensions.  The fix for the typed NLRI is effectively similar.

-- Jeff

---

From: Christoph Loibl <[email-masked]>
Date: Sat, 21 Dec 2019 11:03:55 +0100

Alvaro,

> On 18.12.2019, at 17:17, Alvaro Retana <[email-masked]> wrote:
> 

I agree. I will need to collect the comments and think that I can come up with something that is acceptable while still giving some certain degree of flexibility to protocol extensions. 

I already started working on all the the issues in the document you pointed out on github:

	https://github.com/stoffi92/rfc5575bis <https://github.com/stoffi92/rfc5575bis>

Expect a new -19 to be ready by the end of January, maybe a little earlier.

Cheers,
Christoph