upstream/certbot
1
0
Fork 0
mirror of https://github.com/certbot/certbot.git synced 2026-06-03 22:08:07 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

Merge pull request #6388 from certbot/local-revoke-certname

Browse source 
revoke accepts --cert-name
This commit is contained in:
ohemorange 2018-09-24 18:44:44 -07:00 committed by GitHub
commit 06174bc113
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 44 additions and 170 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
CHANGELOG.md
View file

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

9
certbot/cli.py
View file

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

57
certbot/main.py
View file

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

128
certbot/tests/main_test.py
View file

@ -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."""

18
tests/certbot-boulder-integration.sh
View file

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