diff --git a/CHANGELOG.md b/CHANGELOG.md index 25aff0a82..def17accc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ Certbot adheres to [Semantic Versioning](http://semver.org/). ### Added -* +* `revoke` accepts `--cert-name`, and doesn't accept both `--cert-name` and `--cert-path`. ### Changed diff --git a/certbot/cli.py b/certbot/cli.py index 2c4aa6530..99bf33180 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -96,7 +96,7 @@ obtain, install, and renew certificates: manage certificates: certificates Display information about certificates you have from Certbot - revoke Revoke a certificate (supply --cert-path) + revoke Revoke a certificate (supply --cert-path or --cert-name) delete Delete a certificate manage your account with Let's Encrypt: @@ -387,9 +387,10 @@ VERB_HELP = [ "usage": "\n\n certbot delete --cert-name CERTNAME\n\n" }), ("revoke", { - "short": "Revoke a certificate specified with --cert-path", + "short": "Revoke a certificate specified with --cert-path or --cert-name", "opts": "Options for revocation of certificates", - "usage": "\n\n certbot revoke --cert-path /path/to/fullchain.pem [options]\n\n" + "usage": "\n\n certbot revoke [--cert-path /path/to/fullchain.pem | " + "--cert-name example.com] [options]\n\n" }), ("register", { "short": "Register for account with Let's Encrypt / other ACME server", @@ -1333,7 +1334,7 @@ def _paths_parser(helpful): add(sections, "--cert-path", type=os.path.abspath, default=flag_default("auth_cert_path"), help=cph) elif verb == "revoke": - add(sections, "--cert-path", type=read_file, required=True, help=cph) + add(sections, "--cert-path", type=read_file, required=False, help=cph) else: add(sections, "--cert-path", type=os.path.abspath, help=cph) diff --git a/certbot/main.py b/certbot/main.py index 214378da8..6cd2bbfac 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -532,8 +532,7 @@ def _determine_account(config): def _delete_if_appropriate(config): # pylint: disable=too-many-locals,too-many-branches """Does the user want to delete their now-revoked certs? If run in non-interactive mode, - deleting happens automatically, unless if both `--cert-name` and `--cert-path` were - specified with conflicting values. + deleting happens automatically. :param config: parsed command line arguments :type config: interfaces.IConfig @@ -557,50 +556,13 @@ def _delete_if_appropriate(config): # pylint: disable=too-many-locals,too-many-b reporter_util.add_message("Not deleting revoked certs.", reporter_util.LOW_PRIORITY) return - if not (config.certname or config.cert_path): - raise errors.Error('At least one of --cert-path or --cert-name must be specified.') + # config.cert_path must have been set + # config.certname may have been set + assert config.cert_path - if config.certname and config.cert_path: - # first, check if certname and cert_path imply the same certs - implied_cert_name = cert_manager.cert_path_to_lineage(config) - - if implied_cert_name != config.certname: - cert_path_implied_cert_name = cert_manager.cert_path_to_lineage(config) - cert_path_implied_conf = storage.renewal_file_for_certname(config, - cert_path_implied_cert_name) - cert_path_cert = storage.RenewableCert(cert_path_implied_conf, config) - cert_path_info = cert_manager.human_readable_cert_info(config, cert_path_cert, - skip_filter_checks=True) - - cert_name_implied_conf = storage.renewal_file_for_certname(config, config.certname) - cert_name_cert = storage.RenewableCert(cert_name_implied_conf, config) - cert_name_info = cert_manager.human_readable_cert_info(config, cert_name_cert) - - msg = ("You specified conflicting values for --cert-path and --cert-name. " - "Which did you mean to select?") - choices = [cert_path_info, cert_name_info] - try: - code, index = display.menu(msg, - choices, ok_label="Select", force_interactive=True) - except errors.MissingCommandlineFlag: - error_msg = ('To run in non-interactive mode, you must either specify only one of ' - '--cert-path or --cert-name, or both must point to the same certificate lineages.') - raise errors.Error(error_msg) - - if code != display_util.OK or not index in range(0, len(choices)): - raise errors.Error("User ended interaction.") - - if index == 0: - config.certname = cert_path_implied_cert_name - else: - config.cert_path = storage.cert_path_for_cert_name(config, config.certname) - - elif config.cert_path: + if not config.certname: config.certname = cert_manager.cert_path_to_lineage(config) - else: # if only config.certname was specified - config.cert_path = storage.cert_path_for_cert_name(config, config.certname) - # don't delete if the archive_dir is used by some other lineage archive_dir = storage.full_archive_path( configobj.ConfigObj(storage.renewal_file_for_certname(config, config.certname)), @@ -1066,6 +1028,14 @@ def revoke(config, unused_plugins): # TODO: coop with renewal config """ # For user-agent construction 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) + 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 + raise errors.Error("Error! Exactly one of --cert-path or --cert-name must be specified!") + if config.key_path is not None: # revocation by cert key logger.debug("Revoking %s using cert key %s", config.cert_path[0], config.key_path[0]) @@ -1078,7 +1048,6 @@ def revoke(config, unused_plugins): # TODO: coop with renewal config acme = client.acme_from_config_key(config, acc.key, acc.regr) cert = crypto_util.pyopenssl_load_certificate(config.cert_path[1])[0] logger.debug("Reason code for revocation: %s", config.reason) - try: acme.revoke(jose.ComparableX509(cert), config.reason) _delete_if_appropriate(config) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 47ca235f1..8334068c9 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -240,6 +240,8 @@ class RevokeTest(test_util.TempDirTestCase): shutil.copy(CERT_PATH, self.tempdir) self.tmp_cert_path = os.path.abspath(os.path.join(self.tempdir, 'cert_512.pem')) + with open(self.tmp_cert_path, 'r') as f: + self.tmp_cert = (self.tmp_cert_path, f.read()) self.patches = [ mock.patch('acme.client.BackwardsCompatibleClientV2'), @@ -269,9 +271,10 @@ class RevokeTest(test_util.TempDirTestCase): for patch in self.patches: patch.stop() - def _call(self, extra_args=""): - args = 'revoke --cert-path={0} ' + extra_args - args = args.format(self.tmp_cert_path).split() + def _call(self, args=None): + if not args: + args = 'revoke --cert-path={0} ' + args = args.format(self.tmp_cert_path).split() plugins = disco.PluginsRegistry.find_all() config = configuration.NamespaceConfig( cli.prepare_and_parse_args(plugins, args)) @@ -287,12 +290,25 @@ class RevokeTest(test_util.TempDirTestCase): mock_revoke = mock_acme_client.BackwardsCompatibleClientV2().revoke expected = [] for reason, code in constants.REVOCATION_REASONS.items(): - self._call("--reason " + reason) + args = 'revoke --cert-path={0} --reason {1}'.format(self.tmp_cert_path, reason).split() + self._call(args) expected.append(mock.call(mock.ANY, code)) - self._call("--reason " + reason.upper()) + args = 'revoke --cert-path={0} --reason {1}'.format(self.tmp_cert_path, + reason.upper()).split() + self._call(args) expected.append(mock.call(mock.ANY, code)) self.assertEqual(expected, mock_revoke.call_args_list) + @mock.patch('certbot.main._delete_if_appropriate') + @mock.patch('certbot.storage.cert_path_for_cert_name') + def test_revoke_by_certname(self, mock_cert_path_for_cert_name, + mock_delete_if_appropriate): + args = 'revoke --cert-name=example.com'.split() + mock_cert_path_for_cert_name.return_value = self.tmp_cert + mock_delete_if_appropriate.return_value = False + self._call(args) + self.mock_success_revoke.assert_called_once_with(self.tmp_cert_path) + @mock.patch('certbot.main._delete_if_appropriate') def test_revocation_success(self, mock_delete_if_appropriate): self._call() @@ -359,25 +375,6 @@ class DeleteIfAppropriateTest(test_util.ConfigTestCase): self._call(config) mock_delete.assert_not_called() - # 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.delete') - @mock.patch('certbot.storage.cert_path_for_cert_name') - @test_util.patch_get_utility() - def test_cert_name_only(self, mock_get_utility, - mock_cert_path_for_cert_name, mock_delete, mock_archive, - mock_overlapping_archive_dirs, mock_renewal_file_for_certname): - # pylint: disable = unused-argument - config = self.config - config.certname = "example.com" - config.cert_path = "" - mock_cert_path_for_cert_name.return_value = "/some/reasonable/path" - mock_overlapping_archive_dirs.return_value = False - 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') @@ -440,89 +437,6 @@ class DeleteIfAppropriateTest(test_util.ConfigTestCase): 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') - @mock.patch('certbot.storage.full_archive_path') - @mock.patch('certbot.cert_manager.delete') - @mock.patch('certbot.cert_manager.cert_path_to_lineage') - @test_util.patch_get_utility() - def test_certname_and_cert_path_match(self, mock_get_utility, - mock_cert_path_to_lineage, mock_delete, mock_archive, - mock_overlapping_archive_dirs, mock_renewal_file_for_certname): - # pylint: disable = unused-argument - config = self.config - config.certname = "example.com" - config.cert_path = "/some/reasonable/path" - mock_cert_path_to_lineage.return_value = config.certname - mock_overlapping_archive_dirs.return_value = False - self._call(config) - self.assertEqual(mock_delete.call_count, 1) - - # pylint: disable=too-many-arguments - @mock.patch('certbot.cert_manager.match_and_check_overlaps') - @mock.patch('certbot.storage.full_archive_path') - @mock.patch('certbot.cert_manager.delete') - @mock.patch('certbot.cert_manager.human_readable_cert_info') - @mock.patch('certbot.storage.RenewableCert') - @mock.patch('certbot.storage.renewal_file_for_certname') - @mock.patch('certbot.cert_manager.cert_path_to_lineage') - @test_util.patch_get_utility() - def test_certname_and_cert_path_mismatch(self, mock_get_utility, - mock_cert_path_to_lineage, mock_renewal_file_for_certname, - mock_RenewableCert, mock_human_readable_cert_info, - mock_delete, mock_archive, mock_overlapping_archive_dirs): - # pylint: disable=unused-argument - config = self.config - config.certname = "example.com" - config.cert_path = "/some/reasonable/path" - mock_cert_path_to_lineage = "something else" - mock_RenewableCert.return_value = mock.Mock() - mock_human_readable_cert_info.return_value = "" - mock_overlapping_archive_dirs.return_value = False - from certbot.display import util as display_util - util_mock = mock_get_utility() - util_mock.menu.return_value = (display_util.OK, 0) - self._call(config) - self.assertEqual(mock_delete.call_count, 1) - - # pylint: disable=too-many-arguments - @mock.patch('certbot.cert_manager.match_and_check_overlaps') - @mock.patch('certbot.storage.full_archive_path') - @mock.patch('certbot.cert_manager.delete') - @mock.patch('certbot.cert_manager.human_readable_cert_info') - @mock.patch('certbot.storage.RenewableCert') - @mock.patch('certbot.storage.renewal_file_for_certname') - @mock.patch('certbot.cert_manager.cert_path_to_lineage') - @test_util.patch_get_utility() - def test_noninteractive_certname_cert_path_mismatch(self, mock_get_utility, - mock_cert_path_to_lineage, mock_renewal_file_for_certname, - mock_RenewableCert, mock_human_readable_cert_info, - mock_delete, mock_archive, mock_overlapping_archive_dirs): - # pylint: disable=unused-argument - config = self.config - config.certname = "example.com" - config.cert_path = "/some/reasonable/path" - mock_cert_path_to_lineage.return_value = "some-reasonable-path.com" - mock_RenewableCert.return_value = mock.Mock() - mock_human_readable_cert_info.return_value = "" - mock_overlapping_archive_dirs.return_value = False - # Test for non-interactive mode - util_mock = mock_get_utility() - util_mock.menu.side_effect = errors.MissingCommandlineFlag("Oh no.") - self.assertRaises(errors.Error, self._call, config) - mock_delete.assert_not_called() - - @mock.patch('certbot.cert_manager.delete') - @test_util.patch_get_utility() - def test_no_certname_or_cert_path(self, mock_get_utility, mock_delete): - # pylint: disable=unused-argument - config = self.config - config.certname = None - config.cert_path = None - self.assertRaises(errors.Error, self._call, config) - mock_delete.assert_not_called() - class DetermineAccountTest(test_util.ConfigTestCase): """Tests for certbot.main._determine_account.""" diff --git a/tests/certbot-boulder-integration.sh b/tests/certbot-boulder-integration.sh index 8a8e60798..e250e591b 100755 --- a/tests/certbot-boulder-integration.sh +++ b/tests/certbot-boulder-integration.sh @@ -440,25 +440,15 @@ for subdomain in $subdomains; do fi done -# Test that revocation raises correct error if --cert-name and --cert-path don't match +# Test that revocation raises correct error when both --cert-name and --cert-path specified common --domains le1.wtf -common --domains le2.wtf -out=$(common revoke --cert-path "$root/conf/live/le1.wtf/fullchain.pem" --cert-name "le2.wtf" 2>&1) || true -if ! echo $out | grep "or both must point to the same certificate lineages."; then - echo "Non-interactive revoking with mismatched --cert-name and --cert-path " +out=$(common revoke --cert-path "$root/conf/live/le1.wtf/fullchain.pem" --cert-name "le1.wtf" 2>&1) || true +if ! echo $out | grep "Exactly one of --cert-path or --cert-name must be specified"; then + echo "Non-interactive revoking with both --cert-name and --cert-path " echo "did not raise the correct error!" exit 1 fi -# Revoking by matching --cert-name and --cert-path deletes -common --domains le1.wtf -common revoke --cert-path "$root/conf/live/le1.wtf/fullchain.pem" --cert-name "le1.wtf" -out=$(common certificates) -if echo $out | grep "le1.wtf"; then - echo "Cert le1.wtf should've been deleted! Was revoked via matching --cert-path & --cert-name" - exit 1 -fi - # Test that revocation doesn't delete if multiple lineages share an archive dir common --domains le1.wtf common --domains le2.wtf