[Gen-art] Genart last call review of draft-ietf-acme-acme-11

[Dale Worley <[e-mail]>]

Reviewer: Dale Worley
Review result: Ready with Issues

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
<https://wiki.tools.ietf.org/area/gen/wiki/GenArtfaq>.

Document:  draft-ietf-acme-acme-11
Reviewer:  Dale R. Worley
Review Date:  2018-04-18
IETF LC End Date:  2018-04-18
IESG Telechat date:  2018-05-10

Summary:

       This draft is on the right track but has open issues, described
       in the review.

This draft is much better than the version (-08) that I previously was
the assigned Gen-ART reviewer for.  The overall work and exposition
are quite solid, though there are some open technical issues that need
to be resolved.

* Technical issues

What is the versioning and extensibility system?  -- It seems that the
natural approach is that structures returned by fetches of Acme
resources may contain elements that are not defined in this document,
and a client should ignore them if it doesn't understand them.  This
allows plenty of flexibility for extending the Acme protocol; in
particular, by adding further resources to the directory object.

The handling of "terms of service agreement" seems to be insufficient,
in that none of the information passed around says *what* agreement
the client has agreed to.  The client only sends
"termsOfServiceAgreed:true" in a request, leaving what has been agreed
to unspecified -- and unauditable.  One possibility is to add an
argument for the client to provide the URL from which it fetched the
ToS (so the server knows what agreement the client is referring to)
and the hash of the ToS document (so the client must attest to what
the agreement text was).

6.6.1.  Subproblems

What error type should be used in a problem document when there are
subproblem documents?  It seems that in this situation, the intention
is that the top-level problem document is not intended to carry error
information itself, so you want to define a "subproblems" error type,
to use when there is no natural overall error type.

* Editorial issues

7.1.  Resources

The position of "newNonce" in the diagram is strange.  Does it have a
different relationship to the directory resource than newAccount,
etc.?  Similarly for the "finalize" and "cert" URLs of an order
object. -- Reading further in the draft suggests that the graphical
difference indicates that that these values are optional in the
respective objects, although the text doesn't identify that.

7.1.2.  Account Objects

   contact (optional, array of string):  An array of URLs that the
      server can use to contact the client for issues related to this
      account.  For example, the server may wish to notify the client
      about server-initiated revocation or certificate expiration.

I mentioned this in my review of -08, but it hasn't been fixed:
Strictly, you want "URIs" here, as tel:, sip:, and mailto: URIs are
not URLs [RFC 6068].

7.3.5.  External Account Binding

   To enable ACME account binding, a CA needs to provide the ACME client
   with a MAC key and a key identifier, using some mechanism outside of
   ACME.  The key identifier MUST be an ASCII string.  The MAC key
   SHOULD be provided in base64url-encoded form, to maximize
   compatibility between non-ACME provisioning systems and ACME clients.

I *think* what this means is that the service providing the external
account provides the ACME client with a MAC key and a key identifier,
which the client then uses in constructing its request to the ACME
server.  If that is correct, this text is not making clear (to me) the
distinction between the CA that operates the ACME server (which I take
as the default meaning of "CA" in this document) and the service that
provides the "external account".  I think two different terms are
needed for the two services so as to make the processing described in
this section clear.

7.4.  Applying for Certificate Issuance

This section seems to describe both the process of creating an order
and the process of finalizing an order.  The initial paragraphs regard
creating an order, but the text starting with "Once the client
believes it has fulfilled the server's requirements..." it talks about
finalization.  The text continues to discuss finalization until it
gets to this confusing item:

   o  "ready": The server agrees that the requirements have been
      fulfilled, and is awaiting finalization.  Submit a finalization
      request.

I *think* what is going on is that the text from "The status of the
order will indicate..." through the 5 following items are a *general*
description of the status field which is limited to neither the order
creation step nor the order finalization step.

It seems to me that this section should be divided into three parts,
one describing creating an order, one describing finalizing an order,
and the generalized description of the status values and the
next-steps that each value implies.  The first two parts might be
better expressed as two subsections of 7.4, to clarify which part
of the order life cycle contains which actions.  It's not clear to me
the best way to handle the third part; it may be most useful placed
somewhere else in the document.

7.6.  Certificate Revocation

It is implicit but might be worth mentioning that the server should
only accept revocation requests for certificates that it itself has
issued.  I mention this because the revocation request signed by the
certificate's key does not directly reference an account in the
server, but (I think) provides enough information that the server
could construct an apparently legitimate CRL entry for the certificate
using just the information in the revocation request.  Thus, a sloppy
implementation might skip verifying that it is dealing with a
certificate that it issued.

8.3.  HTTP Challenge

   On receiving a response, the server constructs and stores the key
   authorization from the challenge "token" value and the current client
   account key.

I'm not sure this storage step is necessary, or even visible in the
protocol operation.  (E.g., the server can calculate the key
authorization at any time that it needs to know the value.)  So you
might want to remove this sentence.

Similarly for section 8.4.

9.1.  MIME Type: application/pem-certificate-chain

   Each following certificate SHOULD directly certify one
   preceding it.

This is grammatical, but it leaves me wondering if a typo was made,
because "certify one preceding" and "certify the one preceding" are
both grammatical but have different meanings.  I suggest you replace
it with one of the following:

   Each following certificate SHOULD directly certify the one
   preceding it.

   Each following certificate SHOULD directly certify some certificate
   preceding it.

9.7.8.  Validation Methods

            | tls-sni-01 | RESERVED        | N    | N/A       |
            |            |                 |      |           |
            | tls-sni-02 | RESERVED        | N    | N/A       |

It seems pointless to insert two entries into a registry with no
documentation reference, unless the intention is to reserve these
identifiers from future use.  But in that case, this document is
actively prescribing that the identifiers are reserved, and this
document is the reference for that action.

[END]