Remove keyAuthorization fallback dump in challenges response (#6975)

Fixes #6974.

This PR removes the fallback that consists in retrying to send the keyAuthorization field during a challenge request in case of malformed request.

* Remove keyAuthorization fallback dump in challenges response

* Correct import

* Add changelog entry

CHANGELOG.md

@@ -17,6 +17,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 * Updated Certbot and its plugins to improve the handling of file system permissions
   on Windows as a step towards adding proper Windows support to Certbot.
 * Updated urllib3 to 1.24.2 in certbot-auto.
+* Removed the fallback introduced with 0.32.0 in `acme` to retry a challenge response
+  with a `keyAuthorization` if sending the response without this field caused a
+  `malformed` error to be received from the ACME server.
 
 ### Fixed
 
@@ -26,6 +29,7 @@ Despite us having broken lockstep, we are continuing to release new versions of
 all Certbot components during releases for the time being, however, the only
 package with changes other than its version number was:
 
+* acme
 * certbot
 * certbot-apache
 * certbot-dns-cloudflare

acme/acme/challenges.py

@@ -107,10 +107,6 @@ class KeyAuthorizationChallengeResponse(ChallengeResponse):
     key_authorization = jose.Field("keyAuthorization")
     thumbprint_hash_function = hashes.SHA256
 
-    def __init__(self, *args, **kwargs):
-        super(KeyAuthorizationChallengeResponse, self).__init__(*args, **kwargs)
-        self._dump_authorization_key(False)
-
     def verify(self, chall, account_public_key):
         """Verify the key authorization.
 
@@ -143,20 +139,9 @@ class KeyAuthorizationChallengeResponse(ChallengeResponse):
 
         return True
 
-    def _dump_authorization_key(self, dump):
-        # type: (bool) -> None
-        """
-        Set if keyAuthorization is dumped in the JSON representation of this ChallengeResponse.
-        NB: This method is declared as private because it will eventually be removed.
-        :param bool dump: True to dump the keyAuthorization, False otherwise
-        """
-        object.__setattr__(self, '_dump_auth_key', dump)
-
     def to_partial_json(self):
         jobj = super(KeyAuthorizationChallengeResponse, self).to_partial_json()
-        if not self._dump_auth_key:  # pylint: disable=no-member
-            jobj.pop('keyAuthorization', None)
-
+        jobj.pop('keyAuthorization', None)
         return jobj
 
 

acme/acme/challenges_test.py

@@ -95,8 +95,6 @@ class DNS01ResponseTest(unittest.TestCase):
     def test_to_partial_json(self):
         self.assertEqual({k: v for k, v in self.jmsg.items() if k != 'keyAuthorization'},
                          self.msg.to_partial_json())
-        self.msg._dump_authorization_key(True)  # pylint: disable=protected-access
-        self.assertEqual(self.jmsg, self.msg.to_partial_json())
 
     def test_from_json(self):
         from acme.challenges import DNS01Response
@@ -169,8 +167,6 @@ class HTTP01ResponseTest(unittest.TestCase):
     def test_to_partial_json(self):
         self.assertEqual({k: v for k, v in self.jmsg.items() if k != 'keyAuthorization'},
                          self.msg.to_partial_json())
-        self.msg._dump_authorization_key(True)  # pylint: disable=protected-access
-        self.assertEqual(self.jmsg, self.msg.to_partial_json())
 
     def test_from_json(self):
         from acme.challenges import HTTP01Response
@@ -292,8 +288,6 @@ class TLSSNI01ResponseTest(unittest.TestCase):
     def test_to_partial_json(self):
         self.assertEqual({k: v for k, v in self.jmsg.items() if k != 'keyAuthorization'},
                          self.response.to_partial_json())
-        self.response._dump_authorization_key(True)  # pylint: disable=protected-access
-        self.assertEqual(self.jmsg, self.response.to_partial_json())
 
     def test_from_json(self):
         from acme.challenges import TLSSNI01Response
@@ -428,8 +422,6 @@ class TLSALPN01ResponseTest(unittest.TestCase):
     def test_to_partial_json(self):
         self.assertEqual({k: v for k, v in self.jmsg.items() if k != 'keyAuthorization'},
                          self.msg.to_partial_json())
-        self.msg._dump_authorization_key(True)  # pylint: disable=protected-access
-        self.assertEqual(self.jmsg, self.msg.to_partial_json())
 
     def test_from_json(self):
         from acme.challenges import TLSALPN01Response

acme/acme/client.py

@@ -17,7 +17,6 @@ import requests
 from requests.adapters import HTTPAdapter
 from requests_toolbelt.adapters.source import SourceAddressAdapter
 
-from acme import challenges
 from acme import crypto_util
 from acme import errors
 from acme import jws
@@ -156,23 +155,7 @@ class ClientBase(object):  # pylint: disable=too-many-instance-attributes
         :raises .UnexpectedUpdate:
 
         """
-        # Because sending keyAuthorization in a response challenge has been removed from the ACME
-        # spec, it is not included in the KeyAuthorizationResponseChallenge JSON by default.
-        # However as a migration path, we temporarily expect a malformed error from the server,
-        # and fallback by resending the challenge response with the keyAuthorization field.
-        # TODO: Remove this fallback for Certbot 0.34.0
-        try:
-            response = self._post(challb.uri, response)
-        except messages.Error as error:
-            if (error.code == 'malformed'
-                    and isinstance(response, challenges.KeyAuthorizationChallengeResponse)):
-                logger.debug('Error while responding to a challenge without keyAuthorization '
-                             'in the JWS, your ACME CA server may not support it:\n%s', error)
-                logger.debug('Retrying request with keyAuthorization set.')
-                response._dump_authorization_key(True)  # pylint: disable=protected-access
-                response = self._post(challb.uri, response)
-            else:
-                raise
+        response = self._post(challb.uri, response)
         try:
             authzr_uri = response.links['up']['url']
         except KeyError:

acme/acme/client_test.py

@@ -461,34 +461,6 @@ class ClientTest(ClientTestBase):
             errors.ClientError, self.client.answer_challenge,
             self.challr.body, challenges.DNSResponse(validation=None))
 
-    def test_answer_challenge_key_authorization_fallback(self):
-        self.response.links['up'] = {'url': self.challr.authzr_uri}
-        self.response.json.return_value = self.challr.body.to_json()
-
-        def _wrapper_post(url, obj, *args, **kwargs):  # pylint: disable=unused-argument
-            """
-            Simulate an old ACME CA server, that would respond a 'malformed'
-            error if keyAuthorization is missing.
-            """
-            jobj = obj.to_partial_json()
-            if 'keyAuthorization' not in jobj:
-                raise messages.Error.with_code('malformed')
-            return self.response
-        self.net.post.side_effect = _wrapper_post
-
-        # This challenge response is of type KeyAuthorizationChallengeResponse, so the fallback
-        # should be triggered, and avoid an exception.
-        http_chall_response = challenges.HTTP01Response(key_authorization='test',
-                                                        resource=mock.MagicMock())
-        self.client.answer_challenge(self.challr.body, http_chall_response)
-
-        # This challenge response is not of type KeyAuthorizationChallengeResponse, so the fallback
-        # should not be triggered, leading to an exception.
-        dns_chall_response = challenges.DNSResponse(validation=None)
-        self.assertRaises(
-            errors.Error, self.client.answer_challenge,
-            self.challr.body, dns_chall_response)
-
     def test_retry_after_date(self):
         self.response.headers['Retry-After'] = 'Fri, 31 Dec 1999 23:59:59 GMT'
         self.assertEqual(