Re: [Acme] ACME draft is now in WGLC.

[Martin Thomson <[e-mail]>]

On 11 February 2017 at 05:56, Salz, Rich <[e-mail]> wrote:
> ACME WGLC

It's been a while since I did a review of this, so I spent a few hours on it.

This document is in pretty good shape.  I have a lot of comments (a
LOT), and some of these are serious enough that I'd want to see
another WGLC, but they are relatively minor.  Overall the document is
in very good shape; most of my comments are of the nitpicking variety.

This is a pretty significant piece of work and that this is as good as
it is represents cause for congratulations to those involved (except
me, I'm just a hazard).


Meta

I found the use of lowercase versions of RFC 2119 keywords quite
distracting, and occasionally confusing.  It's often the case that the
lowercase version is used where a simple imperative statement would
work.  I haven't called out many of those in my review, except where
they caused what I think to be a genuine problem.

I have created an editorial pull request on the spec.  There were lots
of tiny fixes (including some opportunities to correct the above).  In
some cases, that PR will conflict with substantial changes that I
think need to happen, so you might want to attend to that first.

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


S5.1

   Clients SHOULD
   support HTTP public key pinning [RFC7469], and servers SHOULD emit
   pinning headers.

and

   ACME clients SHOULD send a User-Agent header in accordance with
   [RFC7231], including the name and version of the ACME software in
   addition to the name and version of the underlying HTTP client
   software.

and

   Such servers SHOULD
   set the Access-Control-Allow-Origin header field to the value "*".

Why?  It's usually good practice with a SHOULD to provide some sort of
reason why choosing not to comply might be necessary, or to otherwise
motivate the choice.  All three seem relatively easy to fix with a
simple sentence (pinning good m'kay, client bugs(?), building a
web-based client).

S5.2

   For all other requests, there MUST be a "kid" field.  This field must
   contain the account URI received by POSTing to the new-reg resource.

MUST?

   Servers MUST NOT respond to GET
   requests for resources that might be considered sensitive.

Since this is a requirement, it might pay to do a little due diligence
on what might be sensitive.  I'm not sure that all implementers will
have the same view (or same motivations) when it comes to this.

  server MUST return an error

This would benefit from a reference to S5.7, as does all the other
instances of error handling.  Though this section includes an example
(that seems unnecessary), it's very hard to tell if this is normative
or not from context.

   In the examples below, JWS objects are shown in the JSON or flattened
   JSON serialization, with the protected header and payload expressed
   as base64url(content) instead of the actual base64-encoded value, so
   that the content is readable.  Some fields are omitted for brevity,
   marked with "...".

I didn't really understand this without an example to use for
reference.  Given that the first actual use of this form is 15 (!)
pages further down the document, maybe you could move this there.

S5.3

   JWK thumbprint [citation needed]

S5.4

I would reference RFC 7230, Section 2.7.3 "http and https URI
Normalization and Comparison" here and note the risk that
normalization could cause comparison failures.  A recommendation that
URLs generated by the server SHOULD be normalized according to that
section so that the risk of normalization is reduced.

S5.5

   The server MUST include a Replay-
   Nonce header field in every successful response to a POST request,
   and SHOULD provide it in error responses as well.

This seems to purposefully neglect to mention other requests, like
GET.  Does the "SHOULD" apply just to POST requests, or is it any
method?

This section says that nonces are mandatory with POST requests, but
does not offer a way to get a nonce that does not require a nonce.
Please make this observation and reference S6.2.

S5.6

Once again, mention is made of error descriptions without a forward reference.

   Additionally, the server
   SHOULD send a "Retry-After" header indicating when the current
   request may succeed again.

"may" or "could"?

S5.7

In a couple of places, a specific error URN is mandated (with MUST),
but this section only uses SHOULD for the error description.

   Authorization and challenge objects can also contain error
   information to indicate why the server was unable to validate
   authorization.

Where and how would this appear?  This needs more definition (I
looked, but I didn't find anything further down).

S6.1

The list of resources could use some references to the sections that
define them.

S6.1.2

   status (required, string):  The status of this account.  Possible
      values are: "valid", "deactivated", and "revoked".  The value
      "deactivated" should be used to indicate user initiated
      deactivation whereas "revoked" should be used to indicate
      administratively initiated deactivation.

I assume that "user" and "administratively" mean "client" and "server"
respectively.  Please clarify this point, it's confusing.

S6.1.3

   authorizations (required, array of string):  For pending orders, the
      authorizations that the client needs to complete before the
      requested certificate can be issued (see Section 6.5).  For final
      orders, the authorizations that were completed.  Each entry is a
      URL from which an authorization can be fetched with a GET request.

Which states correspond to final?

If an order is pending with one authorization complete and another
incomplete, do I read that the complete authorization is NOT listed
here? But that doesn't fit with this:

   The elements of the "authorizations" array are immutable once set.

It might pay to clarify this some.

S6.1.4

   [[ Open issue: More flexible scoping? ]]

Red flag.  In my view, the simple approach is fine.  New authorization
resources are cheap.  They can even be cloned easily, even allowing
one challenge to fulfill multiple.

      A client should attempt to fulfill at most one of these challenges,
      and a server should consider any one of the challenges sufficient
      to make the authorization valid.

lowercase should or SHOULD or MUST?  This really needs more meat if it
is to be useful.  If we accept that a client can attempt to fulfill
more than one challenge on the basis that they might fail, then permit
them to try many, but encourage only one.  On the server side, if a
server later discovers that the challenges are insufficient, then
permit that too.

"A client only needs to complete one of these challenges, though it
MAY attempt more than one challenge if errors prevent it from
completing its first choice.  A server therefore MUST accept any
challenge, though a server MAY still choose not to permit an
authorization for other reasons."

Or something along those lines.  (BTW, I applaud the simplification
from earlier versions.)

S6.3

   A client creates a new account with the server by sending a POST
   request to the server's new-account URI.  The body of the request is
   a stub account object containing only the "contact" field.

In addition to the user account binding (S6.3.2), the example violates
this by including a ToS agreement.  I suspect the above is incorrect.

   [...] the server MUST reject new-account
   requests that do not have the "terms-of-service-agreed" set to
   "true".

Which error code do we use here?

S6.3.1

Is the intent that the "instance" document be loaded in a web browser,
as opposed to being poked by an ACME client?  It might help to
explicitly say that.

S6.3.2

   [...] and MUST reflect [the?] value of the
   "external-account-binding" field in the resulting account object

Is this a direct copy of the MAC that was provided?  Do you realize
that this enables a user with access to any account that was created
with a binding to replay the binding in new accounts without actually
having the MAC key?  Would it be acceptable to just reflect that the
binding exists?  (Potentially bad idea: just include the key id; if
the CA wants, that key identifier could be a URL to the external
account details page and do double-duty.)

Note that this would be a violation of the stated security goal of not
having integrity compromised by a TLS MitM or CDN.  It doesn't
necessarily permit misissuance unless the binding to the account
carries authorizations across, or things like that.

S6.3.3

"NOT REQUIRED" isn't a thing (at least as far as I can tell).  "MAY
omit the "nonce" parameter" would be fine, I think.  Frankly, I don't
see why they can't both be required to include a nonce, but I guess
that the account URI should be plenty specific enough to avoid any
problems with copy and paste of the inner JWS.

S6.3.4

   It is up to server policy
   how long to retain data related to that account, whether to revoke
   certificates issued by that account, and whether to send email to
   that account's contacts.

This is terrible.  If I wish to decommission an account key, then I
can't because I might find that my certificates are all suddenly
revoked.  Think about a large organization that has a pool of
authorized accounts used for managing certificates.  If one of those
needs to fall out of the pool (the machine hosting the key is being
scrapped or rebuilt, for instance), then you don't want to have all
the certificates that it issued disappearing suddenly.

If there are good reasons to revoke certificates, then be definite
about it and say that they go away, at least then people can plan
around the problem.

S6.4

   The body of this response is
   an order object reflecting the client's request and any
   authorizations the client must complete before the certificate will
   be issued.

I don't think that this is "MUST", but it's a confusing word to use here.

BTW, I like the description of the states in terms of what the client
should do.  There are other cases where this would be valuable, such
as in S6.5.


S6.4.1

...mentions "combinations", but I think that this has gone away.

   The server can also provide the client with direct access to an SCT
   for a certificate using a Link relation header field with relation
   "ct-sct".

Where is this link relation type registered?  It isn't in either the
IANA considerations section or in the registry:
http://www.iana.org/assignments/link-relations/link-relations.xhtml

The same goes for the other link relation types that are used in the
example.  I see neither "revoke" nor "directory" being registered.
BTW, "index" would probably do well instead of "directory", "revoke"
seems specific enough to this usage - like "ct-sct" that using a URI
for the relation type would probably be better than registering a link
relation.

S6.5.1

   If the final state is
   "valid", then the server MUST include an "expires" field.  When
   finalizing an authorization, the server MAY remove challenges other
   than the one that was completed, and may modify the "expires" field.

This just said two conflicting things about the "expires" field.  I
think that this could be: "When the authorization is finalized, if it
is "valid" then `expires` MUST be set, otherwise `expires` MUST be
removed.  A server MAY remove challenges other than the one that was
completed from a finalized authorization."

  Usually, the validation process will take some time,

Could this instead be "The validation process could take some time",
rather than making some statement based on what we know now in this
very narrow application of the protocol?

   In responding to poll requests while
   the validation is still in progress, the server MUST return a 202
   (Accepted) response

The 202 is an odd choice here.  The resource exists and has content,
so 200 works perfectly well for this purpose.

The use of Retry-After here is odd, but I think that it's fine.  I
would separate this out in a new paragraph: "A server MAY include a
Retry-After header field in its responses to clients, clients that are
polling for updates SHOULD wait at least this amount of time before
making another request to that resource."

S6.6

   The server MAY disallow a subset of reasonCodes from
      being used by the user.

Does a server ignore the reasonCode, or does it reject the request
when an impermissible code is chosen by the client?

S7

   This section describes an initial set of challenge types.  Each
   challenge must describe:

MUST, I think

S7.1

   UTF-8 form (or, equivalently, ASCII).

A normative citation for RFC 3269 is needed (and maybe even RFC 20).

S7.2

   Because many webservers allocate a
   default HTTPS virtual host to a particular low-privilege tenant user
   in a subtle and non-intuitive manner, the challenge must be completed
   over HTTP, not HTTPS.

MUST

   2.  Verify that the resulting URI is well-formed.

How is this ever not true?  The URI has to be well-formed since the
template is correct by design and the values for domain and token have
already been checked.

S7.3&4

   It is RECOMMENDED that the server open multiple TLS connections from
   various network perspectives, in order to make MitM attacks harder.

Isn't this equally valid for http?  (Or - more to the point - equally
invalid here?)

S9.2

I think that it makes sense here to say somewhere that while a TLS
MitM or CDN might compromise confidentiality (which has some
implications for account holders), they cannot compromise integrity.

Stylistically, this section is two things: a discussion of the ACME
channel and then a discussion of the challenge channel, would
subsections help?

   For identifiers
   where the server already has some public key associated with the
   domain this attack can be prevented by requiring the client to prove
   control of the corresponding private key.

This doesn't seem right.  I believe that the model permits multiple
accounts to act for a single domain.  Not doing so would represent a
pretty big operational burden.  What public key does this then refer
to?  Or is this promise not valid?

S9.3

You should also recommend rate limits on account creation, or your
other mitigations might be weakened.

S9.4

Does an ACME server send Referer[sic]?  What should it set it to?

S9.5

This should probably recommend doing these checks - to the extent
possible - before creating the order.  Though issuance is the ultimate
thing that needs to be controlled, allowing an order to proceed just
makes extra work for all involved.

S10.2

   A CA that accepts TLS-based proof of domain control should attempt to
   check whether a domain is hosted on a domain with a default virtual
   host before allowing an authorization request for this host to use a
   TLS-based challenge.  A default virtual host can be detected by
   initiating TLS connections to the host with random SNI values within
   the namespace used for the TLS-based challenge (the "acme.invalid"
   namespace for "tls-sni-02").

This doesn't seem right.  If the default vhost is an attacker, then
they can generate the answers the server expects.  Based on this text,
I don't actually know what the right answer might be, is it a TLS
unrecognized_name alert?