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

["Mirja Kuehlewind (IETF)" <[e-mail]>]

Hi Dale,

thanks for the detailed review and follow up. I fully agree with your points and am happy to see that these have been resolved quickly (also added a few more comments in my AD ballot)!

Mirja


> Am 12.07.2017 um 05:08 schrieb Dale Worley <[e-mail]>:
> 
> 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.
> 
>