upstream/certbot
1
0
Fork 0
mirror of https://github.com/certbot/certbot.git synced 2026-06-09 00:32:12 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 7

Handle unexpected key type migration. (#8435)

Browse source 
Fixes #8365

This PR adds a control when `certbot certonly` or `certbot run` are called for a certificate that already exists and would eventually be replaced. As described in #8365, this control is here to ensure that the user will not modify the key type of their certificate (eg. ECDSA to RSA) without an explicit approval (set explicitly `--cert-name` and `--key-type`), since RSA is the default if not specified.

* Handle unexpected key type migration.

* Update certbot-ci/certbot_integration_tests/certbot_tests/test_main.py

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
This commit is contained in:
Adrien Ferrand 2020-11-11 21:36:16 +01:00 committed by GitHub
parent 198f5a99bc
commit 8f5787008d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 102 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
certbot-ci/certbot_integration_tests/certbot_tests/test_main.py
View file

@ -499,6 +499,16 @@ def test_renew_with_ec_keys(context):
assert_elliptic_key(key2, SECP384R1)
assert 280 < os.stat(key2).st_size < 320 # ec keys of 384 bits are ~310 bytes
# We expect here that the command will fail because without --key-type specified,
# Certbot must error out to prevent changing an existing certificate key type,
# without explicit user consent (by specifying both --cert-name and --key-type).
with pytest.raises(subprocess.CalledProcessError):
context.certbot([
'certonly',
'--force-renewal',
'-d', certname
])
def test_ocsp_must_staple(context):
"""Test that OCSP Must-Staple is correctly set in the generated certificate."""

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

@ -11,7 +11,7 @@ import josepy as jose
import zope.component
from acme import errors as acme_errors
from acme.magic_typing import Union, Iterable, Optional # pylint: disable=unused-import
from acme.magic_typing import Union, Iterable, Optional, List, Tuple # pylint: disable=unused-import
import certbot
from certbot import crypto_util
from certbot import errors
@ -130,7 +130,33 @@ def _get_and_save_cert(le_client, config, domains=None, certname=None, lineage=N
return lineage
def _handle_subset_cert_request(config, domains, cert):
def _handle_unexpected_key_type_migration(config, cert):
# type: (configuration.NamespaceConfig, storage.RenewableCert) -> None
"""
This function ensures that the user will not implicitly migrate an existing key
from one type to another in the situation where a certificate for that lineage
already exist and they have not provided explicitly --key-type and --cert-name.
:param config: Current configuration provided by the client
:param cert: Matching certificate that could be renewed
"""
if not cli.set_by_cli("key_type") or not cli.set_by_cli("certname"):
new_key_type = config.key_type.upper()
cur_key_type = cert.private_key_type.upper()
if new_key_type != cur_key_type:
msg = ('Are you trying to change the key type of the certificate named {0} '
'from {1} to {2}? Please provide both --cert-name and --key-type on '
'the command line confirm the change you are trying to make.')
msg = msg.format(cert.lineagename, cur_key_type, new_key_type)
raise errors.Error(msg)
def _handle_subset_cert_request(config, # type: configuration.NamespaceConfig
domains, # type: List[str]
cert # type: storage.RenewableCert
):
# type: (...) -> Tuple[str, Optional[storage.RenewableCert]]
"""Figure out what to do if a previous cert had a subset of the names now requested
:param config: Configuration object
@ -147,6 +173,8 @@ def _handle_subset_cert_request(config, domains, cert):
:rtype: `tuple` of `str`
"""
_handle_unexpected_key_type_migration(config, cert)
existing = ", ".join(cert.names())
question = (
"You have an existing certificate that contains a portion of "
@ -177,7 +205,10 @@ def _handle_subset_cert_request(config, domains, cert):
raise errors.Error(USER_CANCELLED)
def _handle_identical_cert_request(config, lineage):
def _handle_identical_cert_request(config, # type: configuration.NamespaceConfig
lineage, # type: storage.RenewableCert
):
# type: (...) -> Tuple[str, Optional[storage.RenewableCert]]
"""Figure out what to do if a lineage has the same names as a previously obtained one
:param config: Configuration object
@ -191,6 +222,8 @@ def _handle_identical_cert_request(config, lineage):
:rtype: `tuple` of `str`
"""
_handle_unexpected_key_type_migration(config, lineage)
if not lineage.ensure_deployed():
return "reinstall", lineage
if renewal.should_renew(config, lineage):
@ -266,6 +299,7 @@ def _find_lineage_for_domains(config, domains):
return _handle_subset_cert_request(config, domains, subset_names_cert)
return None, None
def _find_cert(config, domains, certname):
"""Finds an existing certificate object given domains and/or a certificate name.
@ -289,7 +323,12 @@ def _find_cert(config, domains, certname):
logger.info("Keeping the existing certificate")
return (action != "reinstall"), lineage
def _find_lineage_for_domains_and_certname(config, domains, certname):
def _find_lineage_for_domains_and_certname(config, # type: configuration.NamespaceConfig
domains, # type: List[str]
certname # type: str
):
# type: (...) -> Tuple[str, Optional[storage.RenewableCert]]
"""Find appropriate lineage based on given domains and/or certname.
:param config: Configuration object
@ -316,8 +355,9 @@ def _find_lineage_for_domains_and_certname(config, domains, certname):
if lineage:
if domains:
if set(cert_manager.domains_for_certname(config, certname)) != set(domains):
_handle_unexpected_key_type_migration(config, lineage)
_ask_user_to_confirm_new_names(config, domains, certname,
lineage.names()) # raises if no
lineage.names()) # raises if no
return "renew", lineage
# unnecessarily specified domains or no domains specified
return _handle_identical_cert_request(config, lineage)
@ -386,6 +426,7 @@ def _ask_user_to_confirm_new_names(config, new_domains, certname, old_domains):
if not obj.yesno(msg, "Update cert", "Cancel", default=True):
raise errors.ConfigurationError("Specified mismatched cert name and domains.")
def _find_domains_or_certname(config, installer, question=None):
"""Retrieve domains and certname from config or user input.

51
certbot/tests/main_test.py
View file

@ -49,14 +49,51 @@ RSA2048_KEY_PATH = test_util.vector_path('rsa2048_key.pem')
SS_CERT_PATH = test_util.vector_path('cert_2048.pem')
class TestHandleIdenticalCerts(unittest.TestCase):
"""Test for certbot._internal.main._handle_identical_cert_request"""
def test_handle_identical_cert_request_pending(self):
class TestHandleCerts(unittest.TestCase):
"""Test for certbot._internal.main._handle_* methods"""
@mock.patch("certbot._internal.main._handle_unexpected_key_type_migration")
def test_handle_identical_cert_request_pending(self, mock_handle_migration):
mock_lineage = mock.Mock()
mock_lineage.ensure_deployed.return_value = False
# pylint: disable=protected-access
ret = main._handle_identical_cert_request(mock.Mock(), mock_lineage)
self.assertEqual(ret, ("reinstall", mock_lineage))
self.assertTrue(mock_handle_migration.called)
@mock.patch("certbot._internal.main._handle_unexpected_key_type_migration")
def test_handle_subset_cert_request(self, mock_handle_migration):
mock_config = mock.Mock()
mock_config.expand = True
mock_lineage = mock.Mock()
mock_lineage.names.return_value = ["dummy1", "dummy2"]
ret = main._handle_subset_cert_request(mock_config, ["dummy1"], mock_lineage)
self.assertEqual(ret, ("renew", mock_lineage))
self.assertTrue(mock_handle_migration.called)
@mock.patch("certbot._internal.main.cli.set_by_cli")
def test_handle_unexpected_key_type_migration(self, mock_set):
config = mock.Mock()
config.key_type = "rsa"
cert = mock.Mock()
cert.private_key_type = "ecdsa"
mock_set.return_value = True
main._handle_unexpected_key_type_migration(config, cert)
mock_set.return_value = False
with self.assertRaises(errors.Error) as raised:
main._handle_unexpected_key_type_migration(config, cert)
self.assertTrue("Please provide both --cert-name and --key-type" in str(raised.exception))
mock_set.side_effect = lambda var: var != "certname"
with self.assertRaises(errors.Error) as raised:
main._handle_unexpected_key_type_migration(config, cert)
self.assertTrue("Please provide both --cert-name and --key-type" in str(raised.exception))
mock_set.side_effect = lambda var: var != "key_type"
with self.assertRaises(errors.Error) as raised:
main._handle_unexpected_key_type_migration(config, cert)
self.assertTrue("Please provide both --cert-name and --key-type" in str(raised.exception))
class RunTest(test_util.ConfigTestCase):
@ -163,9 +200,10 @@ class CertonlyTest(unittest.TestCase):
@mock.patch('certbot._internal.cert_manager.lineage_for_certname')
@mock.patch('certbot._internal.cert_manager.domains_for_certname')
@mock.patch('certbot._internal.renewal.renew_cert')
@mock.patch('certbot._internal.main._handle_unexpected_key_type_migration')
@mock.patch('certbot._internal.main._report_new_cert')
def test_find_lineage_for_domains_and_certname(self, mock_report_cert,
mock_renew_cert, mock_domains, mock_lineage):
mock_handle_type, mock_renew_cert, mock_domains, mock_lineage):
domains = ['example.com', 'test.org']
mock_domains.return_value = domains
mock_lineage.names.return_value = domains
@ -175,6 +213,7 @@ class CertonlyTest(unittest.TestCase):
self.assertTrue(mock_domains.call_count == 1)
self.assertTrue(mock_renew_cert.call_count == 1)
self.assertTrue(mock_report_cert.call_count == 1)
self.assertTrue(mock_handle_type.call_count == 1)
# user confirms updating lineage with new domains
self._call(('certonly --webroot -d example.com -d test.com '
@ -183,11 +222,12 @@ class CertonlyTest(unittest.TestCase):
self.assertTrue(mock_domains.call_count == 2)
self.assertTrue(mock_renew_cert.call_count == 2)
self.assertTrue(mock_report_cert.call_count == 2)
self.assertTrue(mock_handle_type.call_count == 2)
# error in _ask_user_to_confirm_new_names
self.mock_get_utility().yesno.return_value = False
self.assertRaises(errors.ConfigurationError, self._call,
('certonly --webroot -d example.com -d test.com --cert-name example.com').split())
'certonly --webroot -d example.com -d test.com --cert-name example.com'.split())
@mock.patch('certbot._internal.cert_manager.domains_for_certname')
@mock.patch('certbot.display.ops.choose_names')
@ -982,6 +1022,7 @@ class MainTest(test_util.ConfigTestCase):
mock_lineage.should_autorenew.return_value = due_for_renewal
mock_lineage.has_pending_deployment.return_value = False
mock_lineage.names.return_value = ['isnot.org']
mock_lineage.private_key_type = 'RSA'
mock_certr = mock.MagicMock()
mock_key = mock.MagicMock(pem='pem_key')
mock_client = mock.MagicMock()