Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update alreadyReplaced to replaces or similar #74

Closed
gregtwallace opened this issue Aug 31, 2024 · 3 comments
Closed

Update alreadyReplaced to replaces or similar #74

gregtwallace opened this issue Aug 31, 2024 · 3 comments

Comments

@gregtwallace
Copy link

Continuing the discussion from: #56

Tl'dr: If a client doesn't understand why the order is malformed, it would have to send a guess after removing the replaces field to see if that was the problem.


Other issues with the replaces field are currently ambiguous. Instead of defining alreadyReplaced define a single error such as replaces or replacesField. If the server returns 409 with this error, it is clear the certificate was already replaced. If the server returns any other error code with this error, it is clear the replaces field is the problem, but the certificate has not already been replaced.

This is helpful for telling clients that they MAY (or maybe even SHOULD) drop the replaces field and try again. If this doesn't exist and a client receives a generic error (e.g., malformed) the client would need to send the order again without replaces to find out if that was the malformed field or if there was a different issue.

This in turn will likely lead clients to perform the order again without the replaces field regardless of error, which will unncessarily double the new order request if the error was caused by something other than the replaces field.

@aarongable
Copy link
Owner

Sorry, I don't understand what you're proposing. It sounds like you want to rename urn:ietf:params:acme:error:alreadyReplaced to urn:ietf:params:acme:error:replacesField (which doesn't sound like an error to me) and then have the meaning of that error be context-dependent depending on what HTTP error code accompanies the problem document? I don't understand the benefit this would bring and it would be a significant departure from how other ACME errors are used both in the specification and in practice.

@gregtwallace
Copy link
Author

gregtwallace commented Sep 3, 2024

I'm not wed to any particular solution. I'm just trying to raise a potential issue for clients.

The scenario is there is an error with the replaces field on a submitted order that is NOT an already replaced error. For example, the server rejects the replaces field due to the list of identifiers not being a perfect match. This rejection should be distinguishable via a defined error vs. using a generic error like malformed. The client will see this defined error and know to remove the replaces field and resubmit the order. If the error is generic, the client would have to "guess" what the problem is, perhaps by removing the replaces field and retrying. In the event the generic error meant a problem other than replaces this is a pointless resubmission as it will just error again.

Perhaps an error badReplaces or malformedReplaces might be a better name. I'm not familiar with the URN formatting, but maybe even something like urn:ietf:params:acme:error:malformed:replaces.

@aarongable
Copy link
Owner

Ah thank you, now I understand. I guess my question is:

This rejection should be distinguishable via a defined error vs. using a generic error like malformed.

Why? The rest of the ACME protocol does not make such distinctions -- today there are a dozen different reasons why a newOrder request might be rejected with reason malformed, and none of them are distinguishable. I don't believe that the replaces field should be special in that regard. And if the ACME Server does want to provide more detail, the problem document format already has a mechanism for that: subproblems. For example, a server could provide one subproblem for each field of the request which has an issue.

The reason we have a new alreadyReplaced error type is because it is describing a truly new condition: the request was fully valid, but the client and server state appear to be out of sync (the client believes the prior cert still needs to be replaced, while the server believes it already has been). I don't believe there's a need for other kinds of errors to reflect more generic issues that could arise with this field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants