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.

Sign up for GitHub

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

Jump to bottom

wrong implementation of ACME spec: Orders should transition to "invalid", not "deactivated" #300

Open
jvanasco opened this issue Feb 22, 2020 · 2 comments · May be fixed by #301
Open

wrong implementation of ACME spec: Orders should transition to "invalid", not "deactivated" #300

jvanasco opened this issue Feb 22, 2020 · 2 comments · May be fixed by #301

Comments

@jvanasco
Copy link
Contributor

I spent the day investigating failing tests, because of a single line in Pebble :(

https://github.com/letsencrypt/pebble/blob/master/core/types.go#L66-L69

// An order is deactivated if **any** of its authzs are deactivated
if authzStatuses[acme.StatusDeactivated] > 0 {
	return acme.StatusDeactivated, nil
}

This is not a correct implementation of the ACME spec. The Order object does not have a "deactivated" state. The spec states it should transition to "invalid".

7.1.3 Order Objects

status (required, string): The status of this order. Possible
values are "pending", "ready", "processing", "valid", and
"invalid". See Section 7.1.6.

7.1.6. Status Changes
Page 32: text following "State Transitions for Authorization Objects"

The order also moves to the "invalid"
state if it expires or one of its authorizations enters a final state
other than "valid" ("expired", "revoked", or "deactivated")

Page 33: chart "State Transitions for Order Objects"

    pending --------------+
       |                  |
       | All authz        |
       | "valid"          |
       V                  |
     ready ---------------+
       |                  |
       | Receive          |
       | finalize         |
       | request          |
       V                  |
   processing ------------+
       |                  |
       | Certificate      | Error or
       | issued           | Authorization failure
       V                  V
     valid             invalid
@jvanasco jvanasco changed the title wrong implementation of ACME spec wrong implementation of ACME spec: Orders should transition to "invalid", not "deactivated" Feb 22, 2020
@jsha jsha linked a pull request Feb 24, 2020 that will close this issue
Open
@jsha
Copy link
Contributor

jsha commented Feb 24, 2020

Good catch. Thanks for reporting it, and my apologies that you spent so much time trying to track it down. I've uploaded a fix at #301. Want to try that out and let me know if it fixes the issue for you?

@jvanasco
Copy link
Contributor Author

Thanks, @jsha looks much better than the patch i put together. i'll run it through my test suite tomorrow or the day after.

I'm actually glad this happened - my code should have been written to catch potentially malformed/incorrect responses.

@jsha jsha mentioned this issue Jun 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Orders don't have a "deactivated" status. letsencrypt/pebble
2 participants
@jvanasco @jsha