Re: [Pce] Genart last call review of draft-ietf-pce-pceps-14

[Dhruv Dhody <[e-mail]>]

Hi Dale, 

Thanks for your detailed review and providing suggested text. 
Sorry for being a little late with this reply. 
Please see inline...

> -----Original Message-----
> From: Pce [mailto:[e-mail]] On Behalf Of Dale Worley
> Sent: 12 July 2017 08:38
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]
> Subject: [Pce] Genart last call review of draft-ietf-pce-pceps-14
> 
> Reviewer: Dale Worley
> Review result: Ready with Nits
> 
> I am the assigned Gen-ART reviewer for this draft.  The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed by the
> IESG for the IETF Chair.  Please treat these comments just like any other
> last call comments.
> 
> For more information, please see the FAQ at
> <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
> 
> Document:  draft-ietf-pce-pceps-14
> Reviewer:  Dale R. Worley
> Review Date:  2017-07-11
> IETF LC End Date:  2017-07-12
> IESG Telechat date:  2017-08-03
> 
> Summary:
> 
>        This draft is basically ready for publication, but has nits
>        that should be fixed before publication.
> 
> Nits/editorial comments:
> 
> A few of these may rise to the level of minor technical issues,
> especially:
> section 3.2: whether error TBA2/2 is distinct from error 1/1 
[[Dhruv Dhody]] The context of these error are different, one is before TLS establishment (startTLS) and another is during PCEP session establishment (which is after TLS has been established). IMHO it is better to keep them distinct. 

section 3.2:
> the receipt of an PCErr during the failure of TLS
>              establishment
[[Dhruv Dhody]] The idea behind Error TBA2/3 and TBA2/4, is to let the peer know if it is unwilling (or willing) to establish PCEP session without TLS from this peer. This is useful information to figure out how the peer should react without human intervention.  
 
> section 3.3: the distinction between StartTLSWait and OpenWait
[[Dhruv Dhody]] Both timers are wait timers but they wait for different messages and used in different context. Also the values can be set differently based on the deployment. 

> section 5:   a full discussion of backward compatibility considerations
[[Dhruv Dhody]] I have added some more details. 

> section 8.2: MIB extension
[[Dhruv Dhody]] I have added some more details.

More details on above inline. 

> 
> 1.  Introduction
> 
>    This document describes a security container for the transport of
>    PCEP messages, and therefore they do not affect the flexibility and
>    extensibility of PCEP.
> 
> There is no plural antecedent for "they" to reference.  I think you could
> use "it", but the Editor may have better suggestions.
> 

[[Dhruv Dhody]] Ack

> 3.2.  Initiating the TLS Procedures
> 
>    with a PCErr message with Error-Type set to [TBA2 by IANA] (PCEP
>    StartTLS failure)
> 
> It seems that we shouldn't use this Error-Type to object to the use of a
> message other than Open and StartTLS as an initial message, as that error
> isn't a misuse of StartTLS per se and is only incidentally related to
> StartTLS.  Indeed, isn't there already an Error-Type for this error
> (unexpected initial message), since two RFC 5440 implementations can
> commit/detect it?  Looking at RFC 5440 section 7.15, I see:
> 
>    Error-Type    Meaning
>       1          PCEP session establishment failure
>                  Error-value=1: reception of an invalid Open message or
>                                 a non Open message.
> 
> It seems to me that this document is adjusting the meaning of error 1/1,
> rather than defining a new error situation.
> 
[[Dhruv Dhody]] We wanted to differentiate between the message exchanges during PCEP session establishment (which starts with open) and with StartTLS (which starts with StartTLS). Note that in case of TLS, we would move to the PCEP session establishment phase after the TLS is established, and we could still receive error 1/1. 
Regarding TBA2/2, in case of PCEPS, a peer could respond with StartTls, Open or PCErr (but all in the context of this document), so we needed a new error. The peer was expecting StartTLS related message but something else happened and thus an error! IMHO putting this under "PCEP StartTLS failure" error-type is better! 

Do you feel strongly about changing this? 

>    If the PCEP speaker that does not support PCEPS, receives a StartTLS
>    message, it MUST behave according to the existing error mechanism
>    described in section 6.2 of [RFC5440] (in case message is received
>    prior to an Open message) or section 6.9 of [RFC5440] (for the case
>    of reception of unknown message).
> 
> I think you want s/MUST/will/, since the described behavior isn't
> specified by this document, but rather by RFC 5440.

[[Dhruv Dhody]] Ack, updated.
> 
> In any case, this paragraph might want to reference the backward-
> compatibility consideration that I would expect to appear in section 5 --
> How does the PCEPS-supporting element deal with the non-PCEPS-supporting
> element after the connection attempt with StartTLS fails?

 [[Dhruv Dhody]] I have added that in section 5. 
> 
>    After the exchange of startTLS messages, if a PCEP speaker cannot
>    establish a TLS connection for some reason (e.g. the required
>    mechanisms for certificate revocation checking are not available), it
>    MUST return a PCErr message (in clear) with Error-Type set to [TBA2
>    by IANA] (PCEP StartTLS failure) and Error-value set to:
> 
> s/startTLS/StartTLS/

[[Dhruv Dhody]] Ack.
> 
> Is there a well-defined way for a participant in a TLS connection start to
> receive *either* a PCErr message in the clear *or* whatever comes next in
> TLS setup -- and know which case has happened?  Is there a way to use
> popular modular TLS libraries and have the application above the library
> receive such a PCErr message?  I don't understand TLS nearly well enough
> to know the answer to this, but it would probably help implementors if
> answers were given to these questions.

[[Dhruv Dhody]] If the TLS handshake is successful, both local and remote parties are aware of this. Same is the case for TLS failure. So receiving an error message in clear will not be a surprise. 
Also, I checked some of the other documents that describe the use of TLS, but did not find any such handling. 

> 
>       The peer MAY choose to re-establish the PCEP session
>       without TLS next.
> 
> I think you mean "The peer that initiated the connection MAY choose to re-
> establish ...".  As written, "the peer" seems to refer to the peer that
> generated the PCErr, but if it was the receiving peer that generated the
> PCErr, you probably don't want it to attempt to re-establish the session
> but rather wait for the initiating peer to do so.
> 

[[Dhruv Dhody]] Ack. Changed peer to receiver. 

>    Given the asymmetric nature of TLS for connection establishment it is
>    relevant to identify the roles of each of the PCEP peers in it.  The
>    PCC SHALL act as TLS client, and the PCE SHALL act as TLS server,
>    according to [RFC5246].
> 
> See comments re section 4 about terminology.  I think you need to have
> terms for the element that is initiating the connection (either a PCC or a
> PCE) and the element that is receiving the connection (always a PCE).
> 

[[Dhruv Dhody]] I have updated this. 

> 3.3.  The StartTLS Message
> 
>    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).  A RECOMMENDED value for StartTLSWait timer
>    is 60 seconds.
> 
> Really, the timer is the time to wait for *any* message to received.
> Open messages will cause start of upward-compatibility mechanisms (if any);
> any other message will be immediately rejected as an error.
> 
> Indeed, isn't there already a timer which a peer uses to wait for the
> other peer to send a message?  Isn't StartTLSWait functionally the same as
> OpenWait (RFC 5440 section 6.2)?
> 

[[Dhruv Dhody]] I have updated the description, but I would still like to keep a different timer similar to OpenWait and KeepWait (in the spirit of RFC 5440). 

>    ... it MUST send a PCErr message and releases ...
> 
> This sentence is grammatically interesting.  One part is logically "it
> MUST send a PCErr message".  The second part might be "it MUST release the
> TCP connection", or "it releases the TCP connection".  Strictly speaking,
> the rules of English grammar cause the release/releases distinction to
> disambiguate whether MUST applies to both parts or only to the first part.
> But I don't think you want to rely on that, and you want to say "... and
> MUST release the TCP connection ...".

[[Dhruv Dhody]] Updated.

> 
> 3.4.  TLS Connection Establishment
> 
>    1.  Immediately negotiate TLS sessions according to [RFC5246].
> 
> In this case, you'd say "Immediately negotiate a TLS session ...".

[[Dhruv Dhody]] Ack

> 
> 3.5.  Peer Identity
> 
>    At least the following parameters of the X.509 certificate SHOULD
>    be exposed:
> 
>    o  Peer's IP address
> 
>    o  Peer's fully qualified domain name (FQDN)
> 
> To the naive (me), these two items seem to refer to the TCP connection
> that was established.  But I suspect that they're intended to refer to the
> IP address and FQDN that might be encoded in the certificate (as in, "The
> following precedence applies: for DNS name validation, subjectAltName:DNS
> has precedence over CN; for IP address validation, subjectAltName:iPAddr
> has precedence over CN.")
> 
> And it seems that the actual remote IP address of the TCP connection and
> whatever remote FQDN or IP address was used to initiate the connection
> should also be exposed to the administrative system.
> 
> So I think there's room to expand on and clarify exactly the data items
> that are intended here.

[[Dhruv Dhody]] The statement above the list clearly state that this is the information from X.509 certificate.
The TCP socket IP address is definitely known and exposed as that this way to identify a session. I have added this text - 

"Note that the remote IP address used for the TCP session establishment is also exposed."

> 
> 4.  Discovery Mechanisms
> 
>    A PCE can advertise its capability to support PCEPS using the IGP
>    advertisement and discovery mechanism.
> 
> If I understand this correctly, IGP is "Interior Gateway Protocol", and
> it's a category of protocols, not a specific protocol.  And what you're
> saying is that a PCE can advertise using the relevant IGP's mechanism.  In
> that case, I think you'd say "the IGP's advertisement and discovery
> mechanism".  (Unless somehow IS-IS and OSPF are seen as variants of the
> same protocol, which can generically be called IGP.)
> 

[[Dhruv Dhody]] Updated.

>    A new
>    capability flag bit for the PCE-CAP-FLAGS sub-TLV that can be
>    announced as attribute to distribute PCEP security support
>    information is proposed in [I-D.wu-pce-discovery-pceps-support]
> 
> s/announced as attribute/announced as an attribute/.
> 

[[Dhruv Dhody]] Updated.

> But you don't want to say "is proposed in ..." because that suggests that
> the proposal might not be approved, and you've already positively stated
> "A PCE can advertise ...".  So you want to say "A new capability flag ...
> is defined in ..." -- and raise draft-wu-pce-discovery-pceps-support to a
> normative reference.
> 
> Similar considerations apply to discovery via DNS
> (draft-wu-pce-dns-pce-discovery) -- you want to mention it here and
> consider it a normative reference.
> 
> -- Unless you choose to make this part of the section clearly hypothetical
> by starting "This document does not specify any method a PCE can advertise
> that it supports (or requires) PCEPS, but two mechanisms have been
> proposed:"

[[Dhruv Dhody]] I would like to avoid normative reference as these are 
not mandatory and we would not want to block publication of this draft. 
I have updated the text accordingly and removed the text from this document 
Regarding DNS. 

> 
>    When DNS is used by a PCC (or a PCE acting as a client, for the rest
>    of the section, PCC refers to both) willing to use PCEPS to locate an
>    appropriate PCE [I-D.wu-pce-dns-pce-discovery], the PCC as an
>    initiating entity, chooses at least one of the returned FQDNs to
>    resolve, which it does by performing DNS "A" or "AAAA" lookups on the
>    FDQN.
> 
> s/FDQN/FQDN/
> 
> This one sentence uses the terms "acting as a client" and "as an
> initiating entity" to mean the same thing.  You should fix on one term.
> And given that the same concept arises in the discussion of TLS initiation
> (section 3.2), you probably want to define the term for "a PCC (or a PCE
> acting as a client)" near the beginning as document-wide terminology.
> Once you've got names for the concepts of the initiating entity and the
> receiving entity, a number of things in the document can then be stated
> more clearly.
> 
>    If the PCC receives a response to its SRV query but it is not able to
>    establish a PCEPS connection using the data received in the response,
>    as initiating entity it MAY fall back to lookup a PCE that uses TCP
>    as transport.
> 
> This is unclear -- if the process of resolving the original FQDN into
> addresses fails to produce an address that can be contacted, how can the
> PCC "fall back to lookup a PCE that uses TCP as transport" -- it has
> already done the looking up, and that failed.
> 
> I suspect you mean that the PCC is required to first attempt to contact
> each address using TLS, and then only if all of those attempts fail is it
> then permitted to fall back to using TCP.  But you don't actually say that.
> 

[[Dhruv Dhody]] I have removed the details from this document. 

> 5.  Backward Compatibility
> 
> It would help if this section gave a more comprehensive discussion of how
> PCEPS-supporting PCEs/PCCs and non-PCEPS-supporting PCEs/PCCs can
> interwork, i.e., how to incrementally deploy PCEPS into an AS.  The two
> major points seem to be (1) arranging that any two elements will
> communicate with the highest level of security that they both implement,
> and (2) any system of PCEPS-supporting and non-PCEPS-supporting elements
> can interwork successfully.  There probably are some "interesting"
> management and security issues involved in this.
> 

[[Dhruv Dhody]] Updated. 

   If a PCEP implementation that does not support PCEPS receives a
   StartTLS message, it would behave according to the existing error
   mechanism of [RFC5440].  On receiving the error, based on the local
   policy, a peer could try to establishing PCEP session without TLS as
   per the procedures defined in [RFC5440].  For successful TLS
   operations with PCEP, both PCEP peers in the network would need to be
   upgraded to support this document.

   An existing PCEP session cannot be upgraded to PCEPS, the session
   needs to be terminated and reestablished as per the procedure
   described in this document.  During the incremental upgrade, the PCEP
   speaker SHOULD allow session establishment with and without TLS.
   Once both PCEP speakers are upgraded to support PCEPS, the PCEP
   session is re-established with TLS, otherwise PCEP session without
   TLS is setup.  A redundant PCE MAY also be used during the
   incremental deployment to take over the PCE undergoing upgrade.  Once
   the upgrade is completed, support for unsecured version SHOULD be
   removed.

> 6.2.  New Error-Values
> 
> I've noted above that the function of Error-Type=TBA2/Error-Value=2 seems
> to duplicate that of Error-Type=1/Error-Value=1.
> 

[[Dhruv Dhody]] See reply above. 

> The meaning of Error-Type=TBA2 is named "StartTLS Failure" here, but the
> rest of the text uses "StartTLS failure".  The table given in RFC
> 5440 section 7.15 is not consistent about capitalization in the names of
> Error-Types, but most entries do not capitalize non-initial words.
> That suggests "StartTLS failure" should be used here.

[[Dhruv Dhody]] Ack.

> 
> 7.  Security Considerations
> 
> There needs to be some consideration of interoperation of mixed PCEPS-
> capable and non-PCEPS-capable elements (unless such mixtures are NOT
> RECOMMENDED).
> 

[[Dhruv Dhody]] yes strict TLS is recommended. 
.

> 8.1.  Control of Function and Policy
> 
>    A PCEP implementation SHOULD allow configuring the following PCEP
>    security parameters:
> 
>    o  StartTLSWait timer value
> 
>    PCEPS implementations MAY provide ...
> 
> If I read this correctly, there is only one security parameter in this
> list.  But since the text says "the following PCEP security parameters:",
> on my first reading, I assumed that "PCEPS implementation MAY provide ..."
> was the beginning of a second item (with the initial "o" omitted).
> 

[[Dhruv Dhody]] Updated.

>    provide ways for the operator to complete the following tasks:
> 
> It looks like all of the following tasks are with regard to a selected
> PCEP session, though only the first task is labeled as such, whereas the
> 2nd through 4th are not.  It seems like you want to say "to complete the
> following tasks in regard to any PCEP session:" and change the 1st to
> "Determine if the session is protected via PCEPS."
> 

[[Dhruv Dhody]] Updated.

> 8.2.  Information and Data Models
> 
>    The PCEP MIB module SHOULD be extended to include PCEPS capabilities,
>    information, and status.
> 
> This isn't a "SHOULD" because it isn't a constraint on implementations,
> it's a statement of what would be a desirable action by the IETF. --
> Unless the idea is that the implementor SHOULD add a (necessarily
> proprietary) extension to the MIB.  In any case, RFC 7420 should be
> referenced.)
> 

[[Dhruv Dhody]] Ack.

> 8.4.  Verify Correct Operations
> 
> This section title is a verb phrase while the rest of the titles are noun
> phrases.  Perhaps "Verification of Correct Operations".
> 

[[Dhruv Dhody]] Changed to "Verifying Correct Operation".

> 8.5.  Requirements on Other Protocols
> 
>    Mechanisms defined in this document do not imply any new requirements
>    on other protocols.
> 
> There is a correlated discovery mechanism:
> draft-wu-pce-discovery-pceps-support defines a correlated change to OSPF
> and IS-IS.  I suppose that isn't *required* by this document, as draft-wu-
> pce-dns-pce-discovery or configuration might be used.  But conceptually,
> the two discovery mechanisms are implied by this document.
> 

[[Dhruv Dhody]] Ack.

> 10.1.  Normative References
> 
> It seems to me that draft-wu-pce-discovery-pceps-support and draft-wu-pce-
> dns-pce-discovery should be considered normative references.
> 
> 

[[Dhruv Dhody]] Updated the text so that they could remain informative. 

Thanks again for your review. 
I have attached the working copy and the diff. 

Regards,
Dhruv

> _______________________________________________
> Pce mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/pce