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

[Jeffrey Haas <[e-mail]>]

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

Alvaro,

On Thu, Dec 12, 2019 at 11:38:57AM -0800, Alvaro Retana wrote:
> On November 4, 2019 at 2:23:41 PM, Christoph Loibl
> [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.

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.

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

I think that's safe for the above.

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

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.


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

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.

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

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)


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

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.

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

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.

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

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.
> 
> NEW (suggestion)>
>    An advertisement containing an unknown Flow Specification component
> should be discarded as specified in Section 5.4 of [RFC7606].

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


-- Jeff