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

V2 order ready status not recognized, causes deserialization error #5856

Closed
cpu opened this issue Apr 12, 2018 · 2 comments
Closed

V2 order ready status not recognized, causes deserialization error #5856

cpu opened this issue Apr 12, 2018 · 2 comments
Assignees
Milestone

Comments

@cpu
Copy link
Contributor

cpu commented Apr 12, 2018

I installed Certbot with (certbot-auto, OS package manager, pip, etc):

Cloned from git:

$> git rev-parse HEAD
6b29d159a2f221c3437770bdb43924ee6f953c4b

I ran this command and it produced this output:

certbot_test --server http://localhost:4001/directory certonly --standalone -d one.wtf --preferred-challenges http-01

Note: This is against a Boulder instance configured with the OrderReadyStatus feature flag enabled (See letsencrypt/boulder#3644).

Certbot's behavior differed from what I expected because:

Certbot POSTed newOrder. In response an order object with "status": "ready" was returned. This caused a DeserializationError indicating "Could not decode 'status' (u'ready'): Deserialization error: Status not recognized".

The "ready" status was added to the ACME specification in draft-10 before Let's Encrypt launched its production ACMEv2 endpoint. Boulder does not use this new status in staging/production yet but we will in the near future (~next month).

Draft-10 says:

Once all of the authorizations listed in the order object are in the "valid" state, the order transitions to the "ready" state.

This state is used to indicate that an order is ready for finalization. Previously the order would remain in "processing" when all of its authorizations were in the "valid" state.

Here is a Certbot log showing the issue (if available):

http://localhost:4001 "POST /acme/new-order HTTP/1.1" 201 323
Received response:
HTTP 201
Boulder-Requester: 2141
Cache-Control: public, max-age=0, no-cache
Content-Type: application/json
Location: http://localhost:4001/acme/order/2141/932
Replay-Nonce: Aeop9czyFGXSMBH0TfD4MwI5klCloEnml8AFsRzBPDU
Date: Thu, 12 Apr 2018 17:06:51 GMT
Content-Length: 323

{
  "status": "ready",
  "expires": "2018-04-19T17:06:51.98458014Z",
  "identifiers": [
    {
      "type": "dns",
      "value": "one.wtf"
    }
  ],
  "authorizations": [
    "http://localhost:4001/acme/authz/qklYRnxxHtf8PAaR8IpgK2ex7uPqWYzWgPEQrPiqEKc"
  ],
  "finalize": "http://localhost:4001/acme/finalize/2141/932"
}
Storing nonce: Aeop9czyFGXSMBH0TfD4MwI5klCloEnml8AFsRzBPDU
Exiting abnormally:
Traceback (most recent call last):
  File "/home/daniel/Code/certbot/venv/bin/certbot", line 11, in <module>
    load_entry_point('certbot', 'console_scripts', 'certbot')()
  File "/home/daniel/Code/certbot/certbot/main.py", line 1266, in main
    return config.func(config, plugins)
  File "/home/daniel/Code/certbot/certbot/main.py", line 1157, in certonly
    lineage = _get_and_save_cert(le_client, config, domains, certname, lineage)
  File "/home/daniel/Code/certbot/certbot/main.py", line 113, in _get_and_save_cert
    renewal.renew_cert(config, domains, le_client, lineage)
  File "/home/daniel/Code/certbot/certbot/renewal.py", line 297, in renew_cert
    new_cert, new_chain, new_key, _ = le_client.obtain_certificate(domains)
  File "/home/daniel/Code/certbot/certbot/client.py", line 294, in obtain_certificate
    orderr = self._get_order_and_authorizations(csr.data, self.config.allow_subset_of_names)
  File "/home/daniel/Code/certbot/certbot/client.py", line 326, in _get_order_and_authorizations
    orderr = self.acme.new_order(csr_pem)
  File "/home/daniel/Code/certbot/acme/acme/client.py", line 779, in new_order
    return self.client.new_order(csr_pem)
  File "/home/daniel/Code/certbot/acme/acme/client.py", line 606, in new_order
    body = messages.Order.from_json(response.json())
  File "/home/daniel/Code/certbot/venv/local/lib/python2.7/site-packages/josepy/json_util.py", line 289, in from_json
    return cls(**cls.fields_from_json(jobj))
  File "/home/daniel/Code/certbot/venv/local/lib/python2.7/site-packages/josepy/json_util.py", line 284, in fields_from_json
    slot, value, error))
DeserializationError: Deserialization error: Could not decode 'status' (u'ready'): Deserialization error: Status not recognized
Please see the logfiles in /tmp/leitSN33/logs for more details.
cpu pushed a commit to letsencrypt/boulder that referenced this issue Apr 12, 2018
In #3614 we adjusted the `SA.NewOrder` function to conditionally call
`ssa.statusForOrder` on the new order when `features.OrderReadyStatus`
was enabled. Unfortunately this call to `ssa.statusForOrder` happened
*before* the `req.BeganProcessing` field was initialized with a pointer
to a `false` bool. The `ssa.statusForOrder` function (correctly) assumes
that `req.BeganProcessing == nil` is illegal and doesn't correspond to
a known status. This results in NewOrder requests returning a 500 error
of the form:
> Internal error - Error creating new order - Order XXX is in an invalid
state. No state known for this order's authorizations

Our integration tests missed this because we didn't have a test case
that issued for a set of names with one account, and then issued again
for the same set of names with the same account.

This commit fixes the original bug by moving the `BeganProcessing`
initialization before the call to `statusForOrder`. This commit also
adds an integration test to catch this sort of bug again in the future.
Prior to the SA fix this test failed with the 500 server internal error
observed by the Certbot team. With the SA fix in place the test passes
again.

Finally, this commit disables the `OrderReadyStatus` feature flag in
`test/config-next/sa.json`. Certbot's ACME implementation breaks when
this flag is enabled (See
certbot/certbot#5856). Since Certbot runs
integration tests against Boulder with config-next we should be
courteous and leave this flag disabled until we are closer to being able
to turn it on for staging/prod.
cpu pushed a commit to letsencrypt/boulder that referenced this issue Apr 12, 2018
This commit disables the `OrderReadyStatus` feature flag in
`test/config-next/sa.json`. Certbot's ACME implementation breaks when
this flag is enabled (See
certbot/certbot#5856). Since Certbot runs
integration tests against Boulder with config-next we should be
courteous and leave this flag disabled until we are closer to being able
to turn it on for staging/prod.
@cpu cpu added the area: acme label Apr 12, 2018
jsha added a commit to letsencrypt/boulder that referenced this issue Apr 12, 2018
In #3614 we adjusted the `SA.NewOrder` function to conditionally call `ssa.statusForOrder` on the new order when `features.OrderReadyStatus` was enabled. Unfortunately this call to `ssa.statusForOrder` happened *before* the `req.BeganProcessing` field was initialized with a pointer to a `false` bool. The `ssa.statusForOrder` function (correctly) assumes that `req.BeganProcessing == nil` is illegal and doesn't correspond to a known status. This results in `NewOrder` requests returning a 500 error
of the form: 
> Internal error - Error creating new order - Order XXX is in an invalid state. No state known for this order's authorizations

Our integration tests missed this because we didn't have a test case that issued for a set of names with one account, and then issued again for the same set of names with the same account.

This PR fixes the original bug by moving the `BeganProcessing` initialization before the call to `statusForOrder`. This PR also adds an integration test to catch this sort of bug again in the future.
Prior to the SA fix this test failed with the 500 server internal error observed by the Certbot team. With the SA fix in place the test passes again.

Finally, this PR disables the `OrderReadyStatus` feature flag in `test/config-next/sa.json`. Certbot's ACME implementation breaks when this flag is enabled (See certbot/certbot#5856). Since Certbot runs integration tests against Boulder with config-next we should be courteous and leave this flag disabled until we are closer to being able to turn it on for staging/prod.
@ohemorange
Copy link
Contributor

Hey just saw this! One of the triage methods we use is to scan for things without a label (since outside contributors can't add them), because that means no one on Certbot has taken a look yet. This will get in a sprint ASAP.

@cpu
Copy link
Contributor Author

cpu commented May 2, 2018

@ohemorange Oops! I won't apply labels myself in the future. I didn't realize that was part of your triage workflow.

Thanks for catching this & 5857

@bmw bmw added this to the 0.25.0 milestone May 2, 2018
sauloperez added a commit to coopdevs/certbot_nginx that referenced this issue Nov 21, 2018
This fixes the error

```
josepy.errors.DeserializationError: Deserialization error: Could not
decode 'status' ('ready'): Deserialization error: Status not recognized
2018-11-21 12:36:10,048:ERROR:certbot.log:An unexpected error occurred:
```

when generating a certificate.

By checking certbot/certbot#5856 we managed to
trace back to the actual version of certbot that fixes this; 0.26.1. We
also found that the `letsencrypt` package does not contain all
dependencies (python3-certbot) and `certbot` should be used instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants