Subject: [Idr] AD Review of draft-ietf-idr-ext-opt-param-09

From: Alvaro Retana <[email-masked]>
Date: Thu, 18 Mar 2021 14:54:46 -0700

Enke/John:

Hi!

Thank you for your persistence in getting this work done!

I have several comments inline below.  I believe that all of them
should be easy to address.

Thanks!

Alvaro.

[Line numbers from idnits.]

...
11	Abstract
...
19	   In this document we update RFC 4271 by extending, in a backward-
20	   compatible manner, the length of the Optional Parameters in the BGP
21	   OPEN.  The Parameter Length field of individual Optional Parameters
22	   is also extended.

[style nit] s/In this document we update/This document updates/g

...
72	1.1.  Requirements Language

74	   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
75	   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
76	   document are to be interpreted as described in RFC 2119 [RFC2119].

[major] Use the template from rfc8174.

78	2.  Protocol Extensions
...
83	   In the event that the length of Optional Parameters in the BGP OPEN
84	   message does not exceed 255, the encodings of the base BGP
85	   specification [RFC4271] MUST be used without alteration.  However, an
86	   implementation MUST be prepared to accept an OPEN message that uses
87	   the encoding of this specification for Optional Parameters of any
88	   length.

[major] "MUST be prepared to accept an OPEN message...of any length."

The OPEN message is not covered by rfc8654, so the length is still
limited to 4k...which means that the full Extended OP length/Parameter
Length cannot be used.

Besides clarifying that, I think that new OPEN Message Error subcodes
are needed for the cases where the length is invalid.  This type of
error is possible in the non-extended version of the OPEN too -- I
guess rfc4271 just assumed that this error would never happen.

90	   If the length of Optional Parameters is greater than 255, the
91	   extended encoding defined here MUST be used.  The (non-extended)
92	   length field MUST be set to 255.  The subsequent octet (which would
93	   be the first Optional Parameter Type in the non-extended format) MUST
94	   be set to 255 as well.  The subsequent two octets carry the actual
95	   length.  In addition, the "Parameter Length" field of each Optional
96	   Parameter is enlarged to two octets.  Other than the larger sizes of
97	   the given fields, there is no change to the BGP OPEN message defined
98	   in [RFC4271].

[minor] This paragraph references the modified OPEN message, but it
hasn't been introduced yet.  Please refer to the figures or move them
(and the description of the fields) to the beginning of this section.
Also, please be consistent with the field names from the figure.

...
111	       0                   1                   2                   3
112	       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
113	       +-+-+-+-+-+-+-+-+
114	       |    Version    |
115	       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
116	       |     My Autonomous System      |
117	       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
118	       |           Hold Time           |
119	       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
120	       |                         BGP Identifier                        |
121	       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
122	       |Non-Ext OP Len.|Non-Ext OP Type|  Extended Opt. Parm. Length   |
123	       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
124	       |                                                               |
125	       |             Optional Parameters (variable)                    |
126	       |                                                               |
127	       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[] Please add a figure number and title.

129	   The non-extended Optional Parameters Length field MUST be set to 255
130	   on transmission, and MUST be ignored on receipt once the use of the
131	   extended format is determined positively by inspection of the (non-
132	   extended) Optional Parameters Type field.

[major] "non-extended Optional Parameters Length"  Please refer to the
names of the fields in the figure.   s/.../non-extended Optional
Parameters Length (Non-Ext OP Len.)

...
148	       0                   1                   2
149	       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4
150	       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
151	       |  Parm. Type   |        Parameter Length       |
152	       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
153	       ~            Parameter Value (variable)         ~
154	       |                                               |
155	       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[] Please add a figure number and title.

...
178	   The choice to mandate that when the extended encoding is in use, the
179	   (non-extended) Optional Parameters Length field must be 255 was made
180	   for backward compatibility with implementations of earlier versions
181	   of this specification.  When the extended encoding is in use, the
182	   value 0 MUST NOT be used in that field since the presence of that
183	   value could have the effect of causing a message parser to never
184	   inspect the following octet.

[] I don't think it's a good idea to justify anything based on
pre-standard implementations.  s/MUST be set to 255 on
transmission/SHOULD be set to 255 on transmission  will cover both
cases without the justification.

[major] "0 MUST NOT be used"  Move this part to where the field is described.

186	3.  Errors

188	   If a BGP speaker supporting this specification (a "new speaker") is
189	   peering with one which does not (an "old speaker") no
190	   interoperability issues arise unless the new speaker needs to encode
191	   Optional Parameters whose length exceeds 255.  In that case, it will
192	   transmit an OPEN message which the old speaker will interpret as
193	   containing an Optional Parameter with type code 255.  Since by
194	   definition the old speaker will not recognize that type code, the old
195	   speaker may be expected to close the connection with a NOTIFICATION
196	   with an Error Code of OPEN Message Error and an Error Subcode of
197	   Unsupported Optional Parameters, according to Section 6.2 of
198	   [RFC4271].

[minor] s/may be expected/is expected

200	   Although the above is the most likely error to be sent, it is not
201	   impossible that the old speaker might detect some other error first,
202	   such as a length error, depending on the details of the
203	   implementation.  In no case would the peering be expected to
204	   establish successfully; the only question is which NOTIFICATION would
205	   be generated.

[] No need to speculate about implementations.  The text above points
at rfc4271 anyway.

207	   We note that in any case, such a peering could not succeed, since by
208	   definition the extended length encoding would not be used by the new
209	   speaker unless the basic encoding was insufficient.

[] I don't understand what you mean here.  It sounds as if the new
speaker shouldn't have used the encoding anyway...  ??

...
219	   It is not considered an error to receive an OPEN message whose
220	   Extended Optional Parameters Length value is less than or equal to
221	   255, any value SHOULD be silently accepted.  It is not considered a
222	   fatal error to receive an OPEN message whose (non-extended) Optional
223	   Parameters Length value is not 255, and whose first Optional
224	   Parameter type code is 255 -- in this case the encoding of this
225	   specification MUST be used for decoding the message.  A warning MAY
226	   be logged.

[major] "any value SHOULD be silently accepted"  When is it ok to not
(silently) accept the message?  IOW, why is this recommended and not
required?  In any case, I don't think that the last part of the
sentence is needed because the first part already says this is not an
error.

[End of Review]

---

From: Jeffrey Haas <[email-masked]>
Date: Fri, 19 Mar 2021 09:31:16 -0400

Alvaro,

> guess rfc4271 just assumed that this error would never happen.

Speaking as a vendor with an implementation here, we currently send
"Unsupported Optional Parameter".  A new sub-code would be fine to help with
diagnostics, but that's all it would do.  The BGP FSM is still getting a
NOTIFICATION to drop the session.

I think the headache you're highlighting is we have two conditions we can
get to here:

1. The length is unacceptable, but fits in the PDU.
2. There's an overrrun of the optional paramters vs. the encapsulating
   BGP message.

-- Jeff

---

From: Alvaro Retana <[email-masked]>
Date: Fri, 19 Mar 2021 13:54:09 -0700

Jeff:

Yes, you’re right.

Are you suggesting that we don’t need that extension in this document, or
that we don’t need it at all?  I would be fine with it not being in this
document — not a high priority, but it is something we probably want to get
to at some point.

Thanks!

Alvaro.

On March 19, 2021 at 9:09:46 AM, Jeffrey Haas ([email-masked]) wrote:

Alvaro,

-- Jeff

---

From: Jeffrey Haas <[email-masked]>
Date: Fri, 19 Mar 2021 17:10:36 -0400

Alvaro,

> On Mar 19, 2021, at 4:54 PM, Alvaro Retana <[email-masked]> wrote:
> Yes, you’re right.  
> 
> Are you suggesting that we don’t need that extension in this document, or that we don’t need it at all?  I would be fine with it not being in this document — not a high priority, but it is something we probably want to get to at some point.

Speaking as an individual contributor, adding at least one sub-code would make sense in the document since it potentially exacerbates the existing issue.

It does mean that no implementation of this would be out of the box compliant.  Given IDR's tradition of two interoperable implementations, I'm not sure how we'd proceed.  Certainly for my own code base, it's probably a 5 line diff.  That said, since the sub-code isn't specified, who knows what the other implementations do?

It's a nice to have bit of debugging, but doesn't impact protocol behavior beyond that.  You'd want to trigger an updated WGLC, I think, if such a change happened.  Given the document requires at least one spin to cover your review, there's certainly time.

-- Jeff

---

From: John Scudder <[email-masked]>
Date: Mon, 22 Mar 2021 20:14:48 +0000

Hi Alvaro, thanks for the review. I’ve uploaded -10 to address your comments, see also my replies below.

—John

> On Mar 18, 2021, at 5:54 PM, Alvaro Retana <[email-masked]> wrote:
> 
> [External Email. Be cautious of content]
> 
> 
> Enke/John:
> 
> Hi!
> 

Done.

> [major] Use the template from rfc8174.

Done.

> limited to 4k...which means that the full Extended OP length/Parameter

I’m inclined to leave this alone. I mean, we could go into that nuance, but then if some future specification extends the OPEN, do we have to go back and update this spec too? Better to leave it alone, I think. It is genuinely obvious, IMO, that you can’t encode more than the enclosing PDU format supports.

> guess rfc4271 just assumed that this error would never happen.

RFC 4271 left a lot to be desired in terms of nuanced error codes. I agree that we could introduce a subcode called something like “optional parameter field length mismatch” to be sent if the optional parameters length exceeds the unconsumed OPEN length. However, I’m disinclined to do it in this document, because it’s a pre-existing problem, it exists with the non-extended encoding as well, so any fix for it should really fix both cases. As such, it seems a funny fit for this doc. 

> Also, please be consistent with the field names from the figure.

On reviewing this, I think that paragraph and the following two paragraphs were redundant with the paragraphs that follow the figure. So, I simply removed them, and replaced them with this: 

   However, if the length of Optional Parameters in the BGP OPEN message 
   does exceed 255, the OPEN message MUST be encoded according to the 
   procedure below.

Done.

> Parameters Length (Non-Ext OP Len.)

Done.

Done.

> cases without the justification.

Now that you mention it, these two paragraphs read strangely to me now — it’s unnecessary historical baggage to be going on about why certain choices were made. I’ve removed them and rewritten the first paragraph following the figure as:

  The non-extended Optional Parameters Length field (Non-Ext OP Len)
  SHOULD be set to 255 on transmission and in any event MUST NOT be set to
  0, and MUST be ignored on receipt once the use of the extended format is
  determined positively by inspection of the Non-Extended Optional
  Parameters Type (Non-Ext OP Type) field.

since the only normative text in those paragraphs was the MUST NOT part. 

If anyone feels the paragraphs should be retained for some reason, we could move them to an appendix. But I think students of history can rely on looking at the draft versions and it’s OK to just leave it out altogether.

> [major] "0 MUST NOT be used"  Move this part to where the field is described.

Done. (See language above.)

> [minor] s/may be expected/is expected

OK.

> [] No need to speculate about implementations.  The text above points

OK. I cut that paragraph. It’s similar to the other two I cut, discussed above.

> speaker shouldn't have used the encoding anyway...  ??

I could explain, but instead I’ve just cut that paragraph as well.

> sentence is needed because the first part already says this is not an

OK, I cut "any value SHOULD be silently accepted”. (And you’re right, it should have been a MUST.)

---

From: John Scudder <[email-masked]>
Date: Mon, 22 Mar 2021 20:17:35 +0000

You’ll see in my reply to Alvaro that I demurred on adding a subcode in -10. That said, if it’s the WG consensus that one should be added, that’s fine, just say the word and I’ll do it.

—John

On Mar 19, 2021, at 5:10 PM, Jeffrey Haas <[email-masked]<mailto:[email-masked]>> wrote:

Alvaro,

On Mar 19, 2021, at 4:54 PM, Alvaro Retana <[email-masked]<mailto:[email-masked]>> wrote:
Yes, you’re right.

-- Jeff

---

From: Alvaro Retana <[email-masked]>
Date: Mon, 22 Mar 2021 14:15:43 -0700

On March 22, 2021 at 4:14:55 PM, John Scudder wrote:

John:

Hi!

I'm just replying to this piece -- I'll take a look at the diffs soon.

> I’m inclined to leave this alone. I mean, we could go into that nuance, but
> then if some future specification extends the OPEN, do we have to go back
> and update this spec too? Better to leave it alone, I think. It is genuinely
> obvious, IMO, that you can’t encode more than the enclosing PDU format
> supports.

Can we at least take out "of any length"?  As you say, if someone else
changes the size of the OPEN message, the enclosing PDU will still
limit the size -- and it won't be "of any length".

BTW, now that I'm reading this again (and the next paragraph from
-10), I think the second sentence above is not needed at all (and how
can "MUST be prepared" be enforced anyway?).

OLD>
   In the event that the length of Optional Parameters in the BGP OPEN
   message does not exceed 255, the encodings of the base BGP
   specification [RFC4271] MUST be used without alteration.  However, an
   implementation MUST be prepared to accept an OPEN message that uses
   the encoding of this specification for Optional Parameters of any
   length.

NEW>
   In the event that the length of Optional Parameters in the BGP OPEN
   message does not exceed 255, the encodings of the base BGP
   specification [RFC4271] MUST be used without alteration.

> RFC 4271 left a lot to be desired in terms of nuanced error codes. I agree
> that we could introduce a subcode called something like “optional parameter
> field length mismatch” to be sent if the optional parameters length exceeds
> the unconsumed OPEN length. However, I’m disinclined to do it in this
> document, because it’s a pre-existing problem, it exists with the
> non-extended encoding as well, so any fix for it should really fix both
> cases. As such, it seems a funny fit for this doc.

As I said in my reply to Jeff, I'm ok with that not being included in
this document.

Thanks!

Alvaro.

---

From: Jeffrey Haas <[email-masked]>
Date: Mon, 22 Mar 2021 17:26:40 -0400

> On Mar 22, 2021, at 5:15 PM, Alvaro Retana <[email-masked]> wrote:
> 

And I wouldn't push for it.  Which means there's probably no chairs' conversation to be had. :-)

-- Jeff

---

From: John Scudder <[email-masked]>
Date: Mon, 22 Mar 2021 21:46:46 +0000

Hi Alvaro,

...
> -10), I think the second sentence above is not needed at all

Can you say more about why you think it’s not needed? Without it, I think it would be reasonable for an eager beaver to decide to treat an Extended Optional Parameter Length of (say) 16 as a hard error, because the spec says not to encode short lengths using the extended format. The authors thought it would be overly strict of an implementation to do that, which is why the text says what it does. Apparently the WG agrees with us, at least as far as we can tell by passing a WGLC. I think taking the text out would be a normative change. Furthermore, we might then want to ask ourselves: are we just deliberately underspecifying? Or do we need to say something in the errors section about this (new) error?

However I would be fine with dialing in “of any length” more tightly:

> (and how

How about “(However, an implementation MUST accept an OPEN message that uses the encoding of this specification even if the length of Optional Parameters is 255 or less.)”

This gets rid of “be prepared” as well, which it sounds like you didn’t like.

—John

---

From: Alvaro Retana <[email-masked]>
Date: Tue, 23 Mar 2021 14:06:14 -0700

Ah, yes.

This proposal works for me.

Thanks!

Alvaro.

On March 22, 2021 at 5:46:48 PM, John Scudder ([email-masked]) wrote:

How about “(However, an implementation MUST accept an OPEN message that
uses the encoding of this specification even if the length of Optional
Parameters is 255 or less.)”

This gets rid of “be prepared” as well, which it sounds like you didn’t
like.

---

From: John Scudder <[email-masked]>
Date: Wed, 24 Mar 2021 00:55:57 +0000

Done, in -11.

(I’m so glad I found out the trick to decoding your secret messages. ;-)

—John

On Mar 23, 2021, at 5:06 PM, Alvaro Retana <[email-masked]<mailto:[email-masked]>> wrote:

Ah, yes.

Thanks!

Alvaro.

On March 22, 2021 at 5:46:48 PM, John Scudder ([email-masked]<mailto:[email-masked]>) wrote:

---

From: Alvaro Retana <[email-masked]>
Date: Fri, 26 Mar 2021 12:10:50 -0700

John:

Hi!

I just started the IETF LC.

Thanks!

Alvaro.

On March 23, 2021 at 8:56:04 PM, John Scudder ([email-masked]) wrote:

Done, in -11.

—John

On Mar 23, 2021, at 5:06 PM, Alvaro Retana <[email-masked]> wrote:

Ah, yes.

Thanks!

Alvaro.

---

From: Enke Chen <[email-masked]>
Date: Fri, 26 Mar 2021 15:21:27 -0700

Hi, John, Alvaro, Jeff and Sue:

Thanks a lot for your help in driving the draft to completion!

-- Enke