diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index ea9c35e1d..49bd2f222 100644 --- a/certbot/CHANGELOG.md +++ b/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. diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 36ebb1a50..f7455db96 100644 --- a/certbot/certbot/_internal/main.py +++ b/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 diff --git a/certbot/certbot/_internal/storage.py b/certbot/certbot/_internal/storage.py index d04bc5774..218a5dd92 100644 --- a/certbot/certbot/_internal/storage.py +++ b/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): diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 2d5d88947..5ad11b120 100644 --- a/certbot/tests/main_test.py +++ b/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') diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 0f696bc34..1e3a1ffd8 100644 --- a/certbot/tests/storage_test.py +++ b/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"]