From 860af81fef3042134b2b71a6b71b4240c75b1886 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Fri, 19 Jun 2020 00:58:19 +0200 Subject: [PATCH] Deferred EFF subscription until the first certificate is successfully issued (#8076) * Base logic * Various controls when email is None * Adapt eff tests * Forward compatibility * Also for csr * Explicit regr or meta updates in account objects * Adapt logic to ask for eff subscription during registering * Adapt tests * Move dry-run control * Add some relevant controls on handle_subscription call checks --- certbot/certbot/_internal/account.py | 112 +++++++++++++++++-------- certbot/certbot/_internal/client.py | 3 +- certbot/certbot/_internal/eff.py | 70 ++++++++++++---- certbot/certbot/_internal/main.py | 10 ++- certbot/tests/account_test.py | 49 +++++++++-- certbot/tests/client_test.py | 15 ++-- certbot/tests/eff_test.py | 118 +++++++++++++++++---------- certbot/tests/main_test.py | 64 +++++++++------ 8 files changed, 303 insertions(+), 138 deletions(-) diff --git a/certbot/certbot/_internal/account.py b/certbot/certbot/_internal/account.py index 61f63bda6..c36559032 100644 --- a/certbot/certbot/_internal/account.py +++ b/certbot/certbot/_internal/account.py @@ -15,6 +15,7 @@ import zope.component from acme import fields as acme_fields from acme import messages +from acme.client import ClientBase # pylint: disable=unused-import from certbot import errors from certbot import interfaces from certbot import util @@ -39,6 +40,8 @@ class Account(object): :ivar datetime.datetime creation_dt: Creation date and time (UTC). :ivar str creation_host: FQDN of host, where account has been created. + :ivar str register_to_eff: If not None, Certbot will register the provided + email during the account registration. .. note:: ``creation_dt`` and ``creation_host`` are useful in cross-machine migration scenarios. @@ -46,15 +49,16 @@ class Account(object): """ creation_dt = acme_fields.RFC3339Field("creation_dt") creation_host = jose.Field("creation_host") + register_to_eff = jose.Field("register_to_eff", omitempty=True) def __init__(self, regr, key, meta=None): self.key = key self.regr = regr self.meta = self.Meta( # pyrfc3339 drops microseconds, make sure __eq__ is sane - creation_dt=datetime.datetime.now( - tz=pytz.UTC).replace(microsecond=0), - creation_host=socket.getfqdn()) if meta is None else meta + creation_dt=datetime.datetime.now(tz=pytz.UTC).replace(microsecond=0), + creation_host=socket.getfqdn(), + register_to_eff=None) if meta is None else meta # try MD5, else use MD5 in non-security mode (e.g. for FIPS systems / RHEL) try: @@ -242,15 +246,47 @@ class AccountFileStorage(interfaces.AccountStorage): return self._load_for_server_path(account_id, self.config.server_path) def save(self, account, client): - self._save(account, client, regr_only=False) + # type: (Account, ClientBase) -> None + """Create a new account. - def save_regr(self, account, acme): - """Save the registration resource. - - :param Account account: account whose regr should be saved + :param Account account: account to create + :param ClientBase client: ACME client associated to the account """ - self._save(account, acme, regr_only=True) + try: + dir_path = self._prepare(account) + self._create(account, dir_path) + self._update_meta(account, dir_path) + self._update_regr(account, client, dir_path) + except IOError as error: + raise errors.AccountStorageError(error) + + def update_regr(self, account, client): + # type: (Account, ClientBase) -> None + """Update the registration resource. + + :param Account account: account to update + :param ClientBase client: ACME client associated to the account + + """ + try: + dir_path = self._prepare(account) + self._update_regr(account, client, dir_path) + except IOError as error: + raise errors.AccountStorageError(error) + + def update_meta(self, account): + # type: (Account) -> None + """Update the meta resource. + + :param Account account: account to update + + """ + try: + dir_path = self._prepare(account) + self._update_meta(account, dir_path) + except IOError as error: + raise errors.AccountStorageError(error) def delete(self, account_id): """Delete registration info from disk @@ -318,32 +354,36 @@ class AccountFileStorage(interfaces.AccountStorage): return dir_path - def _save(self, account, acme, regr_only): + def _prepare(self, account): + # type: (Account) -> str account_dir_path = self._account_dir_path(account.id) util.make_or_verify_dir(account_dir_path, 0o700, self.config.strict_permissions) - try: - with open(self._regr_path(account_dir_path), "w") as regr_file: - regr = account.regr - # 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={}, - uri=regr.uri) - else: - regr = messages.RegistrationResource( - body={}, - uri=regr.uri) - 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: - key_file.write(account.key.json_dumps()) - with open(self._metadata_path( - account_dir_path), "w") as metadata_file: - metadata_file.write(account.meta.json_dumps()) - except IOError as error: - raise errors.AccountStorageError(error) + return account_dir_path + + def _create(self, account, dir_path): + # type: (Account, str) -> None + with util.safe_open(self._key_path(dir_path), "w", chmod=0o400) as key_file: + key_file.write(account.key.json_dumps()) + + def _update_regr(self, account, acme, dir_path): + # type: (Account, ClientBase, str) -> None + with open(self._regr_path(dir_path), "w") as regr_file: + regr = account.regr + # 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={}, + uri=regr.uri) + else: + regr = messages.RegistrationResource( + body={}, + uri=regr.uri) + regr_file.write(regr.json_dumps()) + + def _update_meta(self, account, dir_path): + with open(self._metadata_path(dir_path), "w") as metadata_file: + metadata_file.write(account.meta.json_dumps()) diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index a9bf946cc..039e246de 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -178,7 +178,7 @@ def register(config, account_storage, tos_cb=None): account.report_new_account(config) account_storage.save(acc, acme) - eff.handle_subscription(config) + eff.prepare_subscription(config, acc) return acc, acme @@ -389,6 +389,7 @@ class Client(object): authzr = self.auth_handler.handle_authorizations(orderr, best_effort) return orderr.update(authorizations=authzr) + def obtain_and_enroll_certificate(self, domains, certname): """Obtain and enroll certificate. diff --git a/certbot/certbot/_internal/eff.py b/certbot/certbot/_internal/eff.py index 586697dbb..ae8690821 100644 --- a/certbot/certbot/_internal/eff.py +++ b/certbot/certbot/_internal/eff.py @@ -4,32 +4,68 @@ import logging import requests import zope.component +from acme.magic_typing import Optional # pylint: disable=unused-import + from certbot import interfaces from certbot._internal import constants +from certbot._internal.account import Account # pylint: disable=unused-import +from certbot._internal.account import AccountFileStorage +from certbot.interfaces import IConfig # pylint: disable=unused-import logger = logging.getLogger(__name__) -def handle_subscription(config): - """High level function to take care of EFF newsletter subscriptions. +def prepare_subscription(config, acc): + # type: (IConfig, Account) -> None + """High level function to store potential EFF newsletter subscriptions. The user may be asked if they want to sign up for the newsletter if - they have not already specified. + they have not given their explicit approval or refusal using --eff-mail + or --no-eff-mail flag. - :param .IConfig config: Client configuration. + Decision about EFF subscription will be stored in the account metadata. + + :param IConfig config: Client configuration. + :param Account acc: Current client account. """ - if config.email is None: - if config.eff_email: - _report_failure("you didn't provide an e-mail address") + if config.eff_email is False: return - if config.eff_email is None: - config.eff_email = _want_subscription() - if config.eff_email: - subscribe(config.email) + if config.eff_email is True: + if config.email is None: + _report_failure("you didn't provide an e-mail address") + else: + acc.meta = acc.meta.update(register_to_eff=config.email) + elif config.email and _want_subscription(): + acc.meta = acc.meta.update(register_to_eff=config.email) + + if acc.meta.register_to_eff: + storage = AccountFileStorage(config) + storage.update_meta(acc) + + +def handle_subscription(config, acc): + # type: (IConfig, Account) -> None + """High level function to take care of EFF newsletter subscriptions. + + Once subscription is handled, it will not be handled again. + + :param IConfig config: Client configuration. + :param Account acc: Current client account. + + """ + if config.dry_run: + return + if acc.meta.register_to_eff: + subscribe(acc.meta.register_to_eff) + + acc.meta = acc.meta.update(register_to_eff=None) + storage = AccountFileStorage(config) + storage.update_meta(acc) def _want_subscription(): + # type: () -> bool """Does the user want to be subscribed to the EFF newsletter? :returns: True if we should subscribe the user, otherwise, False @@ -37,16 +73,17 @@ def _want_subscription(): """ prompt = ( - 'Would you be willing to share your email address with the ' - "Electronic Frontier Foundation, a founding partner of the Let's " - 'Encrypt project and the non-profit organization that develops ' - "Certbot? We'd like to send you email about our work encrypting " + 'Would you be willing, once your first certificate is successfully issued, ' + 'to share your email address with the Electronic Frontier Foundation, a ' + "founding partner of the Let's Encrypt project and the non-profit organization " + "that develops Certbot? We'd like to send you email about our work encrypting " "the web, EFF news, campaigns, and ways to support digital freedom. ") display = zope.component.getUtility(interfaces.IDisplay) return display.yesno(prompt, default=False) def subscribe(email): + # type: (str) -> None """Subscribe the user to the EFF mailing list. :param str email: the e-mail address to subscribe @@ -56,11 +93,13 @@ def subscribe(email): data = {'data_type': 'json', 'email': email, 'form_id': 'eff_supporters_library_subscribe_form'} + logger.info('Subscribe to the EFF mailing list (email: %s).', email) logger.debug('Sending POST request to %s:\n%s', url, data) _check_response(requests.post(url, data=data)) def _check_response(response): + # type: (requests.Response) -> None """Check for errors in the server's response. If an error occurred, it will be reported to the user. @@ -81,6 +120,7 @@ def _check_response(response): def _report_failure(reason=None): + # type: (Optional[str]) -> None """Notify the user of failing to sign them up for the newsletter. :param reason: a phrase describing what the problem was diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 264f9667e..cd00297a9 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -721,11 +721,12 @@ def update_account(config, unused_plugins): # the v2 uri. Since it's the same object on disk, put it back to the v1 uri # so that we can also continue to use the account object with acmev1. acc.regr = acc.regr.update(uri=prev_regr_uri) - account_storage.save_regr(acc, cb_client.acme) - eff.handle_subscription(config) + account_storage.update_regr(acc, cb_client.acme) + eff.prepare_subscription(config, acc) add_msg("Your e-mail address was updated to {0}.".format(config.email)) return None + def _install_cert(config, le_client, domains, lineage=None): """Install a cert @@ -1116,6 +1117,7 @@ def run(config, plugins): display_ops.success_renewal(domains) _suggest_donation_if_appropriate(config) + eff.handle_subscription(config, le_client.account) return None @@ -1189,6 +1191,7 @@ def renew_cert(config, plugins, lineage): notify("new certificate deployed with reload of {0} server; fullchain is {1}".format( config.installer, lineage.fullchain), pause=False) + def certonly(config, plugins): """Authenticate & obtain cert, but do not install it. @@ -1220,6 +1223,7 @@ def certonly(config, plugins): cert_path, fullchain_path = _csr_get_and_save_cert(config, le_client) _report_new_cert(config, cert_path, fullchain_path) _suggest_donation_if_appropriate(config) + eff.handle_subscription(config, le_client.account) return domains, certname = _find_domains_or_certname(config, installer) @@ -1237,6 +1241,8 @@ def certonly(config, plugins): key_path = lineage.key_path if lineage else None _report_new_cert(config, cert_path, fullchain_path, key_path) _suggest_donation_if_appropriate(config) + eff.handle_subscription(config, le_client.account) + def renew(config, unused_plugins): """Renew previously-obtained certificates. diff --git a/certbot/tests/account_test.py b/certbot/tests/account_test.py index 6c6e6c860..6d215eed1 100644 --- a/certbot/tests/account_test.py +++ b/certbot/tests/account_test.py @@ -17,6 +17,7 @@ from certbot.compat import misc from certbot.compat import os import certbot.tests.util as test_util + KEY = jose.JWKRSA.load(test_util.load_vector("rsa512_key.pem")) @@ -56,6 +57,32 @@ class AccountTest(unittest.TestCase): self.assertTrue(repr(self.acc).startswith( "