Subject: [Idr] AD Review of draft-ietf-idr-bgp-extended-messages-30

From: Alvaro Retana <[email-masked]>
Date: Fri, 28 Jun 2019 15:19:35 -0500

Dear authors:

Thank you for the continued work on this document!

In general, I think that the document is in better shape than when it was
originally sent for publication.  I reviewed the discussions on the list
and appreciate the level of participation.  However, I find some of the
text not clear -- reviewing the archive helped, but the document doesn't
always reflect the full discussions, resulting in confusion for casual
readers (or even those with BGP experience who have not read the archive or
been involved).  This point is the biggest issue that I have with the
document -- please see my comments/questions inline.

Thanks!

Alvaro.

[Line numbers from idnits.]

...
13 Abstract

15   The BGP specification mandates a maximum BGP message size of 4096
16   octets.  As BGP is extended to support newer AFI/SAFIs and other
17   features, there is a need to extend the maximum message size beyond
18   4096 octets.  This document updates the BGP specification RFC4271 by
19   providing an extension to BGP to extend its current maximum message
20   size from 4096 octets to 65535 octets for all except the OPEN
21   message.

[nit] s/providing an extension to BGP to extend its current/extending the

[major] In reality, the change is being made for everything (including any
new messages) except the OPEN and the KEEPALIVE.

23 Requirements Language

25   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
26   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" are to
27   be interpreted as described in [RFC2119] only when they appear in all
28   upper case.  They may also appear in lower or mixed case as English
29   words, without normative meaning.

[major] Please use the rfc8174 template.

...
79 1.  Introduction

81   The BGP specification [RFC4271] mandates a maximum BGP message size
82   of 4096 octets.  As BGP is extended to support newer AFI/SAFIs and
83   newer capabilities (e.g., BGPsec, [RFC8205], BGP-LS, [RFC7752]),
84   there is a need to extend the maximum message size beyond 4096
85   octets.  This draft provides an extension to BGP to extend its
86   current message size limit from 4096 octets to 65535 octets for all
87   except the OPEN message.

[nit] s/BGPsec, [RFC8205], BGP-LS, [RFC7752]/BGPsec [RFC8205], BGP-LS
[RFC7752]

[minor] s/current message size/message size

89 2.  BGP Extended Message

91   A BGP message over 4096 octets in length is a BGP Extended Message.

93   BGP Extended Messages have maximum message size of 65535 octets.  The
94   smallest message that may be sent consists of a BGP header without a
95   data portion (19 octets).

[nit] s/have maximum message/have a maximum message

[nit] s/smallest message that may be sent consists of/smallest message that
may be sent is a KEEPALIVE, which consists of

97 3.  Extended Message Capability for BGP

99   To advertise the BGP Extended Message Capability to a peer, a BGP
100   speaker uses BGP Capabilities Advertisement [RFC5492].  By
101   advertising the BGP Extended Message Capability to a peer, a BGP
102   speaker conveys that it is able to send, receive, and properly handle
103   BGP Extended Messages.

[minor] What does "properly handle" mean?  I'm thinking that it may refer
to what is indicated in §4.  If it is, please put a forward reference.

105   The BGP Extended Message Capability is a new BGP Capability [RFC5492]
106   defined with Capability code 6 and Capability length 0.

[nit] Switch the order of the first two paragraphs...

108   A peer which does not advertise this capability MUST NOT send BGP
109   Extended Messages, and BGP Extended Messages MUST NOT be sent to it.

111 4.  Operation

113   A BGP speaker that is capable of sending and receiving BGP Extended
114   Messages SHOULD advertise the BGP Extended Message Capability to the
115   peer using BGP Capabilities Advertisement [RFC5492].  A BGP speaker
116   MAY send Extended Messages to its peer only if it has fully exchanged
117   the Extended Message Capability with that peer.

[nit] s/to the peer/to its peers

[minor] s/to its peer/to a peer

[major] What does "fully exchanged" mean?  I'm assuming it means that both
routers advertised/received the capability.  If so, please be specific.

119   The Extended Message Capability applies to all messages except for
120   the OPEN message.  This exception is made to reduce complexity of
121   providing backward compatibility

[nit] There's a "." missing at the end.

123   An implementation that advertises support for BGP Extended Messages
124   MUST be capable of receiving a message with a length up to and
125   including 65535 octets.

[minor] s/length/Length   To make it clear the text is referring to the
Length field in the Header.

127   Applications generating information which might be encapsulated
128   within BGP messages MUST limit the size of their payload to take the
129   maximum message size into account.

131   If a BGP update with a payload longer than 4096 octets is received by
132   a BGP listener who has neither advertised nor agreed to accept BGP
133   Extended Messages, the listener MUST treat this as a malformed update
134   message, and MUST raise an UPDATE Message Error (see [RFC4271] Sec
135   6.3).

[major] We really don't want to look at the payload...we want to look at
the message Length...for all messages, not just UPDATEs.  If that is >
4096, then the appropriate Notification should be sent.  This error is
already covered in §6.1 (Message Header Error Handling) of rfc4271, but I
think it is worth reinforcing here.  Suggestion:

NEW>
   If a BGP message with a Length larger than 4096 octets is received by
   a BGP listener who has not advertised the Extended Message Capability,
   the listener MUST treat this as a malformed message, and MUST generate
   a NOTIFICATION with the Error Subcode set to Bad Message Length
   (see [RFC4271] Sec 6.1).

137   A BGP announcement will, in the normal case, propagate throughout the
138   BGP speaking Internet; and there will undoubtedly be BGP speakers
139   which do not have the Extended Message capability.  Therefore, having
140   an attribute set which can not be decomposed to 4096 octets or less
141   in an Extended Message will likely raise errors.

[major] "an attribute set which can not be decomposed to 4096"  What does
"decompose" mean?  Do you mean that the attribute set (+ and the
corresponding NLRI) could fit in <= 4096 octets?

[minor] Should this text be more explicit about the deployment implications
of features/functionality that requires > 4096?  IOW, if the whole BGP
domain (for lack of a better word, defined as the set of BGP routers that
need to receive the advertisements) can't receive the information, then the
feature/functionality won't work...

143   A BGP speaker with a mixture of peers some of which have negotiated
144   BGP Extended Message capability and some which have not, MUST
145   o  support [RFC7606], and
146   o  "treat as withdraw" (see [RFC7606]) a BGP attribute/NLRI pair
147      (defined as BGP Route) which is too large to be sent to a peer
148      which does not support BGP Extended Messages.

[nit] s/have negotiated BGP Extended Message capability/have exchanged the
BGP Extended Message capability

[major] It seems to me that the formulation above is backwards.  I read the
text as saying "IF both peers support Extended Messages, THEN rfc7606 MUST
be supported".  Instead, the support of rfc7606 should be a requirement to
even exchange the new Capability.  This requirement should be called out in
§3.

[nit] s/(see [RFC7606])/[RFC7606]

[minor] s/treat as withdraw/treat-as-withdraw/g  That is how rfc7607 calls
it.

[major] rfc7606 talks about "errors found in a BGP UPDATE message", which
means that it applies to received UPDATEs.  The text above talks about
using treat-as-withdraw when the route is "too large to be sent to a
peer"...which is not about receiving, but about sending.  IOW, the UPDATE
with the route was already received (with no errors) and now the router is
sending routes.

The point here is that the treat-as-withdraw behavior from rfc7606 doesn't
apply when sending routes: it is only defined for received UPDATEs.

It seems to me that instead of pointing at rfc7606, the text can say
something to the effect of: "if the route is too large to be sent to a peer
which does not support BGP Extended Messages then it MUST be Withdrawn".
Note that rfc4271 already covers this case in §9.2, at least partially:

   If, due to the limits on the maximum size of an UPDATE message (see
   Section 4), a single route doesn't fit into the message, the BGP
   speaker MUST not advertise the route to its peers and MAY choose to
   log an error locally.

...the missing part (from my interpretation of the text above) is the
Withdraw action.

I am honestly not sure if I'm interpreting the text correctly, or if the
intent is to treat-as-withdraw received routes (as intended by rfc7606)
that the router knows are "too large to be sent to a peer which does not
support BGP Extended Messages".  In this case, the text then seems to be
saying that even though the speaker advertised the Capability, it MUST
treat-as-withdraw because it can't send the UPDATE forward.  IOW, neither
the listener nor its peers (all its peers) will have the route.

What is the intent?  I am obviously missing something.

150   The BGP speaker MAY remove some BGP attributes which are eligible to
151   use the Attribute discard approach in [RFC7606].

[major] This sentence is sort of by itself.  I'm not sure if it means that
a node may remove some attributes whenever it wants...or just when there is
a mixture of peers (from the previous paragraph).  If the latter, how does
this "MAY" interact with the "MUST" above?  IOW, if the node MUST
treat-as-withdraw, then it seems too late to decide to drop some attributes.

[major] How would the node know which attributes should be discarded
first?  Are there recommendations??  Are you suggesting to discard until
the size becomes < 4096?

153   In an iBGP mesh, all peers SHOULD support the BGP Extended Message
154   Capability and [RFC7606].  Only then is it consistent to deploy with
155   eBGP peers.

[major] When would it be ok for some of the peers to not support the
Capability?  IOW, why is that not a MUST?  [Note also that the SHOULD is in
conflict with the MUST support rfc7606 from above.  This is the main issue
with this piece of text.]  Are the considerations different if the network
doesn't have an iBGP mesh, but a RR or Confederations deployment?

[major nit] I think you really mean s/support the BGP Extended Message
Capability/advertise the BGP Extended Message Capability

[minor] "Only then is it consistent to deploy with eBGP peers."  Is the
intent that if iBGP doesn't use Extended Messages then eBGP shouldn't be
used either?

157   During the incremental deployment of BGP Extended Messages and
158   [RFC7606] in an iBGP mesh, or with eBGP peers, the operator should
159   monitor any routes dropped as "treat as withdraw".

[minor] ...and any discarded attributes.

161   It is RECOMMENDED that BGP protocol developers and implementers are
162   conservative in their application and use of Extended Messages.
163   Future protocol specifications will need to describe how to handle
164   peers which can only accommodate 4096 octet messages.

[major] "will need to describe"  Should this text be somehow Normative?
"RECOMMENDED" doesn't mean that it is mandatory, but I think that it should
be mandatory (MUST/REQUIRED) to describe how to operate with peers that do
not support Extended Messages.

166 5.  Error Handling

168   A BGP speaker that has the ability to use Extended Messages but has
169   not advertised the BGP Extended Messages capability, presumably due
170   to configuration, SHOULD NOT accept an Extended Message.  A speaker
171   SHOULD NOT implement a more liberal policy accepting BGP Extended
172   Messages.

[major] The text in §4 says that "if...a BGP listener who has neither
advertised nor agreed to accept BGP Extended Messages, the listener MUST
treat this as a malformed update message".  The text above (using SHOULD
NOT) is in conflict with the MUST from §4:  if the received message MUST be
treated as an error, then there's no circumstance where it is ok to accept
the message.  IOW, why isn't MUST NOT used?

174   A BGP speaker that does not advertise the BGP Extended Messages
175   capability might also genuinely not support Extended Messages.  Such
176   a speaker will follow the error handling procedures of [RFC4271] if
177   it receives an Extended Message.  Similarly, any speaker that treats
178   an improper Extended Message as a fatal error, MUST treat it
179   similarly.

[major] What is the last sentence trying to say (similar to what?)?  What
is an "improper Extended Message"?  Is it one that was received even if the
Capability wasn't advertised?

181   The inconsistency between the local and remote BGP speakers MUST be
182   flagged to the network operator through standard operational
183   interfaces.  The information should include the NLRI and as much
184   relevant information as reasonably possible.

[minor] "The inconsistency..."  Which inconsistency? I'm assuming that the
case where an Extended Message is received without having announced the
Capability...but, again, the text in §4 already says what must happen.
IOW, this text is redundant...and may be confusing.

[major] Should other inconsistencies also be flagged?  For example, if the
local peer does advertise the Capability, but the remote peer doesn't...or
viceversa.

...
197 7.  IANA Considerations

199   The IANA has made an early allocation for this new BGP Extended
200   Message Capability referring to this document.

202   Registry:  BGP Capability Code

[major] The registry is actually simply called "Capability Codes" (no BGP
in the name).

204   Value    Description                               Document
205   -----    -----------------------------------       -------------
206   6        BGP-Extended Message                      [this draft]

[nit] Do we really need the "-"?

208 8.  Security Considerations

210   This extension to BGP does not change BGP's underlying security
211   issues; see [RFC4272].

[nit] s/; see [RFC4272]/ [RFC4272]

213   Section 5 allows a receiver to accept an Extended Message even though
214   it had not advertised the capability.  This slippery slope could lead
215   to sloppy implementations sending Extended Messages when the receiver
216   is not prepared to deal with them, e.g. to peer groups.  At best,
217   this will result in errors; at worst, buffer overflows.

[major] See my comments above: I think the text in §5 is in conflict with
the one in §4.

219   Due to increased memory requirements for buffering, there may be
220   increased exposure to resource exhaustion, intentional or
221   unintentional.

223   As this draft requires support for [RFC7606] update error handling,
224   it inherits the security considerations of [RFC7606].  BGP peers may
225   avoid such issues by using Authenticated Encryption with additional
226   Data (AEAD) ciphers [RFC5116] and discard messages that do not
227   verify.

[nit] s/update/UPDATE

[minor] I think that this last sentence is not needed since it is the same
recommendation already in rfc7606.

229   If a remote attacker is able to craft a large BGP Extended Message to
230   send on a path where one or more peers do not support BGP Extended
231   Messages, peers which support BGP Extended Messages may incur
232   resource load (processing, message resizing, etc.) reformatting the
233   large messages.  Worse, ([RFC7606] "treat as withdraw" may
234   consistently withdraw announcements causing inconsistent routing.

[nit] The "(" seems to not belong there.

236   BGP routes are filtered by policies set by the operators.
237   Implementations may provide policies to filter routes that would
238   cause the "treat as withdraw" from being passed by an extended
239   message speaker.

[minor] I don't understand what is being said here.  Are you saying that
the Withdraw could be filtered?

---

From: Randy Bush <[email-masked]>
Date: Sun, 30 Jun 2019 13:07:08 -0700

alvaro:

thank you for the really excellent review.  i hope that -31, just
published, addresses your issue and incorporates the changes and
text you suggested.

thank you again.

randy