Last Call Review of draft-ietf-pce-pceps-14
review-ietf-pce-pceps-14-genart-lc-worley-2017-07-11-00

Request Review of draft-ietf-pce-pceps
Requested rev. no specific revision (document currently at 18)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2017-07-12
Requested 2017-06-28
Authors Diego Lopez, Oscar de Dios, Qin Wu, Dhruv Dhody
Draft last updated 2017-07-11
Completed reviews Secdir Early review of -06 by David Mandelberg (diff)
Rtgdir Last Call review of -12 by Dan Frost (diff)
Secdir Last Call review of -14 by David Mandelberg (diff)
Genart Last Call review of -14 by Dale Worley (diff)
Opsdir Last Call review of -14 by Tianran Zhou (diff)
Assignment Reviewer Dale Worley
State Completed
Review review-ietf-pce-pceps-14-genart-lc-worley-2017-07-11
Reviewed rev. 14 (document currently at 18)
Review result Ready with Nits
Review completed: 2017-07-11

Review
review-ietf-pce-pceps-14-genart-lc-worley-2017-07-11

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.