revoke: try determine the server automatically (#8691)

* revoke: try determine the server automatically

When revoking via --cert-name, use the server from the lineage (unless
overriden by the CLI).

* RenewableCert.storage might be None

* guard against an empty lineage server

certbot/CHANGELOG.md

@@ -21,6 +21,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
   Python 2.
 * Certbot and all of its components no longer depend on the library `six`.
 * The update of certbot-auto itself is now disabled on all RHEL-like systems.
+* When revoking a certificate by `--cert-name`, it is no longer necessary to specify the `--server`
+  if the certificate was obtained from a non-default ACME server.
 * The nginx authenticator now configures all matching HTTP and HTTPS vhosts for the HTTP-01
   challenge. It is now compatible with external HTTPS redirection by a CDN or load balancer.
 

certbot/certbot/_internal/main.py

@@ -1072,8 +1072,7 @@ def certificates(config, unused_plugins):
     cert_manager.certificates(config)
 
 
-# TODO: coop with renewal config
-def revoke(config, unused_plugins):
+def revoke(config, unused_plugins: plugins_disco.PluginsRegistry) -> Optional[str]:
     """Revoke a previously obtained certificate.
 
     :param config: Configuration object
@@ -1090,7 +1089,13 @@ def revoke(config, unused_plugins):
     config.installer = config.authenticator = None
 
     if config.cert_path is None and config.certname:
-        config.cert_path = storage.cert_path_for_cert_name(config, config.certname)
+        # When revoking via --cert-name, take the cert path and server from renewalparams
+        lineage = storage.RenewableCert(
+            storage.renewal_file_for_certname(config, config.certname), config)
+        config.cert_path = lineage.cert_path
+        # --server takes priority over lineage.server
+        if lineage.server and not cli.set_by_cli("server"):
+            config.server = lineage.server
     elif not config.cert_path or (config.cert_path and config.certname):
         # intentionally not supporting --cert-path & --cert-name together,
         # to avoid dealing with mismatched values

certbot/certbot/_internal/storage.py

@@ -6,6 +6,7 @@ import re
 import shutil
 import stat
 
+from typing import Optional
 import configobj
 import parsedatetime
 import pytz
@@ -518,11 +519,15 @@ class RenewableCert(interfaces.RenewableCert):
         return _relpath_from_file(self.archive_dir, from_file)
 
     @property
-    def is_test_cert(self):
+    def server(self) -> Optional[str]:
+        """Returns the ACME server associated with this certificate"""
+        return self.configuration["renewalparams"].get("server", None)
+
+    @property
+    def is_test_cert(self) -> bool:
         """Returns true if this is a test cert from a staging server."""
-        server = self.configuration["renewalparams"].get("server", None)
-        if server:
-            return util.is_staging(server)
+        if self.server:
+            return util.is_staging(self.server)
         return False
 
     def _check_symlinks(self):

certbot/tests/main_test.py

@@ -317,6 +317,7 @@ class RevokeTest(test_util.TempDirTestCase):
         if not args:
             args = 'revoke --cert-path={0} '
             args = args.format(self.tmp_cert_path).split()
+        cli.set_by_cli.detector = None # required to reset set_by_cli state
         plugins = disco.PluginsRegistry.find_all()
         config = configuration.NamespaceConfig(
             cli.prepare_and_parse_args(plugins, args))
@@ -342,13 +343,44 @@ class RevokeTest(test_util.TempDirTestCase):
         self.assertEqual(expected, mock_revoke.call_args_list)
 
     @mock.patch('certbot._internal.main._delete_if_appropriate')
-    @mock.patch('certbot._internal.storage.cert_path_for_cert_name')
-    def test_revoke_by_certname(self, mock_cert_path_for_cert_name,
-            mock_delete_if_appropriate):
+    @mock.patch('certbot._internal.storage.RenewableCert')
+    @mock.patch('certbot._internal.storage.renewal_file_for_certname')
+    def test_revoke_by_certname(self, unused_mock_renewal_file_for_certname,
+                                mock_cert, mock_delete_if_appropriate):
+        mock_cert.return_value = mock.MagicMock(cert_path=self.tmp_cert_path,
+                                                server="https://acme.example")
         args = 'revoke --cert-name=example.com'.split()
-        mock_cert_path_for_cert_name.return_value = self.tmp_cert_path
         mock_delete_if_appropriate.return_value = False
         self._call(args)
+        self.mock_acme_client.assert_called_once_with(mock.ANY, mock.ANY, 'https://acme.example')
+        self.mock_success_revoke.assert_called_once_with(self.tmp_cert_path)
+
+    @mock.patch('certbot._internal.main._delete_if_appropriate')
+    @mock.patch('certbot._internal.storage.RenewableCert')
+    @mock.patch('certbot._internal.storage.renewal_file_for_certname')
+    def test_revoke_by_certname_and_server(self, unused_mock_renewal_file_for_certname,
+                                           mock_cert, mock_delete_if_appropriate):
+        """Revoking with --server should use the server from the CLI"""
+        mock_cert.return_value = mock.MagicMock(cert_path=self.tmp_cert_path,
+                                                server="https://acme.example")
+        args = 'revoke --cert-name=example.com --server https://other.example'.split()
+        mock_delete_if_appropriate.return_value = False
+        self._call(args)
+        self.mock_acme_client.assert_called_once_with(mock.ANY, mock.ANY, 'https://other.example')
+        self.mock_success_revoke.assert_called_once_with(self.tmp_cert_path)
+
+    @mock.patch('certbot._internal.main._delete_if_appropriate')
+    @mock.patch('certbot._internal.storage.RenewableCert')
+    @mock.patch('certbot._internal.storage.renewal_file_for_certname')
+    def test_revoke_by_certname_empty_server(self, unused_mock_renewal_file_for_certname,
+                                             mock_cert, mock_delete_if_appropriate):
+        """Revoking with --cert-name where the lineage server is empty shouldn't crash """
+        mock_cert.return_value = mock.MagicMock(cert_path=self.tmp_cert_path, server=None)
+        args = 'revoke --cert-name=example.com'.split()
+        mock_delete_if_appropriate.return_value = False
+        self._call(args)
+        self.mock_acme_client.assert_called_once_with(
+            mock.ANY, mock.ANY, constants.CLI_DEFAULTS['server'])
         self.mock_success_revoke.assert_called_once_with(self.tmp_cert_path)
 
     @mock.patch('certbot._internal.main._delete_if_appropriate')

certbot/tests/storage_test.py

@@ -765,6 +765,13 @@ class RenewableCertTests(BaseRenewableCertTest):
             self.assertEqual(storage.add_time_interval(base_time, interval),
                              excepted)
 
+    def test_server(self):
+        self.test_rc.configuration["renewalparams"] = {}
+        self.assertEqual(self.test_rc.server, None)
+        rp = self.test_rc.configuration["renewalparams"]
+        rp["server"] = "https://acme.example/dir"
+        self.assertEqual(self.test_rc.server, "https://acme.example/dir")
+
     def test_is_test_cert(self):
         self.test_rc.configuration["renewalparams"] = {}
         rp = self.test_rc.configuration["renewalparams"]