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 <idr-bounces@xxxxxxxx> On Behalf Of Paul Wouters via Datatracker Sent: 23 May 2019 20:40 To: secdir@xxxxxxxx Cc: idr@xxxxxxxx; ietf@xxxxxxxx; draft-ietf-idr-bgp-ls-segment-routing-ext.all@xxxxxxxx Subject: [Idr] Secdir last call review of draft-ietf-idr-bgp-ls-segment-routing-ext-14 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 ? [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 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) ~ +---------------------------------------------------------------+ [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: Type: set to 1161 denoting a SID/Label Node Attribute [KT] This section describes the SID/Label Node TLV and so this is implied. It goes similar for all other TLVs as well. 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. [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. 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 ? [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 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 ~ +---------------------------------------------------------------+ [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. 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? [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: 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 [KT] I've clarified in a previous comment and also updated the figure in the document to illustrate this better. Reserved: 1 octet that SHOULD be set to 0 and MUST be ignored on receipt. Why is that SHOULD not a MUST? [KT] Because it is ignored by the receiver. Also see further below for a similar comment. 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. [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 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 ~ | +---------------+---- | ~ ~ +---------------------------------------------------------------+ [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 See similar comments above. [KT] I do not understand this comment but please check the updated draft and if this is addressed. Section 2.2 These TLVs should only be added to [...] Should that "should" be a SHOULD (or MUST?) [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. 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. [KT] Ack - will rephrase as suggested. 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? [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: This draft -> This document [KT] Ack - fixed. Introduction: This sentence seems malformed: A segment can represent any instruction, topological or service-based. [KT] Fixed. Replace "," with ";". or global within a domain. -> or a global semantic within a domain. [KT] Fixed. Within IGP topologies an SR path -> Within IGP topologies, an SR path [KT] Fixed. Use of "segement" vs "Segment" in the Introduction is inconsistent [KT] Fixed. Figure 1 describes -> Figure 1 denotes [KT] Fixed. then can -> can [KT] Fixed. Section 2.1.1 as a label or an index/SID. -> as a label or as an index/SID [KT] fixed. 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 [KT] Fixed. Section 2.3.4 is considered as a normal routing -> is considered a normal routing [KT] Fixed. Thanks, Ketan _______________________________________________ Idr mailing list Idr@xxxxxxxx https://www.ietf.org/mailman/listinfo/idr
