From 828363b21ae5b9d9b5735db343667b1822853817 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 30 Jun 2017 08:10:55 -0400 Subject: [PATCH] Fix nginx --dry-run (#4889) * Revert "Don't save keys/csr on dry run (#4380)" This reverts commit e034b50363a75d61d1a7e7add2b8aca52684acd1. * Don't save CSRs and keys during dry run * Factor out _test_obtain_certificate_common * Add test_obtain_certificate_dry_run * Wrap key from make_key in util.Key * Wrap result from make_csr in util.CSR --- certbot-nginx/certbot_nginx/tests/util.py | 1 - certbot/client.py | 15 +++++-- certbot/constants.py | 1 - certbot/crypto_util.py | 28 +++++-------- certbot/tests/client_test.py | 49 ++++++++++++++++------- certbot/tests/crypto_util_test.py | 34 ++-------------- 6 files changed, 60 insertions(+), 68 deletions(-) diff --git a/certbot-nginx/certbot_nginx/tests/util.py b/certbot-nginx/certbot_nginx/tests/util.py index 2ee38ec38..6e1b0d8ff 100644 --- a/certbot-nginx/certbot_nginx/tests/util.py +++ b/certbot-nginx/certbot_nginx/tests/util.py @@ -65,7 +65,6 @@ def get_nginx_configurator( in_progress_dir=os.path.join(backups, "IN_PROGRESS"), server="https://acme-server.org:443/new", tls_sni_01_port=5001, - dry_run=False, ), name="nginx", version=version) diff --git a/certbot/client.py b/certbot/client.py index 7896ab7dc..6010dd0a0 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -9,6 +9,7 @@ import OpenSSL import zope.component from acme import client as acme_client +from acme import crypto_util as acme_crypto_util from acme import errors as acme_errors from acme import jose from acme import messages @@ -319,9 +320,17 @@ class Client(object): domains = [d for d in domains if d in auth_domains] # Create CSR from names - key = crypto_util.init_save_key( - self.config.rsa_key_size, self.config.key_dir) - csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir) + if self.config.dry_run: + key = util.Key(file=None, + pem=crypto_util.make_key(self.config.rsa_key_size)) + csr = util.CSR(file=None, form="pem", + data=acme_crypto_util.make_csr( + key.pem, domains, self.config.must_staple)) + else: + key = crypto_util.init_save_key( + self.config.rsa_key_size, self.config.key_dir) + csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir) + certr, chain = self.obtain_certificate_from_csr( domains, csr, authzr=authzr) diff --git a/certbot/constants.py b/certbot/constants.py index 7919b3317..dfdfcc0e8 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -18,7 +18,6 @@ CLI_DEFAULTS = dict( os.path.join(os.environ.get("XDG_CONFIG_HOME", "~/.config"), "letsencrypt", "cli.ini"), ], - dry_run=False, verbose_count=-int(logging.INFO / 10), server="https://acme-v01.api.letsencrypt.org/directory", rsa_key_size=2048, diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index 0ac0eee40..e22effeb7 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -55,15 +55,11 @@ def init_save_key(key_size, key_dir, keyname="key-certbot.pem"): # Save file util.make_or_verify_dir(key_dir, 0o700, os.geteuid(), config.strict_permissions) - if config.dry_run: - key_path = None - logger.debug("Generating key (%d bits), not saving to file", key_size) - else: - key_f, key_path = util.unique_file( - os.path.join(key_dir, keyname), 0o600, "wb") - with key_f: - key_f.write(key_pem) - logger.debug("Generating key (%d bits): %s", key_size, key_path) + key_f, key_path = util.unique_file( + os.path.join(key_dir, keyname), 0o600, "wb") + with key_f: + key_f.write(key_pem) + logger.debug("Generating key (%d bits): %s", key_size, key_path) return util.Key(key_path, key_pem) @@ -90,15 +86,11 @@ def init_save_csr(privkey, names, path): # Save CSR util.make_or_verify_dir(path, 0o755, os.geteuid(), config.strict_permissions) - if config.dry_run: - csr_filename = None - logger.debug("Creating CSR: not saving to file") - else: - csr_f, csr_filename = util.unique_file( - os.path.join(path, "csr-certbot.pem"), 0o644, "wb") - with csr_f: - csr_f.write(csr_pem) - logger.debug("Creating CSR: %s", csr_filename) + csr_f, csr_filename = util.unique_file( + os.path.join(path, "csr-certbot.pem"), 0o644, "wb") + with csr_f: + csr_f.write(csr_pem) + logger.debug("Creating CSR: %s", csr_filename) return util.CSR(csr_filename, csr_pem, "pem") diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 391a407ac..97fd6241d 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -144,6 +144,7 @@ class ClientTest(ClientTestCommon): self.config.allow_subset_of_names = False self.config.config_dir = "/etc/letsencrypt" + self.config.dry_run = False self.eg_domains = ["example.com", "www.example.com"] def test_init_acme_verify_ssl(self): @@ -241,15 +242,37 @@ class ClientTest(ClientTestCommon): self.assertEqual(1, mock_get_utility().notification.call_count) @mock.patch("certbot.client.crypto_util") - @test_util.patch_get_utility() - def test_obtain_certificate(self, unused_mock_get_utility, - mock_crypto_util): - self._mock_obtain_certificate() - + def test_obtain_certificate(self, mock_crypto_util): csr = util.CSR(form="pem", file=None, data=CSR_SAN) mock_crypto_util.init_save_csr.return_value = csr mock_crypto_util.init_save_key.return_value = mock.sentinel.key - domains = ["example.com", "www.example.com"] + + self._test_obtain_certificate_common(mock.sentinel.key, csr) + + mock_crypto_util.init_save_key.assert_called_once_with( + self.config.rsa_key_size, self.config.key_dir) + mock_crypto_util.init_save_csr.assert_called_once_with( + mock.sentinel.key, self.eg_domains, self.config.csr_dir) + + @mock.patch("certbot.client.crypto_util") + @mock.patch("certbot.client.acme_crypto_util") + def test_obtain_certificate_dry_run(self, mock_acme_crypto, mock_crypto): + csr = util.CSR(form="pem", file=None, data=CSR_SAN) + mock_acme_crypto.make_csr.return_value = CSR_SAN + mock_crypto.make_key.return_value = mock.sentinel.key_pem + key = util.Key(file=None, pem=mock.sentinel.key_pem) + + with mock.patch.object(self.client.config, 'dry_run', new=True): + self._test_obtain_certificate_common(key, csr) + + mock_crypto.make_key.assert_called_once_with(self.config.rsa_key_size) + mock_acme_crypto.make_csr.assert_called_once_with( + mock.sentinel.key_pem, self.eg_domains, self.config.must_staple) + mock_crypto.init_save_key.assert_not_called() + mock_crypto.init_save_csr.assert_not_called() + + def _test_obtain_certificate_common(self, key, csr): + self._mock_obtain_certificate() # return_value is essentially set to (None, None) in # _mock_obtain_certificate(), which breaks this test. @@ -258,7 +281,7 @@ class ClientTest(ClientTestCommon): authzr = [] # domain ordering should not be affected by authorization order - for domain in reversed(domains): + for domain in reversed(self.eg_domains): authzr.append( mock.MagicMock( body=mock.MagicMock( @@ -267,14 +290,12 @@ class ClientTest(ClientTestCommon): self.client.auth_handler.get_authorizations.return_value = authzr - self.assertEqual( - self.client.obtain_certificate(domains), - (mock.sentinel.certr, mock.sentinel.chain, mock.sentinel.key, csr)) + with test_util.patch_get_utility(): + result = self.client.obtain_certificate(self.eg_domains) - mock_crypto_util.init_save_key.assert_called_once_with( - self.config.rsa_key_size, self.config.key_dir) - mock_crypto_util.init_save_csr.assert_called_once_with( - mock.sentinel.key, domains, self.config.csr_dir) + self.assertEqual( + result, + (mock.sentinel.certr, mock.sentinel.chain, key, csr)) self._check_obtain_certificate() @mock.patch('certbot.client.Client.obtain_certificate') diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index ea5a81062..729b09dc1 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -30,8 +30,7 @@ class InitSaveKeyTest(test_util.TempDirTestCase): logging.disable(logging.CRITICAL) zope.component.provideUtility( - mock.Mock(strict_permissions=True, dry_run=False), - interfaces.IConfig) + mock.Mock(strict_permissions=True), interfaces.IConfig) def tearDown(self): super(InitSaveKeyTest, self).tearDown() @@ -51,16 +50,6 @@ class InitSaveKeyTest(test_util.TempDirTestCase): self.assertTrue('key-certbot.pem' in key.file) self.assertTrue(os.path.exists(os.path.join(self.tempdir, key.file))) - @mock.patch('certbot.crypto_util.make_key') - def test_success_dry_run(self, mock_make): - zope.component.provideUtility( - mock.Mock(strict_permissions=True, dry_run=True), - interfaces.IConfig) - mock_make.return_value = b'key_pem' - key = self._call(1024, self.tempdir) - self.assertEqual(key.pem, b'key_pem') - self.assertTrue(key.file is None) - @mock.patch('certbot.crypto_util.make_key') def test_key_failure(self, mock_make): mock_make.side_effect = ValueError @@ -74,12 +63,11 @@ class InitSaveCSRTest(test_util.TempDirTestCase): super(InitSaveCSRTest, self).setUp() zope.component.provideUtility( - mock.Mock(strict_permissions=True, dry_run=False), - interfaces.IConfig) + mock.Mock(strict_permissions=True), interfaces.IConfig) @mock.patch('acme.crypto_util.make_csr') @mock.patch('certbot.crypto_util.util.make_or_verify_dir') - def test_success(self, unused_mock_verify, mock_csr): + def test_it(self, unused_mock_verify, mock_csr): from certbot.crypto_util import init_save_csr mock_csr.return_value = b'csr_pem' @@ -90,22 +78,6 @@ class InitSaveCSRTest(test_util.TempDirTestCase): self.assertEqual(csr.data, b'csr_pem') self.assertTrue('csr-certbot.pem' in csr.file) - @mock.patch('acme.crypto_util.make_csr') - @mock.patch('certbot.crypto_util.util.make_or_verify_dir') - def test_success_dry_run(self, unused_mock_verify, mock_csr): - from certbot.crypto_util import init_save_csr - - zope.component.provideUtility( - mock.Mock(strict_permissions=True, dry_run=True), - interfaces.IConfig) - mock_csr.return_value = b'csr_pem' - - csr = init_save_csr( - mock.Mock(pem='dummy_key'), 'example.com', self.tempdir) - - self.assertEqual(csr.data, b'csr_pem') - self.assertTrue(csr.file is None) - class ValidCSRTest(unittest.TestCase): """Tests for certbot.crypto_util.valid_csr."""