Hi Dale,
Thanks for the review. Responses inline below; changes in this PR:
On Wed, Apr 18, 2018 at 9:03 PM, Dale Worley <worley@xxxxxxxxxxx> wrote:
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 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.
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).
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.
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.
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.
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].
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.
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.
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".
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.
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.
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.
A CA cannot revoke certificates it has not 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.
There's no harm in storing it; servers can make their own decisions.
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.
Yeah there's a missing "the".
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.
I've updated the reference, but the objectief here is precisely to reserve these identifiers from future use.
