Merge pull request #1635 from kuba/poll_and_request-timeout

poll_and_ri: handle STATUS_INVALID, add max_attempts (fixes #1634)

acme/acme/client.py

@@ -246,9 +246,7 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
     def retry_after(cls, response, default):
         """Compute next `poll` time based on response ``Retry-After`` header.
 
-        :param response: Response from `poll`.
-        :type response: `requests.Response`
-
+        :param requests.Response response: Response from `poll`.
         :param int default: Default value (in seconds), used when
             ``Retry-After`` header is not present or invalid.
 
@@ -323,22 +321,21 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
             body=jose.ComparableX509(OpenSSL.crypto.load_certificate(
                 OpenSSL.crypto.FILETYPE_ASN1, response.content)))
 
-    def poll_and_request_issuance(self, csr, authzrs, mintime=5):
+    def poll_and_request_issuance(
+            self, csr, authzrs, mintime=5, max_attempts=10):
         """Poll and request issuance.
 
         This function polls all provided Authorization Resource URIs
         until all challenges are valid, respecting ``Retry-After`` HTTP
         headers, and then calls `request_issuance`.
 
-        .. todo:: add `max_attempts` or `timeout`
-
-        :param csr: CSR.
-        :type csr: `OpenSSL.crypto.X509Req` wrapped in `.ComparableX509`
-
+        :param .ComparableX509 csr: CSR (`OpenSSL.crypto.X509Req`
+            wrapped in `.ComparableX509`)
         :param authzrs: `list` of `.AuthorizationResource`
-
         :param int mintime: Minimum time before next attempt, used if
             ``Retry-After`` is not present in the response.
+        :param int max_attempts: Maximum number of attempts before
+            `PollError` with non-empty ``waiting`` is raised.
 
         :returns: ``(cert, updated_authzrs)`` `tuple` where ``cert`` is
             the issued certificate (`.messages.CertificateResource.),
@@ -348,6 +345,9 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
             as the input ``authzrs``.
         :rtype: `tuple`
 
+        :raises PollError: in case of timeout or if some authorization
+            was marked by the CA as invalid
+
         """
         # priority queue with datetime (based on Retry-After) as key,
         # and original Authorization Resource as value
@@ -356,7 +356,8 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
         # recently updated one
         updated = dict((authzr, authzr) for authzr in authzrs)
 
-        while waiting:
+        while waiting and max_attempts:
+            max_attempts -= 1
             # find the smallest Retry-After, and sleep if necessary
             when, authzr = heapq.heappop(waiting)
             now = datetime.datetime.now()
@@ -371,11 +372,16 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
             updated[authzr] = updated_authzr
 
             # pylint: disable=no-member
-            if updated_authzr.body.status != messages.STATUS_VALID:
+            if updated_authzr.body.status not in (
+                    messages.STATUS_VALID, messages.STATUS_INVALID):
                 # push back to the priority queue, with updated retry_after
                 heapq.heappush(waiting, (self.retry_after(
                     response, default=mintime), authzr))
 
+        if not max_attempts or any(authzr.body.status == messages.STATUS_INVALID
+                                   for authzr in six.itervalues(updated)):
+            raise errors.PollError(waiting, updated)
+
         updated_authzrs = tuple(updated[authzr] for authzr in authzrs)
         return self.request_issuance(csr, updated_authzrs), updated_authzrs
 

acme/acme/client_test.py

@@ -271,9 +271,9 @@ class ClientTest(unittest.TestCase):
             # result, increment clock
             clock.dt += datetime.timedelta(seconds=2)
 
-            if not authzr.retries:  # no more retries
+            if len(authzr.retries) == 1:  # no more retries
                 done = mock.MagicMock(uri=authzr.uri, times=authzr.times)
-                done.body.status = messages.STATUS_VALID
+                done.body.status = authzr.retries[0]
                 return done, []
 
             # response (2nd result tuple element) is reduced to only
@@ -289,7 +289,8 @@ class ClientTest(unittest.TestCase):
 
         mintime = 7
 
-        def retry_after(response, default):  # pylint: disable=missing-docstring
+        def retry_after(response, default):
+            # pylint: disable=missing-docstring
             # check that poll_and_request_issuance correctly passes mintime
             self.assertEqual(default, mintime)
             return clock.dt + datetime.timedelta(seconds=response)
@@ -302,8 +303,10 @@ class ClientTest(unittest.TestCase):
 
         csr = mock.MagicMock()
         authzrs = (
-            mock.MagicMock(uri='a', times=[], retries=(8, 20, 30)),
-            mock.MagicMock(uri='b', times=[], retries=(5,)),
+            mock.MagicMock(uri='a', times=[], retries=(
+                8, 20, 30, messages.STATUS_VALID)),
+            mock.MagicMock(uri='b', times=[], retries=(
+                5, messages.STATUS_VALID)),
         )
 
         cert, updated_authzrs = self.client.poll_and_request_issuance(
@@ -327,6 +330,17 @@ class ClientTest(unittest.TestCase):
         ])
         self.assertEqual(clock.dt, datetime.datetime(2015, 3, 27, 0, 1, 7))
 
+        # CA sets invalid | TODO: move to a separate test
+        invalid_authzr = mock.MagicMock(times=[], retries=[messages.STATUS_INVALID])
+        self.assertRaises(
+            errors.PollError, self.client.poll_and_request_issuance,
+            csr, authzrs=(invalid_authzr,), mintime=mintime)
+
+        # exceeded max_attemps | TODO: move to a separate test
+        self.assertRaises(
+            errors.PollError, self.client.poll_and_request_issuance,
+            csr, authzrs, mintime=mintime, max_attempts=2)
+
     def test_check_cert(self):
         self.response.headers['Location'] = self.certr.uri
         self.response.content = CERT_DER

acme/acme/errors.py

@@ -51,3 +51,31 @@ class MissingNonce(NonceError):
         return ('Server {0} response did not include a replay '
                 'nonce, headers: {1}'.format(
                     self.response.request.method, self.response.headers))
+
+
+class PollError(ClientError):
+    """Generic error when polling for authorization fails.
+
+    This might be caused by either timeout (`waiting` will be non-empty)
+    or by some authorization being invalid.
+
+    :ivar waiting: Priority queue with `datetime.datatime` (based on
+        ``Retry-After``) as key, and original `.AuthorizationResource`
+        as value.
+    :ivar updated: Mapping from original `.AuthorizationResource`
+        to the most recently updated one
+
+    """
+    def __init__(self, waiting, updated):
+        self.waiting = waiting
+        self.updated = updated
+        super(PollError, self).__init__()
+
+    @property
+    def timeout(self):
+        """Was the error caused by timeout?"""
+        return bool(self.waiting)
+
+    def __repr__(self):
+        return '{0}(waiting={1!r}, updated={2!r})'.format(
+            self.__class__.__name__, self.waiting, self.updated)

acme/acme/errors_test.py

@@ -1,4 +1,5 @@
 """Tests for acme.errors."""
+import datetime
 import unittest
 
 import mock
@@ -29,5 +30,25 @@ class MissingNonceTest(unittest.TestCase):
         self.assertTrue("{}" in str(self.error))
 
 
+class PollErrorTest(unittest.TestCase):
+    """Tests for acme.errors.PollError."""
+
+    def setUp(self):
+        from acme.errors import PollError
+        self.timeout = PollError(
+            waiting=[(datetime.datetime(2015, 11, 29), mock.sentinel.AR)],
+            updated={})
+        self.invalid = PollError(waiting=[], updated={
+            mock.sentinel.AR: mock.sentinel.AR2})
+
+    def test_timeout(self):
+        self.assertTrue(self.timeout.timeout)
+        self.assertFalse(self.invalid.timeout)
+
+    def test_repr(self):
+        self.assertEqual('PollError(waiting=[], updated={sentinel.AR: '
+                         'sentinel.AR2})', repr(self.invalid))
+
+
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover