Fix nginx --dry-run (#4889)

* Revert "Don't save keys/csr on dry run (#4380)"

This reverts commit e034b50363.

* 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

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

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(
+        if self.config.dry_run:
-            self.config.rsa_key_size, self.config.key_dir)
+            key = util.Key(file=None,
-        csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir)
+                           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)
 

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,

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_f, key_path = util.unique_file(
-        key_path = None
+        os.path.join(key_dir, keyname), 0o600, "wb")
-        logger.debug("Generating key (%d bits), not saving to file", key_size)
+    with key_f:
-    else:
+        key_f.write(key_pem)
-        key_f, key_path = util.unique_file(
+    logger.debug("Generating key (%d bits): %s", key_size, key_path)
-            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_f, csr_filename = util.unique_file(
-        csr_filename = None
+        os.path.join(path, "csr-certbot.pem"), 0o644, "wb")
-        logger.debug("Creating CSR: not saving to file")
+    with csr_f:
-    else:
+        csr_f.write(csr_pem)
-        csr_f, csr_filename = util.unique_file(
+    logger.debug("Creating CSR: %s", csr_filename)
-            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")
 

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, mock_crypto_util):
-    def test_obtain_certificate(self, unused_mock_get_utility,
-                                mock_crypto_util):
-        self._mock_obtain_certificate()
-
         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(
+        with test_util.patch_get_utility():
-            self.client.obtain_certificate(domains),
+            result = self.client.obtain_certificate(self.eg_domains)
-            (mock.sentinel.certr, mock.sentinel.chain, mock.sentinel.key, csr))
 
-        mock_crypto_util.init_save_key.assert_called_once_with(
+        self.assertEqual(
-            self.config.rsa_key_size, self.config.key_dir)
+            result,
-        mock_crypto_util.init_save_csr.assert_called_once_with(
+            (mock.sentinel.certr, mock.sentinel.chain, key, csr))
-            mock.sentinel.key, domains, self.config.csr_dir)
         self._check_obtain_certificate()
 
     @mock.patch('certbot.client.Client.obtain_certificate')

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),
+            mock.Mock(strict_permissions=True), interfaces.IConfig)
-            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),
+            mock.Mock(strict_permissions=True), interfaces.IConfig)
-            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."""