Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-11.txt

[Acee Lindem <[e-mail]>]

Hi Alvaro,

Keyur and I addressed these comments as well. 

> On Feb 9, 2023, at 8:00 AM, Alvaro Retana <[e-mail]> wrote:
> 
> On February 15, 2021 at 10:55:53 AM, Acee Lindem wrote:
> 
> 
> Hi!
> 
> This message covers the initial set of up-front comments/questions.
> 
> Thanks!
> 
> Alvaro.
> 
> 
> 
>> [**]
>> 
>>> It would be ideal for the document to include a "Summary of Operation"
>>> section, similar to ??3/rfc4271. This would also serve as a place to
>>> point back at what applies and what doesn't.
>> 
>> We've added a "Document Overview" in section 1.3. This should suffice in
>> giving the reader a preview of what is coming.
> 
> 241 1.3.  Document Overview
> 
> 243   The document begins with sections defining the precise relationship
> 244   that BGP SPF has with both the base BGP protocol [RFC4271]
> 245   (Section 2) and the BGP Link-State (BGP-LS) extensions [RFC7752]
> 246   (Section 3).  This is required to dispel the notion that BGP SPF is
> 247   an independent protocol.  The BGP peering models, as well as the
> 248   their respective trade-offs are then discussed in Section 4.  The
> 249   remaining sections, which make up the bulk of the document, define
> 250   the protocol enhancements necessary to support BGP SPF.  The BGP-LS
> 251   extensions to support BGP SPF are defined in Section 5.  The
> 252   replacement of the base BGP decision process with the SPF computation
> 253   is specified in Section 6.  Finally, BGP SPF error handling is
> 254   defined in Section 7




> 
> 
> [major] Before I forget, please update the rfc7752 reference to
> draft-ietf-idr-rfc7752bis.  That document is already in front of the
> IESG; I expect to approve it by March.

Changed - this made the diff bigger. 



> 
> 
> [minor] "This is required to dispel the notion that BGP SPF is an
> independent protocol."
> 
> Not needed.


Removed. 

> 
> 
> [nit] The last couple of sentences are a little repetitive, and
> there's a period missing at the end.
> 
> Suggestion>
>   The document begins with sections defining the precise relationship
>   that BGP SPF has with both the base BGP protocol [RFC4271]
>   (Section 2) and the BGP Link-State (BGP-LS) extensions [RFC7752bis]
>   (Section 3). The BGP peering models, as well as the their respective
>   trade-offs are then discussed in Section 4. The remaining sections
>   define the protocol enhancements necessary to support BGP SPF
>   including BGP-LS extensions (Section 5), the replacement decision
>   process with the SPF computation (Section 6), and error handling
>   (Section 7).

Incorporated you suggested text.

> 
> 
> 
>>> (1) Use of the BGP Base (rfc4271)
>>> 
>>> - The well-known RIB structure from BGP (Adj-RIB-In, Loc-RIB, Adj-RIB-Out)
>>> didn't quite make it into BGP SPF. It looks like the LSNDB may correspond
>>> to the Adj-RIB-In, and there is a Local RIB. The non-existence of an
>>> Adj-RIB-Out implies changes. [Note that it is obviously ok to not follow
>>> the same structure -- what needs to be explained are the differences.]
>> 
>> Since we have completely replaced the base BGP decision process, we don't use
>> these terms.
> 
> [major] You did change the "Local RIB" for "LOC-RIB", but the
> definition is not the same as Loc-RIB in rfc4271.  Even if the case is
> different, it would be better to change the terminology (maybe back to
> "Local RIB") to eliminate any confusion.


Changed to “Local-RIB”. 


> 
> 
> ...
>>> - About the attributes... In ??5.1 you mention that attributes that "would
>>> influence the Decision process...are ignored".
>>> 
>>> What about other attributes, the ones that don't influence the Decision
>>> Process? If all the information is encoded in the NLRI or carried in the
>>> BGP-LS Attribute, does BGP SPF need any other attribute? Maybe requiring
>>> that other attributes not to be included (especially the mandatory ones) is
>>> too much, but can it be recommended? Given that BGP (and rfc7606) require
>>> specific actions if the attributes are malformed, it would be bad if we
>>> apply treat-as-withdraw because of an error in an attribute that is not
>>> relevant in BGP SPF. [Note: we need to add this risk to the Security
>>> Considerations.]
> ...
>> We clarified the usage of the attributes in section 2. Error handling is
>> covered in section 7.
> 
> §2:
> 
> 273   Due to the changes to the decision process, there are mechanisms and
> 274   encodings that are no longer applicable.  While not necessarily
> 275   required for computation, the ORIGIN, AS_PATH, MULTI_EXIT_DISC,
> 276   LOCAL_PREF, and NEXT_HOP path attributes are mandatory and will be
> 277   validated.  The ATOMIC_AGGEGATE, and AGGREGATOR are not applicable
> 278   within the context of BGP SPF and SHOULD NOT be advertised.  However,
> 279   if they are advertised, they will be accepted, validated, and
> 280   propagated consistent with the BGP protocol.
> 
> [minor] s/are mandatory/are mandatory [RFC4271]

Fixed.

> 
> To clarify that they are not mandatory in this specification.
> 
> 
> [major] What about other attributes [1]?  I know that most are not
> mandatory but they could still be included in an UPDATE.
> 
> Is it possible to generalize the statement above?  Here's a suggestion:
> 
> s/
>    The ATOMIC_AGGEGATE, and AGGREGATOR are not applicable within
>    the context of BGP SPF and SHOULD NOT be advertised.  However,
>    if they are advertised, they will be accepted, validated, and
>    propagated consistent with the BGP protocol.
> /
>    Unless explicitly specified in the context of BGP SPF, all other
>    attributes SHOULD NOT be advertised.  However, if they are
>    advertised, they will be accepted, validated, and propagated
>    consistent with the BGP protocol.
> 
> [1] https://www.iana.org/assignments/bgp-parameters/bgp-parameters.xhtml#bgp-parameters-2

We made the statement generalized as suggested. 

Unless explicitly specified in the context of BGP SPF, all other attributes SHOULD NOT be
advertised. However, if they are advertised, they will be accepted, validated, and propagated
consistent with the BGP protocol.



> 
> 
> [major] Processing non-applicable attributes represents an increase in
> the attack surface.  A rogue node can advertise a non-applicable
> attribute and affect the routing decisions; for example a malformed
> Community results in "treat-as-withdraw".
> 
> This is not a new threat.  The difference is that for other BGP
> AFI/SAFIs alll the attributes are "applicable".  In the case of BGP
> SPF, this document explicitly says that the nodes will accept,
> validate, and propagate information that is not relevant.
> 
> One option to reduce the attack surface is to ignore/drop
> non-applicable attributes, or even mandate that they must not be
> included.  I understand that implementation-wise that is not the best
> option.
> 
> Please include a couple of sentences in the Security Considerations to
> show that the threat was considered -- that it is an existing threat

We tried to capture this in the “Security Considerations”. 

Additionally, given that the BGP SPF protocol is used to install IPv4 and IPv6
Unicast routes, the BGP SPF protocol is vulnerable to attacks to the routing
control plane that aren't applicable to BGP-LS. One notable Denial-of-Service
attack, would be to include malformed BGP attributes in a replicated
BGP Update, causing the receiving peer to treat the advertised BGP-
LS-SPF to a withdrawal [RFC7606].

> -- ...
> 
> 
> ...
>>> - Using a Confederation ID is mentioned, which seems ok. I don't see an
>>> advantage/reason for having Confederations in a BGP SPF network. Is there
>>> one?
>> 
>> Confederations are no longer applicable.
> 
> From §5.2:
> 
>    The BGP Confederation Member (TLV 517) [RFC7752] is not appliable
>    and SHOULD NOT be included.  If TLV 517 is included, it will be
>    ignored.
> 
> [nit] s/appliable/applicable
> 
> [?] s/will be ignored/MUST be ignored


Fixed. 

Thanks,
Acee