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

[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	 =A0 This NLRI information is encoded using MP_REACH_NLRI and
> 256	 =A0 MP_UNREACH_NLRI attributes as defined in [RFC4760]. =A0Whenever =
the
> 257	 =A0 corresponding application does not require Next Hop information,=
 this
> 258	 =A0 shall be encoded as a 0-octet length Next Hop in the MP_REACH_NL=
RI
> 259	 =A0 attribute (if a non 0-octet Next Hop is present it should be ign=
ored
> 260	 =A0 on receipt).
> =

> I think I get it: the Next Hop is not needed -- so it is either not
> present (0-octet length) or ignored. =A0Is 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). =A0IOW, the use defined here is specific to the FS
> AFI/SAFI pair. =A0The 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>
> =A0 =A0This NLRI information is encoded using MP_REACH_NLRI and MP_UNREAC=
H_NLRI
> =A0 =A0attributes as defined in [RFC4760]. =A0When advertising Flow Speci=
fications,
> =A0 =A0the Length of Next Hop Network Address SHOULD be set to 0. =A0The =
Network
> =A0 =A0Address 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 malform=
ed
> > 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? =A0Should it reset the session? =A0Should it ignore the Update?
> =A0Should it disable the AFI/SAFI? =A0Should it only consider the first
> component (if it appears more than once)? =A0Something 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":
> =

> =A0 =A0If a BGP speaker receives from a neighbor an UPDATE message that
> =A0 =A0contains the MP_REACH_NLRI or MP_UNREACH_NLRI attribute, and if the
> =A0 =A0speaker determines that the attribute is incorrect, the speaker MU=
ST
> =A0 =A0delete all the BGP routes received from that neighbor whose AFI/SA=
FI
> =A0 =A0is the same as the one carried in the incorrect MP_REACH_NLRI or
> =A0 =A0MP_UNREACH_NLRI attribute. For the duration of the BGP session over
> =A0 =A0which the UPDATE message was received, the speaker then SHOULD ign=
ore
> =A0 =A0all the subsequent routes with that AFI/SAFI received over that
> =A0 =A0session.
> =

> The result is then that FS (including the rules/actions already
> received) would now not be used. =A0Personal 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. =A0A note in =A710
> 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,bitma=
sk)
> > 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/f078e9198=
42330d670bc195276ead7bdc05a3351
> >
> > Added the text to the bit-descriptions and IP-header fields - And an ex=
ample
> > 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. =A0What 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 pro=
pose a
> > update to RFC7153 to cleanup the registry independently. - As noticed m=
oving
> > the values around is not an option.
> > -->
> =

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

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. =A0Future NLRI Extensions
> =

> 1149	 =A0 Future Flow Specification extensions may introduce new Flow
> 1150	 =A0 Specification components. =A0In order to facilitate such extens=
ions of
> 1151	 =A0 the Flow Specification NLRI, in addition to the cases described=
 in
> 1152	 =A0 [RFC7606], if BGP encounters an unknown Flow Specification comp=
onent
> 1153	 =A0 in an UPDATE message, it SHOULD also treat this message as Trea=
t-as-
> 1154	 =A0 withdraw as specified in [RFC7606] Section 2.
> =

> [minor] This paragraph describes an error condition, so maybe it belongs =
in =A710.
> =

> [major] rfc7606/=A73 reads:
> =

> =A0 j. =A0Finally, we observe that in order to use the approach of "treat-
> =A0 =A0 =A0 as-withdraw", the entire NLRI field and/or the MP_REACH_NLRI =
and
> =A0 =A0 =A0 MP_UNREACH_NLRI attributes need to be successfully parsed -- =
what
> =A0 =A0 =A0 this entails is discussed in more detail in Section 5. =A0If =
this
> =A0 =A0 =A0 is not possible, the procedures of [RFC4271] and/or [RFC4760]
> =A0 =A0 =A0 continue to apply, meaning that the "session reset" approach =
(or
> =A0 =A0 =A0 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. =A0The 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)>
> =A0 =A0An 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

_______________________________________________
Idr mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/idr