Re: [Pce] Shepherd review of draft-ietf-pce-pceps

[Dhruv Dhody <[e-mail]>]

Hi Cyril,

Thanks for being our shepherd and providing review comments. Please see
inline...

On Mon, Mar 27, 2017 at 12:10 PM, Cyril Margaria <[e-mail]>
wrote:

> Dear authors, PCE WG
>
> I am the shepherd for this document, Please find below my shepherd's
> review of the aforementioned I-D.
>
>
> * General
>
> The document does not need much work to to progress, they are mainly
> clarifications.
>
> * Detailed comments.
>
> Section 3.2
> --
> "Thus the PCEP session is secured via TLS from the
>    start before exchange of any other PCEP message including the Open
>    message."
>
> This sentence comes after the reference to discovery procedures while it
> refers to when the startTLS message is to be used.
> This could be rephrased.
>

​[Dhruv]: How about -

This message MUST be the first PCEP message
issued by the PCEP speaker, willing to use TLS. Note that, this includes the
open message




> --
> "Securing via TLS of an existing PCEP session is not
>    permitted, the session must be closed and re-established with TLS as
>    per the procedure described in this document"
>
>  Should the must be replaced by MUST?
>
>
>
​[Dhruv] Ack​



> --
> "If the PCEP speaker supports PCEPS but cannot establish a TLS
>    connection for some reason"
>
> I believe that in that case both speakers must support PCEPS in order for
> them to send the startTLS and start TLS negotiation. Its unclear when those
> errors can be sent, shouldn't they be sent after the TLS negotiations have
> taken place?
> This comment is repeated in the next sections/comments.
>

​[Dhruv]: Yes, this is after startTLS messages have been exchanged, and once
the PCEP speaker finds out that the TLS connection cannot be established.

We could reword this to -

After the exchange of startTLS messages, if the PCEP speaker cannot
establish a TLS connection
for some reason...


>
> Section 3.3.
>
> --
> "On receiving a StartTLS message from the PCEP
>    peer (i.e.  when the PCEP speaker has sent and received StartTLS
>    message) it is ready to start TLS negotiation and establishment and
>    move to steps described in Section 3.4."
>
> From section 3.2  a peer MAY also send Error-Type set to [TBA2 by IANA]
> (PCEP StartTLS failure) and Error-value 3 (not without TLS) or 4 (ok
> without TLS).
> At which point of the PCEP session are the PCErr sent? is it in step 3 of
>  section 3.4?
>
>
>
​[Dhruv]: Yes. It is the error handling part of step 3 IMHO. ​



> --
> "Once the TCP connection has been successfully established and the
>    StartTLS message sent, the sender MUST start a timer called
>    StartTLSWait timer, after the expiration of which, if no StartTLS
>    message has been received, it sends a PCErr message and releases the
>    TCP connection with Error-Type set to [TBA2 by IANA] and Error-value
>    set to 5 (no StartTLS message received before the expiration of the
>    StartTLSWait timer)."
>
> the following text would be more clear in my opinion
>
> ​​
> "Once the TCP connection has been successfully established and the
>    StartTLS message sent, the sender MUST start a timer called
>    StartTLSWait timer, after the expiration of which, if no StartTLS
>    message has been received, it MUST send a PCErr message and releases the
>    TCP connection with Error-Type set to [TBA2 by IANA] and Error-value
>    set to 5 (no StartTLS message received before the expiration of the
>    StartTLSWait timer)."
>

​[Dhruv]: Ack. ​



>
> Section 3.5
>
>
>
> --
> "PCEPS implementations SHOULD provide mechanisms for
>    associating peer identities with different levels of access and/or
>    authoritativeness, and they MUST provide a mechanism for establish a
>    default level for properly identified peers. "
>
> typo "
> ​​
> for establish" -> "for
> ​​
> establishing"
>
> "PCEPS implementations SHOULD provide mechanisms for
>    associating peer identities with different levels of access and/or
>    authoritativeness, and they MUST provide a mechanism for establishing a
>    default level for properly identified peers. "
>
>
​[Dhruv]: Ack. ​



> --
>
> " Implementations
>    that want to support a wide variety of trust models should expose as
>    many details of the presented certificate to the administrator as
>    possible so that the trust model can be implemented by the
>    administrator. "
>
> should -> SHOULD or it could be rephrased if its not the intent.
>
>
​​[Dhruv]: Ack. ​
​



> --
> "As a suggestion, at least the following parameters of
>    the X.509 certificate should be exposed:"
>
> it can be rephrased as
> "
> ​​
> At least the following parameters of
>    the X.509 certificate SHOULD be exposed:"
>
> or
>
> "its
> ​​
> RECOMMENDED that at least the following parameters of
>    the X.509 certificate are exposed:"
>
>
>
​​​[Dhruv]: Ack. ​​



> Section 3.6
> --
>
> Section 3.2
> ​​
> defines error-values for TLS negotiation failures, should they be used in
> that section? i.e the peer SHOULD send a PCErr and MUST terminate the
> session?
>
>
>
​[Dhruv]: Yes, updated the text to -

   In case the initial TLS negotiation or the peer identity check fails,
   according to the procedures listed in this document, the peer MUST
   send the PCErr message as per the Section 3.2 and then MUST terminate
   the session.



> Section 4.
>
> --
> I am not sure if the second and third paragraph are relevant for PCEPS
> session establishement. Could you indicate what is the relation between
> PCEPS and dns resolution?
>
>
​[Dhruv​]: If DNS is used to discover the PCE IP address, it can also
specify if the PCE supports TLS via the NAPTR records, you can find an
example of this in
https://tools.ietf.org/html/draft-wu-pce-dns-pce-discovery-09#section-6.2.1

Thus this is similar to learning the PCEPS capability via IGP.



>
> Section 6.1
>
> --
>
> Following rfc5226, please  clarify the request for new allocation and
> indicate which registry need to be updated, for instance:
>
>  IANA is requested to allocate new message types within the "PCEP
>  Messages" sub-registry of the PCEP Numbers registry, as follows:
>
>                  Value     Meaning
>          Reference
>                    TBA1      The Start TLS Message (StartTLS)         This
> document
>
> Section 6.2
>
> --
> same as section 6.1, a possible text can be:
>
> IANA is requested to allocate new Error Types and Error Values within
>    the " PCEP-ERROR Object Error Types and Values" sub-registry of the
>    PCEP Numbers registry, as follows:
>
>
>
>
​[Dhruv]: Ack​



> Section 8.2
>
> --
> should the YANG modules also be updated?
>
>
​[Dhruv]: How about - ​

​   An implementation SHOULD allow the operator to configure the PCEPS
   capability as well as various TLS related parameters, as well as view
   the current TLS status for a PCEP session.  To serve this purpose,
   the PCEP YANG module [I-D.ietf-pce-pcep-yang] can be extended to
   include TLS related configuration and state.​



​Thanks for your comments! ​I have attached the diff file.

Working XML:
https://github.com/dhruvdhody-huawei/ietf/blob/master/draft-ietf-pce-pceps-12.xml

​We can update a new version after a confirmation from you! ​

​Regards,
Dhruv​



>
> Thanks,
> Cyril
>