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 section 3.2: the receipt of an PCErr during the failure of TLS establishment section 3.3: the distinction between StartTLSWait and OpenWait section 5: a full discussion of backward compatibility considerations section 8.2: MIB extension 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. 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. 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. 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? 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/ 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. 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. 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). 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)? ... 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 ...". 3.4. TLS Connection Establishment 1. Immediately negotiate TLS sessions according to [RFC5246]. In this case, you'd say "Immediately negotiate a TLS session ...". 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. 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.) 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/. 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:" 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. 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. 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. 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. 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). 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). 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." 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.) 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". 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. 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.