1
0
mirror of https://github.com/certbot/certbot.git synced 2026-01-27 19:42:53 +03:00

Remove Link rel=next for authzs and new-certs. (#4303)

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.

* Revert "Revert "Remove Link rel=next for authzs and new-certs." (#4277)"

This reverts commit 11ec1eb911.

* Save new_authzr_uri with account for older clients.

* Add test that new_authzr_uri exists in regr.

* Restore backwards compatibility for new_authzr_uri.

* Fix account_test.

* Add test for deprecated URI argument to request_challenges.

* Review feedback.

* Fix test

* Add omitempty to new_cert_uri.
This commit is contained in:
Jacob Hoffman-Andrews
2017-03-14 21:44:57 -07:00
committed by Brad Warren
parent f11d7b3f0c
commit 018a304cd6
16 changed files with 97 additions and 105 deletions

View File

@@ -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):
@@ -172,19 +165,10 @@ 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
@@ -193,17 +177,16 @@ class Client(object): # pylint: disable=too-many-instance-attributes
"""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``.
:param str new_authzr_uri: Deprecated. Do not use.
:returns: Authorization Resource.
:rtype: `.AuthorizationResource`
"""
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
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)
@@ -217,6 +200,7 @@ class Client(object): # pylint: disable=too-many-instance-attributes
documentation.
:param str domain: Domain name to be challenged.
:param str new_authzr_uri: Deprecated. Do not use.
:returns: Authorization Resource.
:rtype: `.AuthorizationResource`
@@ -298,7 +282,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)
return updated_authzr, response
def request_issuance(self, csr, authzrs):
@@ -321,7 +305,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})

View File

@@ -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
@@ -132,13 +126,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)
@@ -149,9 +136,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()
@@ -160,10 +144,18 @@ class ClientTest(unittest.TestCase):
self.directory.new_authz,
messages.NewAuthorization(identifier=self.identifier))
def test_request_challenges_deprecated_arg(self):
self._prepare_response_for_request_challenges()
self.client.request_challenges(self.identifier, new_authzr_uri="hi")
self.net.post.assert_called_once_with(
self.directory.new_authz,
messages.NewAuthorization(identifier=self.identifier))
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()
@@ -171,12 +163,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()
@@ -184,12 +171,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()

View File

@@ -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

View File

@@ -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,12 @@ class RegistrationResource(ResourceWithURI):
"""Registration Resource.
:ivar acme.messages.Registration body:
:ivar unicode new_authzr_uri: URI found in the 'next' ``Link`` header
:ivar unicode new_authzr_uri: Deprecated. Do not use.
: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')
new_authzr_uri = jose.Field('new_authzr_uri', omitempty=True)
terms_of_service = jose.Field('terms_of_service', omitempty=True)
@@ -425,11 +425,11 @@ class AuthorizationResource(ResourceWithURI):
"""Authorization Resource.
:ivar acme.messages.Authorization body:
:ivar unicode new_cert_uri: URI found in the 'next' ``Link`` header
:ivar unicode new_cert_uri: Deprecated. Do not use.
"""
body = jose.Field('body', decoder=Authorization.from_json)
new_cert_uri = jose.Field('new_cert_uri')
new_cert_uri = jose.Field('new_cert_uri', omitempty=True)
@Directory.register

View File

@@ -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))

View File

@@ -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)

View File

@@ -98,7 +98,7 @@ def report_new_account(config):
class AccountMemoryStorage(interfaces.AccountStorage):
"""In-memory account strage."""
"""In-memory account storage."""
def __init__(self, initial_accounts=None):
self.accounts = initial_accounts if initial_accounts is not None else {}
@@ -106,7 +106,8 @@ class AccountMemoryStorage(interfaces.AccountStorage):
def find_all(self):
return list(six.itervalues(self.accounts))
def save(self, account):
def save(self, account, acme):
# pylint: disable=unused-argument
if account.id in self.accounts:
logger.debug("Overwriting account: %s", account.id)
self.accounts[account.id] = account
@@ -117,6 +118,16 @@ class AccountMemoryStorage(interfaces.AccountStorage):
except KeyError:
raise errors.AccountNotFound(account_id)
class RegistrationResourceWithNewAuthzrURI(messages.RegistrationResource):
"""A backwards-compatible RegistrationResource with a new-authz URI.
Hack: Certbot versions pre-0.11.1 expect to load
new_authzr_uri as part of the account. Because people
sometimes switch between old and new versions, we will
continue to write out this field for some time so older
clients don't crash in that scenario.
"""
new_authzr_uri = jose.Field('new_authzr_uri')
class AccountFileStorage(interfaces.AccountStorage):
"""Accounts file storage.
@@ -181,16 +192,16 @@ class AccountFileStorage(interfaces.AccountStorage):
account_id, acc.id))
return acc
def save(self, account):
self._save(account, regr_only=False)
def save(self, account, acme):
self._save(account, acme, regr_only=False)
def save_regr(self, account):
def save_regr(self, account, acme):
"""Save the registration resource.
:param Account account: account whose regr should be saved
"""
self._save(account, regr_only=True)
self._save(account, acme, regr_only=True)
def delete(self, account_id):
"""Delete registration info from disk
@@ -204,13 +215,19 @@ class AccountFileStorage(interfaces.AccountStorage):
"Account at %s does not exist" % account_dir_path)
shutil.rmtree(account_dir_path)
def _save(self, account, regr_only):
def _save(self, account, acme, regr_only):
account_dir_path = self._account_dir_path(account.id)
util.make_or_verify_dir(account_dir_path, 0o700, os.geteuid(),
self.config.strict_permissions)
try:
with open(self._regr_path(account_dir_path), "w") as regr_file:
regr_file.write(account.regr.json_dumps())
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 not regr_only:
with util.safe_open(self._key_path(account_dir_path),
"w", chmod=0o400) as key_file:

View File

@@ -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)

View File

@@ -138,7 +138,7 @@ def register(config, account_storage, tos_cb=None):
acc = account.Account(regr, key)
account.report_new_account(config)
account_storage.save(acc)
account_storage.save(acc, acme)
eff.handle_subscription(config)

View File

@@ -32,7 +32,7 @@ class AccountStorage(object):
raise NotImplementedError()
@abc.abstractmethod
def save(self, account): # pragma: no cover
def save(self, account, client): # pragma: no cover
"""Save account.
:raises .AccountStorageError: if account could not be saved

View File

@@ -466,7 +466,7 @@ def register(config, unused_plugins):
# We rely on an exception to interrupt this process if it didn't work.
acc.regr = acme_client.acme.update_registration(acc.regr.update(
body=acc.regr.body.update(contact=('mailto:' + config.email,))))
account_storage.save_regr(acc)
account_storage.save_regr(acc, acme_client.acme)
eff.handle_subscription(config)
add_msg("Your e-mail address was updated to {0}.".format(config.email))

View File

@@ -1,5 +1,6 @@
"""Tests for certbot.account."""
import datetime
import json
import os
import shutil
import stat
@@ -90,10 +91,10 @@ class AccountMemoryStorageTest(unittest.TestCase):
account = mock.Mock(id="x")
self.assertEqual([], self.storage.find_all())
self.assertRaises(errors.AccountNotFound, self.storage.load, "x")
self.storage.save(account)
self.storage.save(account, None)
self.assertEqual([account], self.storage.find_all())
self.assertEqual(account, self.storage.load("x"))
self.storage.save(account)
self.storage.save(account, None)
self.assertEqual([account], self.storage.find_all())
@@ -108,10 +109,14 @@ class AccountFileStorageTest(unittest.TestCase):
self.storage = AccountFileStorage(self.config)
from certbot.account import Account
new_authzr_uri = "hi"
self.acc = Account(
regr=messages.RegistrationResource(
uri=None, new_authzr_uri=None, body=messages.Registration()),
uri=None, body=messages.Registration(),
new_authzr_uri=new_authzr_uri),
key=KEY)
self.mock_client = mock.MagicMock()
self.mock_client.directory.new_authz = new_authzr_uri
def tearDown(self):
shutil.rmtree(self.tmp)
@@ -120,7 +125,7 @@ class AccountFileStorageTest(unittest.TestCase):
self.assertTrue(os.path.isdir(self.config.accounts_dir))
def test_save_and_restore(self):
self.storage.save(self.acc)
self.storage.save(self.acc, self.mock_client)
account_path = os.path.join(self.config.accounts_dir, self.acc.id)
self.assertTrue(os.path.exists(account_path))
for file_name in "regr.json", "meta.json", "private_key.json":
@@ -130,10 +135,19 @@ class AccountFileStorageTest(unittest.TestCase):
account_path, "private_key.json"))[stat.ST_MODE] & 0o777) in ("0400", "0o400"))
# restore
self.assertEqual(self.acc, self.storage.load(self.acc.id))
loaded = self.storage.load(self.acc.id)
self.assertEqual(self.acc, loaded)
def test_save_and_restore_old_version(self):
"""Saved regr should include a new_authzr_uri for older Certbots"""
self.storage.save(self.acc, self.mock_client)
path = os.path.join(self.config.accounts_dir, self.acc.id, "regr.json")
with open(path, "r") as f:
regr = json.load(f)
self.assertTrue("new_authzr_uri" in regr)
def test_save_regr(self):
self.storage.save_regr(self.acc)
self.storage.save_regr(self.acc, self.mock_client)
account_path = os.path.join(self.config.accounts_dir, self.acc.id)
self.assertTrue(os.path.exists(account_path))
self.assertTrue(os.path.exists(os.path.join(
@@ -143,7 +157,7 @@ class AccountFileStorageTest(unittest.TestCase):
os.path.join(account_path, file_name)))
def test_find_all(self):
self.storage.save(self.acc)
self.storage.save(self.acc, self.mock_client)
self.assertEqual([self.acc], self.storage.find_all())
def test_find_all_none_empty_list(self):
@@ -164,14 +178,14 @@ class AccountFileStorageTest(unittest.TestCase):
self.assertRaises(errors.AccountNotFound, self.storage.load, "missing")
def test_load_id_mismatch_raises_error(self):
self.storage.save(self.acc)
self.storage.save(self.acc, self.mock_client)
shutil.move(os.path.join(self.config.accounts_dir, self.acc.id),
os.path.join(self.config.accounts_dir, "x" + self.acc.id))
self.assertRaises(errors.AccountStorageError, self.storage.load,
"x" + self.acc.id)
def test_load_ioerror(self):
self.storage.save(self.acc)
self.storage.save(self.acc, self.mock_client)
mock_open = mock.mock_open()
mock_open.side_effect = IOError
with mock.patch("six.moves.builtins.open", mock_open):
@@ -183,10 +197,11 @@ class AccountFileStorageTest(unittest.TestCase):
mock_open.side_effect = IOError # TODO: [None, None, IOError]
with mock.patch("six.moves.builtins.open", mock_open):
self.assertRaises(
errors.AccountStorageError, self.storage.save, self.acc)
errors.AccountStorageError, self.storage.save,
self.acc, self.mock_client)
def test_delete(self):
self.storage.save(self.acc)
self.storage.save(self.acc, self.mock_client)
self.storage.delete(self.acc.id)
self.assertRaises(errors.AccountNotFound, self.storage.load, self.acc.id)

View File

@@ -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)
)

View File

@@ -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,

View File

@@ -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

View File

@@ -382,6 +382,9 @@ class DetermineAccountTest(unittest.TestCase):
self.config = configuration.NamespaceConfig(self.args)
self.accs = [mock.MagicMock(id='x'), mock.MagicMock(id='y')]
self.account_storage = account.AccountMemoryStorage()
# For use in saving accounts: fake out the new_authz URL.
self.mock_client = mock.MagicMock()
self.mock_client.directory.new_authz = "hi"
def _call(self):
# pylint: disable=protected-access
@@ -391,14 +394,14 @@ class DetermineAccountTest(unittest.TestCase):
return _determine_account(self.config)
def test_args_account_set(self):
self.account_storage.save(self.accs[1])
self.account_storage.save(self.accs[1], self.mock_client)
self.config.account = self.accs[1].id
self.assertEqual((self.accs[1], None), self._call())
self.assertEqual(self.accs[1].id, self.config.account)
self.assertTrue(self.config.email is None)
def test_single_account(self):
self.account_storage.save(self.accs[0])
self.account_storage.save(self.accs[0], self.mock_client)
self.assertEqual((self.accs[0], None), self._call())
self.assertEqual(self.accs[0].id, self.config.account)
self.assertTrue(self.config.email is None)
@@ -406,7 +409,7 @@ class DetermineAccountTest(unittest.TestCase):
@mock.patch('certbot.client.display_ops.choose_account')
def test_multiple_accounts(self, mock_choose_accounts):
for acc in self.accs:
self.account_storage.save(acc)
self.account_storage.save(acc, self.mock_client)
mock_choose_accounts.return_value = self.accs[1]
self.assertEqual((self.accs[1], None), self._call())
self.assertEqual(