Minor fixes in responses to pre-RFC-Editor review with EKR

[bifurcation]

EKR and I did a pass through the IESG comments, and found a couple minor things that were missed.

[ekr]

I think you would want to say that you can't register the same key twice.

[cpu]

Why SHOULD NOT instead of MUST NOT for reusing an in-use account public key in a finalize CSR?

[bifurcation]

I would be OK changing this to MUST NOT.

[cpu]

Why SHOULD instead of MUST for rejecting a new-account request with a public key already associated with an account on the server?

[felixfontein]

I thought a new-account request with a public key already associated with an account is the way to retrieve the account URI, given the public key? I think neither SHOULD nor MUST is a good idea because of that.

[felixfontein]

I guess the sentence should be "ACME servers MUST NOT create a new account using an account key already associated with an account on the server.", without mentioning the new-account endpoint.

[cpu]

I think adding "MUST NOT allow account key roll-over to a previously-used account key" is overly prescriptive and under specified for this late in the game.

Why isn't this restriction also mentioned in the key rollover section of the draft? The list of steps the server needs to take in https://tools.ietf.org/html/draft-ietf-acme-acme-16#section-7.3.5 mentions "Check that no account exists whose account key is the same as the key in the "jwk" header parameter of the inner JWS." but nothing about also verifying that the new key has never been used by the account previously.

What threat is this addressing that justifies the new complexity? Are we sure the security win is proportional? What error is returned if a client tries to roll-over to a key that has been used by the account in the past? How much key history does the server need to keep? What happens if an ACME account decides to roll-over to a new randomly generated keypair every minute? What should servers do to avoid that being abused?

[jsha]

Note that the "MUST NOT allow account key roll-over to a previously-used account key" is a requirement on ACME clients rather than ACME servers. It's not clear why a client would bother to implement such a check, since it requires keeping around old keys, and so far client implementers have shown a desire to minimize the amount of state they need to retain.

So I'm against adding this, on the principle that we shouldn't add MUSTs that won't actually get implemented (and don't break anything if they're not implemented).

[cpu]

Note that the "MUST NOT allow account key roll-over to a previously-used account key" is a requirement on ACME clients rather

Ahhhh! I apologize. I totally missed that part.

I agree with you that it seems like it isn't likely to get implemented. I'm also dubious of the value if the server doesn't enforce this constraint as well, and as mentioned I think the server enforcing it has its own set of complications.

[ekr]

I read "MUST NOT allow" as a clunky way of saying "MUST NOT do" and given that the client probably generates the account key itself, this seems quite easy to enforce.

WRT the value if the server doesn't enforce it, I'm not following why that would be the case. We, for instance, often want clients to generate fresh DH shares, even though the server usually does not check that they have done so.

[felixfontein]

There are clients (such as the one I'm working on) which simply use whatever the user provides, and which don't have any state (besides what the user provides, i.e. path to account private key, and maybe the account URI). There's no way to implement this since key generation is not done by them, and the user could simply switch back and forth between two different keys -- the client wouldn't notice. I guess there are some more clients which work similarly.

[jsha]

If that's the intent, the clause should just be written as "the subscriber MUST NOT xxx". But I think that's not likely to be effective; most subscribers aren't reading the RFC, just using an off-the-shelf client.

Maybe this belongs more in the security considerations section, describing what will happen if a user does rotate to an old key for some reason?

[ekr]

I don't think that that's right. You usually have to read IETF specs with the mindset that everything on one side of the connection is a monolithic unit, and this is no exception

[jsha]

So you'd be satisfied with a "subscriber MUST" here in place of the "client MUST"?

[bifurcation]

I think the desired state here is that if the client or server has the state to determine that key reuse has occurred, then it must check and reject reuse. (Arguably, you can never use the same account key for multiple accounts if you only deal with one account at a time.) Would you be more comfortable with these requirements if they had that caveat? Say:

Clients or servers that maintain information about multiple accounts or historical account keys MUST NOT allow the same key to be used for multiple accounts, and MUST NOT allow key roll-over to a previously-used key.

[cpu]

I think the desired state here is that if the client or server has the state to determine that key reuse has occurred, then it must check and reject reuse.

Do any clients meeting this criteria exist? Does the benefit of addressing this niche justify last-minute specification changes? I feel like its no in both cases.

If we absolutely have to land this text to make forward progress then I'm not strongly opposed but it doesn't seem practically useful to me.

[jsha]

Reviewing the conversation on this, I think this SHOULD is too strong and should be a MAY. For example, Let's Encrypt scopes nonces by client IP, for ease of implementation. One could easily imagine another implementation that chooses to scope by account.

[bifurcation]

There's an interop concern here, in that clients need to know how they can use nonces. I would probably make the slightly stronger argument that we should forbid servers from doing things that would break the obvious ways for clients to handle nonces (i.e., in one pool per client).

For example, a server could have a scheme where (1) a newNonce-issued nonce is good for talking to any resource, but (2) nonces issued from other resources can only be used with resources of the same class. That would cause a lot of fail/newNonce/retry loops for a client doing the obvious thing.

TBH, incorporating client IP seems kind of on the edge, as it's not crazy to imagine that a client could change addresses during the course of an ACME issuance flow.

How about something like this:

Servers SHOULD NOT scope nonces more finely than per-account. In particular, servers MUST ensure that a nonce issued to a client by the newNonce resource can be used by the same client to perform any ACME request (within a reasonable period of time).

The latter part ensures that the fail/newNonce/retry loop works.

[cpu]

There's an interop concern here, in that clients need to know how they can use nonces.

I don't think that's true. Clients that send a nonce to a server from the wrong scope will get back a nonce rejection error that itself has a new nonce to use that the server will accept subsequently. The spec already says that a client SHOULD retry in this case. Some imagined (e.g. not implemented, not discussed on list by anyone) server nonce implementations like per-resource scoping might provoke more retries from clients but that seems like a fair trade-off for a hypothetical design choice and not a concern that justifies changing the draft text.

How about something like this:

Servers SHOULD NOT scope nonces more finely than per-account.

I don't think the text should have a "SHOULD NOT" added that directly contradicts all of the existing server implementations I'm aware of, and certainly the largest production ACME implementation and its ecosystem of compatible clients.

At this stage of the process I really think that every change should have an extremely strong justification. This change contradicts all implementation experience we have and the rationale for it (e.g. supporting servers implementing per-resource nonce schemes) does not seem strong enough to meet the bar for merging.

[jsha]

Note that the "MUST NOT allow account key roll-over to a previously-used account key" is a requirement on ACME clients rather than ACME servers. It's not clear why a client would bother to implement such a check, since it requires keeping around old keys, and so far client implementers have shown a desire to minimize the amount of state they need to retain.

So I'm against adding this, on the principle that we shouldn't add MUSTs that won't actually get implemented (and don't break anything if they're not implemented).

[ekr]

No, I don't think that follows. What I'm saying is that the way you say that is "the client".

…

On Mon, Nov 12, 2018 at 10:44 AM Jacob Hoffman-Andrews < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In draft-ietf-acme-acme.md <#467 (comment)>: > @@ -3380,8 +3384,13 @@ account holder could take within the scope of ACME: For this reason, it is RECOMMENDED that account key pairs be used for no other purpose besides ACME authentication. For example, the public key of an account -key pair SHOULD NOT be included in a certificate. ACME clients and servers -SHOULD verify that a CSR submitted in a finalize request does not contain a +key pair MUST NOT be included in a certificate. ACME clients MUST NOT reuse +the same account key for multiple accounts, and MUST NOT allow account key +roll-over to a previously-used account key. ACME servers MUST NOT create a new So you'd be satisfied with a "subscriber MUST" here in place of the "client MUST"? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#467 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABD1oQoFhGpKZY5a1SQvg4DUziZfREVwks5uub3TgaJpZM4YMoTs> .

[ekr]

I'm not really seeing the problem with "clients MUST NOT reuse". In this context, "client" refers to everything on that endpoint.

…

On Tue, Nov 27, 2018 at 7:27 AM Richard Barnes ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In draft-ietf-acme-acme.md <#467 (comment)>: > @@ -3380,8 +3384,13 @@ account holder could take within the scope of ACME: For this reason, it is RECOMMENDED that account key pairs be used for no other purpose besides ACME authentication. For example, the public key of an account -key pair SHOULD NOT be included in a certificate. ACME clients and servers -SHOULD verify that a CSR submitted in a finalize request does not contain a +key pair MUST NOT be included in a certificate. ACME clients MUST NOT reuse +the same account key for multiple accounts, and MUST NOT allow account key +roll-over to a previously-used account key. ACME servers MUST NOT create a new I think the desired state here is that *if* the client or server has the state to determine that key reuse has occurred, then it must check and reject reuse. (Arguably, you can never use the same account key for multiple accounts if you only deal with one account at a time.) Would you be more comfortable with these requirements if they had that caveat? Say: Clients or servers that maintain information about multiple accounts or historical account keys MUST NOT allow the same key to be used for multiple accounts, and MUST NOT allow key roll-over to a previously-used key. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#467 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABD1oUC7N20R4w9q0ksRMUsA6wufmj33ks5uzVnQgaJpZM4YMoTs> .

[jsha]

@ekr what do you think of "clients MUST NOT intentionally reuse keys"? The addition of "intentional" would hopefully make it clear that client software is not required to keep a copy of all past keys around to ensure it can never possibly reuse them.

[ekr]

We don't usually treat software as having intent. I don't see that it's a problem to forbid the endpoint from doing it. That simply isn't a levy on some specific piece of client software to keep a list; it's a levy on the system of the client software and whatever is managing the keys.

Happy to have a call to discuss this.

[jsha]

In the interest of making progress I'm going to drop my objection to the "clients MUST NOT reuse" clause. I still object to "servers SHOULD scope nonces globally." I think we should just drop that clause - it's a very significant change to add in post-post-post-WGLC, for something that hasn't been an issue in practice.

[bifurcation]

@cpu @jsha @ekr I've updated the text around nonce scoping and key reuse in a way that I hope makes folks more comfortable. For nonce reuse, the text is focused on the interop problem, i.e., making the "get a fresh nonce and retry" cycle work; otherwise, nonce scope is left unspecified. For key reuse, I tried to be clearer about who is responsible for doing what under what circumstances.

The latest push also mostly fixes the XSS issue noted in #470. There's still some residual risk that a keyAuthorization will look like something else, but at least now the client's response is required to be a keyAuthorization.

[cpu]

The updated text looks good to me, just two newline nits to request fixes for

[cpu]

I think a spurious newline slipped in here

[cpu]

ditto re: newline here

[jsha]

Excellent. Thanks for the revisions, @bifurcation !

[cpu]

Thanks @bifurcation 👍

[felixfontein]

Thanks everyone!

[ekr]

I'm not really happy with this text. The requirement ought to be that you are forbidden from reusing, period. Happy to have a call to discuss, but as-is, I'm not OK with this.

[shred]

I hope I'm not too late for discussion... But what is "and possibly in other requests" supposed to mean? Does it mean that a client MUST NOT or SHOULD NOT use the Replay-Nonce from the badNonce error for other requests?

I'm not happy with this idea. Until now, nonces were not bound to a certain type of request. A client could just remember the nonce it received from the last request, and use it for the next request. If I take this change literally, a client would now have to store multiple nonces, and find the one matching for a certain request. I see no advantage or increase of security in that.

Or did I just misunderstand it?

[bifurcation]

You've misunderstood a little, but in a totally understandable way :) Right now, ACME doesn't say anything about how nonces are scoped -- in principle, the server could scope them per-client, per-request-type, etc. There are valid reasons for servers to do some of these things, mainly related to managing server state.

My intent with this text was to specify the minimum necessary scope constraint for a client and server to interoperate. Without this requirement, the worst case is that the server has some crazy nonce management strategy and client can never send a valid request. The requirement for the nonce to be valid on retry assures that the worst case is that the client has to do every request twice. Obviously, I hope that most servers will use more sensible strategies that don't cause this condition!

I agree that the current text is awkward, though. Would it be helpful to replace that parenthetical with something like the following?

Other than this constraint, ACME does not constrain how servers scope nonces. Clients are MAY assume that nonces have broad scope, e.g., by having a single pool of nonces used for all requests. However, when retrying in response to a "badNonce" error, the client MUST use the nonce provided in the error response. Obviously, servers should scope nonces broadly enough that retries are not needed very often.

[shred]

Thank you for the explanation. The new text explains it much better, IMHO.

It also makes clear what I was afraid of. The nonce that came with the error response MUST be used for the retry. In my client implementation, if there is an unrelated request sent between the error response and the reattempting request, the unrelated request would use the nonce given by the error response, and the reattempt would use the nonce that came from the response of the unrelated request.

I agree that this is rather hypothetical, and could only happen if multiple threads are using the client for communication, but it's still possible. On the positive side, the new text clarifies that now. 😄

[ekr]

I think Richard's proposed text here is good and addresses the review comment. @richsalz, are you good with this? I think it's the last thing.

[richsalz]

Been following the thread, and I agree this is good now. Thanks!

[ekr]

@bifurcation: anyone else you need to clear this with or shall we issue the next I-D and send it to the RFC Ed

…

On Mon, Dec 17, 2018 at 9:09 AM Rich Salz ***@***.***> wrote: ***@***.**** approved this pull request. Been following the thread, and I agree this is good now. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#467 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABD1oXD9lndx1wATQ4-aBmS_GVPVL3fmks5u58_ngaJpZM4YMoTs> .

[cpu]

anyone else you need to clear this with or shall we issue the
next I-D and send it to the RFC Ed

@ekr at the minimum someone needs to update the document with @bifurcation's proposed text first. I think @jsha should also have a chance to review the changes made over the weekend.

[cpu]

Consensus seems to be that there needs to be a clarifying update to the nonce text. This shouldn't be merged until that update is in-document.

[cpu]

One nit, but nothing I'm willing to block the PR on.

Thanks @bifurcation

[cpu]

nit: I don't think "Obviously," is warranted. Just say "Servers should scope nonces broadly enough ..."

[bifurcation]

Merging despite CI failure because the CI failure appears to be tooling-related. I have verified locally that the document builds correctly.