[Pce] TSV-ART review of draft-ietf-pce-stateful-sync-optimizations

["Adrian Farrel" <[e-mail]>]

Document: draft-ietf-pce-stateful-sync-optimizations
Reviewer: Adrian Farrel
Review Date: February 21, 2017
IETF LC End Date: February 28, 2017
IESG Telechat date: March 16, 2017

Review result: Has Issues

I've reviewed this document as part of TSV-ART's ongoing effort to review key
IETF documents. These comments were written primarily for the transport area
directors, but are copied to the document's authors and the working group for
information and to allow any issues raised to be addressed.

Please always CC tsv-art at ietf.org if you reply to or forward this review.

This draft presents motivations for optimizations to the base state
synchronization procedures used by a Path Computation Element (PCE) to ensure
its state matches that held in the network, that held by Path Computation
Clients (PCCs), and that held by cooperating PCEs. Those procedures are
described in draft-ietf-pce-stateful-pce also in IETF last call. The draft then
defines extensions to the Path Computation Element Communication Protocol (PCEP)
to achieve these optimizations.

In the spirit of full disclosure, I have been an active member of the PCE
working group and participated in the work that resulted in
draft-ietf-pce-stateful-pce, but I have not previously paid attention to this
document.

-- TSV-ART review comments:

PCEP is a TCP-based protocol. A separate piece of work (draft-ietf-pce-pceps)
examines how to secure PCEP using secure transport.

This document makes no changes to how PCEP uses the transport. However, it does
discuss (section 3.3.2) re-establishing peering after one peer moves and changes
its "transport-level identity (such as IP address)". This is done by
establishing a new TCP connection and referencing the old PCEP session as the
new one is established (i.e., all issues - including this rather obvious attack
vector) are in PCEP and do not affect transport.

-- Other review comments

== Major issues

I found it quite hard to work out the objects and flags to include on an
Open. The problem (for me) is that there are three levels of function
a. no support for this I-D
b. support for synchronisation avoidance if DBvs match with full resynch
   otherwise
c. support for synchronisation avoidance if DBvs match with partial 
   resynch otherwise
and three indicators:
- S flag on Open
- D flag on Open
- presence of LSP-DB-VERSION TLV on Open

S flag seems to serve two purposes:
- indicates that LSP-DB-VERSION TLV must be used on PCRpt messages
- indicates that resynch avoidance can be attempted

D flag seems to mean:
- Partial (incremental) synchronisation can be attempted

Presence of LSP-DB-VERSION TLV on Open seems to indicate:
- value of DBv to compare to see if resynch can be avoided
- value of DBv to compare to perform partial resynch

I can't find a description of the behavior at startup with no saved DB.
I think it is to omit the LSP-DB-VERSION TLV but still set the S flag.
Is the only reason you need the S flag that you don't support the value
of zero for a DBv? That is, you could use the presence of the
LSP-DB-VERSION TLV as indication that synchronisation avoidance is
supported if you had a value you could transmit to indicate "I have no
DB".

I am missing a statement in Section 3 that makes it clear that in the
absence of the LSP-SYNC-CAPABILITY (D bit) from section 4, all synchs
are either NOP or full.

Finally, it is not clear (to me) why the PCC needs to include an
LSP-DB-VERSION TLV in its Open message. I can see that it helps the  
PCE to predict what will happen next, but it isn't actually a 
necessary part of the protocol AFAICS.

---

The function introduced for 3.3.2 has far reaching implications for a
PCE system. I wonder whether the WG noticed it and considered those
implications.

Up until now a PCEP session has been known in the context of the
PCE/PCC address pair. You are changing this by introducing a 
SPEAKER-ENTITY-ID that represents the PCEP speaker for all time.
You are not saying anything about how the assignment of this value
is made consistent across the domain (indeed, you seem to offer a
range of implementation choices that could clash), or how uniqueness
might be policed. Furthermore, you only give a "SHOULD" as the 
mechanism to identify interdomain PCEs, and that doesn't seem to 
provide a reliable mechanism (actually, even the mechanism for
this "SHOULD" is a bit vague).

You are saying that one of the PCE/PCC can change its address and
re-assume a PCEP session without any further impact.

Now, if the PCC changes its address there will be "interesting"
consequences for the LSPs for which it is the head end. Renumbering
events under the feet of live TE-LSPs are not well examined, but it
looks like the LSPs might cease to be.

So, perhaps you are looking at what happens when a PCE changes its
address. This *might* be a more likely event if the PCE is virtualised
and moves around in a DC.  

But, how does a PCC/PCE pairing get discovered and authorised? If you
are relying on discovery through the routing protocol, then you will 
need to add the SPEAKER-ENTITY-ID to the advertisement or else it is
hit or miss for the PCC to find the right PCE. If you are relying on
configuration, then I suspect that the SPEAKER-ENTITY-ID is not needed
because the configuration actions can achieve the right effect.

Perhaps the use case you have in mind is when:
- PCEP sessions are initiated by the PCE
- The PCC is configured to accept any PCE
- The PCE might wander taking its DB with it
This has a some utility, but a lot of security concerns and error cases.
What if a new PCE comes alive using a SPEAKER-ENTITY-ID that is already
in use? What if a bogus PCE claims to have the same SPEAKER-ENTITY-ID as
a PCE that has just failed? (And, of course, what stops a bogus PCE from
randomly latching on to PCCs in the network).

Maybe your plan is that the PCC will be configured with acceptable
SPEAKER-ENTITY-IDs?

You might also have had some ideas about PCE redundancy here. For
example, a backup PCE with access to the DB used by the primary PCE
could want to step in and take over seamlessly. In this case, of
course, you would have two PCEs with the same SPEAKER-ENTITY-ID.

In short, I don't think you have described this use case and its
consequences, and you are trying to optimise a terribly small corner
case.

(Actually, this would all have been a lot easier if I had known about
D=0 all the way through section 3, but that is just a comment about how
hard the document is to parse for a slow learner like me.)

=== Minor issues

The phrase "reliable state synchronization mechanism" is interesting
as it is not used in draft-ietf-pce-stateful-pce-18.txt where 
synchronization is described only in terms of "checkpointing state".
Is any implication to be drawn from this use of "reliable"?

---

In Section 2 you have...

   Within this document, when describing PCE-PCE communications, the
   requesting PCE fills the role of a PCC.  This provides a saving in
   documentation without loss of function.

I know what you intend, but this is a little confusing in the case that
of "PCE-triggered Initial Synchronization" when (of course) the 
requesting PCE fills the role of a PCE and the responding PCE fills the
role of PCC.

Maybe you can write:

   Within this document, when describing PCE-PCE communications, one of
   the PCEs fills the role of a PCC.  This provides a saving in
   documentation without loss of generality.

---

3.2

   "PCE speaker receiving this node should send back a PCErr
   s/node/value/
   s/should/SHOULD/
   But why "SHOULD" not "MUST"? What is the optional alternative?

---

3.2

   If state synchronization avoidance has not been enabled on
   a PCEP session, the PCC SHOULD NOT include the LSP-DB-VERSION TLV in
   the LSP Object and the PCE SHOULD ignore it were it to receive one.

Why "SHOULD ignore" and not "MUST ignore"? What else might a PCE do with
the information?

---

Because of wrapping you will get situations where PCE(DBv) > PCC(DBv)
yet the PCC has more recent state.  This is normal and I think you just
have to say that the assumption is *always* that the PCC has more 
recent state and that updates must be done from the value of PCE(DBv)
incrementally and potentially through the wrap until the value of the
PCC(DBv) is reached.

---

3.3.1 has

   The LSP State Database Version Number (LSP-DB-VERSION) TLV is an
   optional TLV that MAY be included in the OPEN object and the LSP
   object.

The LSP object is allowed on the PCRpt, the PCUpd, the PCReq, and the
PCRep message. What does the LSP-DB-VERSION TLV mean on a PCUpd, a 
PCReq, or a PCRep?

---

It would help IANA and the RFC editor, and avoid any issues if you
used different TBD flags (such as TBD1, TBD2,...) for the different
cases of code point assignments to be made.

Why do you feel the need to recommend values for assignment in the
IANA considerations section? 

---

In 3.3.1 it would probably be wise to state how the 64 bit number is
represented across the 8 octets.

---

4.2
   The PCC MAY force a full LSP DB
   synchronization by setting the D flag to zero in the OPEN message.
Do you mean "The PCE"?
The PCC is in control of the synchronization transmissions and doesn't
need to force anything with the D flag. Maybe the PCC can "announce"
full synchronization.

---

4.2
   It is not necessary for a PCC to store a complete history of LSP
   Database change, but rather remember the LSP state changes (including
   LSP modification, setup and deletion) that happened between the PCEP
   session(s) restart in order to carry out incremental state
   synchronization.

Nice, but...
Consider the PCRpt messages that were in flight when the PCE or the 
session went down.  Did they get included in the PCE's DB? That doesn't
matter because the PCE will announce it via the DBv in the new Open. But
does the PCC need to have remembered them? That depends on whether it
thinks they arrived at the PCE and were processed. A situation made
worse by the inclusion of multiple LSPs on one PCRpt message which 
could give rise to some being added to the DB and some lost.

Similarly...

   After the synchronization procedure finishes, the
   PCC can dump this history information.

How does the PCC know that this state has made it into the PCE's DB?

Maybe you intend to get around this with...

   If a PCC finds out it does not have sufficient information to
   complete incremental synchronisation after advertising incremental
   LSP state synchronization capability, it MUST send a PCErr with
   Error-Type 20 and Error-Value 5 'A PCC indicates to a PCE that it can
   not complete the state synchronization' (defined in
   [I-D.ietf-pce-stateful-pce]) and terminate the session.  The PCC
   SHOULD re-establish the session with the D bit set to 0 in the OPEN
   message.

This seems extreme, since the PCC could just do full resynch anyway.

---

In 5.2

Why do you need the 'Attempt to trigger synchronization when the
TRIGGERED-SYNC capability has not been advertised' error code? Surely
it is enough that the PCE will have received F=0. Recall that the 
Open messages can overlap. Tearing down the session just to change 
the setting of the F bit is unnecessary.

Then

   The PCC SHOULD NOT send PCRpt messages to the stateful PCE before it
   triggers the State Synchronization.

Please tell us about the circumstances under which it MAY send PCRpt
and how, given the PCE has to be able to handle those, it is helpful
to have a "SHOULD NOT" here.                                 

And to be clear, you are saying that all updates to the DB, including 
all new delegations must be deferred and held as though the PCE was
not in the session.

But what happens if the PCE sends a PCUpd? Does the PCC decline to
respond? Or does it respond but leave out the LSP-DB-VERSION? Or do
we apply section 5.6 of draft-ietf-pce-stateful-pce
   A PCE SHOULD NOT send PCUpd messages to a PCC before State
   Synchronization is complete.  A PCC SHOULD NOT send PCReq messages to
   a PCE before State Synchronization is complete.  This is to allow the
   PCE to get the best possible view of the network before it starts
   computing new paths.
In which case I wonder why the PCE even opened the session since nothing
can be done on the session until the PCE is ready to synch.

---

6.2
   To trigger re-synchronization for an LSP, the PCE MUST first mark the
   LSP as stale and then send a Path Computation State Update (PCUpd)
   for it, with the SYNC flag in the LSP object set to 1.

I think "mark the LSP as stale" is a local implementation thing. It 
doesn't go on the wire, does it? So probably no need to tell us about
it. The subsequent PCRpt will cause an update to the DB anyway.

---

6.2

   The PCUpd message MUST include an empty ERO as its
   intended path 

I guess you have this because draft-ietf-pce-stateful-pce says an ERO
is mandatory in a PCUpd. That's a bit heavy, and you could choose to
say the ERO is ignored on a PCUpd if the SYNC flag is set. But anyway
can you clarify that an empty ERO is:
1. sent with object length 4
2. the empty ERO is not an indication to the PCC that it can perform
   local path computation and resignal the LSP according to its own
   whim (i.e., even the empty ERO has to be ignored)

---
6.2
   If the TRIGGERED-RESYNC capability is not advertised by a PCE and the
   PCC receives a PCUpd with the SYNC flag set to 1, it MUST send a
   PCErr with the SRP-ID-number of the PCUpd, Error-Type 20 and Error-
   Value TBD (suggested value - 4) 'Attempt to trigger synchronization
   when the TRIGGERED-SYNC capability has not been advertised' (see
   Section 8.1).

This is all very well for PCCs that support this I-D but don't want to
do triggered resynch. What about a PCC that doesn't even support this
I-D? How will it process this PCUpd (which, obviously, it should not
receive)? I'm sure you can just find a relevant paragraph of 
draft-ietf-pce-stateful-pce to point to.

---

7. and 8.3
Hang on! Section 7 is commandeering bits while section 8.3 is requesting
them from the registry.
I suggest Section 7 should be reduced to:
- a pointer to draft-ietf-pce-stateful-pce where the TLV is defined
- a list of the new bits, their identifier letters, and their meanings
- a forward pointer to section 8.3

---

9.6.  Impact On Network Operations
I think you under-value your work. The whole point (as explained in your
various motivation sections) of your work is to reduce and control the
loading on the management plane in the operational network.
Conversely, as discussed above, the use of the T-flag means that a PCE
session is up but unusable. That has an impact on network operations 
and is something the operator will need to be able to see ("My PCE is
up, there is a PCEP session, but nothing is happening!")

---

I believe section 10 needs to discuss attacks that come through 
impersonating a "PCE that has moved". I.e., I harvest the 
SPEAKER-ENTITY-ID on a current session, I bring that session down,
I open a new session from my pseudo-PCE and impersonate the old PCE
by spoofing the SPEAKER-ENTITY-ID. I can then attack the PCC through
repeated re-synch; I can gather private info from the PCC; I can
pretend to synch so that when the old PCE comes back the PCC has
discarded incremental info and has to do a full resynch.

Section 10 should also discuss mitigations, not just point out
attacks.

Lastly, you say in section 10
   The PCC is free to drop any trigger re-synchronization
   request without additional processing.
This might have been good to explore in an earlier section. I think
that PCEP generally makes an assumption that a received message will
be acted on, but what you have done is lock a PCE/PCC session by
discarding a trigger request. Without a response the PCE and PCC are
in a form of deadlock - the PCE thinks all state is stale and is
waiting for an update. Depending on how you read things, it is not
allowed to receive (or process) and PCRpt that is not a SYNC or any
PCReq. 
                                  
=== Nits

Please s/draft/document/ in the Abstract and Introduction

---

In Section 2 you have...

   This document uses the following terms defined in
   [I-D.ietf-pce-stateful-pce]: Delegation, Redelegation Timeout
   Interval, LSP State Report, LSP Update Request, LSP State Database.

But draft-ietf-pce-stateful-pce defers to draft-ietf-pce-stateful-pce-app
for "Delegation".

---

4.2
   [I-D.ietf-pce-stateful-pce] describes state synchronization and
   Section 3 describes state synchronization avoidance by using LSP-DB-
   VERSION TLV in its OPEN object.
s/Section 3/Section 3 of this document/

---

4.2
   After the synchronization procedure finishes, the
   PCC can dump this history information.
s/dump/discard/

---

6.2
   Support of PCE-triggered state synchronization is advertised by both
s/synchronization/re-synchronization/