Repository: certbot/certbot
Pull request: Respect Retry-After header when polling for order finalization
Fixes #10273.

Problem statement:
[Bug]: Respect Retry-After header when polling for order finalization (if given)
```
If a request to finalize an order is successful, the server will
   return a 200 (OK) with an updated order object.  The status of the
   order will indicate what action the client should take:

   o  "invalid": The certificate will not be issued.  Consider this
      order process abandoned.

   o  "pending": The server does not believe that the client has
      fulfilled the requirements.  Check the "authorizations" array for
      entries that are still pending.

   o  "ready": The server agrees that the requirements have been
      fulfilled, and is awaiting finalization.  Submit a finalization
      request.

   o  "processing": The certificate is being issued.  Send a POST-as-GET
      request after the time given in the Retry-After header field of
      the response, if any.

   o  "valid": The server has issued the certificate and provisioned its
      URL to the "certificate" field of the order.  Download the
      certificate.
```

We do not check for `processing` and respect Retry-After, if given. We should.

Hints from issue discussion:

Commit message: update changelog

Patch to review:
diff --git a/acme/src/acme/_internal/tests/client_test.py b/acme/src/acme/_internal/tests/client_test.py
index fb900e38c79..8380f090fa1 100644
--- a/acme/src/acme/_internal/tests/client_test.py
+++ b/acme/src/acme/_internal/tests/client_test.py
@@ -281,7 +281,9 @@ def test_finalize_order_invalid_status(self):
         with pytest.raises(errors.Error, match="The certificate order failed"):
             self.client.finalize_order(self.orderr, datetime.datetime(9999, 9, 9))
 
-    def test_finalize_order_orderNotReady(self):
+    @mock.patch('acme.client.time.sleep')
+    @mock.patch('acme.client.datetime')
+    def test_finalize_order_orderNotReady(self, dt_mock, mock_sleep):
         # https://github.com/certbot/certbot/issues/9766
         updated_order_processing = self.order.update(status=messages.STATUS_PROCESSING)
         updated_order_ready = self.order.update(status=messages.STATUS_READY)
@@ -297,11 +299,18 @@ def test_finalize_order_orderNotReady(self):
                                           updated_order_ready.to_json(),
                                           updated_order_valid.to_json()]
 
+        dt_mock.datetime.now.return_value = datetime.datetime(2015, 3, 27)
+        dt_mock.timedelta = datetime.timedelta
+        self.response.headers['Retry-After'] = '50'
+
         post = mock.MagicMock()
         post.side_effect = [messages.Error.with_code('orderNotReady'), # first begin_finalization
-                            self.response, # first poll_finalization poll --> still returns pending
+                            # sleep 1
+                            self.response, # first poll_finalization poll --> returns processing
+                            # retry-after sleep here
                             self.response, # second poll_finalization poll --> returns ready
                             mock.MagicMock(), # second begin_finalization
+                            # sleep 1
                             self.response, # third poll_finalization poll --> returns valid
                             self.response # fetch cert
                             ]
@@ -309,6 +318,7 @@ def test_finalize_order_orderNotReady(self):
 
         self.client.finalize_order(self.orderr, datetime.datetime(9999, 9, 9))
         assert self.net.post.call_count == 6
+        assert mock_sleep.call_args_list == [((1,),), ((50,),), ((1,),)]
 
     def test_finalize_order_otherErrorCode(self):
         post = mock.MagicMock()
diff --git a/acme/src/acme/client.py b/acme/src/acme/client.py
index 02ed8ad0bff..e92df7888b7 100644
--- a/acme/src/acme/client.py
+++ b/acme/src/acme/client.py
@@ -251,9 +251,10 @@ def poll_finalization(self, orderr: messages.OrderResource,
         :returns: finalized order (with certificate)
         :rtype: messages.OrderResource
         """
-
+        sleep_seconds: float = 1
         while datetime.datetime.now() < deadline:
-            time.sleep(1)
+            if sleep_seconds > 0:
+                time.sleep(sleep_seconds)
             response = self._post_as_get(orderr.uri)
             body = messages.Order.from_json(response.json())
             if body.status == messages.STATUS_INVALID:
@@ -269,6 +270,7 @@ def poll_finalization(self, orderr: messages.OrderResource,
                 # fulfilled, and is awaiting finalization.  Submit a finalization
                 # request.
                 self.begin_finalization(orderr)
+                sleep_seconds = 1
             elif body.status == messages.STATUS_VALID and body.certificate is not None:
                 # "valid": The server has issued the certificate and provisioned its
                 # URL to the "certificate" field of the order.  Download the
@@ -280,6 +282,15 @@ def poll_finalization(self, orderr: messages.OrderResource,
                     alt_chains = [self._post_as_get(url).text for url in alt_chains_urls]
                     orderr = orderr.update(alternative_fullchains_pem=alt_chains)
                 return orderr
+            else:
+                assert body.status == messages.STATUS_PROCESSING
+                # the only other option is messages.STATUS_PENDING, which we should never hit here
+                # because we've already checked for it earlier
+                retry_after = self.retry_after(response, 1)
+                # Whatever Retry-After the ACME server requests, the polling must not take
+                # longer than the overall deadline
+                retry_after = min(retry_after, deadline)
+                sleep_seconds = (retry_after - datetime.datetime.now()).total_seconds()
         raise errors.TimeoutError()
 
     def finalize_order(self, orderr: messages.OrderResource, deadline: datetime.datetime,
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 011eb84703f..a330d75b74e 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -27,7 +27,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
   `ready`, and resubmits finalization request before polling for `valid` to download
   certificate. This conforms to RFC 8555 more accurately and avoids race conditions where
   all authorizations are fulfilled but order has not yet transitioned to ready state on
-  the server when the finalization request is sent.
+  the server when the finalization request is sent. It also respects retry-after when
+  polling for finalization readiness.
 * The --preferred-profile and --required-profile flags now have their values stored in
   the renewal configuration so the same setting will be used on renewal.
 


Code review comments:
diff_hunk: @@ -280,6 +282,15 @@ def poll_finalization(self, orderr: messages.OrderResource,
                     alt_chains = [self._post_as_get(url).text for url in alt_chains_urls]
                     orderr = orderr.update(alternative_fullchains_pem=alt_chains)
                 return orderr
+            else:
+                assert body.status == messages.STATUS_PROCESSING
+                # the only other option is messages.STATUS_PENDING, which we should never hit here
+                # because we've already checked for it earlier
path: acme/src/acme/client.py
text: @user1:
how do we know the status cannot be pending here?

there's a good chance i'm just missing something, but the only other reference to pending i see is [here](https://github.com/certbot/certbot/blob/723fe64d4dbc778f1e8fd0b93320c1a30dc6fa95/acme/src/acme/client.py#L202) which i believe is about authorizations. can we be confident that orders atomically move out of the pending state when all authorizations are valid? even so, if this is the check you're referring to, i (maybe unnecessarily) worry that things could break in the future as it relies on behavior across functions and the order they were called

whatcha think?

@author:
No, you're right, this could happen non-atomically. I was conflating _authorization_ pending checks with _order_ pending checks. And on that, the spec says:

> Order objects are created in the "pending" state.  Once all of the
>  authorizations listed in the order object are in the "valid" state,
>  the order transitions to the "ready" state.  

Which should be quick, and we do wait a second, but technically isn't required to be atomic. I'll get rid of the assert; the code should otherwise work as-is. Or I could check the status before checking for a retry-after, which I think should only be there when it's in processing, but the code should behave the same either way.