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

[Dhruv Dhody <[e-mail]>]

Hi John,

Thanks for your review. Not an author, but had some thoughts...

On Wed, Sep 29, 2021 at 1:01 AM John Scudder <jgs=
[e-mail]> wrote:

> 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.
>
>
The diff was quite useful IMHO.

<Major Snip>

>
> ***************
> *** 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).
>
>
[Dhruv]: It is a normal practice to reject the whole message in PCEP. Say a
PCE sends a PCUpd message for an LSP with multiple updated parameters, one
would apply the update only once it has verified all the parameters and
send an error in case any are found to be invalid. The PCErr message
includes an SRP object that identifies the full PCUpd message is being
rejected with the reason for the rejection optionally identified.

This was discussed during the WGLC and the conclusion was to reject the
whole message in the case of multiple binding TLVs as well. The SRP in
PCErr would identify that the complete message is being rejected here as
well.


>      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"?
> +
>

[Dhruv]: It should be PCC's own choosing.

<snip>

> ***************
> *** 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
>
>
>
[Dhruv]: The only reason to include this text is for "P=1 in the LSP
object" i.e. the presence of the LSP object with P flag set is to be
considered as a PCECC operation, which would not be the case in RFC9050.



>
> ***************
> *** 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?
>

[Dhruv]: IMHO it highlights how this new capability of "allocating binding
SID" can be exploited by a rogue PCE to misdirect traffic. Path {A, B,
BSID_1}  can be misdirected just by assigning the BSID_1 value to a
different LSP making it a lot easier (and harder to detect).



>
>      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".
>
>
>
[Dhruv]: This one is my fault. It is on my to-do list to update.



>
> ***************
> *** 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?
>
[Dhruv]: My guess would be it is leftover text -- needs to be removed.



>
>   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.
>
> [Dhruv]: The experimantal values are kept limited (see
> https://www.rfc-editor.org/rfc/rfc8356.html#appendix-A). In the case of
> new BT, there needs to be agreemeent accross WGs, not sure if it is
> particularly helpful to have experimental codepoint in this case!


Thanks!
Dhruv