Allow non-interactive revocation without deleting certificates (#5386)

* Add --delete-after-revoke flags

* Use delete_after_revoke value

* Add delete_after_revoke unit tests

* Add integration tests for delete-after-revoke.

certbot/cli.py

@@ -1220,6 +1220,18 @@ def _create_subparsers(helpful):
                                                    key=constants.REVOCATION_REASONS.get)),
                 action=_EncodeReasonAction, default=flag_default("reason"),
                 help="Specify reason for revoking certificate. (default: unspecified)")
+    helpful.add("revoke",
+                "--delete-after-revoke", action="store_true",
+                default=flag_default("delete_after_revoke"),
+                help="Delete certificates after revoking them.")
+    helpful.add("revoke",
+                "--no-delete-after-revoke", action="store_false",
+                dest="delete_after_revoke",
+                default=flag_default("delete_after_revoke"),
+                help="Do not delete certificates after revoking them. This "
+                     "option should be used with caution because the 'renew' "
+                     "subcommand will attempt to renew undeleted revoked "
+                     "certificates.")
     helpful.add("rollback",
                 "--checkpoints", type=int, metavar="N",
                 default=flag_default("rollback_checkpoints"),

certbot/constants.py

@@ -71,6 +71,7 @@ CLI_DEFAULTS = dict(
     user_agent_comment=None,
     csr=None,
     reason=0,
+    delete_after_revoke=None,
     rollback_checkpoints=1,
     init=False,
     prepare=False,

certbot/main.py

@@ -536,9 +536,11 @@ def _delete_if_appropriate(config): # pylint: disable=too-many-locals,too-many-b
     display = zope.component.getUtility(interfaces.IDisplay)
     reporter_util = zope.component.getUtility(interfaces.IReporter)
 
-    msg = ("Would you like to delete the cert(s) you just revoked?")
+    attempt_deletion = config.delete_after_revoke
-    attempt_deletion = display.yesno(msg, yes_label="Yes (recommended)", no_label="No",
+    if attempt_deletion is None:
-            force_interactive=True, default=True)
+        msg = ("Would you like to delete the cert(s) you just revoked?")
+        attempt_deletion = display.yesno(msg, yes_label="Yes (recommended)", no_label="No",
+                force_interactive=True, default=True)
 
     if not attempt_deletion:
         reporter_util.add_message("Not deleting revoked certs.", reporter_util.LOW_PRIORITY)

certbot/tests/cli_test.py

@@ -164,6 +164,8 @@ class ParseTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertTrue("--cert-path" in out)
         self.assertTrue("--key-path" in out)
         self.assertTrue("--reason" in out)
+        self.assertTrue("--delete-after-revoke" in out)
+        self.assertTrue("--no-delete-after-revoke" in out)
 
         out = self._help_output(['-h', 'config_changes'])
         self.assertTrue("--cert-path" not in out)
@@ -412,6 +414,18 @@ class ParseTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
     def test_no_directory_hooks_unset(self):
         self.assertTrue(self.parse([]).directory_hooks)
 
+    def test_delete_after_revoke(self):
+        namespace = self.parse(["--delete-after-revoke"])
+        self.assertTrue(namespace.delete_after_revoke)
+
+    def test_delete_after_revoke_default(self):
+        namespace = self.parse([])
+        self.assertEqual(namespace.delete_after_revoke, None)
+
+    def test_no_delete_after_revoke(self):
+        namespace = self.parse(["--no-delete-after-revoke"])
+        self.assertFalse(namespace.delete_after_revoke)
+
 
 class DefaultTest(unittest.TestCase):
     """Tests for certbot.cli._Default."""

certbot/tests/main_test.py

@@ -298,25 +298,29 @@ class RevokeTest(test_util.TempDirTestCase):
         self._call()
         self.assertFalse(mock_delete.called)
 
-class DeleteIfAppropriateTest(unittest.TestCase):
+class DeleteIfAppropriateTest(test_util.ConfigTestCase):
     """Tests for certbot.main._delete_if_appropriate """
 
-    def setUp(self):
-        self.config = mock.Mock()
-        self.config.namespace = mock.Mock()
-        self.config.namespace.noninteractive_mode = False
-
     def _call(self, mock_config):
         from certbot.main import _delete_if_appropriate
         _delete_if_appropriate(mock_config)
 
-    @mock.patch('certbot.cert_manager.delete')
+    def _test_delete_opt_out_common(self, mock_get_utility):
+        with mock.patch('certbot.cert_manager.delete') as mock_delete:
+            self._call(self.config)
+        mock_delete.assert_not_called()
+        self.assertTrue(mock_get_utility().add_message.called)
+
     @test_util.patch_get_utility()
-    def test_delete_opt_out(self, mock_get_utility, mock_delete):
+    def test_delete_flag_opt_out(self, mock_get_utility):
+        self.config.delete_after_revoke = False
+        self._test_delete_opt_out_common(mock_get_utility)
+
+    @test_util.patch_get_utility()
+    def test_delete_prompt_opt_out(self, mock_get_utility):
         util_mock = mock_get_utility()
         util_mock.yesno.return_value = False
-        self._call(self.config)
+        self._test_delete_opt_out_common(mock_get_utility)
-        mock_delete.assert_not_called()
 
     # pylint: disable=too-many-arguments
     @mock.patch('certbot.storage.renewal_file_for_certname')
@@ -397,6 +401,28 @@ class DeleteIfAppropriateTest(unittest.TestCase):
         self._call(config)
         self.assertEqual(mock_delete.call_count, 1)
 
+    # pylint: disable=too-many-arguments
+    @mock.patch('certbot.storage.renewal_file_for_certname')
+    @mock.patch('certbot.cert_manager.match_and_check_overlaps')
+    @mock.patch('certbot.storage.full_archive_path')
+    @mock.patch('certbot.cert_manager.cert_path_to_lineage')
+    @mock.patch('certbot.cert_manager.delete')
+    @test_util.patch_get_utility()
+    def test_opt_in_deletion(self, mock_get_utility, mock_delete,
+            mock_cert_path_to_lineage, mock_full_archive_dir,
+            mock_match_and_check_overlaps, mock_renewal_file_for_certname):
+        # pylint: disable = unused-argument
+        config = self.config
+        config.namespace.delete_after_revoke = True
+        config.cert_path = "/some/reasonable/path"
+        config.certname = ""
+        mock_cert_path_to_lineage.return_value = "example.com"
+        mock_full_archive_dir.return_value = ""
+        mock_match_and_check_overlaps.return_value = ""
+        self._call(config)
+        self.assertEqual(mock_delete.call_count, 1)
+        self.assertFalse(mock_get_utility().yesno.called)
+
     # pylint: disable=too-many-arguments
     @mock.patch('certbot.storage.renewal_file_for_certname')
     @mock.patch('certbot.cert_manager.match_and_check_overlaps')

tests/boulder-integration.sh

@@ -345,9 +345,14 @@ common auth --must-staple --domains "must-staple.le.wtf"
 openssl x509 -in "${root}/conf/live/must-staple.le.wtf/cert.pem" -text | grep '1.3.6.1.5.5.7.1.24'
 
 # revoke by account key
-common revoke --cert-path "$root/conf/live/le.wtf/cert.pem"
+common revoke --cert-path "$root/conf/live/le.wtf/cert.pem" --delete-after-revoke
 # revoke renewed
-common revoke --cert-path "$root/conf/live/le1.wtf/cert.pem"
+common revoke --cert-path "$root/conf/live/le1.wtf/cert.pem" --no-delete-after-revoke
+if [ ! -d "$root/conf/live/le1.wtf" ]; then
+    echo "cert deleted when --no-delete-after-revoke was used!"
+    exit 1
+fi
+common delete --cert-name le1.wtf
 # revoke by cert key
 common revoke --cert-path "$root/conf/live/le2.wtf/cert.pem" \
     --key-path "$root/conf/live/le2.wtf/privkey.pem"