From e8518bf206020bc0163f7db33eb7b31126bac6a2 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 26 Aug 2020 14:03:15 -0700 Subject: [PATCH 1/8] Fix finding Augeas in the ARM snaps (#8230) * Find Augeas on all architectures. * Add changelog entry. * add comment --- certbot.wrapper | 31 +++++++++++++++++++++++++++++++ certbot/CHANGELOG.md | 3 ++- snap/snapcraft.yaml | 6 ++---- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/certbot.wrapper b/certbot.wrapper index c779e9939..909f2e369 100755 --- a/certbot.wrapper +++ b/certbot.wrapper @@ -1,5 +1,36 @@ #!/bin/bash set -e + +# This code is based on snapcraft's own patch to work around this problem at +# https://github.com/snapcore/snapcraft/blob/a97fb5c7ea553a1bd20f4887a7c3393e75761890/patches/ctypes_init.diff. +# We may not build the Certbot snap for all of these architectures (and as of +# writing this we do not), but we keep the code for them to avoid having to +# solve this problem again in the future if we add support for new +# architectures. +case "${SNAP_ARCH}" in + 'arm64') + ARCH_TRIPLET='aarch64-linux-gnu';; + 'armhf') + ARCH_TRIPLET='arm-linux-gnueabihf';; + 'i386') + ARCH_TRIPLET='i386-linux-gnu';; + 'ppc64el') + ARCH_TRIPLET='powerpc64le-linux-gnu';; + 'powerpc') + ARCH_TRIPLET='powerpc-linux-gnu';; + 'amd64') + ARCH_TRIPLET='x86_64-linux-gnu';; + 's390x') + ARCH_TRIPLET='s390x-linux-gnu';; + *) + echo "Unrecongized value of SNAP_ARCH: ${SNAP_ARCH}" >&2 + exit 1 +esac + +PARTIAL_LIBRARY_PATH="${SNAP}/usr/lib/${ARCH_TRIPLET}/" +export LD_LIBRARY_PATH="${PARTIAL_LIBRARY_PATH}:${LD_LIBRARY_PATH}" +export CERTBOT_AUGEAS_PATH="${PARTIAL_LIBRARY_PATH}libaugeas.so.0" + join() { sep=$1 first=$2 diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 5ef2769a9..1ec9f1699 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -14,7 +14,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Fixed -* +* The problem causing the Apache plugin in the Certbot snap on ARM systems to + fail to load the Augeas library it depends on has been fixed. More details about these changes can be found on our GitHub repo. diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml index c2dab9180..b29899c31 100644 --- a/snap/snapcraft.yaml +++ b/snap/snapcraft.yaml @@ -24,7 +24,6 @@ apps: environment: PATH: "$SNAP/bin:$SNAP/usr/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games" AUGEAS_LENS_LIB: "$SNAP/usr/share/augeas/lenses/dist" - LD_LIBRARY_PATH: "$SNAP/usr/lib/x86_64-linux-gnu/:$LD_LIBRARY_PATH" CERTBOT_SNAPPED: "True" renew: command: certbot.wrapper -q renew @@ -32,7 +31,6 @@ apps: environment: PATH: "$SNAP/bin:$SNAP/usr/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games" AUGEAS_LENS_LIB: $SNAP/usr/share/augeas/lenses/dist - LD_LIBRARY_PATH: "$SNAP/usr/lib/x86_64-linux-gnu/:$LD_LIBRARY_PATH" CERTBOT_SNAPPED: "True" # Run approximately twice a day with randomization timer: 00:00~24:00/2 @@ -44,7 +42,7 @@ parts: source: . constraints: [$SNAPCRAFT_PART_SRC/snap-constraints.txt] python-packages: - - git+https://github.com/basak/python-augeas.git@snap + - git+https://github.com/certbot/python-augeas.git@certbot-patched - ./acme - ./certbot - ./certbot-apache @@ -54,7 +52,7 @@ parts: # Old versions of this file used to unstage # lib/python3.8/site-packages/augeas.py to avoid conflicts between # python-augeas 0.5.0 which was pinned in snap-constraints.txt and - # Robie's python-augeas fork which creates an auto-generated cffi file at + # our python-augeas fork which creates an auto-generated cffi file at # the same path. Since we've combined things in one part and removed the # python-augeas pinning, unstaging this file had a different, unintended # effect so we now stage the file to keep the auto-generated cffi file. From f66a592e37d9426edcd21a7db046043efa84536e Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 26 Aug 2020 14:04:37 -0700 Subject: [PATCH 2/8] Try switching to the buster ARM image. (#8234) --- tests/letstest/apache2_targets.yaml | 12 ++++++------ tests/letstest/scripts/test_apache2.sh | 2 +- tests/letstest/targets.yaml | 12 ++++++------ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/tests/letstest/apache2_targets.yaml b/tests/letstest/apache2_targets.yaml index d584705ca..2fa64568c 100644 --- a/tests/letstest/apache2_targets.yaml +++ b/tests/letstest/apache2_targets.yaml @@ -8,12 +8,6 @@ targets: type: ubuntu virt: hvm user: ubuntu - - ami: ami-008680ee60f23c94b - name: ubuntu20.04_arm64 - type: ubuntu - virt: hvm - user: ubuntu - machine_type: a1.medium - ami: ami-0545f7036167eb3aa name: ubuntu19.10 type: ubuntu @@ -36,6 +30,12 @@ targets: type: ubuntu virt: hvm user: admin + - ami: ami-0dcd54b7d2fff584f + name: debian10_arm64 + type: ubuntu + virt: hvm + user: admin + machine_type: a1.medium - ami: ami-003f19e0e687de1cd name: debian9 type: ubuntu diff --git a/tests/letstest/scripts/test_apache2.sh b/tests/letstest/scripts/test_apache2.sh index e39cee9ef..ba3d94379 100755 --- a/tests/letstest/scripts/test_apache2.sh +++ b/tests/letstest/scripts/test_apache2.sh @@ -12,7 +12,7 @@ then # For apache 2.4, set up ServerName sudo sed -i '/ServerName/ s/#ServerName/ServerName/' $CONFFILE sudo sed -i '/ServerName/ s/www.example.com/'$PUBLIC_HOSTNAME'/' $CONFFILE - if [ $(python3 -V 2>&1 | cut -d" " -f 2 | cut -d. -f1,2 | sed 's/\.//') -ne 38 ] + if [ $(python3 -V 2>&1 | cut -d" " -f 2 | cut -d. -f1,2 | sed 's/\.//') -lt 36 ] then # Upgrade python version using pyenv because py3.5 is deprecated # Don't upgrade if it's already 3.8 because pyenv doesn't work great on arm, and diff --git a/tests/letstest/targets.yaml b/tests/letstest/targets.yaml index 711053469..f6d3dd42f 100644 --- a/tests/letstest/targets.yaml +++ b/tests/letstest/targets.yaml @@ -8,12 +8,6 @@ targets: type: ubuntu virt: hvm user: ubuntu - - ami: ami-008680ee60f23c94b - name: ubuntu20.04_arm64 - type: ubuntu - virt: hvm - user: ubuntu - machine_type: a1.medium - ami: ami-0545f7036167eb3aa name: ubuntu19.10 type: ubuntu @@ -31,6 +25,12 @@ targets: type: ubuntu virt: hvm user: admin + - ami: ami-0dcd54b7d2fff584f + name: debian10_arm64 + type: ubuntu + virt: hvm + user: admin + machine_type: a1.medium #----------------------------------------------------------------------------- # Other Redhat Distros - ami: ami-0916c408cb02e310b From ae7b4a1755655b0bf87614ff6eb75531e6c454b0 Mon Sep 17 00:00:00 2001 From: Daniel Drexler Date: Wed, 26 Aug 2020 15:22:51 -0700 Subject: [PATCH 3/8] Support Register Unsafely in Update (#8212) * Allow user to remove email using update command Fixes #3162. Slight change to control flow to replace current email addresses with an empty list. Also add appropriate result message when an email is removed. * Update ACME to allow update to remove fields - New field type "UnFalseyField" that treats all non-None fields as non-empty - Contact changed to new field type to allow sending of empty contact field - Certbot update adjusted to use tuple instead of None when empty - Test updated to check more logic - Unrelated type hint added to keep pycharm gods happy * Moved some mocks into decorators * Restore default to `contact` but do not serialize - Add `to_partial_json` and `fields_to_partial_json` to Registration - Store private variable noting if the value of the `contact` field was provided by the user. - Change message when updating without email to reflect removal of all contact info. - Add note in changelog that `update_account` with the `--register-unsafely-without-email` flag will remove contact from an account. * Reverse logic for field handling on serialization Now forcably add contact when serilizing, but go back to base `jose` field type. * Responding to Review - change out of date name - update several comments - update `from_data` function of `Registration` - Update test to remove superfluous mock * Responding to review - Change comments to make from_data more clear - Remove code worried about None (omitempty has got my back) - Update test to be more reliable - Add typing import with comment to avoid pylint bug --- AUTHORS.md | 1 + acme/acme/messages.py | 56 +++++++++++++++++++++++++++++-- acme/tests/messages_test.py | 13 +++++++ certbot/CHANGELOG.md | 5 ++- certbot/certbot/_internal/main.py | 25 ++++++++------ certbot/tests/main_test.py | 37 ++++++++++++++++++++ 6 files changed, 124 insertions(+), 13 deletions(-) diff --git a/AUTHORS.md b/AUTHORS.md index 0cedcbd19..56fe2a3d8 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -61,6 +61,7 @@ Authors * [Daniel Albers](https://github.com/AID) * [Daniel Aleksandersen](https://github.com/da2x) * [Daniel Convissor](https://github.com/convissor) +* [Daniel "Drex" Drexler](https://github.com/aeturnum) * [Daniel Huang](https://github.com/dhuang) * [Dave Guarino](https://github.com/daguar) * [David cz](https://github.com/dave-cz) diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 030e62d6c..8c9bf9701 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -315,6 +315,9 @@ class Registration(ResourceBody): # on new-reg key server ignores 'key' and populates it based on # JWS.signature.combined.jwk key = jose.Field('key', omitempty=True, decoder=jose.JWK.from_json) + # Contact field implements special behavior to allow messages that clear existing + # contacts while not expecting the `contact` field when loading from json. + # This is implemented in the constructor and *_json methods. contact = jose.Field('contact', omitempty=True, default=()) agreement = jose.Field('agreement', omitempty=True) status = jose.Field('status', omitempty=True) @@ -327,24 +330,73 @@ class Registration(ResourceBody): @classmethod def from_data(cls, phone=None, email=None, external_account_binding=None, **kwargs): - """Create registration resource from contact details.""" + """ + Create registration resource from contact details. + + The `contact` keyword being passed to a Registration object is meaningful, so + this function represents empty iterables in its kwargs by passing on an empty + `tuple`. + """ + + # Note if `contact` was in kwargs. + contact_provided = 'contact' in kwargs + + # Pop `contact` from kwargs and add formatted email or phone numbers details = list(kwargs.pop('contact', ())) if phone is not None: details.append(cls.phone_prefix + phone) if email is not None: details.extend([cls.email_prefix + mail for mail in email.split(',')]) - kwargs['contact'] = tuple(details) + + # Insert formatted contact information back into kwargs + # or insert an empty tuple if `contact` provided. + if details or contact_provided: + kwargs['contact'] = tuple(details) if external_account_binding: kwargs['external_account_binding'] = external_account_binding return cls(**kwargs) + def __init__(self, **kwargs): + """Note if the user provides a value for the `contact` member.""" + if 'contact' in kwargs: + # Avoid the __setattr__ used by jose.TypedJSONObjectWithFields + object.__setattr__(self, '_add_contact', True) + super(Registration, self).__init__(**kwargs) + def _filter_contact(self, prefix): return tuple( detail[len(prefix):] for detail in self.contact # pylint: disable=not-an-iterable if detail.startswith(prefix)) + def _add_contact_if_appropriate(self, jobj): + """ + The `contact` member of Registration objects should not be required when + de-serializing (as it would be if the Fields' `omitempty` flag were `False`), but + it should be included in serializations if it was provided. + + :param jobj: Dictionary containing this Registrations' data + :type jobj: dict + + :returns: Dictionary containing Registrations data to transmit to the server + :rtype: dict + """ + if getattr(self, '_add_contact', False): + jobj['contact'] = self.encode('contact') + + return jobj + + def to_partial_json(self): + """Modify josepy.JSONDeserializable.to_partial_json()""" + jobj = super(Registration, self).to_partial_json() + return self._add_contact_if_appropriate(jobj) + + def fields_to_partial_json(self): + """Modify josepy.JSONObjectWithFields.fields_to_partial_json()""" + jobj = super(Registration, self).fields_to_partial_json() + return self._add_contact_if_appropriate(jobj) + @property def phones(self): """All phones found in the ``contact`` field.""" diff --git a/acme/tests/messages_test.py b/acme/tests/messages_test.py index 890a5f413..3458105b2 100644 --- a/acme/tests/messages_test.py +++ b/acme/tests/messages_test.py @@ -254,6 +254,19 @@ class RegistrationTest(unittest.TestCase): from acme.messages import Registration hash(Registration.from_json(self.jobj_from)) + def test_default_not_transmitted(self): + from acme.messages import NewRegistration + empty_new_reg = NewRegistration() + new_reg_with_contact = NewRegistration(contact=()) + + self.assertEqual(empty_new_reg.contact, ()) + self.assertEqual(new_reg_with_contact.contact, ()) + + self.assertTrue('contact' not in empty_new_reg.to_partial_json()) + self.assertTrue('contact' not in empty_new_reg.fields_to_partial_json()) + self.assertTrue('contact' in new_reg_with_contact.to_partial_json()) + self.assertTrue('contact' in new_reg_with_contact.fields_to_partial_json()) + class UpdateRegistrationTest(unittest.TestCase): """Tests for acme.messages.UpdateRegistration.""" diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 1ec9f1699..d01d59069 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -27,6 +27,8 @@ More details about these changes can be found on our GitHub repo. this concerns the plugin name, CLI flags, and keys in credential files. The prefixed form is still supported but is deprecated, and will be removed in a future release. * Added `--nginx-sleep-seconds` (default `1`) for environments where nginx takes a long time to reload. +* Added the ability to remove email and phone contact information from an account + using `update_account --register-unsafely-without-email` ### Changed @@ -37,7 +39,8 @@ More details about these changes can be found on our GitHub repo. ### Fixed -* +* The `acme` library can now tell the ACME server to clear contact information by passing an empty + `tuple` to the `contact` field of a `Registration` message. More details about these changes can be found on our GitHub repo. diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 30f4dd0a2..d60e6edba 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -11,7 +11,7 @@ import josepy as jose import zope.component from acme import errors as acme_errors -from acme.magic_typing import Union +from acme.magic_typing import Union, Iterable, Optional # pylint: disable=unused-import import certbot from certbot import crypto_util from certbot import errors @@ -590,7 +590,7 @@ def _init_le_client(config, authenticator, installer): :type config: interfaces.IConfig :param authenticator: Acme authentication handler - :type authenticator: interfaces.IAuthenticator + :type authenticator: Optional[interfaces.IAuthenticator] :param installer: Installer object :type installer: interfaces.IInstaller @@ -703,17 +703,17 @@ def update_account(config, unused_plugins): if not accounts: return "Could not find an existing account to update." - if config.email is None: - if config.register_unsafely_without_email: - return ("--register-unsafely-without-email provided, however, a " - "new e-mail address must\ncurrently be provided when " - "updating a registration.") + if config.email is None and not config.register_unsafely_without_email: config.email = display_ops.get_email(optional=False) acc, acme = _determine_account(config) cb_client = client.Client(config, acc, None, None, acme=acme) + # Empty list of contacts in case the user is removing all emails + + acc_contacts = () # type: Iterable[str] + if config.email: + acc_contacts = ['mailto:' + email for email in config.email.split(',')] # We rely on an exception to interrupt this process if it didn't work. - acc_contacts = ['mailto:' + email for email in config.email.split(',')] prev_regr_uri = acc.regr.uri acc.regr = cb_client.acme.update_registration(acc.regr.update( body=acc.regr.body.update(contact=acc_contacts))) @@ -722,8 +722,13 @@ def update_account(config, unused_plugins): # so that we can also continue to use the account object with acmev1. acc.regr = acc.regr.update(uri=prev_regr_uri) 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)) + + if config.email is None: + add_msg("Any contact information associated with this account has been removed.") + else: + eff.prepare_subscription(config, acc) + add_msg("Your e-mail address was updated to {0}.".format(config.email)) + return None diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index d2d7ccf79..52d8c24d0 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1404,6 +1404,43 @@ class MainTest(test_util.ConfigTestCase): "user@example.org"]) self.assertTrue("Could not find an existing account" in x[0]) + @mock.patch('certbot._internal.main._determine_account') + @mock.patch('certbot._internal.eff.prepare_subscription') + @mock.patch('certbot._internal.main.account') + def test_update_account_remove_email(self, mocked_account_module, mock_prepare, mock_det_acc): + # Mock account storage and the account object returned + mocked_storage = mock.MagicMock() + mocked_account = mock.MagicMock() + + mocked_account_module.AccountFileStorage.return_value = mocked_storage + mocked_storage.find_all.return_value = [mocked_account] + mock_det_acc.return_value = (mocked_account, "foo") + + # Mock registration body to verify calls are made + mock_regr_body = mock.MagicMock() + + # mocked_account.regr is overwritten in update, requiring an odd mock setup + mocked_account.regr.body = mock_regr_body + + x = self._call( + ["update_account", "--register-unsafely-without-email"]) + + + # When update succeeds, the return value of update_account() is None + self.assertTrue(x[0] is None) + # and we got supposedly did update the registration from + # the server + client_mock = x[3] + self.assertTrue(client_mock.Client().acme.update_registration.called) + + self.assertTrue(mock_regr_body.update.called) + self.assertTrue('contact' in mock_regr_body.update.call_args[1]) + self.assertEqual(mock_regr_body.update.call_args[1]['contact'], ()) + # and we saved the updated registration on disk + self.assertTrue(mocked_storage.update_regr.called) + # ensure we didn't try to subscribe (no email to subscribe with) + self.assertFalse(mock_prepare.called) + @mock.patch('certbot._internal.main.display_ops.get_email') @test_util.patch_get_utility() def test_update_account_with_email(self, mock_utility, mock_email): From 70731dd75b9b6c4ae51530998deeb7598fd9f7d9 Mon Sep 17 00:00:00 2001 From: Daniel Drexler Date: Thu, 27 Aug 2020 09:45:10 -0700 Subject: [PATCH 4/8] Move changes to the right section of the changelog (#8236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixing a mistake in pull request #8212 where I recorded my changes in an already released version 😳. - Moving new changes out of a previous changelog and into the next releases' changelog --- certbot/CHANGELOG.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index d01d59069..6f39a27b9 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -6,7 +6,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Added -* +* Added the ability to remove email and phone contact information from an account + using `update_account --register-unsafely-without-email` ### Changed @@ -16,6 +17,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * The problem causing the Apache plugin in the Certbot snap on ARM systems to fail to load the Augeas library it depends on has been fixed. +* The `acme` library can now tell the ACME server to clear contact information by passing an empty + `tuple` to the `contact` field of a `Registration` message. More details about these changes can be found on our GitHub repo. @@ -27,8 +30,6 @@ More details about these changes can be found on our GitHub repo. this concerns the plugin name, CLI flags, and keys in credential files. The prefixed form is still supported but is deprecated, and will be removed in a future release. * Added `--nginx-sleep-seconds` (default `1`) for environments where nginx takes a long time to reload. -* Added the ability to remove email and phone contact information from an account - using `update_account --register-unsafely-without-email` ### Changed @@ -39,8 +40,6 @@ More details about these changes can be found on our GitHub repo. ### Fixed -* The `acme` library can now tell the ACME server to clear contact information by passing an empty - `tuple` to the `contact` field of a `Registration` message. More details about these changes can be found on our GitHub repo. From d62d853ea4c03676f1a10248d01b91aa61eaec3e Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 27 Aug 2020 13:25:57 -0700 Subject: [PATCH 5/8] Clean up --register-unsafely-without-email docs (#8223) * Clean up --register-unsafely text. * update unsafe_suggestion * remove unused import * Expand scary message. --- certbot/certbot/_internal/cli/__init__.py | 11 ++++------- certbot/certbot/display/ops.py | 8 ++++---- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/certbot/certbot/_internal/cli/__init__.py b/certbot/certbot/_internal/cli/__init__.py index 54485e113..45165054d 100644 --- a/certbot/certbot/_internal/cli/__init__.py +++ b/certbot/certbot/_internal/cli/__init__.py @@ -171,13 +171,10 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): ["register", "automation"], "--register-unsafely-without-email", action="store_true", default=flag_default("register_unsafely_without_email"), help="Specifying this flag enables registering an account with no " - "email address. This is strongly discouraged, because in the " - "event of key loss or account compromise you will irrevocably " - "lose access to your account. You will also be unable to receive " - "notice about impending expiration or revocation of your " - "certificates. Updates to the Subscriber Agreement will still " - "affect you, and will be effective 14 days after posting an " - "update to the web site.") + "email address. This is strongly discouraged, because you will be " + "unable to receive notice about impending expiration or " + "revocation of your certificates or problems with your Certbot " + "installation that will lead to failure to renew.") helpful.add( ["register", "update_account", "unregister", "automation"], "-m", "--email", default=flag_default("email"), diff --git a/certbot/certbot/display/ops.py b/certbot/certbot/display/ops.py index 1c4ee47d4..fc369176b 100644 --- a/certbot/certbot/display/ops.py +++ b/certbot/certbot/display/ops.py @@ -6,7 +6,6 @@ import zope.component from certbot import errors from certbot import interfaces from certbot import util -from certbot.compat import misc from certbot.compat import os from certbot.display import util as display_util @@ -33,9 +32,10 @@ def get_email(invalid=False, optional=True): msg = "Enter email address (used for urgent renewal and security notices)\n" unsafe_suggestion = ("\n\nIf you really want to skip this, you can run " "the client with --register-unsafely-without-email " - "but make sure you then backup your account key from " - "{0}\n\n".format(os.path.join( - misc.get_default_folder('config'), 'accounts'))) + "but you will then be unable to receive notice about " + "impending expiration or revocation of your " + "certificates or problems with your Certbot " + "installation that will lead to failure to renew.\n\n") if optional: if invalid: msg += unsafe_suggestion From 3ce87d1fcb1a11b554c9e22727d602cb86f0b8bf Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Wed, 2 Sep 2020 20:45:38 +0200 Subject: [PATCH 6/8] Test PIP_NO_BUILD_ISOLATION (#8255) Fixes #8252 With @bmw we digged quite a lot on why the failure happens on ARM snap, and here we what we understood: * the failure occurs since the version 50 of setuptools is available * normally, we should not be impacted because the setuptools version used in the snap build should be the one installed by the `core20` base snap, because the build occurs in a `venv` created with `--system-site-packages` * BUT associated with the build isolation provided by recent versions of pip (to implement PEP 517), a bad interaction happens: following the definition of the build system provided by `cryptography`, pip installs the most recent version of setuptools on a separate path for the build (because `cryptography` just asks for a minimal version of `setuptools`), then features of this version conflict with the old version of `setuptools` initially present * the exact interaction is described here: https://github.com/pypa/pip/issues/6264#issuecomment-685230919. Basically the new version of `setuptools` triggers some hacks, that are then applied at runtime on the old version of `setuptools` that is also still available in `sys.path` at this point, and breaks the build. To fix that, one can disable the isolation build on cryptography, by passing `PIP_NO_ISOLATION_BUILD=no` to pip. It is the purpose of this PR. This will have the consequence to not be PEP 517 compliant: if needed the `cryptography` library will be built using the `setuptools` available in the system. In general I think it makes sense for the snap build purpose, since we control precisely the build environment, and makes consistent build that will not be broken by a new version of a build system if library maintainers did not provide a strict version of it in their build requirements. However we need now to take care about having a compatible build system for all libraries that may have specific requirements in their build system using the PEP 517 definition in `pyproject.toml`. I think as of now that it is a safe move if we keep using the most recent version of `setuptools` available in Ubuntu 20.04, and it is the case here for snap builds. It may however be problematic if some libraries require another build system than `setuptools` and do not provide a fallback to a `setuptools` build. For the record, `dns-lexicon`, that I maintain, uses `poetry` and so a PEP 517 compliant definition of a build system, but provides also this fallback (https://github.com/AnalogJ/lexicon/blob/master/setup.py). Full test suite compiling the snaps for the 3 architectures using this PR is available here: https://dev.azure.com/certbot/certbot/_build/results?buildId=2596&view=results --- snap/snapcraft.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml index b29899c31..7076f7e18 100644 --- a/snap/snapcraft.yaml +++ b/snap/snapcraft.yaml @@ -75,6 +75,7 @@ parts: build-packages: [gcc, libffi-dev, libssl-dev, git, libaugeas-dev, python3-dev] build-environment: - SNAPCRAFT_PYTHON_VENV_ARGS: --system-site-packages + - PIP_NO_BUILD_ISOLATION: "no" override-pull: | snapcraftctl pull cd $SNAPCRAFT_PART_SRC From 98615564ed3f6e23b2ae9055d3ddd33e4a058d58 Mon Sep 17 00:00:00 2001 From: alexzorin Date: Fri, 4 Sep 2020 20:57:46 +1000 Subject: [PATCH 7/8] log: Don't print backtrace on ^c/KeyboardInterrupt (#8259) --- certbot/certbot/_internal/log.py | 3 +++ certbot/tests/log_test.py | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/certbot/certbot/_internal/log.py b/certbot/certbot/_internal/log.py index 3b87f9bca..3d417c3a5 100644 --- a/certbot/certbot/_internal/log.py +++ b/certbot/certbot/_internal/log.py @@ -319,6 +319,9 @@ def post_arg_parse_except_hook(exc_type, exc_value, trace, debug, log_path): # logger.DEBUG should be used if debug or not issubclass(exc_type, Exception): assert constants.QUIET_LOGGING_LEVEL <= logging.ERROR + if exc_type is KeyboardInterrupt: + logger.error('Exiting due to user request.') + sys.exit(1) logger.error('Exiting abnormally:', exc_info=exc_info) else: logger.debug('Exiting abnormally:', exc_info=exc_info) diff --git a/certbot/tests/log_test.py b/certbot/tests/log_test.py index 5cd287c2e..76764e61b 100644 --- a/certbot/tests/log_test.py +++ b/certbot/tests/log_test.py @@ -306,7 +306,7 @@ class PostArgParseExceptHookTest(unittest.TestCase): self.log_path = 'foo.log' def test_base_exception(self): - exc_type = KeyboardInterrupt + exc_type = BaseException mock_logger, output = self._test_common(exc_type, debug=False) self._assert_exception_logged(mock_logger.error, exc_type) self._assert_logfile_output(output) @@ -342,6 +342,11 @@ class PostArgParseExceptHookTest(unittest.TestCase): self._assert_exception_logged(mock_logger.debug, exc_type) self._assert_quiet_output(mock_logger, output) + def test_keyboardinterrupt(self): + exc_type = KeyboardInterrupt + mock_logger, output = self._test_common(exc_type, debug=False) + mock_logger.error.assert_called_once_with('Exiting due to user request.') + def _test_common(self, error_type, debug): """Returns the mocked logger and stderr output.""" mock_err = six.StringIO() From a2951b4db1e8bbcac4ee4471c62c270b0628024f Mon Sep 17 00:00:00 2001 From: alexzorin Date: Sat, 5 Sep 2020 04:51:01 +1000 Subject: [PATCH 8/8] snap: Fix "stack smashing" error in wrapper (#8249) * snap: Fix "stack smashing" error in wrapper certbot.wrapper had implicit dependencies on sed, awk and coreutils, which were being accidentally provided through the host system. Because certbot.wrapper modifies LD_LIBRARY_PATH, this was causing some systems to load an incompatible combination of shared libraries, resulting sed crashing. This commit reduces the dependencies of this script to just gawk, and explicitly stages it as part of the Certbot snap. It additionally moves invocations of all host system programs to a moment prior to the modification of LD_LIBRARY_PATH, and the invocation of snapped programs to after the modification. Fixes #8245 * snap: Don't modify LD_LIBRARY_PATH * leftover tracing * snap: revert curl/jq in wrapper, use gawk for now --- certbot.wrapper | 20 ++++---------------- certbot/CHANGELOG.md | 1 + snap/hooks/configure | 2 +- snap/hooks/prepare-plug-plugin | 2 +- snap/snapcraft.yaml | 2 ++ 5 files changed, 9 insertions(+), 18 deletions(-) diff --git a/certbot.wrapper b/certbot.wrapper index 909f2e369..b7ce120bd 100755 --- a/certbot.wrapper +++ b/certbot.wrapper @@ -1,4 +1,4 @@ -#!/bin/bash +#!/bin/sh set -e # This code is based on snapcraft's own patch to work around this problem at @@ -27,21 +27,9 @@ case "${SNAP_ARCH}" in exit 1 esac -PARTIAL_LIBRARY_PATH="${SNAP}/usr/lib/${ARCH_TRIPLET}/" -export LD_LIBRARY_PATH="${PARTIAL_LIBRARY_PATH}:${LD_LIBRARY_PATH}" -export CERTBOT_AUGEAS_PATH="${PARTIAL_LIBRARY_PATH}libaugeas.so.0" +export CERTBOT_AUGEAS_PATH="${SNAP}/usr/lib/${ARCH_TRIPLET}/libaugeas.so.0" -join() { - sep=$1 - first=$2 - if [ "$first" != "" ]; then - shift 2 - echo -n "${first}" - for item in "$@"; do echo -n "${sep}${item}"; done - echo - fi -} +CERTBOT_PLUGIN_PATH="$(snap connections certbot | gawk 'BEGIN {ORS=""} NR>1 { if ($1 == "content[certbot-1]") { split($3,a,":"); PLUGINS=PLUGINS":/snap/"a[1]"/current/lib/python3.8/site-packages/"; next; } } END { print substr(PLUGINS, 2) }')" +export CERTBOT_PLUGIN_PATH -paths=$(for plugin_snap in $(snap connections certbot|sed -n '2,$p'|awk '$1=="content[certbot-1]"{print $3}'|cut -d: -f1); do echo /snap/$plugin_snap/current/lib/python3.8/site-packages; done) -export CERTBOT_PLUGIN_PATH=$(join : $paths) exec certbot "$@" diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 6f39a27b9..78495ceda 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -19,6 +19,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). fail to load the Augeas library it depends on has been fixed. * The `acme` library can now tell the ACME server to clear contact information by passing an empty `tuple` to the `contact` field of a `Registration` message. +* Fixed the `*** stack smashing detected ***` error in the Certbot snap on some systems. More details about these changes can be found on our GitHub repo. diff --git a/snap/hooks/configure b/snap/hooks/configure index 2678c47b2..9545f8b8a 100644 --- a/snap/hooks/configure +++ b/snap/hooks/configure @@ -1,3 +1,3 @@ -#!/bin/bash -e +#!/bin/sh -e exit 0 diff --git a/snap/hooks/prepare-plug-plugin b/snap/hooks/prepare-plug-plugin index ee309addf..f2f7ff86b 100644 --- a/snap/hooks/prepare-plug-plugin +++ b/snap/hooks/prepare-plug-plugin @@ -1,4 +1,4 @@ -#!/bin/bash -e +#!/bin/sh -e if [ "$(snapctl get trust-plugin-with-root)" = "ok" ]; then # allow the connection, but reset config to allow for other slots to go through this auth flow diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml index 7076f7e18..2cd5271c4 100644 --- a/snap/snapcraft.yaml +++ b/snap/snapcraft.yaml @@ -71,6 +71,8 @@ parts: - python3-distutils - python3-pkg-resources - python3.8-minimal + # added for certbot.wrapper script: + - gawk # To build cryptography and cffi if needed build-packages: [gcc, libffi-dev, libssl-dev, git, libaugeas-dev, python3-dev] build-environment: