From 1380e59f56447c34799340aecb380553e9fa2fe1 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 13 Feb 2017 19:50:33 -0800 Subject: [PATCH] Remove Link rel=next for authzs and new-certs. (#4194) An early version of the spec indicated that clients should process issuance sequentially, following Link rel=next from an account URL to an authz URL, to a new-cert URL. However, the spec has long since moved to putting these URLs in the directory. Certbot nominally supports either; This change consolidates on always using the directory, simplifying things and making the transition to the latest ACME spec easier. --- acme/acme/client.py | 40 ++++++++---------------------- acme/acme/client_test.py | 40 ++++++------------------------ acme/acme/jose/json_util.py | 2 +- acme/acme/messages.py | 6 +---- acme/acme/messages_test.py | 6 +---- acme/examples/example_client.py | 3 +-- certbot/auth_handler.py | 3 +-- certbot/tests/account_test.py | 2 +- certbot/tests/acme_util.py | 1 - certbot/tests/auth_handler_test.py | 3 +-- certbot/tests/display/ops_test.py | 4 +-- 11 files changed, 26 insertions(+), 84 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index 0324967cf..d01555c75 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -71,20 +71,13 @@ class Client(object): # pylint: disable=too-many-instance-attributes self.directory = directory @classmethod - def _regr_from_response(cls, response, uri=None, new_authzr_uri=None, - terms_of_service=None): + def _regr_from_response(cls, response, uri=None, terms_of_service=None): if 'terms-of-service' in response.links: terms_of_service = response.links['terms-of-service']['url'] - if 'next' in response.links: - new_authzr_uri = response.links['next']['url'] - - if new_authzr_uri is None: - raise errors.ClientError('"next" link missing') return messages.RegistrationResource( body=messages.Registration.from_json(response.json()), uri=response.headers.get('Location', uri), - new_authzr_uri=new_authzr_uri, terms_of_service=terms_of_service) def register(self, new_reg=None): @@ -117,7 +110,7 @@ class Client(object): # pylint: disable=too-many-instance-attributes # (c.f. acme-spec #94) return self._regr_from_response( - response, uri=regr.uri, new_authzr_uri=regr.new_authzr_uri, + response, uri=regr.uri, terms_of_service=regr.terms_of_service) def update_registration(self, regr, update=None): @@ -174,43 +167,30 @@ class Client(object): # pylint: disable=too-many-instance-attributes return self.update_registration( regr.update(body=regr.body.update(agreement=regr.terms_of_service))) - def _authzr_from_response(self, response, identifier, - uri=None, new_cert_uri=None): - # pylint: disable=no-self-use - if new_cert_uri is None: - try: - new_cert_uri = response.links['next']['url'] - except KeyError: - raise errors.ClientError('"next" link missing') - + def _authzr_from_response(self, response, identifier, uri=None): authzr = messages.AuthorizationResource( body=messages.Authorization.from_json(response.json()), - uri=response.headers.get('Location', uri), - new_cert_uri=new_cert_uri) + uri=response.headers.get('Location', uri)) if authzr.body.identifier != identifier: raise errors.UnexpectedUpdate(authzr) return authzr - def request_challenges(self, identifier, new_authzr_uri=None): + def request_challenges(self, identifier): """Request challenges. :param .messages.Identifier identifier: Identifier to be challenged. - :param str new_authzr_uri: ``new-authorization`` URI. If omitted, - will default to value found in ``directory``. :returns: Authorization Resource. :rtype: `.AuthorizationResource` """ new_authz = messages.NewAuthorization(identifier=identifier) - response = self.net.post(self.directory.new_authz - if new_authzr_uri is None else new_authzr_uri, - new_authz) + response = self.net.post(self.directory.new_authz, new_authz) # TODO: handle errors assert response.status_code == http_client.CREATED return self._authzr_from_response(response, identifier) - def request_domain_challenges(self, domain, new_authzr_uri=None): + def request_domain_challenges(self, domain): """Request challenges for domain names. This is simply a convenience function that wraps around @@ -225,7 +205,7 @@ class Client(object): # pylint: disable=too-many-instance-attributes """ return self.request_challenges(messages.Identifier( - typ=messages.IDENTIFIER_FQDN, value=domain), new_authzr_uri) + typ=messages.IDENTIFIER_FQDN, value=domain)) def answer_challenge(self, challb, response): """Answer challenge. @@ -300,7 +280,7 @@ class Client(object): # pylint: disable=too-many-instance-attributes """ response = self.net.get(authzr.uri) updated_authzr = self._authzr_from_response( - response, authzr.body.identifier, authzr.uri, authzr.new_cert_uri) + response, authzr.body.identifier, authzr.uri) # TODO: check and raise UnexpectedUpdate return updated_authzr, response @@ -324,7 +304,7 @@ class Client(object): # pylint: disable=too-many-instance-attributes content_type = DER_CONTENT_TYPE # TODO: add 'cert_type 'argument response = self.net.post( - authzrs[0].new_cert_uri, # TODO: acme-spec #90 + self.directory.new_cert, req, content_type=content_type, headers={'Accept': content_type}) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 179a8a08c..0f4506203 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -40,6 +40,8 @@ class ClientTest(unittest.TestCase): 'https://www.letsencrypt-demo.org/acme/revoke-cert', messages.NewAuthorization: 'https://www.letsencrypt-demo.org/acme/new-authz', + messages.CertificateRequest: + 'https://www.letsencrypt-demo.org/acme/new-cert', }) from acme.client import Client @@ -56,7 +58,6 @@ class ClientTest(unittest.TestCase): self.new_reg = messages.NewRegistration(**dict(reg)) self.regr = messages.RegistrationResource( body=reg, uri='https://www.letsencrypt-demo.org/acme/reg/1', - new_authzr_uri='https://www.letsencrypt-demo.org/acme/new-reg', terms_of_service='https://www.letsencrypt-demo.org/tos') # Authorization @@ -72,8 +73,7 @@ class ClientTest(unittest.TestCase): typ=messages.IDENTIFIER_FQDN, value='example.com'), challenges=(challb,), combinations=None) self.authzr = messages.AuthorizationResource( - body=self.authz, uri=authzr_uri, - new_cert_uri='https://www.letsencrypt-demo.org/acme/new-cert') + body=self.authz, uri=authzr_uri) # Request issuance self.certr = messages.CertificateResource( @@ -98,18 +98,12 @@ class ClientTest(unittest.TestCase): self.response.json.return_value = self.regr.body.to_json() self.response.headers['Location'] = self.regr.uri self.response.links.update({ - 'next': {'url': self.regr.new_authzr_uri}, 'terms-of-service': {'url': self.regr.terms_of_service}, }) self.assertEqual(self.regr, self.client.register(self.new_reg)) # TODO: test POST call arguments - def test_register_missing_next(self): - self.response.status_code = http_client.CREATED - self.assertRaises( - errors.ClientError, self.client.register, self.new_reg) - def test_update_registration(self): # "Instance of 'Field' has no to_json/update member" bug: # pylint: disable=no-member @@ -142,13 +136,6 @@ class ClientTest(unittest.TestCase): self.response.json.return_value = self.regr.body.to_json() self.assertEqual(self.regr, self.client.query_registration(self.regr)) - def test_query_registration_updates_new_authzr_uri(self): - self.response.json.return_value = self.regr.body.to_json() - self.response.links = {'next': {'url': 'UPDATED'}} - self.assertEqual( - 'UPDATED', - self.client.query_registration(self.regr).new_authzr_uri) - def test_agree_to_tos(self): self.client.update_registration = mock.Mock() self.client.agree_to_tos(self.regr) @@ -159,9 +146,6 @@ class ClientTest(unittest.TestCase): self.response.status_code = http_client.CREATED self.response.headers['Location'] = self.authzr.uri self.response.json.return_value = self.authz.to_json() - self.response.links = { - 'next': {'url': self.authzr.new_cert_uri}, - } def test_request_challenges(self): self._prepare_response_for_request_challenges() @@ -172,8 +156,9 @@ class ClientTest(unittest.TestCase): def test_request_challenges_custom_uri(self): self._prepare_response_for_request_challenges() - self.client.request_challenges(self.identifier, 'URI') - self.net.post.assert_called_once_with('URI', mock.ANY) + self.client.request_challenges(self.identifier) + self.net.post.assert_called_once_with( + 'https://www.letsencrypt-demo.org/acme/new-authz', mock.ANY) def test_request_challenges_unexpected_update(self): self._prepare_response_for_request_challenges() @@ -181,12 +166,7 @@ class ClientTest(unittest.TestCase): identifier=self.identifier.update(value='foo')).to_json() self.assertRaises( errors.UnexpectedUpdate, self.client.request_challenges, - self.identifier, self.authzr.uri) - - def test_request_challenges_missing_next(self): - self.response.status_code = http_client.CREATED - self.assertRaises(errors.ClientError, self.client.request_challenges, - self.identifier) + self.identifier) def test_request_domain_challenges(self): self.client.request_challenges = mock.MagicMock() @@ -194,12 +174,6 @@ class ClientTest(unittest.TestCase): self.client.request_challenges(self.identifier), self.client.request_domain_challenges('example.com')) - def test_request_domain_challenges_custom_uri(self): - self.client.request_challenges = mock.MagicMock() - self.assertEqual( - self.client.request_challenges(self.identifier, 'URI'), - self.client.request_domain_challenges('example.com', 'URI')) - def test_answer_challenge(self): self.response.links['up'] = {'url': self.challr.authzr_uri} self.response.json.return_value = self.challr.body.to_json() diff --git a/acme/acme/jose/json_util.py b/acme/acme/jose/json_util.py index d474f4aac..4baadda5e 100644 --- a/acme/acme/jose/json_util.py +++ b/acme/acme/jose/json_util.py @@ -267,7 +267,7 @@ class JSONObjectWithFields(util.ImmutableMap, interfaces.JSONDeSerializable): if missing: raise errors.DeserializationError( - 'The following field are required: {0}'.format( + 'The following fields are required: {0}'.format( ','.join(missing))) @classmethod diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 54cd25c94..c3df4998c 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -191,7 +191,7 @@ class Directory(jose.JSONDeSerializable): try: return self[name.replace('_', '-')] except KeyError as error: - raise AttributeError(str(error)) + raise AttributeError(str(error) + ': ' + name) def __getitem__(self, name): try: @@ -315,12 +315,10 @@ class RegistrationResource(ResourceWithURI): """Registration Resource. :ivar acme.messages.Registration body: - :ivar unicode new_authzr_uri: URI found in the 'next' ``Link`` header :ivar unicode terms_of_service: URL for the CA TOS. """ body = jose.Field('body', decoder=Registration.from_json) - new_authzr_uri = jose.Field('new_authzr_uri') terms_of_service = jose.Field('terms_of_service', omitempty=True) @@ -425,11 +423,9 @@ class AuthorizationResource(ResourceWithURI): """Authorization Resource. :ivar acme.messages.Authorization body: - :ivar unicode new_cert_uri: URI found in the 'next' ``Link`` header """ body = jose.Field('body', decoder=Authorization.from_json) - new_cert_uri = jose.Field('new_cert_uri') @Directory.register diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py index b3454f25b..e84c3e992 100644 --- a/acme/acme/messages_test.py +++ b/acme/acme/messages_test.py @@ -225,14 +225,12 @@ class RegistrationResourceTest(unittest.TestCase): from acme.messages import RegistrationResource self.regr = RegistrationResource( body=mock.sentinel.body, uri=mock.sentinel.uri, - new_authzr_uri=mock.sentinel.new_authzr_uri, terms_of_service=mock.sentinel.terms_of_service) def test_to_partial_json(self): self.assertEqual(self.regr.to_json(), { 'body': mock.sentinel.body, 'uri': mock.sentinel.uri, - 'new_authzr_uri': mock.sentinel.new_authzr_uri, 'terms_of_service': mock.sentinel.terms_of_service, }) @@ -346,9 +344,7 @@ class AuthorizationResourceTest(unittest.TestCase): from acme.messages import AuthorizationResource authzr = AuthorizationResource( uri=mock.sentinel.uri, - body=mock.sentinel.body, - new_cert_uri=mock.sentinel.new_cert_uri, - ) + body=mock.sentinel.body) self.assertTrue(isinstance(authzr, jose.JSONDeSerializable)) diff --git a/acme/examples/example_client.py b/acme/examples/example_client.py index 261b37603..1386491b1 100644 --- a/acme/examples/example_client.py +++ b/acme/examples/example_client.py @@ -32,8 +32,7 @@ acme.agree_to_tos(regr) logging.debug(regr) authzr = acme.request_challenges( - identifier=messages.Identifier(typ=messages.IDENTIFIER_FQDN, value=DOMAIN), - new_authzr_uri=regr.new_authzr_uri) + identifier=messages.Identifier(typ=messages.IDENTIFIER_FQDN, value=DOMAIN)) logging.debug(authzr) authzr, authzr_response = acme.poll(authzr) diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 6e9ab25a7..53346a77c 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -63,8 +63,7 @@ class AuthHandler(object): """ for domain in domains: - self.authzr[domain] = self.acme.request_domain_challenges( - domain, self.account.regr.new_authzr_uri) + self.authzr[domain] = self.acme.request_domain_challenges(domain) self._choose_challenges(domains) diff --git a/certbot/tests/account_test.py b/certbot/tests/account_test.py index 8ed591c98..7367717bf 100644 --- a/certbot/tests/account_test.py +++ b/certbot/tests/account_test.py @@ -110,7 +110,7 @@ class AccountFileStorageTest(unittest.TestCase): from certbot.account import Account self.acc = Account( regr=messages.RegistrationResource( - uri=None, new_authzr_uri=None, body=messages.Registration()), + uri=None, body=messages.Registration()), key=KEY) def tearDown(self): diff --git a/certbot/tests/acme_util.py b/certbot/tests/acme_util.py index 5e6b190a7..f0549666a 100644 --- a/certbot/tests/acme_util.py +++ b/certbot/tests/acme_util.py @@ -96,6 +96,5 @@ def gen_authzr(authz_status, domain, challs, statuses, combos=True): # pylint: disable=star-args return messages.AuthorizationResource( uri="https://trusted.ca/new-authz-resource", - new_cert_uri="https://trusted.ca/new-cert", body=messages.Authorization(**authz_kwargs) ) diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index 046eb5ef1..9d22843db 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -309,7 +309,6 @@ class PollChallengesTest(unittest.TestCase): new_authzr = messages.AuthorizationResource( uri=authzr.uri, - new_cert_uri=authzr.new_cert_uri, body=messages.Authorization( identifier=authzr.body.identifier, challenges=new_challbs, @@ -437,7 +436,7 @@ def gen_auth_resp(chall_list): for chall in chall_list] -def gen_dom_authzr(domain, unused_new_authzr_uri, challs, combos=True): +def gen_dom_authzr(domain, challs, combos=True): """Generates new authzr for domains.""" return acme_util.gen_authzr( messages.STATUS_PENDING, domain, challs, diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index f6de33a92..f2a9b3d07 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -104,10 +104,10 @@ class ChooseAccountTest(unittest.TestCase): self.key = KEY self.acc1 = account.Account(messages.RegistrationResource( - uri=None, new_authzr_uri=None, body=messages.Registration.from_data( + uri=None, body=messages.Registration.from_data( email="email1@g.com")), self.key) self.acc2 = account.Account(messages.RegistrationResource( - uri=None, new_authzr_uri=None, body=messages.Registration.from_data( + uri=None, body=messages.Registration.from_data( email="email2@g.com", phone="phone")), self.key) @classmethod