From bf818036eb4f23164f92800b9ea83591149c0814 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 24 May 2019 15:20:54 -0700 Subject: [PATCH 1/7] Revert "Fix unpinned dependencies tests towards botocore and urllib3 (#7081)" (#7101) This reverts commit 51a7e7cd1928352a96b6849004e176bbd6cd1863. --- certbot-dns-route53/setup.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/certbot-dns-route53/setup.py b/certbot-dns-route53/setup.py index ab9bc4d06..09cd4acd2 100644 --- a/certbot-dns-route53/setup.py +++ b/certbot-dns-route53/setup.py @@ -6,11 +6,6 @@ version = '0.35.0.dev0' # Remember to update local-oldest-requirements.txt when changing the minimum # acme/certbot version. install_requires = [ - # boto3 requires urllib<1.25 while requests 2.22+ requires urllib<1.26 - # Since pip lacks a real dependency graph resolver, it will peak the constraint only from - # requests, and install urllib==1.25.2. Setting an explicit dependency here solves the issue. - # Check https://github.com/boto/botocore/issues/1733 for resolution in botocore. - 'urllib3<1.25', 'acme>=0.29.0', 'certbot>=0.34.0', 'boto3', From d2a2b880903967f49fde6c54126e61b40455c91d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 28 May 2019 14:36:10 -0700 Subject: [PATCH 2/7] Update Ubuntu AMI to 19.04. (#7099) --- tests/letstest/apache2_targets.yaml | 4 ++-- tests/letstest/targets.yaml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/letstest/apache2_targets.yaml b/tests/letstest/apache2_targets.yaml index dda4d4b1c..4da6abb15 100644 --- a/tests/letstest/apache2_targets.yaml +++ b/tests/letstest/apache2_targets.yaml @@ -1,8 +1,8 @@ targets: #----------------------------------------------------------------------------- #Ubuntu - - ami: ami-064bd2d44a1d6c097 - name: ubuntu18.10 + - ami: ami-08ab45c4343f5f5c6 + name: ubuntu19.04 type: ubuntu virt: hvm user: ubuntu diff --git a/tests/letstest/targets.yaml b/tests/letstest/targets.yaml index d1eba43de..340fe6bf8 100644 --- a/tests/letstest/targets.yaml +++ b/tests/letstest/targets.yaml @@ -1,8 +1,8 @@ targets: #----------------------------------------------------------------------------- #Ubuntu - - ami: ami-064bd2d44a1d6c097 - name: ubuntu18.10 + - ami: ami-08ab45c4343f5f5c6 + name: ubuntu19.04 type: ubuntu virt: hvm user: ubuntu From 7d35f95293b40159ef15f38eed448168e158ce56 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Wed, 29 May 2019 00:16:12 +0200 Subject: [PATCH 3/7] Avoid to delete both webroot_map and webroot_path (#7095) * Always restore webroot_path in renewal config. * Add unit tests to ensure correct behavior * Add changelog * Add certbot as modified package --- CHANGELOG.md | 4 +++- certbot/plugins/webroot_test.py | 13 +++++++++++++ certbot/renewal.py | 10 +++++----- certbot/tests/renewal_test.py | 24 ++++++++++++++++++++++++ 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cc3ff41f..8669d34a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,12 +16,14 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Fixed -* +* Renewal parameter `webroot_path` is always saved, avoiding some regressions + when `webroot` authenticator plugin is invoked with no challenge to perform. Despite us having broken lockstep, we are continuing to release new versions of all Certbot components during releases for the time being, however, the only package with changes other than its version number was: +* certbot * certbot-dns-rfc2136 More details about these changes can be found on our GitHub repo. diff --git a/certbot/plugins/webroot_test.py b/certbot/plugins/webroot_test.py index 3a902b91f..bca1045d8 100644 --- a/certbot/plugins/webroot_test.py +++ b/certbot/plugins/webroot_test.py @@ -295,6 +295,19 @@ class WebrootActionTest(unittest.TestCase): self.assertEqual( config.webroot_map[self.achall.domain], self.path) + def test_webroot_map_partial_without_perform(self): + # This test acknowledges the fact that webroot_map content will be partial if webroot + # plugin perform method is not invoked (corner case when all auths are already valid). + # To not be a problem, the webroot_path must always been conserved during renew. + # This condition is challenged by: + # certbot.tests.renewal_tests::RenewalTest::test_webroot_params_conservation + # See https://github.com/certbot/certbot/pull/7095 for details. + other_webroot_path = tempfile.mkdtemp() + args = self.parser.parse_args("-w {0} -d {1} -w {2} -d bar".format( + self.path, self.achall.domain, other_webroot_path).split()) + self.assertEqual(args.webroot_map, {self.achall.domain: self.path}) + self.assertEqual(args.webroot_path, [self.path, other_webroot_path]) + def _get_config_after_perform(self, config): from certbot.plugins.webroot import Authenticator auth = Authenticator(config, "webroot") diff --git a/certbot/renewal.py b/certbot/renewal.py index f932269b6..4b4a5a082 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -106,11 +106,11 @@ def _restore_webroot_config(config, renewalparams): restoring logic is not able to correctly parse it from the serialized form. """ - if "webroot_map" in renewalparams: - if not cli.set_by_cli("webroot_map"): - config.webroot_map = renewalparams["webroot_map"] - elif "webroot_path" in renewalparams: - logger.debug("Ancient renewal conf file without webroot-map, restoring webroot-path") + if "webroot_map" in renewalparams and not cli.set_by_cli("webroot_map"): + config.webroot_map = renewalparams["webroot_map"] + # To understand why webroot_path and webroot_map processing are not mutually exclusive, + # see https://github.com/certbot/certbot/pull/7095 + if "webroot_path" in renewalparams and not cli.set_by_cli("webroot_path"): wp = renewalparams["webroot_path"] if isinstance(wp, six.string_types): # prior to 0.1.0, webroot_path was a string wp = [wp] diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index dac585239..e33869e13 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -28,6 +28,29 @@ class RenewalTest(test_util.ConfigTestCase): renewal._restore_webroot_config(config, renewalparams) self.assertEqual(config.webroot_path, ['/var/www/']) + @mock.patch('certbot.renewal.cli.set_by_cli') + def test_webroot_params_conservation(self, mock_set_by_cli): + # For more details about why this test is important, see: + # certbot.plugins.webroot_test::WebrootActionTest::test_webroot_map_partial_without_perform + from certbot import renewal + mock_set_by_cli.return_value = False + + renewalparams = { + 'webroot_map': {'test.example.com': '/var/www/test'}, + 'webroot_path': ['/var/www/test', '/var/www/other'], + } + renewal._restore_webroot_config(self.config, renewalparams) # pylint: disable=protected-access + self.assertEqual(self.config.webroot_map, {'test.example.com': '/var/www/test'}) + self.assertEqual(self.config.webroot_path, ['/var/www/test', '/var/www/other']) + + renewalparams = { + 'webroot_map': {}, + 'webroot_path': '/var/www/test', + } + renewal._restore_webroot_config(self.config, renewalparams) # pylint: disable=protected-access + self.assertEqual(self.config.webroot_map, {}) + self.assertEqual(self.config.webroot_path, ['/var/www/test']) + class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase): """Tests for certbot.renewal.restore_required_config_elements.""" @@ -89,5 +112,6 @@ class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase): self.assertRaises( errors.Error, self._call, self.config, renewalparams) + if __name__ == "__main__": unittest.main() # pragma: no cover From 561534b754344f3ce24b52c9ac938d352f552c3d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 28 May 2019 23:54:26 -0700 Subject: [PATCH 4/7] Move IRC notifications to #certbot-devel. (#7098) * Move IRC notifications to #certbot-devel. * Don't use notice. --- .travis.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 52595c111..1eee09898 100644 --- a/.travis.yml +++ b/.travis.yml @@ -276,8 +276,11 @@ notifications: email: false irc: channels: - - secure: "SGWZl3ownKx9xKVV2VnGt7DqkTmutJ89oJV9tjKhSs84kLijU6EYdPnllqISpfHMTxXflNZuxtGo0wTDYHXBuZL47w1O32W6nzuXdra5zC+i4sYQwYULUsyfOv9gJX8zWAULiK0Z3r0oho45U+FR5ZN6TPCidi8/eGU+EEPwaAw=" + # This is set to a secure variable to prevent forks from sending + # notifications. This value was created by installing + # https://github.com/travis-ci/travis.rb and running + # `travis encrypt "chat.freenode.net#certbot-devel"`. + - secure: "EWW66E2+KVPZyIPR8ViENZwfcup4Gx3/dlimmAZE0WuLwxDCshBBOd3O8Rf6pBokEoZlXM5eDT6XdyJj8n0DLslgjO62pExdunXpbcMwdY7l1ELxX2/UbnDTE6UnPYa09qVBHNG7156Z6yE0x2lH4M9Ykvp0G0cubjPQHylAwo0=" on_cancel: never on_success: never on_failure: always - use_notice: true From 4c299be9657ab8ab4a0c9be79c7b4c7d2f76286d Mon Sep 17 00:00:00 2001 From: Pete Cooper Date: Wed, 29 May 2019 22:16:16 +0100 Subject: [PATCH 5/7] Update docs/cli-help.txt -- typo and formatting (#7105) * Update docs/cli-help.txt -- yypo and formatting 'areusing' -> 'are using' * Update cli.py -- formatting See https://github.com/certbot/certbot/pull/7105 Addresses https://github.com/certbot/certbot/pull/7105#issuecomment-497079342 --- certbot/cli.py | 2 +- docs/cli-help.txt | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 866b64aa6..b7c021ed4 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -1453,7 +1453,7 @@ def _plugins_parsing(helpful, plugins): "using DNSimple for DNS).")) helpful.add(["plugins", "certonly"], "--dns-dnsmadeeasy", action="store_true", default=flag_default("dns_dnsmadeeasy"), - help=("Obtain certificates using a DNS TXT record (if you are" + help=("Obtain certificates using a DNS TXT record (if you are " "using DNS Made Easy for DNS).")) helpful.add(["plugins", "certonly"], "--dns-gehirn", action="store_true", default=flag_default("dns_gehirn"), diff --git a/docs/cli-help.txt b/docs/cli-help.txt index 3e5fdc53b..f65384bbc 100644 --- a/docs/cli-help.txt +++ b/docs/cli-help.txt @@ -454,8 +454,8 @@ plugins: using DigitalOcean for DNS). (default: False) --dns-dnsimple Obtain certificates using a DNS TXT record (if you are using DNSimple for DNS). (default: False) - --dns-dnsmadeeasy Obtain certificates using a DNS TXT record (if you - areusing DNS Made Easy for DNS). (default: False) + --dns-dnsmadeeasy Obtain certificates using a DNS TXT record (if you are + using DNS Made Easy for DNS). (default: False) --dns-gehirn Obtain certificates using a DNS TXT record (if you are using Gehirn Infrastracture Service for DNS). (default: False) From 926c8c198cfb6158b7ad59988702c30e33f13d53 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Thu, 30 May 2019 16:09:09 +0200 Subject: [PATCH 6/7] Remove dependency on acme in certbot-ci (#7055) Following discussion in #6947 (comment), I have second thoughts about relying on acme in certbot-ci. Indeed, I think it is a good design to not rely in tests on the code you are testing. Obviously in unit tests it is very difficult, since most of the time the unit that is tested needs input generated by other part of the code. However it is not really a problem in a unit test, as its purpose is to make assertions about a specific portion of the code, not the others parts. In the scope of integration tests, the software tested is treated as a black box. In this case, having some parts of the test logic that use in fact part of the code in the black box, increase the risk that some assertions compared two results coming from the same flawed logic from the tested software. Since using acme in certbot-ci is only saving few lines of code, I think it does not worth the risk and the added complexity to declare acme as a dependency. I prefer to duplicate these lines and keep certbot-ci free of any dependency coming from the certbot project. --- .../certbot_integration_tests/utils/misc.py | 21 ++++++++++++------- certbot-ci/setup.py | 1 - 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/certbot-ci/certbot_integration_tests/utils/misc.py b/certbot-ci/certbot_integration_tests/utils/misc.py index 4647a229e..2144b9cee 100644 --- a/certbot-ci/certbot_integration_tests/utils/misc.py +++ b/certbot-ci/certbot_integration_tests/utils/misc.py @@ -23,8 +23,6 @@ from cryptography.hazmat.primitives.asymmetric import ec from cryptography.hazmat.primitives.serialization import Encoding, PrivateFormat, NoEncryption from six.moves import socketserver, SimpleHTTPServer -from acme import crypto_util - RSA_KEY_TYPE = 'rsa' ECDSA_KEY_TYPE = 'ecdsa' @@ -250,13 +248,20 @@ def generate_csr(domains, key_path, csr_path, key_type=RSA_KEY_TYPE): else: raise ValueError('Invalid key type: {0}'.format(key_type)) - key_bytes = crypto.dump_privatekey(crypto.FILETYPE_PEM, key) - with open(key_path, 'wb') as file: - file.write(key_bytes) + with open(key_path, 'wb') as file_h: + file_h.write(crypto.dump_privatekey(crypto.FILETYPE_PEM, key)) - csr_bytes = crypto_util.make_csr(key_bytes, domains) - with open(csr_path, 'wb') as file: - file.write(csr_bytes) + req = crypto.X509Req() + san = ', '.join(['DNS:{0}'.format(item) for item in domains]) + san_constraint = crypto.X509Extension(b'subjectAltName', False, san.encode('utf-8')) + req.add_extensions([san_constraint]) + + req.set_pubkey(key) + req.set_version(2) + req.sign(key, 'sha256') + + with open(csr_path, 'wb') as file_h: + file_h.write(crypto.dump_certificate_request(crypto.FILETYPE_ASN1, req)) def read_certificate(cert_path): diff --git a/certbot-ci/setup.py b/certbot-ci/setup.py index 88372bffc..eecbe2887 100644 --- a/certbot-ci/setup.py +++ b/certbot-ci/setup.py @@ -5,7 +5,6 @@ from setuptools import find_packages version = '0.32.0.dev0' install_requires = [ - 'acme', 'coverage', 'cryptography', 'pyopenssl', From 641aba68b1c2e57a912e5edf95178cd29fd66b4b Mon Sep 17 00:00:00 2001 From: Felix Lechner Date: Thu, 30 May 2019 15:02:15 -0700 Subject: [PATCH 7/7] Ignore editor backups when running hooks. (#7109) * Ignore editor backups when running hooks. When processing hooks, certbot also runs editor backups even though such files are outdated, clearly warranted correction and may quite possibly be defective. That behavior could lead to unexpected breakage, and perhaps even pose security risks---for example, if a previous script was careless with file permissions. As an aggravating factor, the backup runs after the corrected version and could unintentionally override a fix the user thought was properly implemented. This commit causes editor backup files ending in tilde (~) to be excluded when running hooks. Additional information can be found here: https://github.com/certbot/certbot/issues/7107 https://community.letsencrypt.org/t/editor-backup-files-executed-as-renewal-hooks/94750 * Add unit test for hook scripts with filenames ending in tilde. * Provide changelog entry for not running hook scripts ending in tilde. * Add Felix Lechner to the list of contributors. --- AUTHORS.md | 1 + CHANGELOG.md | 2 ++ certbot/hooks.py | 5 +++-- certbot/tests/hook_test.py | 6 ++++++ 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/AUTHORS.md b/AUTHORS.md index 6ee739bc0..0e8d88a4d 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -75,6 +75,7 @@ Authors * [Fabian](https://github.com/faerbit) * [Faidon Liambotis](https://github.com/paravoid) * [Fan Jiang](https://github.com/tcz001) +* [Felix Lechner](https://github.com/lechner) * [Felix Schwarz](https://github.com/FelixSchwarz) * [Felix Yan](https://github.com/felixonmars) * [Filip Ochnik](https://github.com/filipochnik) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8669d34a5..83e62c792 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * Renewal parameter `webroot_path` is always saved, avoiding some regressions when `webroot` authenticator plugin is invoked with no challenge to perform. +* Scripts in Certbot hook directories are no longer executed when their + filenames end in a tilde. Despite us having broken lockstep, we are continuing to release new versions of all Certbot components during releases for the time being, however, the only diff --git a/certbot/hooks.py b/certbot/hooks.py index 7de846ae4..34e06e0a3 100644 --- a/certbot/hooks.py +++ b/certbot/hooks.py @@ -266,5 +266,6 @@ def list_hooks(dir_path): :rtype: sorted list of absolute paths to executables in dir_path """ - paths = (os.path.join(dir_path, f) for f in os.listdir(dir_path)) - return sorted(path for path in paths if util.is_exe(path)) + allpaths = (os.path.join(dir_path, f) for f in os.listdir(dir_path)) + hooks = [path for path in allpaths if util.is_exe(path) and not path.endswith('~')] + return sorted(hooks) diff --git a/certbot/tests/hook_test.py b/certbot/tests/hook_test.py index 2a3742aa2..2ed7d4229 100644 --- a/certbot/tests/hook_test.py +++ b/certbot/tests/hook_test.py @@ -480,6 +480,12 @@ class ListHooksTest(util.TempDirTestCase): self.assertEqual(self._call(self.tempdir), [name]) + def test_ignore_tilde(self): + name = os.path.join(self.tempdir, "foo~") + create_hook(name) + + self.assertEqual(self._call(self.tempdir), []) + def create_hook(file_path): """Creates an executable file at the specified path.