[Pce] AD review of draft-ietf-pce-binding-label-sid-10

[John Scudder <[e-mail]>]

Dear Authors,

Here’s my review of this document. Overall I think it is close to ready, I have a few questions I’d like answered and some editorial changes I’ve suggested, but none of it should be too difficult. 

I’ve supplied my comments in the form of an edited copy of the draft. You can use your favorite diff tool to review them; I’ve attached a PDF of the rfcdiff output for your convenience if you’d like to use it. I’ve also pasted a traditional diff below in case you want to use it for in-line reply. I’d appreciate feedback regarding whether you found this a useful way to receive my comments as compared to a more traditional numbered list of comments with selective quotation from the draft.

Thanks,

—John

P.S.: Thanks to Julien for the helpful shepherd writeup.


*** draft-ietf-pce-binding-label-sid-10.txt	2021-09-28 13:33:58.000000000 -0400
--- draft-ietf-pce-binding-label-sid-10-jgs-markup.txt	2021-09-28 15:13:00.000000000 -0400
***************
*** 133,138 ****
--- 133,141 ----
     As described in [RFC8402], a Binding Segment Identifier (BSID) is
     bound to a Segment Routing (SR) Policy, instantiation of which may
     involve a list of SIDs.  Any packets received with an active segment
+                      ^^^^
+                      Please expand SID on first use
+                      
     equal to a BSID are steered onto the bound SR Policy.  A BSID may be
     either a local (SR Local Block (SRLB)) or a global (SR Global Block
     (SRGB)) SID.  As per Section 6.4 of
***************
*** 159,167 ****
  
     A binding label/SID has local significance to the ingress node of the
     corresponding TE path.  When a stateful PCE is deployed for setting
!    up TE paths, it may be desirable for PCC to report the binding label/
     SID to the stateful PCE for the purpose of enforcing end-to-end TE/SR
!    policy.  A sample Data Center (DC) use-case is illustrated in the
  
  
  
--- 162,170 ----
  
     A binding label/SID has local significance to the ingress node of the
     corresponding TE path.  When a stateful PCE is deployed for setting
!    up TE paths, it may be desirable for a PCC to report the binding label/
     SID to the stateful PCE for the purpose of enforcing end-to-end TE/SR
!    policy.  A sample Data Center (DC) use-case is illustrated in 
  
  
  
***************
*** 178,187 ****
     the PCE.  In order for the access node to steer the traffic over the
     SR-TE LSP, the PCE passes the SID stack {Y, X} where Y is the prefix
     SID of the gateway node-1 to the access node.  In the absence of the
!    binding SID X, the PCE should pass the SID stack {Y, A, B, C, D} to
!    the access node.  This example also illustrates the additional
!    benefit of using the binding SID to reduce the number of SIDs imposed
!    on the access nodes with a limited forwarding capacity.
  
  
             SID stack
--- 181,193 ----
     the PCE.  In order for the access node to steer the traffic over the
     SR-TE LSP, the PCE passes the SID stack {Y, X} where Y is the prefix
     SID of the gateway node-1 to the access node.  In the absence of the
!    binding SID X, the PCE would pass the SID stack {Y, A, B, C, D} to
!    the access node.  This example illustrates the
!    benefit of using the binding label/SID to reduce the number of SIDs imposed
!    on access nodes with a limited forwarding capacity.
!    ^^
!    do you mean imposed on, or imposed by? If the access nodes 
!    are doing the imposing, I think this should say "imposed by"
  
  
             SID stack
***************
*** 205,211 ****
  
  
  
!                 Figure 1: A sample Use-case of Binding SID
  
     A PCC could report to the stateful PCE the binding label/SID it
     allocated via a Path Computation LSP State Report (PCRpt) message.
--- 211,217 ----
  
  
  
!                 Figure 1: A Sample Use-case of Binding SID
  
     A PCC could report to the stateful PCE the binding label/SID it
     allocated via a Path Computation LSP State Report (PCRpt) message.
***************
*** 271,277 ****
  4.  Path Binding TLV
  
     The new optional TLV called "TE-PATH-BINDING TLV" (whose format is
!    shown in the Figure 2) is defined to carry the binding label/SID for
     a TE path.  This TLV is associated with the LSP object specified in
     [RFC8231].  This TLV can also be carried in the PCEP-ERROR object
  
--- 277,283 ----
  4.  Path Binding TLV
  
     The new optional TLV called "TE-PATH-BINDING TLV" (whose format is
!    shown in Figure 2) is defined to carry the binding label/SID for
     a TE path.  This TLV is associated with the LSP object specified in
     [RFC8231].  This TLV can also be carried in the PCEP-ERROR object
  
***************
*** 282,290 ****
  Internet-Draft              Binding Label/SID                  July 2021
  
  
!    [RFC5440] in case of error.  Multiple instance of TE-PATH-BINDING
     TLVs MAY be present in the LSP and PCEP-ERROR object.  The type of
     this TLV is 55 (early allocated by IANA).  The length is variable.
  
         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
--- 288,299 ----
  Internet-Draft              Binding Label/SID                  July 2021
  
  
!    [RFC5440] in case of error.  Multiple instances of TE-PATH-BINDING
     TLVs MAY be present in the LSP and PCEP-ERROR object.  The type of
     this TLV is 55 (early allocated by IANA).  The length is variable.
+                   ^^^^^^^^^^^^^^^^^^^^^^^^^
+                   add a note for the RFC Editor to remove 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
***************
*** 304,316 ****
     according to the rules specified in [RFC5440].  The value portion of
     the TLV comprises:
  
!    Binding Type (BT): A one-octet field identifies the type of binding
     included in the TLV.  This document specifies the following BT
     values:
  
     o  BT = 0: The binding value is a 20-bit MPLS label value.  The TLV
        is padded to 4-bytes alignment.  The Length MUST be set to 7 and
        the first 20 bits are used to encode the MPLS label value.
  
     o  BT = 1: The binding value is a 32-bit MPLS label stack entry as
        per [RFC3032] with Label, TC [RFC5462], S, and TTL values encoded.
--- 313,333 ----
     according to the rules specified in [RFC5440].  The value portion of
     the TLV comprises:
  
!    Binding Type (BT): A one-octet field that identifies the type of binding
     included in the TLV.  This document specifies the following BT
     values:
  
     o  BT = 0: The binding value is a 20-bit MPLS label value.  The TLV
        is padded to 4-bytes alignment.  The Length MUST be set to 7 and
        the first 20 bits are used to encode the MPLS label value.
+       
+ It took me a moment of referring to RFC 5440 §7.1 to assure myself that
+ length of 7 is correct regardless of padding to 4 byte alignment. It 
+ might be worth adding a note to this effect, to head off other readers
+ raising this as a concern. I don't insist, though. If you did want to
+ make this change, you could do something like this: "The Length MUST be
+ set to 7 (the padding is not included in the length, as per RFC 5440
+ Section 7.1)..."
  
     o  BT = 1: The binding value is a 32-bit MPLS label stack entry as
        per [RFC3032] with Label, TC [RFC5462], S, and TTL values encoded.
***************
*** 318,324 ****
        values according to its local policy.  The Length MUST be set to
        8.
  
!    o  BT = 2: The binding value is an SRv6 SID with a format of a
        16-octet IPv6 address, representing the binding SID for SRv6.  The
        Length MUST be set to 20.
  
--- 335,341 ----
        values according to its local policy.  The Length MUST be set to
        8.
  
!    o  BT = 2: The binding value is an SRv6 SID with the format of a
        16-octet IPv6 address, representing the binding SID for SRv6.  The
        Length MUST be set to 20.
  
***************
*** 375,381 ****
  
     This section specifies the format of the Binding Value in the TE-
     PATH-BINDING TLV when the BT is set to 3 for the SRv6 Binding SIDs
!    [RFC8986], as shown in Figure 4.
  
  
  
--- 392,398 ----
  
     This section specifies the format of the Binding Value in the TE-
     PATH-BINDING TLV when the BT is set to 3 for the SRv6 Binding SIDs
!    [RFC8986]. The format is shown in Figure 4.
  
  
  
***************
*** 467,472 ****
--- 484,494 ----
     sender MAY choose to set the BT to 2, in which case the receiving
     implementation chooses how to interpret the SRv6 Endpoint Behavior
     and SID Structure according to local policy.
+    
+ Are there any security or operational implications if BT is 2? I don't
+ see any called out. Are there any new attacks enabled if BT is 3 and an 
+ attacker falsifies the various length values? (Of course the answers to 
+ these might be "no" but please give it a think.)
  
     If a PCC wishes to withdraw a previously reported binding value, it
     MUST send a PCRpt message with the specific TE-PATH-BINDING TLV with
***************
*** 490,495 ****
--- 512,546 ----
     error, the PCC rejects the PCUpd or PCInitiate message in its
     entirety and can include the offending TE-PATH-BINDING TLV in the
     PCEP-ERROR object.
+    
+ Rejecting the message in its entirety makes sense, but it does imply
+ that an implementation MUST be prepared to unwind any actions it has
+ taken earlier on the basis of the message. It's a pretty common 
+ pattern for an implementation to take individual actions implied by
+ a message immediately as they're encountered, which means this 
+ unwinding is potentially non-trivial. For example, suppose a PCE 
+ requires a PCC to allocate three specific binding values, so it
+ sends a PCUpd containing three TE-PATH-BINDING TLVs. The first one 
+ is successfully allocated, and so is the second, but for some reason
+ the third is unable to be allocated. Now the PCC must de-allocate
+ the first two, in addition to sending a TBD2+TBD4 PCErr message and
+ possibly includes the third TLV in the PCEP-ERROR object.  
+ 
+ Also, I guess the PCE must infer that the first two bindings were not 
+ allocated, even though it has only received an error for the third.
+ This would be true even if the PCC implementation doesn't have to do
+ any de-allocation -- suppose the PCC has a clever implementation that
+ only commits the actions in the PCUpd once it knows they will all 
+ succeed, it's still the case that the PCE will get an error for one
+ but must know that the error applies to the others that were carried
+ in the PCUpd.
+ 
+ Is this analysis correct, or if not, why not? If it's correct, it 
+ seems to me as though it might well be worth devoting a few sentences
+ to it, unless these issues are already well covered in one of the
+ foundational RFCs (in which case, I'd appreciate a pointer, I haven't
+ done a thorough enough review of the normative documents to catch all 
+ such things).
  
     If a PCE wishes to request the withdrawal of a previously reported
     binding value, it MUST send a PCUpd message with the specific TE-
***************
*** 507,513 ****
--- 558,573 ----
  
  
     In some cases, a stateful PCE can request the PCC to allocate any
+                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     binding value.  It instructs the PCC by sending a PCUpd message
+    ^^^^^^^^^^^^^
+    I don't understand what that means. My *guess* is that you mean
+    the PCE is just saying "hey PCC, please allocate a binding value
+    and tell me what it is".  Is that right?  If so I think this 
+    could be worded even more clearly, perhaps "a stateful PCE may
+    want to request that the PCE allocate a binding value of the PCE's 
+    own choosing"?
+    
     containing an empty TE-PATH-BINDING TLV, i.e., no binding value is
     specified (bringing the Length field of the TLV to 4).  A PCE can
     also request a PCC to allocate a binding value at the time of
***************
*** 524,529 ****
--- 584,592 ----
     including any other valid instances of TE-PATH-BINDING TLVs, if any.
     The resulting error message MAY include the offending TE-PATH-BINDING
     TLV in the PCEP-ERROR object.
+    
+ The earlier discussion of unwinding on error, and the PCE needing to
+ consider all bindings to have failed, applies.
  
     If a PCC receives a TE-PATH-BINDING TLV in any message other than
     PCUpd or PCInitiate, it MUST close the corresponding PCEP session
***************
*** 545,551 ****
  
     In PCEP messages, LSP route information is carried in the Explicit
     Route Object (ERO), which consists of a sequence of subobjects.
!    [RFC8664] defines a new ERO subobject "SR-ERO subobject" capable of
     carrying a SID as well as the identity of the node/adjacency (NAI)
     represented by the SID.  The NAI Type (NT) field indicates the type
     and format of the NAI contained in the SR-ERO.  In case of binding
--- 608,614 ----
  
     In PCEP messages, LSP route information is carried in the Explicit
     Route Object (ERO), which consists of a sequence of subobjects.
!    [RFC8664] defines the "SR-ERO subobject" capable of
     carrying a SID as well as the identity of the node/adjacency (NAI)
     represented by the SID.  The NAI Type (NT) field indicates the type
     and format of the NAI contained in the SR-ERO.  In case of binding
***************
*** 564,569 ****
--- 627,642 ----
  
     Type = 10 ("Reception of an invalid object") and Error-Value = 11
     ("Malformed object").
+    
+ Everything beginning with "as per section 5.2.1" is basically a 
+ re-wording of the text already in RFC 8664.  I would prefer you not
+ re-specify something that's already specified in the base document,
+ so at the very least the sentence "If these conditions are not met" 
+ would need to be re-worded to make it clear that it's just reminding
+ the reader of what RFC 8664 says.  But, instead I think it would be 
+ better to go with something like: "RFC 8664 Section 5.2.1 specifies 
+ bit settings and error handling in the case when NT=0", and remove
+ the above text.
  
  7.  Binding SID in SRv6-ERO
  
***************
*** 576,581 ****
--- 649,668 ----
     and a PCErr message is sent by the PCC with Error-Type = 10
     ("Reception of an invalid object") and Error-Value = 11 ("Malformed
     object").
+    
+ I like the wording of this text a little better, since it makes it 
+ clear you're only repeating RFC 8664 for convenience, not specifying
+ something new.  However I still prefer pointing to the authority
+ rather than pasting it.  
+ 
+ In any case I suggest re-wording the second sentence, making the 
+ entire section something like this:
+ 
+    [I-D.ietf-pce-segment-routing-ipv6] defines the "SRv6-ERO 
+    subobject" for an SRv6 SID.  Similarly to SR-ERO (Section 6), the 
+    NAI MUST NOT be included and the NT MUST be set to zero.  RFC 8664 
+    Section 5.2.1 specifies bit settings and error handling in the 
+    case when NT=0. 
  
  8.  PCE Allocation of Binding label/SID
  
***************
*** 596,615 ****
     the procedures and PCEP extensions for using the PCE as the central
     controller.
  
!    For an implementation that supports PCECC operations as per
     [I-D.ietf-pce-pcep-extension-for-pce-controller], the binding label/
!    SID MAY also be allocated by the PCE itself.  Both peers need to
     exchange the PCECC capability as described in
     [I-D.ietf-pce-pcep-extension-for-pce-controller] before the PCE can
     allocate the binding label/SID on its own.
  
!    A new P flag in the LSP object [RFC8231] is introduced to indicate
     the allocation needs to be made by the PCE:
  
     o  P (PCE-allocated binding label/SID): If the bit is set to 1, it
        indicates that the PCC requests PCE to make allocations for this
        LSP.  The TE-PATH-BINDING TLV in the LSP object identifies that
!       the allocation is for binding label/SID.  A PCC MUST set this bit
  
  
  
--- 683,702 ----
     the procedures and PCEP extensions for using the PCE as the central
     controller.
  
!    When PCECC operations are supported as per
     [I-D.ietf-pce-pcep-extension-for-pce-controller], the binding label/
!    SID MAY be allocated by the PCE itself.  Both peers need to
     exchange the PCECC capability as described in
     [I-D.ietf-pce-pcep-extension-for-pce-controller] before the PCE can
     allocate the binding label/SID on its own.
  
!    A new P flag in the LSP object [RFC8231] is introduced to indicate that
     the allocation needs to be made by the PCE:
  
     o  P (PCE-allocated binding label/SID): If the bit is set to 1, it
        indicates that the PCC requests PCE to make allocations for this
        LSP.  The TE-PATH-BINDING TLV in the LSP object identifies that
!       the allocation is for a binding label/SID.  A PCC MUST set this bit
  
  
  
***************
*** 618,630 ****
  Internet-Draft              Binding Label/SID                  July 2021
  
  
!       to 1 and include a TE-PATH-BINDING TLV in the LSP object to
!       request for allocation of binding label/SID by the PCE in the PCEP
        message.  A PCE MUST also set this bit to 1 and include a TE-PATH-
        BINDING TLV to indicate that the binding label/SID is allocated by
        PCE and encoded in the PCEP message towards the PCC.  Further, a
        PCE MUST set this bit to 0 and include a TE-PATH-BINDING TLV in
!       the LSP object to indicate that the binding label/SID should be
        allocated by the PCC as described in Section 5.
  
     Note that -
--- 705,717 ----
  Internet-Draft              Binding Label/SID                  July 2021
  
  
!       to 1 and include a TE-PATH-BINDING TLV in the LSP object if it wishes to
!       request allocation of binding label/SID by the PCE in the PCEP
        message.  A PCE MUST also set this bit to 1 and include a TE-PATH-
        BINDING TLV to indicate that the binding label/SID is allocated by
        PCE and encoded in the PCEP message towards the PCC.  Further, a
        PCE MUST set this bit to 0 and include a TE-PATH-BINDING TLV in
!       the LSP object if it wishes to indicate that the binding label/SID should be
        allocated by the PCC as described in Section 5.
  
     Note that -
***************
*** 634,640 ****
        PCInitiate message or PCUpd message by setting P=1 and including
        TE-PATH-BINDING TLV in the LSP object.
  
!    o  To let the PCC allocates the binding label/SID, a PCE MUST set P=0
        and include an empty TE-PATH-BINDING TLV ( i.e., no binding value
        is specified) in the LSP object in PCInitiate/PCUpd message.
  
--- 721,727 ----
        PCInitiate message or PCUpd message by setting P=1 and including
        TE-PATH-BINDING TLV in the LSP object.
  
!    o  To let the PCC allocate the binding label/SID, a PCE MUST set P=0
        and include an empty TE-PATH-BINDING TLV ( i.e., no binding value
        is specified) in the LSP object in PCInitiate/PCUpd message.
  
***************
*** 655,660 ****
--- 742,758 ----
        *  Send a PCErr message with Error-Type=19 (Invalid Operation) and
           Error-Value=16 (Attempted PCECC operations when PCECC
           capability was not advertised)
+          
+ You need to update your reference to point to RFC 9050. You should 
+ then either reference §5.4 and NOT copy-and-paste the relevant text,
+ or you should (less desirably, IMO) update your copy-and-paste to 
+ the text published with RFC 9050.  Notably, RFC 9050 covers the case
+ of a legacy PCEP speaker, whereas the text in this spec doesn't. 
+ 
+ IMO since RFC 9050 already specifies what to do when the capability
+ isn't exchanged, there is no need to say anything at all in this 
+ document, so unless you have a compelling reason (what?) to keep it,
+ please remove this bullet and its two sub-bullets.
  
        *  Terminate the PCEP session
  
***************
*** 664,670 ****
     Note that the specific BSID could be from the PCE-controlled or the
     PCC-controlled label space.  The PCE can directly allocate the label
     from the PCE-controlled label space using P=1 as described above,
!    whereas the PCE can request for the allocation of a specific BSID
  
  
  
--- 762,768 ----
     Note that the specific BSID could be from the PCE-controlled or the
     PCC-controlled label space.  The PCE can directly allocate the label
     from the PCE-controlled label space using P=1 as described above,
!    whereas the PCE can request the allocation of a specific BSID
  
  
  
***************
*** 745,754 ****
     [RFC8281] and [RFC8664] are applicable to this specification.  No
     additional security measure is required.
  
!    As described [RFC8664], SR allows a network controller to instantiate
     and control paths in the network.  A rogue PCE can manipulate binding
     SID allocations to move traffic around for some other LSP that uses
     BSID in its SR-ERO.
  
     Thus, as per [RFC8231], it is RECOMMENDED that these PCEP extensions
     only be activated on authenticated and encrypted sessions across PCEs
--- 843,857 ----
     [RFC8281] and [RFC8664] are applicable to this specification.  No
     additional security measure is required.
  
!    As described in [RFC8664], SR allows a network controller to instantiate
     and control paths in the network.  A rogue PCE can manipulate binding
     SID allocations to move traffic around for some other LSP that uses
     BSID in its SR-ERO.
+    
+ Try as I might, I'm not able to figure out what, specifically, the above
+ sentence (that begins "a rogue PCE") means.  I mean, I get it, a rogue 
+ PCE can misdirect traffic.  Beyond that, can you help me understand what
+ you meant to convey here?
  
     Thus, as per [RFC8231], it is RECOMMENDED that these PCEP extensions
     only be activated on authenticated and encrypted sessions across PCEs
***************
*** 773,778 ****
--- 876,889 ----
  
     The PCEP YANG module [I-D.ietf-pce-pcep-yang] could be extended to
     include policy configuration for binding label/SID allocation.
+    
+ It looks like pcep-yang expired a couple months ago, but I think 
+ that's just an oversight and doesn't reflect the WG abandoning
+ the work.  Is there any intention on the part of the WG, to 
+ extend it as described?  My understanding is that yes, the WG does
+ intend to do this -- in that case I think this section would be 
+ stronger if you indicated that, something like "The PCEP YANG module
+ will be extended..." instead of "could be". 
  
  
  
***************
*** 809,814 ****
--- 920,928 ----
     apply to the PCEP extensions defined in this document.  Further, the
     mechanism described in this document can help the operator to request
     control of the LSPs at a particular PCE.
+    
+ I don't understand the final sentence (the one that begins "further"). 
+ Can you help me?
  
  12.  IANA Considerations
  
***************
*** 852,857 ****
--- 966,976 ----
                           Behavior and
                           Structure
                  4-255    Unassigned            This document
+                 
+ Just curious, have you considered any Experimental Use values?  The 
+ PCEP registries are inconsistent in that regard so I can't tell 
+ whether this is a "you should" or "you could" or even something that
+ doesn't make sense in this specific case.
  
     IANA is requested to create a new subregistry "TE-PATH-BINDING TLV
     Flag field" to manage the Flag field in the TE-PATH-BINDING TLV.  New
***************
*** 882,888 ****
  
  12.3.  PCEP Error Type and Value
  
!    This document defines a new Error-type and Error-Values for the PCErr
     message.  IANA is requested to allocate new error-type and error-
     values within the "PCEP-ERROR Object Error Types and Values"
     subregistry of the PCEP Numbers registry, as follows:
--- 1001,1007 ----
  
  12.3.  PCEP Error Type and Value
  
!    This document defines a new Error-type and associated Error-Values for the PCErr
     message.  IANA is requested to allocate new error-type and error-
     values within the "PCEP-ERROR Object Error Types and Values"
     subregistry of the PCEP Numbers registry, as follows:
***************
*** 911,917 ****
  
  13.  Acknowledgements
  
!    We like to thank Milos Fabian, Mrinmoy Das, Andrew Stone, Tom Petch,
     Aijun Wang, Olivier Dugeon, and Adrian Farrel for their valuable
     comments.
  
--- 1030,1036 ----
  
  13.  Acknowledgements
  
!    We would like to thank Milos Fabian, Mrinmoy Das, Andrew Stone, Tom Petch,
     Aijun Wang, Olivier Dugeon, and Adrian Farrel for their valuable
     comments.
  
***************
*** 1016,1021 ****
--- 1135,1142 ----
                a Central Controller (PCECC) of LSPs", draft-ietf-pce-
                pcep-extension-for-pce-controller-14 (work in progress),
                March 2021.
+               
+ Please update the reference to RFC 9050.
  
     [I-D.ietf-pce-segment-routing-ipv6]
                Li, C., Negi, M., Sivabalan, S., Koldychev, M.,