Implement POST-as-GET requests (#6522)

* Setup an integration tests env against Pebble, that enforce post-as-get

* Implement POST-as-GET requests, with fallback to GET.

* Fix unit tests

* Fix coverage.

* Fix or ignore lint errors

* Corrections after review

* Correct test

* Try a simple delegate approach

* Add a test

* Simplify test mocking

* Clean comment

acme/acme/client.py

@@ -200,22 +200,6 @@ class ClientBase(object):  # pylint: disable=too-many-instance-attributes
 
         return datetime.datetime.now() + datetime.timedelta(seconds=seconds)
 
-    def poll(self, authzr):
-        """Poll Authorization Resource for status.
-
-        :param authzr: Authorization Resource
-        :type authzr: `.AuthorizationResource`
-
-        :returns: Updated Authorization Resource and HTTP response.
-
-        :rtype: (`.AuthorizationResource`, `requests.Response`)
-
-        """
-        response = self.net.get(authzr.uri)
-        updated_authzr = self._authzr_from_response(
-            response, authzr.body.identifier, authzr.uri)
-        return updated_authzr, response
-
     def _revoke(self, cert, rsn, url):
         """Revoke certificate.
 
@@ -237,6 +221,7 @@ class ClientBase(object):  # pylint: disable=too-many-instance-attributes
             raise errors.ClientError(
                 'Successful revocation must return HTTP OK status')
 
+
 class Client(ClientBase):
     """ACME client for a v1 API.
 
@@ -389,6 +374,22 @@ class Client(ClientBase):
             body=jose.ComparableX509(OpenSSL.crypto.load_certificate(
                 OpenSSL.crypto.FILETYPE_ASN1, response.content)))
 
+    def poll(self, authzr):
+        """Poll Authorization Resource for status.
+
+        :param authzr: Authorization Resource
+        :type authzr: `.AuthorizationResource`
+
+        :returns: Updated Authorization Resource and HTTP response.
+
+        :rtype: (`.AuthorizationResource`, `requests.Response`)
+
+        """
+        response = self.net.get(authzr.uri)
+        updated_authzr = self._authzr_from_response(
+            response, authzr.body.identifier, authzr.uri)
+        return updated_authzr, response
+
     def poll_and_request_issuance(
             self, csr, authzrs, mintime=5, max_attempts=10):
         """Poll and request issuance.
@@ -652,13 +653,29 @@ class ClientV2(ClientBase):
         body = messages.Order.from_json(response.json())
         authorizations = []
         for url in body.authorizations:
-            authorizations.append(self._authzr_from_response(self.net.get(url), uri=url))
+            authorizations.append(self._authzr_from_response(self._post_as_get(url), uri=url))
         return messages.OrderResource(
             body=body,
             uri=response.headers.get('Location'),
             authorizations=authorizations,
             csr_pem=csr_pem)
 
+    def poll(self, authzr):
+        """Poll Authorization Resource for status.
+
+        :param authzr: Authorization Resource
+        :type authzr: `.AuthorizationResource`
+
+        :returns: Updated Authorization Resource and HTTP response.
+
+        :rtype: (`.AuthorizationResource`, `requests.Response`)
+
+        """
+        response = self._post_as_get(authzr.uri)
+        updated_authzr = self._authzr_from_response(
+            response, authzr.body.identifier, authzr.uri)
+        return updated_authzr, response
+
     def poll_and_finalize(self, orderr, deadline=None):
         """Poll authorizations and finalize the order.
 
@@ -682,7 +699,7 @@ class ClientV2(ClientBase):
         responses = []
         for url in orderr.body.authorizations:
             while datetime.datetime.now() < deadline:
-                authzr = self._authzr_from_response(self.net.get(url), uri=url)
+                authzr = self._authzr_from_response(self._post_as_get(url), uri=url)
                 if authzr.body.status != messages.STATUS_PENDING:
                     responses.append(authzr)
                     break
@@ -717,12 +734,12 @@ class ClientV2(ClientBase):
         self._post(orderr.body.finalize, wrapped_csr)
         while datetime.datetime.now() < deadline:
             time.sleep(1)
-            response = self.net.get(orderr.uri)
+            response = self._post_as_get(orderr.uri)
             body = messages.Order.from_json(response.json())
             if body.error is not None:
                 raise errors.IssuanceError(body.error)
             if body.certificate is not None:
-                certificate_response = self.net.get(body.certificate,
+                certificate_response = self._post_as_get(body.certificate,
                                                     content_type=DER_CONTENT_TYPE).text
                 return orderr.update(body=body, fullchain_pem=certificate_response)
         raise errors.TimeoutError()
@@ -740,6 +757,32 @@ class ClientV2(ClientBase):
         """
         return self._revoke(cert, rsn, self.directory['revokeCert'])
 
+    def _post_as_get(self, *args, **kwargs):
+        """
+        Send GET request using the POST-as-GET protocol if needed.
+        The request will be first issued using POST-as-GET for ACME v2. If the ACME CA servers do
+        not support this yet and return an error, request will be retried using GET.
+        For ACME v1, only GET request will be tried, as POST-as-GET is not supported.
+        :param args:
+        :param kwargs:
+        :return:
+        """
+        if self.acme_version >= 2:
+            # We add an empty payload for POST-as-GET requests
+            new_args = args[:1] + (None,) + args[1:]
+            try:
+                return self._post(*new_args, **kwargs)  # pylint: disable=star-args
+            except messages.Error as error:
+                if error.code == 'malformed':
+                    logger.debug('Error during a POST-as-GET request, '
+                                 'your ACME CA may not support it:\n%s', error)
+                    logger.debug('Retrying request with GET.')
+                else:  # pragma: no cover
+                    raise
+
+        # If POST-as-GET is not supported yet, we use a GET instead.
+        return self.net.get(*args, **kwargs)
+
 
 class BackwardsCompatibleClientV2(object):
     """ACME client wrapper that tends towards V2-style calls, but
@@ -769,12 +812,7 @@ class BackwardsCompatibleClientV2(object):
             self.client = ClientV2(directory, net=net)
 
     def __getattr__(self, name):
-        if name in vars(self.client):
+        return getattr(self.client, name)
-            return getattr(self.client, name)
-        elif name in dir(ClientBase):
-            return getattr(self.client, name)
-        else:
-            raise AttributeError()
 
     def new_account_and_tos(self, regr, check_tos_cb=None):
         """Combined register and agree_tos for V1, new_account for V2
@@ -944,7 +982,7 @@ class ClientNetwork(object):  # pylint: disable=too-many-instance-attributes
         :rtype: `josepy.JWS`
 
         """
-        jobj = obj.json_dumps(indent=2).encode()
+        jobj = obj.json_dumps(indent=2).encode() if obj else b''
         logger.debug('JWS payload:\n%s', jobj)
         kwargs = {
             "alg": self.alg,

acme/acme/client_test.py

@@ -1,4 +1,5 @@
 """Tests for acme.client."""
+# pylint: disable=too-many-lines
 import copy
 import datetime
 import json
@@ -730,9 +731,10 @@ class ClientV2Test(ClientTestBase):
         authz_response2 = self.response
         authz_response2.json.return_value = self.authz2.to_json()
         authz_response2.headers['Location'] = self.authzr2.uri
-        self.net.get.side_effect = (authz_response, authz_response2)
 
-        self.assertEqual(self.client.new_order(CSR_SAN_PEM), self.orderr)
+        with mock.patch('acme.client.ClientV2._post_as_get') as mock_post_as_get:
+            mock_post_as_get.side_effect = (authz_response, authz_response2)
+            self.assertEqual(self.client.new_order(CSR_SAN_PEM), self.orderr)
 
     @mock.patch('acme.client.datetime')
     def test_poll_and_finalize(self, mock_datetime):
@@ -821,6 +823,30 @@ class ClientV2Test(ClientTestBase):
         self.response.json.return_value = self.regr.body.update(
             contact=()).to_json()
 
+    def test_post_as_get(self):
+        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.net.post.assert_called_once_with(
+                self.authzr2.uri, None, acme_version=2,
+                new_nonce_url='https://www.letsencrypt-demo.org/acme/new-nonce')
+            self.client.net.get.assert_not_called()
+
+            class FakeError(messages.Error):  # pylint: disable=too-many-ancestors
+                """Fake error to reproduce a malformed request ACME error"""
+                def __init__(self):  # pylint: disable=super-init-not-called
+                    pass
+                @property
+                def code(self):
+                    return 'malformed'
+            self.client.net.post.side_effect = FakeError()
+
+            self.client.poll(self.authzr2)  # pylint: disable=protected-access
+
+            self.client.net.get.assert_called_once_with(self.authzr2.uri)
+
 
 class MockJSONDeSerializable(jose.JSONDeSerializable):
     # pylint: disable=missing-docstring

tests/certbot-pebble-integration.sh

@@ -0,0 +1,16 @@
+#!/bin/bash
+# Simple integration test. Make sure to activate virtualenv beforehand
+# (source venv/bin/activate) and that you are running Pebble test
+# instance (see ./pebble-fetch.sh).
+
+cleanup_and_exit() {
+    EXIT_STATUS=$?
+    unset SERVER
+    exit $EXIT_STATUS
+}
+
+trap cleanup_and_exit EXIT
+
+export SERVER=https://localhost:14000/dir
+
+./tests/certbot-boulder-integration.sh

tests/pebble-fetch.sh

@@ -0,0 +1,41 @@
+#!/bin/bash
+# Download and run Pebble instance for integration testing
+set -xe
+
+PEBBLE_VERSION=2018-11-02
+
+# We reuse the same GOPATH-style directory than for Boulder.
+# Pebble does not need it, but it will make the installation consistent with Boulder's one.
+export GOPATH=${GOPATH:-$HOME/gopath}
+PEBBLEPATH=${PEBBLEPATH:-$GOPATH/src/github.com/letsencrypt/pebble}
+
+mkdir -p ${PEBBLEPATH}
+
+cat << UNLIKELY_EOF > "$PEBBLEPATH/docker-compose.yml"
+version: '3'
+
+services:
+ pebble:
+  image: letsencrypt/pebble:${PEBBLE_VERSION}
+  command: pebble -strict ${PEBBLE_STRICT:-false} -dnsserver 10.77.77.1
+  ports:
+    - 14000:14000
+  environment:
+    - PEBBLE_VA_NOSLEEP=1
+UNLIKELY_EOF
+
+docker-compose -f "$PEBBLEPATH/docker-compose.yml" up -d pebble
+
+set +x  # reduce verbosity while waiting for boulder
+for n in `seq 1 150` ; do
+  if curl -k https://localhost:14000/dir 2>/dev/null; then
+    break
+  else
+    sleep 1
+  fi
+done
+
+if ! curl -k https://localhost:14000/dir 2>/dev/null; then
+  echo "timed out waiting for pebble to start"
+  exit 1
+fi