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

cli: make key_path and cert_path always be a str (#8687)

Browse source 
There is some code in [`_paths_parser`](ae3ed200c0/certbot/certbot/_internal/cli/paths_parser.py (L17-L34)) which has the effect of varying the value type of `config.cert_path` and `config.key_path` based on the CLI verb. When the verb is `revoke`, the type is a tuple `(path: str, contents: bytes)`, otherwise it is a single `str` representing the file path. (I wasn't able to find a written reason as to why it works this way).

This commit removes that special `revoke` case and ensures it is always a `str`.

Why change it now?

I am trying to write some changes and there's some code in `cert_manager` which only works if the verb is `revoke`, you hack `config.cert_path` to be a tuple beforehand, or you [(not actually in `master`) try sniff for the value type](49911afaa6/certbot/certbot/_internal/cert_manager.py (L224-L227)). I have a bad feeling about such workarounds. I would prefer to just make these variables simpler to use, but I'm open to opinions.

In addition to the test suites, I've manually tested `revoke` (including by `--key-path`) and `install`. Are there other places I may have missed?

Unblocks #8636 and #8671.
This commit is contained in:
alexzorin 2021-02-26 06:32:21 +11:00 committed by GitHub
parent 025eb16c7a
commit f71298f661
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 34 additions and 39 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
certbot/certbot/_internal/cert_manager.py
View file

@ -223,7 +223,7 @@ def cert_path_to_lineage(cli_config):
"""
acceptable_matches = _acceptable_matches()
match = match_and_check_overlaps(cli_config, acceptable_matches,
lambda x: cli_config.cert_path[0], lambda x: x.lineagename)
lambda x: cli_config.cert_path, lambda x: x.lineagename)
return match[0]
@ -254,7 +254,7 @@ def match_and_check_overlaps(cli_config, acceptable_matches, match_func, rv_func
matched = _search_lineages(cli_config, find_matches, [], acceptable_matches)
if not matched:
raise errors.Error("No match found for cert-path {0}!".format(cli_config.cert_path[0]))
raise errors.Error("No match found for cert-path {0}!".format(cli_config.cert_path))
elif len(matched) > 1:
raise errors.OverlappingMatchFound()
return matched

20
certbot/certbot/_internal/cli/paths_parser.py
View file

@ -2,7 +2,6 @@
paths for certificates"""
from certbot.compat import os
from certbot._internal.cli import (
read_file,
flag_default,
config_help
)
@ -14,22 +13,21 @@ def _paths_parser(helpful):
if verb == "help":
verb = helpful.help_arg
cph = "Path to where certificate is saved (with auth --csr), installed from, or revoked."
sections = ["paths", "install", "revoke", "certonly", "manage"]
cpkwargs = {
"type": os.path.abspath,
"help": "Path to where certificate is saved (with certonly --csr), installed "
"from, or revoked"
}
if verb == "certonly":
add(sections, "--cert-path", type=os.path.abspath,
default=flag_default("auth_cert_path"), help=cph)
cpkwargs["default"] = flag_default("auth_cert_path")
elif verb == "revoke":
add(sections, "--cert-path", type=read_file, required=False, help=cph)
else:
add(sections, "--cert-path", type=os.path.abspath, help=cph)
cpkwargs["required"] = False
add(["paths", "install", "revoke", "certonly", "manage"], "--cert-path", **cpkwargs)
section = "paths"
if verb in ("install", "revoke"):
section = verb
# revoke --key-path reads a file, install --key-path takes a string
add(section, "--key-path",
type=((verb == "revoke" and read_file) or os.path.abspath),
add(section, "--key-path", type=os.path.abspath,
help="Path to private key for certificate installation "
"or revocation (if account key is missing)")

15
certbot/certbot/_internal/main.py
View file

@ -1098,15 +1098,18 @@ def revoke(config, unused_plugins):
if config.key_path is not None: # revocation by cert key
logger.debug("Revoking %s using certificate key %s",
config.cert_path[0], config.key_path[0])
crypto_util.verify_cert_matches_priv_key(config.cert_path[0], config.key_path[0])
key = jose.JWK.load(config.key_path[1])
config.cert_path, config.key_path)
crypto_util.verify_cert_matches_priv_key(config.cert_path, config.key_path)
with open(config.key_path, 'rb') as f:
key = jose.JWK.load(f.read())
acme = client.acme_from_config_key(config, key)
else: # revocation by account key
logger.debug("Revoking %s using Account Key", config.cert_path[0])
logger.debug("Revoking %s using Account Key", config.cert_path)
acc, _ = _determine_account(config)
acme = client.acme_from_config_key(config, acc.key, acc.regr)
cert = crypto_util.pyopenssl_load_certificate(config.cert_path[1])[0]
with open(config.cert_path, 'rb') as f:
cert = crypto_util.pyopenssl_load_certificate(f.read())[0]
logger.debug("Reason code for revocation: %s", config.reason)
try:
acme.revoke(jose.ComparableX509(cert), config.reason)
@ -1114,7 +1117,7 @@ def revoke(config, unused_plugins):
except acme_errors.ClientError as e:
return str(e)
display_ops.success_revocation(config.cert_path[0])
display_ops.success_revocation(config.cert_path)
return None

7
certbot/certbot/_internal/storage.py
View file

@ -58,7 +58,7 @@ def renewal_file_for_certname(config, certname):
return path
def cert_path_for_cert_name(config, cert_name):
def cert_path_for_cert_name(config: interfaces.IConfig, cert_name: str) -> str:
""" If `--cert-name` was specified, but you need a value for `--cert-path`.
:param `configuration.NamespaceConfig` config: parsed command line arguments
@ -66,10 +66,7 @@ def cert_path_for_cert_name(config, cert_name):
"""
cert_name_implied_conf = renewal_file_for_certname(config, cert_name)
fullchain_path = configobj.ConfigObj(cert_name_implied_conf)["fullchain"]
with open(fullchain_path) as f:
cert_path = (fullchain_path, f.read())
return cert_path
return configobj.ConfigObj(cert_name_implied_conf)["fullchain"]
def config_with_defaults(config=None):

16
certbot/tests/cert_manager_test.py
View file

@ -526,7 +526,7 @@ class CertPathToLineageTest(storage_test.BaseRenewableCertTest):
self._write_out_ex_kinds()
self.fullchain = os.path.join(self.config.config_dir, 'live', 'example.org',
'fullchain.pem')
self.config.cert_path = (self.fullchain, '')
self.config.cert_path = self.fullchain
def _call(self, cli_config):
from certbot._internal.cert_manager import cert_path_to_lineage
@ -556,21 +556,21 @@ class CertPathToLineageTest(storage_test.BaseRenewableCertTest):
mock_acceptable_matches.return_value = [lambda x: x.cert_path]
test_cert_path = os.path.join(self.config.config_dir, 'live', 'example.org',
'cert.pem')
self.config.cert_path = (test_cert_path, '')
self.config.cert_path = test_cert_path
self.assertEqual('example.org', self._call(self.config))
@mock.patch('certbot._internal.cert_manager._acceptable_matches')
def test_options_archive_cert(self, mock_acceptable_matches):
# Also this and the next test check that the regex of _archive_files is working.
self.config.cert_path = (os.path.join(self.config.config_dir, 'archive', 'example.org',
'cert11.pem'), '')
self.config.cert_path = os.path.join(self.config.config_dir, 'archive', 'example.org',
'cert11.pem')
mock_acceptable_matches.return_value = [lambda x: self._archive_files(x, 'cert')]
self.assertEqual('example.org', self._call(self.config))
@mock.patch('certbot._internal.cert_manager._acceptable_matches')
def test_options_archive_fullchain(self, mock_acceptable_matches):
self.config.cert_path = (os.path.join(self.config.config_dir, 'archive',
'example.org', 'fullchain11.pem'), '')
self.config.cert_path = os.path.join(self.config.config_dir, 'archive',
'example.org', 'fullchain11.pem')
mock_acceptable_matches.return_value = [lambda x:
self._archive_files(x, 'fullchain')]
self.assertEqual('example.org', self._call(self.config))
@ -586,7 +586,7 @@ class MatchAndCheckOverlaps(storage_test.BaseRenewableCertTest):
self._write_out_ex_kinds()
self.fullchain = os.path.join(self.config.config_dir, 'live', 'example.org',
'fullchain.pem')
self.config.cert_path = (self.fullchain, '')
self.config.cert_path = self.fullchain
def _call(self, cli_config, acceptable_matches, match_func, rv_func):
from certbot._internal.cert_manager import match_and_check_overlaps
@ -595,7 +595,7 @@ class MatchAndCheckOverlaps(storage_test.BaseRenewableCertTest):
def test_basic_match(self):
from certbot._internal.cert_manager import _acceptable_matches
self.assertEqual(['example.org'], self._call(self.config, _acceptable_matches(),
lambda x: self.config.cert_path[0], lambda x: x.lineagename))
lambda x: self.config.cert_path, lambda x: x.lineagename))
@mock.patch('certbot._internal.cert_manager._search_lineages')
def test_no_matches(self, mock_search_lineages):

7
certbot/tests/main_test.py
View file

@ -287,10 +287,7 @@ class RevokeTest(test_util.TempDirTestCase):
super(RevokeTest, self).setUp()
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.tmp_cert_path = os.path.abspath(os.path.join(self.tempdir, 'cert_512.pem'))
patches = [
mock.patch('acme.client.BackwardsCompatibleClientV2'),
@ -349,7 +346,7 @@ class RevokeTest(test_util.TempDirTestCase):
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_cert_path_for_cert_name.return_value = self.tmp_cert_path
mock_delete_if_appropriate.return_value = False
self._call(args)
self.mock_success_revoke.assert_called_once_with(self.tmp_cert_path)

4
certbot/tests/storage_test.py
View file

@ -934,14 +934,14 @@ class CertPathForCertNameTest(BaseRenewableCertTest):
self._write_out_ex_kinds()
self.fullchain = os.path.join(self.config.config_dir, 'live', 'example.org',
'fullchain.pem')
self.config.cert_path = (self.fullchain, '')
self.config.cert_path = self.fullchain
def _call(self, cli_config, certname):
from certbot._internal.storage import cert_path_for_cert_name
return cert_path_for_cert_name(cli_config, certname)
def test_simple_cert_name(self):
self.assertEqual(self._call(self.config, 'example.org'), (self.fullchain, 'fullchain'))
self.assertEqual(self._call(self.config, 'example.org'), self.fullchain)
def test_no_such_cert_name(self):
self.assertRaises(errors.CertStorageError, self._call, self.config, 'fake-example.org')