Re: [pcp] WGLC: draft-ietf-pcp-base-12.txt

[Paul Selkirk <[e-mail]>]

I'm sorry this review is so long; I didn't have time to write a shorter one.

I also apologize if this touches on issues that are being or have been
hashed out on the mailing list; I'm way behind on WG mail.

There are a few places where I have issues with how the protocol
actually works, a few clear (but easily cleared) errors in the
current draft (marked with "!!"), and a lot of editorial nits.

For the errors and nits, I'd be willing to make the necessary changes
myself, if Dan can give me the document source (since I seem to be
listed as a co-author now).

----------------
Policy issues:

- I'm not happy with the way PEER has evolved, to where it can create
mappings.  It's heading in the direction of making MAP a special case
of PEER (with remote port=0, remote peer=0 as wildcards).  I also don't
think the 7.3 use case is compelling enough to justify the added
complexity in the protocol (or even the suggested added complexity in
the client).

- I don't like the PCP Client's IP Address field.  I don't think it adds
sufficient value to be worth spending 16 bytes in every packet, plus
additional processing time.  I would support moving it to an Option, or
at least making it an optional field - e.g. if it's sent as all-zeroes
in the request, the server MUST process the request, and MAY either fill
it in, or leave it zeroed.  The change log says "This addition was
consensus at IETF80", but that doesn't match my notes of the meeting -
it was discussed as a possible approach to a possible problem, but no
sense of the meeting was ever asked.

- UNAUTH_THIRD_PARTY_INTERNAL_ADDRESS could be removed, and replaced
with NOT_AUTHORIZED (more on that inline).

- There is no discussion about levels of conformance.  There's some
discussion about particular options being optional (e.g. THIRD_PARTY
not supported in certain deployment scenarios), but no overall
statement of what features must be supported by all conformant
implementations.  e.g. If I had a small embedded server that supported
MAP opcodes but not PEER, would it be conformant?

----------------
General nits:

Normalize capitalization in all instances of "OpCode" and "Option".
(8 instances of "opcode", 5 instance of "Opcode", 76 instances of
"option").

Normalize capitalization of things like Internal Port and Remote Peer
Port (didn't count them).

Change "error code" to "result code" (5 instances).

Possibly use "NAT" as accepted vernacular throughout, even when
referring to NAPT, unless there's some clear distinction to be made by
referring to NAPT.

There are many places where the text has unnecessary embedded
definitions of implicit and/or explicit mappings:
7.4, 8.6 (twice), 11.1, 12.2, 13, 13.1, 13.2.

----------------
Line-by-line review:

1.  Introduction

   PCP is primarily designed to be implemented in
   the context of both Carrier-Grade NATs (CGN) and small NATs (e.g.,
   residential NATs).

Remove "primarily" - what other kind of NATs are there?

   PCP allows hosts to operate server for a long
   time (e.g., a webcam) or a short time (e.g., while playing a game or
   on a phone call)

A webcam could be long or short term, ditto a game or phone call.
Even the client may not be able to predict how long a mapping may be
needed.  It's immaterial to the protocol.

2.1.  Deployment Scenarios

   PCP can be used in various deployment scenarios, including:
   ...

Make this a real bulleted list, rather than a long bulleted sentence.
Also reorder to group them as current and emerging scenarios, e.g.

   PCP can be used in various deployment scenarios, including:

   o  Basic NAT [RFC3022]

   o  Network Address and Port Translation (NAPT) [RFC3022], such as
      commonly deployed in residential NAT devices

   o  Carrier-Grade NAT [I-D.ietf-behave-lsn-requirements]

   o  Dual-Stack Lite (DS-Lite) [I-D.ietf-softwire-dual-stack-lite]

   o  Layer-2 aware NAT [I-D.miles-behave-l2nat] and Dual-Stack Extra
      Lite [I-D.arkko-dual-stack-extra-lite]

   o  NAT64, both Stateful [RFC6146] and Stateless [RFC6145]

   o  IPv4 and IPv6 simple firewall control [RFC6092]

2.3.  Single-homed Customer Premises Network

   The PCP machinery assumes a single-homed host model.

"machinery" is an odd word
s/The PCP machinery/PCP/

   This
   restriction exists because otherwise there would need to be one PCP
   server for each egress, because the host could not reliably determine
   which egress path packets would take, so the client would need to be
   able to reliably make the same internal/external mapping in every NAT
   gateway, which in general is not possible (because the other NATs
   would likely have the necessary port mapped to another host).

run-on sentence

3.  Terminology

Add "PCP-controlled device" = NAT or firewall

   PCP Client:
      A PCP Client can issue PCP request on behalf of a third

s/request/requests/

      An interworking
      function from Universal Plug and Play Internet Gateway Device
      (UPnP IGD, [IGD]) to PCP is another example of a PCP Client.

Capitalize "interworking function"

   Interworking Function:
      a functional element responsible for interworking another protocol
      with PCP.  For example interworking between UPnP IGD [IGD] with
      PCP.

Capitalize "a"

Also, define "interworking"; maybe something like
      A functional element responsible for translating another protocol
      to PCP.

   subscriber:
      an entity provided access to the network.  In the case of a
      commercial ISP, this is typically a single home.

Capitalize "subscriber", "an"

   5-tuple  The 5 pieces of information that fully identify a flow, from
      the perspective of a subscriber: source IP address, destination IP

Line break before "The"

Also, change "subscriber" to "internal host" or "PCP client" for
consistency.

4.  Relationship between PCP Server and its NAT/firewall

   The PCP server receives PCP requests.  The PCP server might be
   integrated within the NAT or firewall device (as shown in Figure 1)
   which is expected to be a common deployment.

   It is also possible to operate the PCP server in a separate device
   from the NAT, so long as such operation is indistinguishable from the
   PCP client's perspective.

This is completely redundant to the Terminology entry.
In this case, I'd radically trim the terminology section, e.g.

   PCP Server:
      A network element which receives and processes PCP requests from a
      PCP client.  Generally this is a PCP-capable NAT gateway or
      firewall.

5.1.  Request Header

   OpCode:  Opcodes are defined in Section 8 and Section 9.

This seems like it needs something more.

   Requested Lifetime:  The Requested Lifetime field is an unsigned 32-
      bit integer, in seconds, ranging from 0 to 4,294,967,295 seconds.

s/The Requested Lifetime field is //

   PCP Client's IP Address:  The IP address of the PCP client, from the
      PCP client's perspective.  If IPv4, only the first 32 bits are
      used, the other bits MUST be set to 0.

This needs a little explanation, e.g. "If the enclosing IP packet is
IPv4, only the first 32 bits are used; the other bits MUST be sent as
0 and MUST be ignore when received."

This is discussed/explained/rationalized in 6.2, but as "PCP Client IP
Address" (no apostrophe s).

It must be optional - if it's all zeroes, the server will not reject
the packet, and will either send back its idea of the client's IP
address, or leave it zero'd.

Also normalize capitalization: "address" is lowercase in both of the
packet diagrams.

5.2.  Response Header

   Lifetime:  The Lifetime field is an unsigned 32-bit integer, in
      seconds, ranging from 0 to 4,294,967,295 seconds.

s/The Lifetime field is //

   Epoch:  The server's Epoch value.  See Section 6.5 for discussion.
      This value is set in both success and error responses.

This needs a concise description here, e.g. "seconds since start of
epoch".

5.3.  Options

   A PCP OpCode can be extended with an Option.

PCP packets can be extended with Options. (i.e. one or more)

   A given Option MAY be included in a request containing a specific
   OpCode.  The handling of an Option by the PCP client and PCP server
   MUST be specified in an appropriate document and MUST include whether
   the PCP Option can appear (one or more times) in a request and/or
   response, and indicate the contents of the Option in the request and
   in the response.  If several Options are included in a PCP request or
   response, they MAY be encoded in any order by the PCP client and are
   processed in the order received.

rewrite as
   The handling of an Option by the PCP client and PCP server MUST be
   specified in an appropriate document, which MUST include whether
   the PCP Option can appear in a request and/or response, whether it
   can appear more than once, and indicate what sort of Option data it
   conveys.  If several Options are included in a PCP request, they
   MAY be encoded in any order by the PCP client, but MUST be
   processed by the PCP server in the order in which they appear.

(The original wording of the last sentence was contradicted by the
next paragraph: "The response MUST encode the Options in the same
order".)

Actually, the whole paragraph is redundant to later text describing
what an Option definition must include.

   The response MUST encode the Options in the same order,
   but may omit some PCP Options in the response, as is necessary to
   indicate the PCP server does not understand that Option or that

s/may/MAY/
s/as is necessary //

   If the "O" bit (high bit) in the OpCode is clear,

   o  the PCP server MUST only generate a positive PCP response if it
      can successfully process the PCP request and this Option.

   o  if the PCP server does not implement this Option, or cannot
      perform the function indicated by this Option (e.g., due to a
      parsing error with the option), it MUST generate a failure
      response with code UNSUPP_OPTION or MALFORMED_OPTION (as
      appropriate) and include the UNPROCESSED option in the response
      (Section 6.7.1).

Remove the first bullet, make one sentence, change "failure response"
to "error response":

If the "O" bit (high bit) in the OpCode is clear, the PCP server MUST
process this Option.  If the PCP server does not implement this
Option, or cannot perform the function indicated by this Option (e.g.,
due to a parsing error with the option), it MUST generate an error
response with result code UNSUPP_OPTION or MALFORMED_OPTION (as
appropriate), and include the UNPROCESSED option in the response
(Section 6.7.1).

PREFER_FAILURE and FILTER do not include "purpose", but include
non-boilerplate "is included in responses".

Option boilerplate fields should start with a capital letter, e.g.
         Name: <mnemonic>
         Number: <value>
         Purpose: <textual description>
         Valid for OpCodes: <list of OpCodes>
         Length: <rules for length>
         May appear in: <requests/responses/both>
         Maximum occurrences: <count>

5.4.  Result Codes

   The following result codes may be returned as a result of any OpCode
   received by the PCP server.  The only success result code is 0, other
   values indicate an error.  If a PCP server has encountered multiple
   errors during processing of a request, it SHOULD use the most
   specific error message.

s/0,/0;/
s/has encountered/encounters/

   4  UNSUPP_OPTION, unsupported Option.  This error only occurs if the
      Option is in the mandatory-to-process range.

i.e. the "O" bit is clear

6.  General PCP Operation

   The order of operation is that a PCP client generates
   and sends a request to the PCP server, which processes the request
   and generates a response back to the PCP client.

Is it really necessary to say this?  Or is it saying something
normative, like this is a lock-step protocol with no request batching
or pipelining?  In any case, it doesn't belong in the same paragraph
as the discussion of idempotency.

6.1.  General PCP Client: Generating a Request

I'd still like to see this broken out into separate sections on server
discovery, retransmission timers, and server failover.

   The PCP client sends a PCP message the first
   server in its list of PCP servers.

s/message/request to/

It should be stated that there should be only one NAT on each
interface, and the purpose of having multiple server addresses is to
have multiple ways to contact *one* server.  e.g. A PCP server
controlling a DS-Lite AFTR may be reachable by one or more IPv6
addresses, and may also be reachable by IPv4 encapsulated in IPv6.

6.2.  General PCP Server: Processing a Request

   If the source IP address of the received packet does not match the
   contents of the PCP Client IP Address field, a response is generated
   with the ADDRESS_MISMATCH result code.  This is done to detect and
   prevent accidental use of PCP where a non-PCP-aware NAT or NAPT
   exists between the PCP client and PCP server.

s/PCP Client IP Address/PCP Client's IP Address/

!! ADDRESS_MISMATCH is not defined anywhere (should be in 5.4).

   Error responses have the same packet layout as success responses,
   with fields from the request copied into the response, and fields
   assigned by the PCP server are set as indicated in Figure 3

period.

6.3.  General PCP Client: Processing a Response

   If the result code is SERVER_OVERLOADED, clients SHOULD NOT send
   *any* further requests to that PCP server for the indicated error
   lifetime.

s/clients/the PCP client/

   For other error result codes, The PCP client SHOULD NOT
   resend the same request for the indicated error lifetime.

s/The/the/

   If a PCP
   server indicates an error lifetime in excess of 30 minutes, A PCP
   client MAY choose to set its retry timer to 30 minutes.

s/A/a/

   If the PCP client has discovered a new PCP server (e.g., connected to
   a new network), the PCP client MAY immediately begin communicating
   with this PCP server, without regard to hold times from communicating
   with a previous PCP server.

(See section 6.1.)

6.5.  Epoch

   When a client notices that the PCP server reduced its Epoch value,
   the PCP clients will send PCP requests to refresh their mappings.

This is redundant to the immediately preceding sentence.

6.6.  Version Negotiation

   When sending a response containing the UNSUPP_VERSION result code,
   the PCP message MUST be 12 octets long.

i.e. no PCP Client's IP Address or Opcode section - Is this really
what you meant, or was this not updated when PCP Client's IP Address
was added?

I think this restriction is a mistake, and the response should follow
normal guidelines, e.g. copy the whole request into the response.
This restriction was added in -08 with no explanation in the change log.

6.7.1.  UNPROCESSED Option

   If
   a certain Option appeared more than once in the PCP request, that
   Option value can appear once or as many times as it occurred in the
   request.

This was also changed in -08, was "If a certain Option appeared more
than once in the PCP request, that Option value only appears once in
the option-code fields."  I'm not sure this change really adds anything,
but it seems harmless.

         may appear in: responses, and only if the result code is non-
         zero.

I would only expect to see it on UNSUPP_OPTION or MALFORMED_OPTION.

7.  Introduction to MAP and PEER OpCodes

   There are four uses for the MAP and PEER OpCodes defined in this
   document: a host operating a server and wanting an incoming
   connection; a host operating a client and wanting to optimize the
   application keepalive traffic or restore lost state in its NAT; and a
   host operating a client and server on the same port.

That's three (missing 7.3 For Restoring Lost Implicit TCP Dynamic
Mapping State).

The pseudocode is complicated, and has unnecessary stuff, but I'm not
going to fight it.

The pseudocode for PEER (figure 7) doesn't process the PCP response.

7.3.  For Restoring Lost Implicit TCP Dynamic Mapping State

This use case is problematical and contentious, because it requires
the PCP server to create an implicit dynamic mapping, which may not be
possible on that NAT (e.g. it may use different port ranges for
implicit and PCP-controlled mappings).  It's also at odds with the
description of PEER in 9.1, which still says "The PEER OpCodes provide
a single function: the ability for the PCP client to query and
(possibly) extend the lifetime of an existing mapping."  OTOH, it's in
line with later text in 9.3, but that needs to be tightened up and
made normative.

7.4.  For Operating a Symmetric Client/Server

Organizationally, I'd like to see this as 7.2 (after Operating a
Server).  a) It's a closely related topic, and b) this would put the
two MAP scenarios together.

      PCP does not attempt to change or
      dictate how a NAT creates its mappings (endpoint independent
      mapping, or otherwise)

s/mappings/implicit mappings/
After all, explicit dynamic mappings are EIM by definition.

8.  MAP OpCodes

    MAP6=2:   create a mapping between an internal target address and
              external IPv6 address (e.g., NAT46, or firewall)

s/target //

   Note also that all NAT mappings (created by PCP or otherwise) are by
   necessity bidirectional and symmetrical.  For any packet going in one
   direction (in or out) that is translated by the NAT, a reply going in
   the opposite direction needs to have the corresponding opposite
   translation done so that the reply arrives at the right endpoint.
   This means that if a client creates a MAP mapping, and then later
   sends an outgoing packet using the mapping's internal source port,
   the NAT should translate that packet's Internal Address and Port to
   the mapping's External Address and Port, so that replies addressed to
   the External Address and Port are correctly translated to the
   mapping's Internal Address and Port.

Interesting but irrelevant.

8.1.  OpCode Packet Formats

   For both of the MAP OpCodes, if the assigned
   external IP address and assigned external port match the request's
   Internal IP address and port, the functionality is purely a firewall;
   otherwise it pertains to a network address translator which might
   also perform firewall-like functions.

Again, interesting but irrelevant.

8.2.  OpCode-Specific Result Codes

PEER should mention that it uses the same result codes as MAP.

   In addition to the general PCP result codes (Section 5.4), the
   following additional result codes may be returned as a result of the
   four MAP OpCodes received by the PCP server.

!! s/four/two/

   26 EXCESSIVE_REMOTE_PEERS, indicates the PCP server was not able to
      create the filters in this request.  This result code MUST only be
      returned if the MAP request contained the REMOTE_FILTER Option.
      See Section 10.3 for processing information.  This is a long
      lifetime error.

!! s/REMOTE_FILTER/FILTER/

8.3.  OpCode-Specific Client: Generating a Request

   The request MAY contain values in the suggested-external-ip-address
   and suggested-external-port fields.

rewrite as
   The request MAY contain values in the Suggested External Port and
   Suggested External IP Address fields.

8.4.  OpCode-Specific Server: Processing a Request

   If the server is overloaded by requests (from a particular client or
   from all clients), it MAY simply discard requests, as the requests
   will be retried by PCP clients, or MAY generate the SERVER_OVERLOADED
   error response, or both.

Remove "or both" - "discard" implies "without responding".

Also, this is generic, and should be moved to section 6.2.

   If the request contains internal-port=0 and the lifetime is non-zero,
   the server MUST generate a MALFORMED_REQUEST error.
rewrite as
   If the lifetime is non-zero, and the request contains internal-port=0
   or protocol=0, the server MUST generate a MALFORMED_REQUEST error.

   If the requested lifetime is not zero, it indicates a request to
   create a mapping or extend the lifetime of an existing mapping.
add
   If the requested lifetime is zero, it indicates a request to delete
   an existing mapping or set of mappings.

   If the PCP server can allocate the suggested external port, and the
   request did not contain the PREFER_FAILURE Option, it SHOULD do so.
rewrite as
   If the PCP server can allocate the suggested external port, it SHOULD
   do so.

Again, don't pretend a bulleted list is really a long sentence.  And
even if it's structured as an overly long sentence, the proper
connector is not ", or;" but "; or"

   By default, a PCP-controlled device MUST NOT create mappings for a
   protocol not indicated in the request.  For example, if the request
   was for a TCP mapping, a UDP mapping MUST NOT be created.

As opposed to NAT-PMP. Say it.

   If all of the proceeding operations were successful (did not generate
   an error response), then the requested mappings are created or
   refreshed

s/mappings/mapping/
only one per request

8.5.  OpCode-Specific Client: Processing a Response

   After performing common PCP response processing,
add "(see section 6.3)"

    the response is
   further matched with an outstanding request by comparing the
   protocol, internal IP address, internal port.
add "and" before "internal port".

   On error responses,
change to "an error response"

   the assigned external address and assigned external port can also be
   used to match the responses (which is useful if several requests with
   the PREFER_FAILURE option are outstanding).

That makes no sense.  On an error response, the "assigned" external
addr/port are copied from the request.  The client should not have
multiple requests in flight to map the same internal addr/port to
different external addr/port.

   If a successful response,

This clause no verb.  "On a successful response" or "If the response
indicates success".

   the PCP client can use the external IP
   address and port(s) as desired.  Typically the PCP client will
   communicate the external IP address and port(s) to another host on
   the Internet using an application-specific rendezvous mechanism such
   as DNS SRV records.

It's only "port(s)" if there are multiple requests.  Each request
creates a mapping for exactly one port - unless the NAT happens to be
NAT-only, and the request is just to learn to external addr.  Still, I
think it would be clearer to treat each request as creating a single
addr/port mapping.

   On an error response, clients SHOULD NOT repeat the same request to
   the same PCP server within the lifetime returned in the response.

Except the client is supposed to initiate renewal at 1/2 the lifetime.

8.6.  Mapping Lifetime and Deletion

   It is RECOMMENDED that the
   server be configured to restrict lifetimes to less than 24 hours,
   because they will consume ports

s/configured/configurable/
s/they/mappings/

   Mappings created by PCP MAP requests are not
   special or different to mappings created other ways.

This is British usage.  Since the spelling and grammar are mostly
American in this document, you want to say "different from".

   it is implementation-dependent if outgoing traffic extends the
   lifetime of such mappings

add "beyond the PCP-assigned lifetime"

   o  If the internal port is non-zero (port!=0) and protocol is non-
      zero (protocol!=0), it indicates a request to delete the indicated
      mapping immediately.

maybe "the single indicated mapping"

   o  If the internal port is zero (port==0) and the protocol is non-
      zero (protocol!=0), it indicates a request to delete all mappings
      for this Internal Address for the given internal port for all
      transport protocols.

No, that's backwards.  It indicates a request to delete all mappings
for this Internal Address on this protocol.

The effect you describe would be for port!=0 && protocol==0, which is
not specified in this document, because it's not actually useful.

   The suggested external address and port fields are ignored in
   requests where the requested lifetime is 0.

The Suggested External Port and Suggested External IP Address fields...

   If the PCP client attempts to delete an implicit dynamic
   mapping (e.g., created by a TCP SYN), the PCP server deletes the
   mapping and responds with the SUCCESS result code.

Not all NATs may support this.  And shouldn't it be a PEER opcode with
lifetime==0?

   If
   the PCP MAP request was for port=0 (indicating 'all ports'), the PCP
   server deletes all of the explicit dynamic mappings it can (but not
   any implicit mappings)

Compare and contrast with deleting a single implicit mapping.

   An explicit dynamic mapping MUST NOT have its lifetime
   reduced by transport protocol messages (e.g., TCP RST, TCP FIN).

This should be a separate paragraph, perhaps with some justification.

   To reduce unwanted traffic and data corruption, UDP and TCP ports
   should not be immediately re-used for an interval (TIME_WAIT interval
   as discussed in [RFC0793]).  However, the PCP server MUST allow the
   same subscriber and same internal address to re-acquire the same port
   during that interval.

As I mentioned in a previous review, these two requirements are
incompatible.  The MUST in the second sentence changes the "should not"
in the first sentence to a MUST NOT; or the "should not" in the first
sentence changes the MUST in the second sentence to a SHOULD.  It's more
realistic in the SHOULD form, so I'd go with that.

Also remove the word "immediately".  Possibly refer to this explicitly
as a hold-down timer.

   *  all ports, all protocols, all Internal Addresses for which the
   client is authorized:  internal address=0, via the THIRD_PARTY option

I would still recommend setting protocol==0, port==0.

8.7.  Subscriber Renumbering and Address Change Events

   PCP defined in this document
   does not provide machinery to reduce the subscriber renumbering
   problem.

Well, duh.

9.1.  OpCode Packet Formats

   The PEER OpCodes provide a single function: the ability for the PCP
   client to query and (possibly) extend the lifetime of an existing
   mapping.

No longer true.

   Requested Lifetime (in common header):  Requested lifetime of this
      mapping, in seconds.  Note that, depending on the implementation
      of the PCP-controlled device, it may not be possible to reduce the
      lifetime of a mapping (or delete it, with requested lifetime=0)
      using PEER.

There is no discussion about deleting mappings with PEER.

External_AF is the only packet field with an underscore.  I would
expect it to be "External AF".

   External_AF:  For success responses, this contains the address family
      of the external IP address associated with this peer connection,
      to properly decode the External IP Address.  This field is
      necessary because the Remote Peer's IP Address is from the PCP
      client's perspective, whereas the External_AF and External IP
      Address are from the PCP-controlled device's perspective.  As an
      example, if the PCP-controlled device is a NAT64, the PCP client
      only knows the remote peer's IPv6 address, whereas the NAT64 knows
      the remote peer's IPv4 address.  Values are from IANA's address
      family numbers (IPv4 is 1, IPv6 is 2).  For error responses, the
      value is copied from the request.

So this actually creates a difference between MAP and PEER in NAT64:
with MAP, the client would have to know that it's behind a NAT64, and
send a MAP4 request over v6.  Or possibly it would send MAP6, and the
NAT64 would return a v4-mapped v6 address, which the client would then
figure out is mapped, and advertise the corresponding v4 address to
its peers?  Whereas on a PEER6, it has this peer address that's really
v4-mapped v6, and the NAT64 returns an external addr that's explicitly
v4.  It all sounds complicated, and I'd like Simon to explain just how
he sees this all working in NAT64.

   External Port:  For success responses, this is the external port
      number, assigned by the NAT (or firewall) to this mapping.  If
      firewall or 1:1 NAT, this will match the internal port.  For error
      responses, this MUST be 0.

The last restriction doesn't make sense, and doesn't match the usage
in the MAP opcodes ("On error responses, the value from Suggested
External Port is used.").  On an error response, the external addr and
port should be ignored - they could be copied from the request, they
could be zeroed, but they don't contain any meaningful data in either
case.

   External IP Address:  For success responses, this contains the
      external IP address, assigned by the NAT (or firewall) to this
      mapping.  This field allows the PCP client and its remote peer to
      determine if there is another NAT between the PCP-controlled NAT
      and remote peer.

And what will they do with that information?

9.2.  OpCode-Specific Client: Generating a Request

   The PEER4 or PEER6 OpCodes MAY be sent before or after establishing
   bi-directional communication with the remote peer.  If sent before,
   PEER4 or PEER6 OpCodes will create a mapping in the PCP-controlled
   device (for the purpose described in Section 7.3), and the client
   SHOULD set the External_AF and Suggested External IP Address to the
   values of the previous mapping.

and Suggested External Port

   If sent after, the PEER4 or PEER6
   OpCodes query (and control) the implicit dynamic mapping (for the
   purpose described in Section 7.2).

and the Suggested External Port and Suggested External IP Address
fields SHOULD be set to zero.

   If the described mapping does not yet exist yet, it
   is created, honoring and the Suggested External Port and Suggested
   External IP Address are honored 

s/and //
s/are honored //

   (if possible; if not possible, a
   mapping on a different IP address or different port is created).

This implies that the PREFER_FAILURE Option is also valid for PEER
opcodes.

   By
   having PEER create such a mapping, we avoid a race condition between
   the PEER request or the initial outgoing packet arriving at the NAT
   gateway first

If the race condition is really a concern, we just mandate a delay
before sending the PEER request.  The PEER response would also tell
whether the implicit mapping had been successfully created.

It should be possible to delete mappings with PEER, just as for MAP.

Mention THIRD_PARTY with PEER.  Also USER_EX_QUOTA.

MAP and PEER processing should follow pretty much the same path on the
server; the spec should explicitly say what's the same and what's
different.

9.4.  OpCode-Specific Client: Processing a Response

   If a successful response, the application can use the assigned

This clause no verb.  "On a successful response" or "If the response
indicates success".

   If the PCP client wishes to keep this mapping alive beyond the
   indicated lifetime, it SHOULD issue a new PCP request prior to the
   expiration.  That is, inside->outside traffic is not sufficient to
   ensure the mapping will continue to exist.  It is RECOMMENDED to send
   a single renewal request packet when a mapping is halfway to
   expiration time, then, if no SUCCESS response is received, another
   single renewal request 3/4 of the way to expiration time, and then
   another at 7/8 of the way to expiration time, and so on, subject to
   the constraint that renewal requests MUST NOT be sent less than four
   seconds apart (a PCP client MUST NOT ever-closer-together requests in
   the last few seconds before a mapping expires).

For MAP, the language about renewing mappings is in the Generating a
Request section, not the Processing a Response section.

10.1.  THIRD_PARTY Option for MAP and PEER OpCodes

   A THIRD_PARTY Option MUST NOT contain the same address as the source
   address of the packet.  A PCP server receiving a THIRD_PARTY Option
   specifying the same address as the source address of the packet MUST
   return a MALFORMED_REQUEST result code.  This is because many PCP
   servers may not implement the THIRD_PARTY Option at all, and a client
   using the THIRD_PARTY Option to specify the same address as the
   source address of the packet will cause mapping requests to fail
   where they would otherwise have succeeded.

I would expect the operation to succeed if the server supports
THIRD_PARTY, because the client is making a valid request, albeit a
bit oddly.  And if the server doesn't support THIRD_PARTY, it will
return UNSUPP_OPTION, regardless of the contents of the option.

I would prefer to have to option header appear in all option layout
diagrams, for clarity, e.g.

    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   | Option Code=4 |  Reserved     |   Option-Length=4 or 16       |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   :                                                               :
   :    Internal IP Address (32 bits of 128 bits, depending        :
   :                                       on Option length)       :
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

   The following additional result codes may be returned as a result of
   using this Option.

s/codes/code/

   51 UNAUTH_THIRD_PARTY_INTERNAL_ADDRESS

How about using the existing NOT_AUTHORIZED code?  In fact, 8.4 says
the server MUST return NOT_AUTHORIZED in this specific case, and 9.3
doesn't mention THIRD_PARTY, so UNAUTH_THIRD_PARTY_INTERNAL_ADDRESS is
only mentioned in this section.  Seriously, this scenario would seem
to be covered under the following clause from NOT_AUTHORIZED: "the PCP
client requested a mapping that cannot be fulfilled by the PCP
server's security policy."

Paragraph break before "Determining which PCP clients are authorized
to use the THIRD_PARTY option depends on the deployment scenario."

   For home deployments (where the PCP server is
   embedded in the NAT device), this option MUST NOT be processed.

I would say "where the PCP server and the NAT are embedded in a home
gateway device".  Also, THIRD_PARTY would typically not be implemented
on a home gateway, so would generate UNSUPP_OPTION.

   It is RECOMMENDED that PCP servers embedded into customer premise
   equipment be configured to refuse third party mappings by default.
   With this default, if a user wants to create a third party mapping,
   the user needs to interact out-of-band with their customer premise
   router (e.g., using its HTTP administrative interface).

This is largely redundant to the previous paragraph.

   It is RECOMMENDED that PCP servers embedded into service provider NAT
   and firewall devices be configured to permit the THIRD_PARTY option,...

This whole paragraph is confusing, and it's hard to tell what's
actually being recommended or dis-recommended here.  The simple fact
of the matter is that THIRD_PARTY is most useful in DS-Lite, where the
NAT can send packets "directly" to internal hosts that don't embed a
PCP client; i.e. the B4 embeds a UPnP IWF and/or a NAT-PMP IWF and/or
a PCP-PCP proxy.

In a NAT444 scenario, the CGN can't "see" the internal hosts, so
THIRD_PARTY is pointless.  In NAT64 and NAT46, there isn't an obvious
place for an IWF or proxy, so it probably isn't useful there, although
I haven't spent a lot of time thinking about it.

   If the packet arrived from a
   different address, the PCP server MUST generate an
   UNAUTH_THIRD_PARTY_INTERNAL_ADDRESS error.

s/arrived/arrives/

10.2.  PREFER_FAILURE Option for MAP OpCodes

   This option is only used with the MAP4 and MAP6 OpCodes.

Since we redefined PEER to create or re-create mappings, we should
also support PREFER_FAILURE, and should mention it here and in 11.3.1.

If UNAUTH_THIRD_PARTY_INTERNAL_ADDRESS is defined under the
THIRD_PARTY Option (and I've argued that it shouldn't be defined at
all), then CANNOT_PROVIDE_EXTERNAL_PORT should be defined under the
PREFER_FAILURE Option.  Likewise EXCESSIVE_REMOTE_PEERS should be
defined under the FILTER Option.  But really, I'd prefer to keep all
the result codes in 8.2.

10.3.  FILTER Option for MAP OpCodes

   This Option indicates filtering incoming packets is desired.  The
   remote peer port and remote peer IP Address indicate the permitted
   remote peer's source IP address and port for packets from the
   Internet.

s/remote peer port/Remote Peer Port/
s/remote peer IP Address/Remote Peer IP Address/

Change the "prefix-length" field name to "Prefix Length".

I've never been able to figure out why "Reserved" is the first field,
not the second.

Note that FILTER is unidirectional - it doen't prevent an internal
host from sending packets to any remote host, it just prevents remote
hosts from replying.  I'm not saying this needs to be changed.

      is included in responses: MUST, if it appeared in the request

As previously noted, this field is not in the 5.3 boilerplate.
In 5.3, we should say that, if a request includes an Option that is
marked mandatory-to-process, the response (whether success or error)
MUST include that Option.

      length: 2 if used with MAP4, 5 if used with MAP6

!! Wrong; this is the old length (measured in 4-byte words).
Should be "8 if used with MAP4, 20 if used with MAP6".

   If a MAP request has a lifetime of 0 and contains the
   FILTER option, the error MALFORMED_OPTION is returned.

This is an example of the nonsensical-option clause of 8.4.

11.1.  Implementing MAP with non-EIM port-mapping NAPT

   For implicit dynamic mappings, some existing NAT devices have
   endpoint-independent mapping (EIM) behavior while other NAPT devices
   have non-endpoint-independent mapping (non-EIM) behavior.

Here you use NAT and NAPT in the same sentence.  As I said before, I
would refer to all such devices by the vernacular "NAT", unless
there's some clear distinction to be made by referring to NAPT.

   1.  have implicit dynamic mappings (e.g., TCP SYN) use a different
   2.  on arrival of a packet (from the Internet or from an internal

Capitalize

BTW, I prefer the second approach, which uses the most specific match
(somewhat analogously to kernel packet routing).  OTOH, I know Francis
prefers the first approach.

As I see it, the conflict would only arise if there was an existing
implicit mapping, and then an explicit mapping was created.  If the
explicit mapping exists, the NAT will use it for all new connections
from that internal addr/port/proto, rather than creating a new
implicit mapping.

11.2.  Interaction of MAP with Implicit Dynamic Mappings

I'm a bit confused by this one.  If the NAT creates an implicit
mapping that conflicts with an explicit mapping, it's a different
(more specific) mapping in the NAT's mapping table.  Even if the NAT
deletes that mapping on termination of the connection, that won't
affect the (more general) explicit mapping - it would still allow
incoming connections.

13.  Security Considerations

   This document defines Port Control Protocol and two types of OpCodes,
   PEER and MAP.  The PEER OpCode allows querying and extending (if
   permitted) the lifetime of an existing implicit dynamic mapping, so a
   host can reduce its keepalive messages.  The MAP OpCode allows
   creating a mapping so a host can receive incoming unsolicited
   connections from the Internet in order to run a server.

This whole paragraph can be deleted, because it doesn't say anything
new, and it doesn't say anything about security.

14.1.  Port Number

   PCP will use port 5351 (currently assigned by IANA to NAT-PMP).  We
   request that IANA re-assign that same port number to PCP, and
   relinquish UDP port 44323.

The bit about relinquishing port 44323 should be marked for removal by
the RFC Editor.

14.2.  OpCodes

   Additional OpCodes in the range 4-95 can be created via Standards
   Action [RFC5226], and the range 96-126 is for Private Use [RFC5226].

!! s/4/5/ (4 is PEER6)
How/why was the number 95 chosen?

14.3.  Result Codes

   IANA shall create a new registry for PCP result codes, numbered
   0-255, initially populated with the result codes from Section 5.4,
   Section 8.2, Section 10.3, and Section 10.1.  The values 0 and 255
   are reserved.

10.3 doesn't define a result code.

   Additional Result Codes can be defined via Specification Required
   [RFC5226].

There needs to be a Private range for result codes, to support Private
OpCodes and Options.

14.4.  Options

   IANA shall create a new registry for PCP Options, numbered 0-255 with
   an associated mnemonic.  The values 0-127 are mandatory-to-process,
   and 128-255 are optional-to-process.  The initial registry contains
   the options described in Section 10 and Section 10.1.  The option
   values 127 and 255 are reserved.

and sections 10.2 and 10.3 - why don't we just say section 10 (as a
whole), and drop the reference to 10.1?