Catch and ignore orderNotReady response when posting a request to begin finalization and poll until ready (#10239)

Fixes #9766.

acme/src/acme/_internal/tests/client_test.py

@@ -254,6 +254,26 @@ class ClientV2Test(unittest.TestCase):
         with pytest.raises(errors.IssuanceError):
             self.client.finalize_order(self.orderr, deadline)
 
+    @mock.patch('acme.client.ClientV2.begin_finalization')
+    def test_finalize_order_ready(self, mock_begin):
+        # https://github.com/certbot/certbot/issues/9766
+        updated_order_ready = self.order.update(status=messages.STATUS_READY)
+
+        updated_order_valid = self.order.update(
+            certificate='https://www.letsencrypt-demo.org/acme/cert/',
+            status=messages.STATUS_VALID)
+        updated_orderr = self.orderr.update(body=updated_order_valid, fullchain_pem=CERT_SAN_PEM)
+
+        self.response.text = CERT_SAN_PEM
+
+        self.response.json.side_effect = [updated_order_ready.to_json(),
+                                           updated_order_valid.to_json()]
+
+        deadline = datetime.datetime(9999, 9, 9)
+        assert self.client.finalize_order(self.orderr, deadline) == updated_orderr
+        assert self.response.json.call_count == 2
+        assert mock_begin.call_count == 2
+
     def test_finalize_order_invalid_status(self):
         # https://github.com/certbot/certbot/issues/9296
         order = self.order.update(error=None, status=messages.STATUS_INVALID)
@@ -261,6 +281,43 @@ class ClientV2Test(unittest.TestCase):
         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):
+        # 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)
+
+        updated_order_valid = self.order.update(
+            certificate='https://www.letsencrypt-demo.org/acme/cert/',
+            status=messages.STATUS_VALID)
+        updated_orderr = self.orderr.update(body=updated_order_valid, fullchain_pem=CERT_SAN_PEM)
+
+        self.response.text = CERT_SAN_PEM
+
+        self.response.json.side_effect = [updated_order_processing.to_json(),
+                                          updated_order_ready.to_json(),
+                                          updated_order_valid.to_json()]
+
+        post = mock.MagicMock()
+        post.side_effect = [messages.Error.with_code('orderNotReady'), # first begin_finalization
+                            self.response, # first poll_finalization poll --> still returns pending
+                            self.response, # second poll_finalization poll --> returns ready
+                            mock.MagicMock(), # second begin_finalization
+                            self.response, # third poll_finalization poll --> returns valid
+                            self.response # fetch cert
+                            ]
+        self.net.post = post
+
+        self.client.finalize_order(self.orderr, datetime.datetime(9999, 9, 9))
+        assert self.net.post.call_count == 6
+
+    def test_finalize_order_otherErrorCode(self):
+        post = mock.MagicMock()
+        post.side_effect = [messages.Error.with_code('serverInternal')]
+        self.net.post = post
+
+        with pytest.raises(messages.Error):
+            self.client.finalize_order(self.orderr, datetime.datetime(9999, 9, 9))
+
     def test_finalize_order_timeout(self):
         deadline = datetime.datetime.now() - datetime.timedelta(seconds=60)
         with pytest.raises(errors.TimeoutError):
@@ -342,7 +399,7 @@ class ClientV2Test(unittest.TestCase):
         with mock.patch('acme.client.ClientV2._authzr_from_response') as mock_client:
             mock_client.return_value = self.authzr2
 
-            self.client.poll(self.authzr2)  # pylint: disable=protected-access
+            self.client.poll(self.authzr2)
 
             self.client.net.post.assert_called_once_with(
                 self.authzr2.uri, None,
@@ -716,7 +773,6 @@ class ClientNetworkWithMockedResponseTest(unittest.TestCase):
         assert self.response.checked
 
     def test_post(self):
-        # pylint: disable=protected-access
         assert self.response == self.net.post(
             'uri', self.obj, content_type=self.content_type)
         assert self.response.checked
@@ -745,7 +801,6 @@ class ClientNetworkWithMockedResponseTest(unittest.TestCase):
         check_response = mock.MagicMock()
         check_response.side_effect = messages.Error.with_code('badNonce')
 
-        # pylint: disable=protected-access
         self.net._check_response = check_response
         with pytest.raises(messages.Error):
             self.net.post('uri',
@@ -756,7 +811,6 @@ class ClientNetworkWithMockedResponseTest(unittest.TestCase):
         check_response.side_effect = [messages.Error.with_code('malformed'),
                                       self.response]
 
-        # pylint: disable=protected-access
         self.net._check_response = check_response
         with pytest.raises(messages.Error):
             self.net.post('uri',
@@ -767,7 +821,6 @@ class ClientNetworkWithMockedResponseTest(unittest.TestCase):
         post_once.side_effect = [messages.Error.with_code('badNonce'),
                                       self.response]
 
-        # pylint: disable=protected-access
         assert self.response == self.net.post(
             'uri', self.obj, content_type=self.content_type)
 

acme/src/acme/client.py

@@ -224,6 +224,11 @@ class ClientV2:
 
         :returns: updated order
         :rtype: messages.OrderResource
+
+        :raises .messages.Error: If server indicates order is not yet in ready state,
+            it will return a 403 (Forbidden) error with a problem document/error code of type
+            "orderNotReady"
+
         """
         csr = x509.load_pem_x509_csr(orderr.csr_pem)
         wrapped_csr = messages.CertificateRequest(csr=csr)
@@ -239,6 +244,10 @@ class ClientV2:
         Poll an order that has been finalized for its status.
         If it becomes valid, obtain the certificate.
 
+        If a finalization request previously returned `orderNotReady`,
+        poll until ready, send a new finalization request, and continue
+        polling until valid as above.
+
         :returns: finalized order (with certificate)
         :rtype: messages.OrderResource
         """
@@ -248,12 +257,22 @@ class ClientV2:
             response = self._post_as_get(orderr.uri)
             body = messages.Order.from_json(response.json())
             if body.status == messages.STATUS_INVALID:
+                # "invalid": The certificate will not be issued.  Consider this
+                # order process abandoned.
                 if body.error is not None:
                     raise errors.IssuanceError(body.error)
                 raise errors.Error(
                     "The certificate order failed. No further information was provided "
                     "by the server.")
+            elif body.status == messages.STATUS_READY:
+                # "ready": The server agrees that the requirements have been
+                # fulfilled, and is awaiting finalization.  Submit a finalization
+                # request.
+                self.begin_finalization(orderr)
             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
+                # certificate.
                 certificate_response = self._post_as_get(body.certificate)
                 orderr = orderr.update(body=body, fullchain_pem=certificate_response.text)
                 if fetch_alternative_chains:
@@ -276,7 +295,11 @@ class ClientV2:
         :rtype: messages.OrderResource
 
         """
+        try:
             self.begin_finalization(orderr)
+        except messages.Error as e:
+            if e.code != 'orderNotReady':
+                raise e
         return self.poll_finalization(orderr, deadline, fetch_alternative_chains)
 
     def revoke(self, cert: x509.Certificate, rsn: int) -> None:

certbot/CHANGELOG.md

@@ -19,7 +19,11 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
-*
+* Order finalization now catches `orderNotReady` response, polls until order status is
+  `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.
 
 More details about these changes can be found on our GitHub repo.