Subject: [secdir] Secdir last call review of draft-ietf-idr-bgp-ls-segment-routing-ext-14

From: Paul Wouters via Datatracker <[email-masked]>
Date: Thu, 23 May 2019 08:10:02 -0700

Reviewer: Paul Wouters
Review result: Has Issues

Reviewer: Paul Wouters
Review result: Has Issues

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the
IESG. These comments were written primarily for the benefit of the
security area directors. Document editors and WG chairs should treat
these comments just like any other last call comments.

Note: I'm not a segment routing or BGP expert, so I might have
misunderstood some things. As far as I can understand the Security
Considerations, it seems that it is adequately pointing to other documents
and mentions, with solutions, new concerns raised by implememting this
document.

I do have a number of comments and suggested improvements for the
tables and figures layout, and some questions about IANA registries.
See below for details.

Paul

Section 2.1

Can the IANA registry be referenced here so the reader can easilly go to
see the entire list of Node Attribute TLVs ?

Section 2.1.1

I personally find Figures to have more readable with less -+-+-+-+
symbols eg to change Figure 2 to:

    0                   1                   2                   3
    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
   +-------------------------------+-------------------------------+
   |               Type            |            Length             |
   +-------------------------------+-------------------------------+
   ~                        SID/Label (variable)                   ~
   +---------------------------------------------------------------+

Note that since the SID/Label is a variable length, I used the "~"
symbol as in common to donate that. And important in this case too,
as I was thinking it was a fixed size but reading the description
found out it was either 3 or 4 bytes. I see the document uses //
for this elsewhere, which is also okay.

	Type: 1161

I would write:

	Type: set to 1161 denoting a SID/Label Node Attribute

	Length: Either 3 or 4 depending whether [...]

I find this language confusing. It suggests the Length field can be
of size 3 or 4, instead of saying the Length field value can be 3 or 4.

	then the 20 rightmost bits represent a label

Should it be specified what to do with the 4 leftmost bits? Are they
reserved? used for something else ?

Section 2.1.2

I find the split Range Size vs / SID/Label confusing. The table is
supposed to give me an idea of the byte stream, but by being split it
two it doesn't do that well. How about this:

    0                   1                   2                   3
    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
   +-------------------------------+-------------------------------+
   |               Type            |          Length               |
   +---------------+---------------+-------------------------------+
   |      Flags    |   Reserved    |  Range Size, or               |
   +---------------+---------------+                               |
   ~                            SID/Label sub-TLV                  ~
   +---------------------------------------------------------------+

I find this bit a little misleading in our way of writing it:

   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                  Range Size                   |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   //                SID/Label sub-TLV (variable)                 //
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Because I don't think Range Size is really 20 bits and the next 4
bits can be used for another payload. I assume those unspecified 4 bits
are reserved and set to 0?

	Type: 1034

I would write:

	Type: set to 1034 denoting an SR Capabilities Node Attribute

	Length: Variable.  Minimum length is 12.

Again, it find this confusing. The length field itself is not variable
length and has no minimum length of 12. I would write:

	Length: two octects in network order, specifying the length of
	        the Range Size or SID/Label sub-TLV

	Reserved: 1 octet that SHOULD be set to 0 and MUST be ignored on
	receipt.

Why is that SHOULD not a MUST?

	One or more entries, each of which have the following format:

Is this really "one or more entries"? The table above does not show that
at all. Can we only have more entries of the same capabilities or can they
be mixed (eg one range size and two SID/Labels ??) If there is more than
one entry, how would one know whether these are RAnge Sizes or SID/Labels?
If there is only one, then the Length denotes that implicitely.

	Since the SID/Label sub-TLV is
	used to indicate the first label of the SRGB range, only label
	encoding is valid under the SR Capabilities TLV.

Does this mean the above "one or more entries" is actually restricted and
means "one or more rang sizes, followed by 0 or 1 SID/Labels" ?

The ambiguity perceived means I can not fully deducate the field
contents as it is currently specified.

Section 2.1.3

Similar remarks to the sections above here regarding the field descriptions.
I would enclose Algorithm 1, since 1 is the minimum and close the table,
since our content description does end (based on the Length field)

    0                   1                   2                   3
    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
   +-------------------------------+-------------------------------+
   |            Type               |            Length             |
   +---------------+---------------+---------------+---------------+
   |  Algorithm 1  |  Algorithm... ~  Algorithm N  ~               |
   +---------------+----                                           |
   ~                                                               ~
   +---------------------------------------------------------------+

Section 2.1.4

See my similar comments above. Additionally, one Range Size and SID label
should be added to the diagram unconditionally, as the minimum is one,
and then the block follows of optional additional blocks of range size
plus SID/Label.

Section 2.1.5

See similar comments above.

Section 2.2

	These TLVs should only be added to [...]

Should that "should" be a SHOULD (or MUST?)

Section 2.2.1

See similar comments above that apply to this table and values. Here
it is even more important because Length appears even more variable
because Flags is described as a static "one octet". And then "weight"
isn't given an octet size but the following Reserved field is.

Should the SHOULD in the reserved field description be a MUST ?
Especially for reserved fields, I see no reason why it is not a MUST. If
this is left unchecked by an implementation and possibly filled with bogus
data, while that will not break anything NOW, as the document also states
"MUST ignore", but once any of these fields get defined in the future, it
would break. So it is better to dictate now to not leave it with potential
garbage values.

[The Following sections all have the same characteristics as the above ones,
 so I won't mention these further in the review, but I think these should
 be fixed similarly.]

Section 2.2.3

This table seems to extend an existing table in what I hope is some
kind of IANA registry? Can that be referenced here? If there is no
IANA registry of these, and this seems to extend a list of entries from
RFC 7752 and RFC 8571, I would suggest this document creates that new
IANA registry and populates it with all those values.

Section 2.3

Similar here, should this be a new IANA Registry?

Section 2.3.4

If the Prefix NLRI is not considered a "normal routing prefix", what is
it considered as? What implications does that have?

	need to be interpreted accordingly

Is that "need to" not a MUST ? or perhaps rephrase this as:

   The Flags field of this TLV are interpreted accordingly to the
   respective underlying IS-IS, OSPFv2 or OSPFv3 protocol.  

Section 3

We normally don't say early code points were assigned. We just say what we
request(ed) from IANA. Also, can the registies named be linked to IANA?

"should be left empty" -> "is left empty" 

The table in Section 3.1 seems to extend an existing table/registry. Can
it be named and linked so the reader can jump the the IANA registry ?

NITS:

Abstract:

This draft -> This document

Introduction:

This sentence seems malformed:

	A segment can
	represent any instruction, topological or service-based.

or global within a domain. -> or a global semantic within a domain.

Within IGP topologies an SR path -> Within IGP topologies, an SR path

Use of "segement" vs "Segment" in the Introduction is inconsistent

Figure 1 describes -> Figure 1 denotes

then can -> can

Section 2.1.1

as a label or an index/SID. -> as a label or as an index/SID

Section 2.2.1

	The Protocol-ID of the BGP-LS Link
	NLRI should be used to determine the underlying protocol
	specification for parsing these fields.

I would change the "should be" to an "is", as there is no chocie here. That is
how it is. Similar in other sections, such as 2.3.1

Section 2.3.4

is considered as a normal routing -> is considered a normal routing

---

From: "Ketan Talaulikar (ketant)" <[email-masked]>
Date: Thu, 30 May 2019 13:38:07 +0000

Hi Paul,

Thanks for your review and apologies for the delay in getting back to you on this.

I've just posted v15 of the draft to address your comments below.

https://datatracker.ietf.org/doc/html/draft-ietf-idr-bgp-ls-segment-routing-ext-15

Please also check inline below for more detailed response to all your comments.

-----Original Message-----
From: Idr <[email-masked]> On Behalf Of Paul Wouters via Datatracker
Sent: 23 May 2019 20:40
To: [email-masked]
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: [Idr] Secdir last call review of draft-ietf-idr-bgp-ls-segment-routing-ext-14

Reviewer: Paul Wouters

Reviewer: Paul Wouters

I have reviewed this document as part of the security directorate's ongoing effort to review all IETF documents being processed by the IESG. These comments were written primarily for the benefit of the security area directors. Document editors and WG chairs should treat these comments just like any other last call comments.

Note: I'm not a segment routing or BGP expert, so I might have misunderstood some things. As far as I can understand the Security Considerations, it seems that it is adequately pointing to other documents and mentions, with solutions, new concerns raised by implememting this document.

I do have a number of comments and suggested improvements for the tables and figures layout, and some questions about IANA registries.
See below for details.

Paul

Section 2.1

Can the IANA registry be referenced here so the reader can easilly go to see the entire list of Node Attribute TLVs ?
[KT] This is the entire list of new Node Attribute TLVs introduced in this document. There is only a single IANA registry for all TLVs for BGP-LS - based on your further comments, I've updated the BGP-LS Registry name in the IANA section 3. I don't think it is required to put IANA references all over the document. FYI - the registry is at https://www.iana.org/assignments/bgp-ls-parameters/bgp-ls-parameters.xhtml 

Section 2.1.1

I personally find Figures to have more readable with less -+-+-+-+ symbols eg to change Figure 2 to:

    0                   1                   2                   3

[KT] When TLVs are drawn with a reference to a 32 bit scheme, I've always seen the representation being made as is done in this document (I guess we see the bits better when we do so!). E.g. BGP base spec RFC4271 or BGP-LS base RFC7752 and tons of other RFCs.

Note that since the SID/Label is a variable length, I used the "~"
symbol as in common to donate that. And important in this case too, as I was thinking it was a fixed size but reading the description found out it was either 3 or 4 bytes. I see the document uses // for this elsewhere, which is also okay.

[KT] Agree. I've fixed the pictures to use // consistently in the figures for variable sizes.

	Type: 1161

I would write:

[KT] This section describes the SID/Label Node TLV and so this is implied. It goes similar for all other TLVs as well.

I find this language confusing. It suggests the Length field can be of size 3 or 4, instead of saying the Length field value can be 3 or 4.

[KT] The size of type and length fields are fixed in the protocol. It is also shown in the pictorial representation. What is being specified is the value in both the cases so there should be no confusion for anyone familiar with BGP/BGP-LS.

Should it be specified what to do with the 4 leftmost bits? Are they reserved? used for something else ?
[KT] Since the MPLS label is of size 20 bits, this field cannot have a value greater than one represented by those 20 bits. The leftmost 4 bits would therefore be 0. I have updated the text now to clarify this. 

Section 2.1.2

I find the split Range Size vs / SID/Label confusing. The table is supposed to give me an idea of the byte stream, but by being split it two it doesn't do that well. How about this:

    0                   1                   2                   3

[KT] Please check the explanation in text below the figure. It is split into two because there can be one or more sets of "Range + SID/Label sub-TLV". I have updated the figure to illustrate this better.

Because I don't think Range Size is really 20 bits and the next 4 bits can be used for another payload. I assume those unspecified 4 bits are reserved and set to 0?

[KT] Range size is 3 octets - i.e. 24 bits. After that is the SID/Label sub-TLV as described in sec 2.1.1.

	Type: 1034

I would write:

Again, it find this confusing. The length field itself is not variable length and has no minimum length of 12. I would write:

[KT] I've clarified in a previous comment and also updated the figure in the document to illustrate this better.

[KT] Because it is ignored by the receiver. Also see further below for a similar comment.

Is this really "one or more entries"? The table above does not show that at all. Can we only have more entries of the same capabilities or can they be mixed (eg one range size and two SID/Labels ??) If there is more than one entry, how would one know whether these are RAnge Sizes or SID/Labels?
If there is only one, then the Length denotes that implicitely.

Does this mean the above "one or more entries" is actually restricted and means "one or more rang sizes, followed by 0 or 1 SID/Labels" ?

The ambiguity perceived means I can not fully deducate the field contents as it is currently specified.

[KT] I hope the updated figure and my previous comment will help clarify. 

Section 2.1.3

Similar remarks to the sections above here regarding the field descriptions.
I would enclose Algorithm 1, since 1 is the minimum and close the table, since our content description does end (based on the Length field)

    0                   1                   2                   3

[KT] I've updated the figure and the description. I hope it clarifies.

Section 2.1.4

See my similar comments above. Additionally, one Range Size and SID label should be added to the diagram unconditionally, as the minimum is one, and then the block follows of optional additional blocks of range size plus SID/Label.

[KT] Updated the figure as in previous section.

Section 2.1.5

[KT] I do not understand this comment but please check the updated draft and if this is addressed.

Section 2.2

[KT] It was not intended to be normative.

Section 2.2.1

See similar comments above that apply to this table and values. Here it is even more important because Length appears even more variable because Flags is described as a static "one octet". And then "weight"
isn't given an octet size but the following Reserved field is.

[KT] I've updated the weight field to indicate it's 1 octet size.

Should the SHOULD in the reserved field description be a MUST ?
Especially for reserved fields, I see no reason why it is not a MUST. If this is left unchecked by an implementation and possibly filled with bogus data, while that will not break anything NOW, as the document also states "MUST ignore", but once any of these fields get defined in the future, it would break. So it is better to dictate now to not leave it with potential garbage values.

[KT] This is something that has been used for reserved field and reserved bits in many IETF RFCs/drafts that I've known. If an implementation is not following the SHOULD and instead filing bogus data then it better have a good reason to do so. Perhaps it might be using that reserved field for some purpose before it is officially allocated by IANA/IETF action?

[The Following sections all have the same characteristics as the above ones,  so I won't mention these further in the review, but I think these should  be fixed similarly.]

Section 2.2.3

This table seems to extend an existing table in what I hope is some kind of IANA registry? Can that be referenced here? If there is no IANA registry of these, and this seems to extend a list of entries from RFC 7752 and RFC 8571, I would suggest this document creates that new IANA registry and populates it with all those values.

[KT] Please refer to my previous comment on IANA registry and the updated text for IANA section in the new version. Also https://www.iana.org/assignments/bgp-ls-parameters/bgp-ls-parameters.xhtml 

Section 2.3

Similar here, should this be a new IANA Registry?
[KT] Sec 3 explains the IANA aspects - there is no new registry being introduced.

Section 2.3.4

If the Prefix NLRI is not considered a "normal routing prefix", what is it considered as? What implications does that have?
[KT] It is considered as a prefix used for the advertisement of the mapping range. I've updated the text to clarify this.

[KT] Ack - will rephrase as suggested.

Section 3

[KT] These code points were actually assigned via early allocation process and we've been asked to explicitly clarify the status of assignments as the document progresses through the WG and IESG. I am not sure what you mean by "linked to IANA". Perhaps it was because we did not say that this is from the BGP-LS Parameter's registry? If so, I've updated the text to indicate this.

"should be left empty" -> "is left empty" 
[KT] Since this is the request for IANA action, the language is so.

The table in Section 3.1 seems to extend an existing table/registry. Can it be named and linked so the reader can jump the the IANA registry ?

[KT] It is named and I've also updated to specifically indicate the BGP-LS Parameter's top level registry. I've not seen URLs used thus, if that is what you meant by "linked".

NITS:

Abstract:

[KT] Ack - fixed.

Introduction:

	A segment can
	represent any instruction, topological or service-based.
[KT] Fixed. Replace "," with ";".

[KT] Fixed.

[KT] Fixed.

[KT] Fixed.

[KT] Fixed.

then can -> can
[KT] Fixed.

Section 2.1.1

[KT] fixed.

Section 2.2.1

I would change the "should be" to an "is", as there is no chocie here. That is how it is. Similar in other sections, such as 2.3.1
[KT] Fixed.

Section 2.3.4

[KT] Fixed.

Thanks,
Ketan

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