Subject: Re: [Idr] Routing directorate QA review of draft-ietf-idr-ext-opt-param-04

From: Susan Hares <[email-masked]>
Date: Wed, 22 Jun 2016 18:34:33 -0400

Matthew
Thank you for the QA review.  The authors will get back to you with their changes.Sue

Sent via the Samsung Galaxy Note5, an AT&T 4G LTE smartphone-------- Original message --------From: "Bocci, Matthew (Nokia - GB)" <[email-masked]> Date: 6/21/2016  6:10 AM  (GMT-05:00) To: [email-masked], Susan Hares <[email-masked]>, [email-masked], [email-masked], [email-masked] Subject: [Idr] Routing directorate QA review of	draft-ietf-idr-ext-opt-param-04 

Authors,
 
I have been asked to do a QA review of draft-ietf-idr-ext-opt-param-04.txt.

Summary: 
 
The document is reasonably straight forward and is well written. I have a few comments below.

Comments:
 
Minor Issues:
 
1) Section 2, Protocol Extensions.

You have labelled the existing Length and Type fields as 0xFF. I assume the meaning of the second is still 'Type' since that is

what any implementation would reasonably interpret it as, and that is the registry you are using a code point from. So it

might be better to say in the text above the figure at the top of page 3 that the length and type fields in [RFC4271]

are set to 0xFF.
 
Also, you don't explicitly define what a receiver should do with the length field if the type is 0xFF. Does it ignore it,

or does it check that it is 0xFF and treat the OPEN message as malformed if it is < 0xFF?
 
Since the document changes the procedures in RFC4271 for BGP Open optional parameters where length > 255, in that the

original length field is no longer to be interpreted as the actual length, then I think you should mark this draft as

'Updates: 4271'.
 
2) Section 5: Security Extensions
The security considerations section seems to be lacking detail and amounts to one line:

 "This extension to BGP does not change the underlying security issues"

 It might be worth being a little more explicit, or at least use wording similar to RFC5492, and saying that it does not

 add any new security issues that are not inherent in BGP [RFC4272]. 

Regards
 
Matthew

---

From: "John G. Scudder" <[email-masked]>
Date: Fri, 24 Jun 2016 14:43:26 -0400

Hi Matthew and all,

Thanks for the review. Some comments below.

> From: "Bocci, Matthew (Nokia - GB)" <[email-masked]>
> Date: 6/21/2016 6:10 AM (GMT-05:00)
> To: [email-masked], Susan Hares <[email-masked]>, [email-masked], [email-masked], [email-masked]
> Subject: [Idr] Routing directorate QA review of	draft-ietf-idr-ext-opt-param-04
...

> might be better to say in the text above the figure at the top of page 3 that the length and type fields in [RFC4271]

Is your proposal to use the diagram verbatim from 4271, but say in the prose that the type and length are 0xFF? I'm fine with whatever seems clearer to folks.

> or does it check that it is 0xFF and treat the OPEN message as malformed if it is < 0xFF?

This is a good question, and TBH I don't recall why we specified a value for the legacy length field instead of simply saying it should be ignored.
Maybe Enke remembers? Right now I'm inclined to say it should be ignored on receipt, but I'm open to discussion. I guess even if we make that change we can still spec it be sent as 0xFF, for purposes of debuggability, but this is not a big deal.

> original length field is no longer to be interpreted as the actual length, then I think you should mark this draft as

Good point, will fix.

>  add any new security issues that are not inherent in BGP [RFC4272]. 

Good point, will change.

Thanks,

--John

---

From: Enke Chen <[email-masked]>
Date: Fri, 24 Jun 2016 15:29:18 -0700

Hi, John:

Please see below.

> Maybe Enke remembers? Right now I'm inclined to say it should be ignored on receipt, but I'm open to discussion. I guess even if we make that change we can still spec it be sent as 0xFF, for purpos…

Here is what RFC 4271 says about the "Optional Parameters Length": 

         This 1-octet unsigned integer indicates the total length of the
         Optional Parameters field in octets.  If the value of this
         field is zero, no Optional Parameters are present.

If the "Optional Parameters Length" field is 0, then the Optional Parameters
will not be parsed and thus the new length field would not be reached.

I am not sure, but that might be the reason that we have it as 0xFF.

It needs to be at least 1 for the "Parm. Type" field to be parsed.  Once the
"Parm. type" is parsed, and if its value is 0xFF, then the two-octet length
field follows.  The procedure is more than just "ignoring" the field. 

Thanks.  -- Enke

---

From: "Jakob Heitz (jheitz)" <[email-masked]>
Date: Sun, 26 Jun 2016 23:39:06 +0000

How about:

"An optional parameter with parameter type of 0xFF MUST NOT appear in the
list of optional parameters other than in first place and if it does appear,
then the optional parameters length MUST be 0xFF."

There is really no reason to entertain other combinations and I don't
want to have to write code to handle them.

Thanks,
jakob.

> -----Original Message-----
> From: Idr [mailto:[email-masked]] On Behalf Of Enke Chen (enkechen)
> Sent: Friday, June 24, 2016 3:29 PM
> To: John G. Scudder <[email-masked]>
> Cc: [email-masked]; [email-masked]; [email-masked]; Bocci, Matthew (Nokia - GB)
> <[email-masked]>; Susan Hares <[email-masked]>
> Subject: Re: [Idr] Routing directorate QA review of draft-ietf-idr-ext-opt-param-04
> 
> Hi, John:
> 
> Please see below.
> 
> > Is your proposal to use the diagram verbatim from 4271, but say in the prose that the type and length are 0xFF?
> I'm fine with whatever seems clearer to folks.
> >
> > Maybe Enke remembers? Right now I'm inclined to say it should be ignored on receipt, but I'm open to discussion. I
> guess even if we make that change we can still spec it be sent as 0xFF, for purposes of debuggability, but this is
> not a big deal.
> >
> 
> _______________________________________________
> Idr mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/idr

---

From: "John G. Scudder" <[email-masked]>
Date: Mon, 27 Jun 2016 11:49:51 -0400

> Is your proposal to use the diagram verbatim from 4271, but say in the prose that the type and length are 0xFF? I'm fine with whatever seems clearer to folks.

Upon reviewing the draft, I prefer to leave the diagram as is. The context is that the diagram is preceded by "the OPEN message format is modified to be the following", and then the diagram is given:

...blah blah...
       |                         BGP Identifier                        |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       |      0xFF     |      0xFF     |  Extended Opt. Parm. Length   |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       |                                                               |
       |             Optional Parameters (variable)                    |
       |                                                               |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

By contrast, here's the diagram from RFC 4271:

...blah blah...
       |                         BGP Identifier                        |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       | Opt Parm Len  |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       |                                                               |
       |             Optional Parameters (variable)                    |
       |                                                               |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

If we want to keep the RFC 4271 diagram verbatim, I think quite a bit of rewrite would ensue and it probably wouldn't result in a more readable document, especially from the perspective of a person implementing the new spec. And of course, someone implementing stock 4271 doesn't need to refer to the draft. In looking back at your note, I think the disconnect is here:

>> what any implementation would reasonably interpret it as, and that is the registry you are using a code point from. 

Upon consideration, I'd say no. In the context of the draft (as you point out, the proposed update to 4271), the two fields constitute a 16-bit field which, when set to all ones, indicates the next sixteen bits are to be used as a length field, and the sixteen bits after that to be used as a type field. In a legacy implementation, it's just as you say -- those sixteen bits are length and type.

My suggestion is instead of modifying the diagram, we could revise the prefatory text as follows.

OLD:
Accordingly, when the length of Optional Parameters in the BGP OPEN
message is greater than 255, the OPEN message format is modified to 
be the following:

NEW:
Accordingly, when the length of Optional Parameters in the BGP OPEN
message is greater than 255, the OPEN message format is modified as
follows, repurposing the Optional Parameters Length field as well as
the first Parameter Type field to indicate the use of the extended 
format:

Seem reasonable? The question is addressed to the entire WG, not just Matthew. In particular, it would be interesting to hear from implementors as to whether the spec is clear and unambiguous as to when and how to use the extended format.

Thanks,

--John

---

From: "John G. Scudder" <[email-masked]>
Date: Mon, 27 Jun 2016 12:25:07 -0400

> then the optional parameters length MUST be 0xFF."

And it's a fatal error if it's not?

(Remember this is before the session becomes established, so the principle of service survival above all that would applied in the error-handling spec doesn't apply.)

> want to have to write code to handle them.

Is there some reason insisting that the non-extended length field be 255 is better than ignoring it once you've determined by inspection of the non-extended parameter type that the extended encoding is in use? The logic would be, if the (one byte, non-extended) parameter type = 255, then use the extended encoding, and ignore the one byte non-extended length field.

If we made this change, the spec would say, MUST send non-extended length as 255, MUST ignore on receipt if the following eight bits (the non-extended type field) are 255.

Thanks,

--John

---

From: "Jakob Heitz (jheitz)" <[email-masked]>
Date: Mon, 27 Jun 2016 17:26:26 +0000

> From: John G. Scudder [mailto:[email-masked]]
> Sent: Monday, June 27, 2016 9:25 AM
> To: Jakob Heitz (jheitz) <[email-masked]>
> Cc: Enke Chen (enkechen) <[email-masked]>; [email-masked]; [email-masked]; [email-masked];
> Bocci, Matthew (Nokia - GB) <[email-masked]>; Susan Hares <[email-masked]>

Absolutely. As you say, the session is not up yet.

> 
> (Remember this is before the session becomes established, so the principle of service survival above all that would
> applied in the error-handling spec doesn't apply.)
> 
> Is there some reason insisting that the non-extended length field be 255 is better than ignoring it once you've
> determined by inspection of the non-extended parameter type that the extended encoding is in use? The logic would be,
> if the (one byte, non-extended) parameter type = 255, then use the extended encoding, and ignore the one byte non-
> extended length field.
> 
> If we made this change, the spec would say, MUST send non-extended length as 255, MUST ignore on receipt if the
> following eight bits (the non-extended type field) are 255.

To Enke's point, can we make the non-extended length at least 1?
Legacy speakers...

In addition: An otional parameter of type 255 MUST NOT appear in the
list of optional parameters other than in first place.

Should we set a maximum length of 4k?
There will always be smaller boxes wishing to speak BGP that will never support extended messages.

---

From: Enke Chen <[email-masked]>
Date: Mon, 27 Jun 2016 10:26:57 -0700

Hi, John:

I now remember the reason for having 0xFF there.  It was something that we discussed years ago.

It was something like: since the actual length is larger than 255 and there is only one octet,
we just go with the largest possible value 255.  (The value does not matter once the real length
field is reached.)

Thanks.  -- Enke

---

From: "John G. Scudder" <[email-masked]>
Date: Mon, 27 Jun 2016 14:52:43 -0400

> Absolutely. As you say, the session is not up yet.

I don't really mind doing this. I also don't mind not doing it, since it's basically a superfluous check -- the Optional Parameter Type is sufficient to fully determine that the message uses the extended format. My slight preference is for not doing it, since it's a few more lines of code (for the check), for a condition that is exceedingly unlikely to occur, and doesn't matter if it does occur. So why spend the code?

> To Enke's point, can we make the non-extended length at least 1?

Yes, you will see my suggestion was to make the length 255 on Tx (as you and Enke point out, it has to be >= 1, so might as well choose 255 as any other value, and it jumps out nicely in a packet dump). Up for discussion was whether to enforce the value on Rx, or ignore. You've spoken in favor of enforcing it, although I'm not sure what the reason for your preference is. 

> list of optional parameters other than in first place.

Good point, thanks. I'll work something like that in.

> There will always be smaller boxes wishing to speak BGP that will never support extended messages.

I'm inclined to leave this alone -- if you can fit in <= 4k you will. If you can't fit in <= 4k, a peering with such a box isn't going to work regardless. So either it'll be fine anyway, or it won't work nohow.

--John

---

From: "Jakob Heitz (jheitz)" <[email-masked]>
Date: Mon, 27 Jun 2016 19:24:08 +0000

Fine. Works for me.

Thanks,
Jakob.

> Sent: Monday, June 27, 2016 11:53 AM
> I don't really mind doing this. I also don't mind not doing it, since it's basically a superfluous check -- the
> Optional Parameter Type is sufficient to fully determine that the message uses the extended format. My slight
> preference is for not doing it, since it's a few more lines of code (for the check), for a condition that is
> exceedingly unlikely to occur, and doesn't matter if it does occur. So why spend the code?
> 
> Yes, you will see my suggestion was to make the length 255 on Tx (as you and Enke point out, it has to be >= 1, so
> might as well choose 255 as any other value, and it jumps out nicely in a packet dump). Up for discussion was
> whether to enforce the value on Rx, or ignore. You've spoken in favor of enforcing it, although I'm not sure what
> the reason for your preference is.
> 
> I'm inclined to leave this alone -- if you can fit in <= 4k you will. If you can't fit in <= 4k, a peering with such
> a box isn't going to work regardless. So either it'll be fine anyway, or it won't work nohow.
> 
> --John

---

From: "John G. Scudder" <[email-masked]>
Date: Tue, 28 Jun 2016 22:18:53 -0400

[removed rtg-dir from cc, seemed excessive]

-05 posted which I believe addresses all comments to date. Please take a look. I've adjusted the diagram per (my understanding of) Matthew's comments. I'm ambivalent about it -- I'm not sure it's clearer, but I did manage to increase the page count :-o. I could go either way, if you have a distinct preference please say so.

--John

> On Jun 27, 2016, at 3:24 PM, Jakob Heitz (jheitz) <[email-masked]> wrote:
> 
> Fine. Works for me.
> 
> Thanks,
> Jakob.
>

---

From: "Jakob Heitz (jheitz)" <[email-masked]>
Date: Wed, 29 Jun 2016 02:34:42 +0000

Looks good to me.

Thanks,
Jakob.

> Sent: Tuesday, June 28, 2016 7:19 PM
> To: [email-masked]
> Cc: Enke Chen (enkechen) <[email-masked]>; [email-masked]; Bocci, Matthew (Nokia - GB)
> <[email-masked]>; Susan Hares <[email-masked]>; Jakob Heitz (jheitz) <[email-masked]>
> -05 posted which I believe addresses all comments to date. Please take a look. I've adjusted the diagram per (my
> understanding of) Matthew's comments. I'm ambivalent about it -- I'm not sure it's clearer, but I did manage to
> increase the page count :-o. I could go either way, if you have a distinct preference please say so.
> 
> --John
> 
> >>>> determined by inspection of the non-extended parameter type that the extended encoding is in use? The logic
> would
> >> be,

---

From: "Bocci, Matthew (Nokia - GB)" <[email-masked]>
Date: Wed, 29 Jun 2016 16:02:19 +0000

Looks good to me too. Thank you for addressing my comments.

Matthew

On 29/06/2016, 03:34, "Jakob Heitz (jheitz)" <[email-masked]> wrote:

Looks good to me.

Thanks,
Jakob.