From 1e46d26ac3511ca92e14c98c823a82286142ebb6 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 22 Feb 2018 16:28:50 -0800 Subject: [PATCH] Fix ACMEv2 issues (#5612) * Add post wrapper to automatically add acme_version * Add uri to authzr. * Only add kid when account is set. * Add content_type when downloading certificate. * Only save new_authz URL when it exists. * Handle combinations in ACMEv1 and ACMEv2. * Add tests for ACMEv2 "combinations". --- acme/acme/client.py | 61 ++++++++++++-------- certbot/account.py | 17 ++++-- certbot/auth_handler.py | 12 +++- certbot/client.py | 4 +- certbot/tests/auth_handler_test.py | 93 ++++++++++++++++++++++++------ 5 files changed, 134 insertions(+), 53 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index 97f529aae..9854aae31 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -70,7 +70,7 @@ class ClientBase(object): # pylint: disable=too-many-instance-attributes terms_of_service=terms_of_service) def _send_recv_regr(self, regr, body): - response = self.net.post(regr.uri, body, acme_version=self.acme_version) + response = self._post(regr.uri, body) # TODO: Boulder returns httplib.ACCEPTED #assert response.status_code == httplib.OK @@ -82,6 +82,13 @@ class ClientBase(object): # pylint: disable=too-many-instance-attributes response, uri=regr.uri, terms_of_service=regr.terms_of_service) + def _post(self, *args, **kwargs): + """Wrapper around self.net.post that adds the acme_version. + + """ + kwargs.setdefault('acme_version', self.acme_version) + return self.net.post(*args, **kwargs) + def update_registration(self, regr, update=None): """Update registration. @@ -143,8 +150,7 @@ class ClientBase(object): # pylint: disable=too-many-instance-attributes :raises .UnexpectedUpdate: """ - response = self.net.post(challb.uri, response, - acme_version=self.acme_version) + response = self._post(challb.uri, response) try: authzr_uri = response.links['up']['url'] except KeyError: @@ -216,12 +222,11 @@ class ClientBase(object): # pylint: disable=too-many-instance-attributes :raises .ClientError: If revocation is unsuccessful. """ - response = self.net.post(self.directory[messages.Revocation], + response = self._post(self.directory[messages.Revocation], messages.Revocation( certificate=cert, reason=rsn), - content_type=None, - acme_version=self.acme_version) + content_type=None) if response.status_code != http_client.OK: raise errors.ClientError( 'Successful revocation must return HTTP OK status') @@ -271,8 +276,7 @@ class Client(ClientBase): """ new_reg = messages.NewRegistration() if new_reg is None else new_reg - response = self.net.post(self.directory[new_reg], new_reg, - acme_version=1) + response = self._post(self.directory[new_reg], new_reg) # TODO: handle errors assert response.status_code == http_client.CREATED @@ -308,8 +312,7 @@ class Client(ClientBase): if new_authzr_uri is not None: logger.debug("request_challenges with new_authzr_uri deprecated.") new_authz = messages.NewAuthorization(identifier=identifier) - response = self.net.post(self.directory.new_authz, new_authz, - acme_version=1) + response = self._post(self.directory.new_authz, new_authz) # TODO: handle errors assert response.status_code == http_client.CREATED return self._authzr_from_response(response, identifier) @@ -351,12 +354,11 @@ class Client(ClientBase): req = messages.CertificateRequest(csr=csr) content_type = DER_CONTENT_TYPE # TODO: add 'cert_type 'argument - response = self.net.post( + response = self._post( self.directory.new_cert, req, content_type=content_type, - headers={'Accept': content_type}, - acme_version=1) + headers={'Accept': content_type}) cert_chain_uri = response.links.get('up', {}).get('url') @@ -552,8 +554,7 @@ class ClientV2(ClientBase): :returns: Registration Resource. :rtype: `.RegistrationResource` """ - response = self.net.post(self.directory['newAccount'], new_account, - acme_version=2) + response = self._post(self.directory['newAccount'], new_account) # "Instance of 'Field' has no key/contact member" bug: # pylint: disable=no-member regr = self._regr_from_response(response) @@ -577,11 +578,11 @@ class ClientV2(ClientBase): identifiers.append(messages.Identifier(typ=messages.IDENTIFIER_FQDN, value=name)) order = messages.NewOrder(identifiers=identifiers) - response = self.net.post(self.directory['newOrder'], order) + response = self._post(self.directory['newOrder'], order) body = messages.Order.from_json(response.json()) authorizations = [] for url in body.authorizations: - authorizations.append(self._authzr_from_response(self.net.get(url))) + authorizations.append(self._authzr_from_response(self.net.get(url), uri=url)) return messages.OrderResource( body=body, uri=response.headers.get('Location'), @@ -643,7 +644,7 @@ class ClientV2(ClientBase): csr = OpenSSL.crypto.load_certificate_request( OpenSSL.crypto.FILETYPE_PEM, orderr.csr_pem) wrapped_csr = messages.CertificateRequest(csr=jose.ComparableX509(csr)) - self.net.post(orderr.body.finalize, wrapped_csr) + self._post(orderr.body.finalize, wrapped_csr) while datetime.datetime.now() < deadline: time.sleep(1) response = self.net.get(orderr.uri) @@ -651,17 +652,29 @@ class ClientV2(ClientBase): if body.error is not None: raise errors.IssuanceError(body.error) if body.certificate is not None: - certificate_response = self.net.get(body.certificate).text + certificate_response = self.net.get(body.certificate, + content_type=DER_CONTENT_TYPE).text return orderr.update(body=body, fullchain_pem=certificate_response) raise errors.TimeoutError() class BackwardsCompatibleClientV2(object): """ACME client wrapper that tends towards V2-style calls, but - supports V1 servers. + supports V1 servers. - :ivar int acme_version: 1 or 2, corresponding to the Let's Encrypt endpoint - :ivar .ClientBase client: either Client or ClientV2 + .. note:: While this class handles the majority of the differences + between versions of the ACME protocol, if you need to support an + ACME server based on version 3 or older of the IETF ACME draft + that uses combinations in authorizations (or lack thereof) to + signal that the client needs to complete something other than + any single challenge in the authorization to make it valid, the + user of this class needs to understand and handle these + differences themselves. This does not apply to either of Let's + Encrypt's endpoints where successfully completing any challenge + in an authorization will make it valid. + + :ivar int acme_version: 1 or 2, corresponding to the Let's Encrypt endpoint + :ivar .ClientBase client: either Client or ClientV2 """ def __init__(self, net, key, server): @@ -829,7 +842,9 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes } if acme_version == 2: kwargs["url"] = url - kwargs["kid"] = self.account["uri"] + # newAccount and revokeCert work without the kid + if self.account is not None: + kwargs["kid"] = self.account["uri"] kwargs["key"] = self.key # pylint: disable=star-args return jws.JWS.sign(jobj, **kwargs).json_dumps(indent=2) diff --git a/certbot/account.py b/certbot/account.py index 41e980097..70d9a7fc3 100644 --- a/certbot/account.py +++ b/certbot/account.py @@ -223,12 +223,17 @@ class AccountFileStorage(interfaces.AccountStorage): try: with open(self._regr_path(account_dir_path), "w") as regr_file: regr = account.regr - with_uri = RegistrationResourceWithNewAuthzrURI( - new_authzr_uri=acme.directory.new_authz, - body=regr.body, - uri=regr.uri, - terms_of_service=regr.terms_of_service) - regr_file.write(with_uri.json_dumps()) + # If we have a value for new-authz, save it for forwards + # compatibility with older versions of Certbot. If we don't + # have a value for new-authz, this is an ACMEv2 directory where + # an older version of Certbot won't work anyway. + if hasattr(acme.directory, "new-authz"): + regr = RegistrationResourceWithNewAuthzrURI( + new_authzr_uri=acme.directory.new_authz, + body=regr.body, + uri=regr.uri, + terms_of_service=regr.terms_of_service) + regr_file.write(regr.json_dumps()) if not regr_only: with util.safe_open(self._key_path(account_dir_path), "w", chmod=0o400) as key_file: diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 47d806b94..9cc10d4b4 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -24,7 +24,7 @@ class AuthHandler(object): :class:`~acme.challenges.Challenge` types :type auth: :class:`certbot.interfaces.IAuthenticator` - :ivar acme.client.Client acme: ACME client API. + :ivar acme.client.BackwardsCompatibleClientV2 acme: ACME client API. :ivar account: Client's Account :type account: :class:`certbot.account.Account` @@ -100,10 +100,16 @@ class AuthHandler(object): """Retrieve necessary challenges to satisfy server.""" logger.info("Performing the following challenges:") for dom in domains: + dom_challenges = self.authzr[dom].body.challenges + if self.acme.acme_version == 1: + combinations = self.authzr[dom].body.combinations + else: + combinations = tuple((i,) for i in range(len(dom_challenges))) + path = gen_challenge_path( - self.authzr[dom].body.challenges, + dom_challenges, self._get_chall_pref(dom), - self.authzr[dom].body.combinations) + combinations) dom_achalls = self._challenge_factory( dom, path) diff --git a/certbot/client.py b/certbot/client.py index 2d7288ce3..0f4fa760d 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -212,8 +212,8 @@ class Client(object): :ivar .IAuthenticator auth: Prepared (`.IAuthenticator.prepare`) authenticator that can solve ACME challenges. :ivar .IInstaller installer: Installer. - :ivar acme.client.Client acme: Optional ACME client API handle. - You might already have one from `register`. + :ivar acme.client.BackwardsCompatibleClientV2 acme: Optional ACME + client API handle. You might already have one from `register`. """ diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index 3633b673d..394002206 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -81,6 +81,7 @@ class HandleAuthorizationsTest(unittest.TestCase): self.mock_account = mock.Mock(key=util.Key("file_path", "PEM")) self.mock_net = mock.MagicMock(spec=acme_client.Client) + self.mock_net.acme_version = 1 self.handler = AuthHandler( self.mock_auth, self.mock_net, self.mock_account, []) @@ -90,13 +91,13 @@ class HandleAuthorizationsTest(unittest.TestCase): def tearDown(self): logging.disable(logging.NOTSET) - @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") - def test_name1_tls_sni_01_1(self, mock_poll): - mock_poll.side_effect = self._validate_all - - authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES) + def _test_name1_tls_sni_01_1_common(self, combos): + authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=combos) mock_order = mock.MagicMock(authorizations=[authzr]) - authzr = self.handler.handle_authorizations(mock_order) + + with mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") as mock_poll: + mock_poll.side_effect = self._validate_all + authzr = self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_net.answer_challenge.call_count, 1) @@ -112,8 +113,15 @@ class HandleAuthorizationsTest(unittest.TestCase): self.assertEqual(len(authzr), 1) + def test_name1_tls_sni_01_1_acme_1(self): + self._test_name1_tls_sni_01_1_common(combos=True) + + def test_name1_tls_sni_01_1_acme_2(self): + self.mock_net.acme_version = 2 + self._test_name1_tls_sni_01_1_common(combos=False) + @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") - def test_name1_tls_sni_01_1_http_01_1_dns_1(self, mock_poll): + def test_name1_tls_sni_01_1_http_01_1_dns_1_acme_1(self, mock_poll): mock_poll.side_effect = self._validate_all self.mock_auth.get_chall_pref.return_value.append(challenges.HTTP01) self.mock_auth.get_chall_pref.return_value.append(challenges.DNS01) @@ -138,17 +146,43 @@ class HandleAuthorizationsTest(unittest.TestCase): self.assertEqual(len(authzr), 1) @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") - def test_name3_tls_sni_01_3(self, mock_poll): - self.mock_net.request_domain_challenges.side_effect = functools.partial( - gen_dom_authzr, challs=acme_util.CHALLENGES) - + def test_name1_tls_sni_01_1_http_01_1_dns_1_acme_2(self, mock_poll): + self.mock_net.acme_version = 2 mock_poll.side_effect = self._validate_all + self.mock_auth.get_chall_pref.return_value.append(challenges.HTTP01) + self.mock_auth.get_chall_pref.return_value.append(challenges.DNS01) + + authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=False) + mock_order = mock.MagicMock(authorizations=[authzr]) + authzr = self.handler.handle_authorizations(mock_order) + + self.assertEqual(self.mock_net.answer_challenge.call_count, 1) + + self.assertEqual(mock_poll.call_count, 1) + chall_update = mock_poll.call_args[0][0] + self.assertEqual(list(six.iterkeys(chall_update)), ["0"]) + self.assertEqual(len(chall_update.values()), 1) + + self.assertEqual(self.mock_auth.cleanup.call_count, 1) + cleaned_up_achalls = self.mock_auth.cleanup.call_args[0][0] + self.assertEqual(len(cleaned_up_achalls), 1) + self.assertEqual(cleaned_up_achalls[0].typ, "tls-sni-01") + + # Length of authorizations list + self.assertEqual(len(authzr), 1) + + def _test_name3_tls_sni_01_3_common(self, combos): + self.mock_net.request_domain_challenges.side_effect = functools.partial( + gen_dom_authzr, challs=acme_util.CHALLENGES, combos=combos) + authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES), gen_dom_authzr(domain="1", challs=acme_util.CHALLENGES), gen_dom_authzr(domain="2", challs=acme_util.CHALLENGES)] mock_order = mock.MagicMock(authorizations=authzrs) - authzr = self.handler.handle_authorizations(mock_order) + with mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") as mock_poll: + mock_poll.side_effect = self._validate_all + authzr = self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_net.answer_challenge.call_count, 3) @@ -167,6 +201,13 @@ class HandleAuthorizationsTest(unittest.TestCase): self.assertEqual(len(authzr), 3) + def test_name3_tls_sni_01_3_common_acme_1(self): + self._test_name3_tls_sni_01_3_common(combos=True) + + def test_name3_tls_sni_01_3_common_acme_2(self): + self.mock_net.acme_version = 2 + self._test_name3_tls_sni_01_3_common(combos=False) + @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") def test_debug_challenges(self, mock_poll): zope.component.provideUtility( @@ -194,30 +235,44 @@ class HandleAuthorizationsTest(unittest.TestCase): mock_order = mock.MagicMock(authorizations=[]) self.assertRaises(errors.AuthorizationError, self.handler.handle_authorizations, mock_order) - @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") - def test_preferred_challenge_choice(self, mock_poll): - authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] + def _test_preferred_challenge_choice_common(self, combos): + authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=combos)] mock_order = mock.MagicMock(authorizations=authzrs) - mock_poll.side_effect = self._validate_all self.mock_auth.get_chall_pref.return_value.append(challenges.HTTP01) self.handler.pref_challs.extend((challenges.HTTP01.typ, challenges.DNS01.typ,)) - self.handler.handle_authorizations(mock_order) + with mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") as mock_poll: + mock_poll.side_effect = self._validate_all + self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_auth.cleanup.call_count, 1) self.assertEqual( self.mock_auth.cleanup.call_args[0][0][0].typ, "http-01") - def test_preferred_challenges_not_supported(self): - authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] + def test_preferred_challenge_choice_common_acme_1(self): + self._test_preferred_challenge_choice_common(combos=True) + + def test_preferred_challenge_choice_common_acme_2(self): + self.mock_net.acme_version = 2 + self._test_preferred_challenge_choice_common(combos=False) + + def _test_preferred_challenges_not_supported_common(self, combos): + authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=combos)] mock_order = mock.MagicMock(authorizations=authzrs) self.handler.pref_challs.append(challenges.HTTP01.typ) self.assertRaises( errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + def test_preferred_challenges_not_supported_acme_1(self): + self._test_preferred_challenges_not_supported_common(combos=True) + + def test_preferred_challenges_not_supported_acme_2(self): + self.mock_net.acme_version = 2 + self._test_preferred_challenges_not_supported_common(combos=False) + def _validate_all(self, unused_1, unused_2): for dom in six.iterkeys(self.handler.authzr): azr = self.handler.authzr[dom]