Subject: [Acme] Genart last call review of draft-ietf-acme-acme-11

From: Dale Worley <[email-masked]>
Date: Wed, 18 Apr 2018 18:03:22 -0700

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]

---

From: Richard Barnes <[email-masked]>
Date: Tue, 24 Apr 2018 11:30:19 -0400

Hi Dale,

Thanks for the review.  Responses inline below; changes in this PR:

https://github.com/ietf-wg-acme/acme/pull/424

> particular, by adding further resources to the directory object.

The WG discussed versioning / extensibility several times, and concluded
that on the one hand, there are enough extension points in the protocol to
cover many cases (e.g., new challenge types), and on the other hand,
extensions that are not compatible with that approach can be handled with a
new API endpoint.  Clients have always needed to be configured with an HTTP
URL for the directory; this just notes that they also need to know what
flavor of ACME is running there.

In other words, there's no need for version negotiation in-band to the
protocol.

> and the hash of the ToS document (so the client must attest to what

This mechanism has been reviewed by multiple CAs and found to be sufficient
for their needs.  A prior version had an explicit URL, and it was found to
cause interop problems.

> respective objects, although the text doesn't identify that.

I don't think it's all that useful to parse close semantics into this
diagram.  "newNonce" is different, in that it serves an underlying
transport purpose, rather than a certificate management purpose.

> Strictly, you want "URIs" here, as tel:, sip:, and mailto: URIs are

For readability in the implementer community, where "URL" is much more
widely used than "URI", the convention in this document is to use "URL".
It seems unlikely this will lead to interop problems.

> needed for the two services so as to make the processing described in

You seem to be envisioning a scenario where a CA relies on accounts
established with some third party service, which AFAIK no CA actually does.

Changed "a CA" to "the CA operating the ACME server".

> somewhere else in the document.

As you point out, there's no ideal place for the text in question.  Either
it's at the front, in which case finalization hasn't been discussed yet, or
it's at the end, where some things are redundant.  I think it's OK where it
is.

> certificate that it issued.

A CA cannot revoke certificates it has not issued.

> might want to remove this sentence.

There's no harm in storing it; servers can make their own decisions.

>    Each following certificate SHOULD directly certify some certificate

Yeah there's a missing "the".

> document is the reference for that action.

I've updated the reference, but the objectief here is precisely to reserve
these identifiers from future use.

---

From: Richard Barnes <[email-masked]>
Date: Tue, 24 Apr 2018 11:30:18 -0400

Hi Dale,

---

From: Sophie Herold <[email-masked]>
Date: Tue, 24 Apr 2018 17:55:47 +0200

Hi,

> There's no harm in storing it; servers can make their own decisions.

Storing the key authorization avoids interference between a pending
authorization and an account key roll-over.

Best,
Sophie

---

From: [email-masked] (Dale R. Worley)
Date: Tue, 24 Apr 2018 20:39:47 -0400

Richard Barnes <[email-masked]> writes:
> [...]

Yeah, that makes sense.

>> [...]

I'll take their word for it.  Personally, I wouldn't consider it
particularly sufficient for *my* needs to have an alleged assent to
terms of service without any clear record of what I'm assenting *to*.
OTOH, given that there's no such record, it'd be difficult for a CA to
refute my claim of what the agreement was...

>> to use when there is no natural overall error type.

Any comments here?

> transport purpose, rather than a certificate management purpose.

I'd say that if two things are shown differently in a diagram, then they
ought to *be* different, and if they're the same, they should be shown
the same way.  Certainly, when I looks at a diagram and one thing is not
shown the same way as the others, I want to know what the semantics of
that is.

>> [...]

The title of the section is "External Account Binding", and I
reflexively take "external" to mean "external to the relationship
between the CA and the client".  I think what you want to do is revise
the first paragraph

   The server MAY require a value for the "externalAccountBinding" field
   to be present in "newAccount" requests.  This can be used to
   associate an ACME account with an existing account >>that the user has
   with the CA<< in a non-ACME system, such as a CA customer database.

And perhaps change the section title to "Binding to a non-ACME Account".

Dale