diff --git a/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py b/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py index 272084217..3650f64f0 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py @@ -37,16 +37,19 @@ def assert_elliptic_key(key: str, curve: Type[EllipticCurve]) -> None: assert isinstance(key.curve, curve) -def assert_rsa_key(key: str) -> None: +def assert_rsa_key(key: str, key_size: Optional[int] = None) -> None: """ Asserts that the key at the given path is an RSA key. :param str key: path to key + :param int key_size: if provided, assert that the RSA key is of this size """ with open(key, 'rb') as file: privkey1 = file.read() key = load_pem_private_key(data=privkey1, password=None, backend=default_backend()) assert isinstance(key, RSAPrivateKey) + if key_size: + assert key_size == key.key_size def assert_hook_execution(probe_path: str, probe_content: str) -> None: diff --git a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py index 4a3395217..2827ae939 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py @@ -8,6 +8,7 @@ import subprocess import time from typing import Iterable from typing import Generator +from typing import Tuple from typing import Type from cryptography.hazmat.primitives.asymmetric.ec import EllipticCurve @@ -463,6 +464,42 @@ def test_reuse_key(context: IntegrationTestsContext) -> None: assert len({cert1, cert2, cert3}) == 3 +def test_new_key(context: IntegrationTestsContext) -> None: + """Tests --new-key and its interactions with --reuse-key""" + def private_key(generation: int) -> Tuple[str, str]: + pk_path = join(context.config_dir, f'archive/{certname}/privkey{generation}.pem') + with open(pk_path, 'r') as file: + return file.read(), pk_path + + certname = context.get_domain('newkey') + + context.certbot(['--domains', certname, '--reuse-key', + '--key-type', 'rsa', '--rsa-key-size', '4096']) + privkey1, _ = private_key(1) + + # renew: --new-key should replace the key, but keep reuse_key and the key type + params + context.certbot(['renew', '--cert-name', certname, '--new-key']) + privkey2, privkey2_path = private_key(2) + assert privkey1 != privkey2 + assert_saved_lineage_option(context.config_dir, certname, 'reuse_key', 'True') + assert_rsa_key(privkey2_path, 4096) + + # certonly: it should replace the key but the key size will change + context.certbot(['certonly', '-d', certname, '--reuse-key', '--new-key']) + privkey3, privkey3_path = private_key(3) + assert privkey2 != privkey3 + assert_saved_lineage_option(context.config_dir, certname, 'reuse_key', 'True') + assert_rsa_key(privkey3_path, 2048) + + # certonly: it should be possible to change the key type and keep reuse_key + context.certbot(['certonly', '-d', certname, '--reuse-key', '--new-key', '--key-type', 'ecdsa', + '--cert-name', certname]) + privkey4, privkey4_path = private_key(4) + assert privkey3 != privkey4 + assert_saved_lineage_option(context.config_dir, certname, 'reuse_key', 'True') + assert_elliptic_key(privkey4_path, SECP256R1) + + def test_incorrect_key_type(context: IntegrationTestsContext) -> None: with pytest.raises(subprocess.CalledProcessError): context.certbot(['--key-type="failwhale"']) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 4258ac580..6a6d39af0 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -6,7 +6,10 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Added -* +* Added `--new-key`. When renewing or replacing a certificate that has `--reuse-key` + set, it will force a new private key to be generated. + Combining `--reuse-key` and `--new-key` will replace the certificate's private key + and then reuse it for future renewals. ### Changed diff --git a/certbot/certbot/_internal/cli/__init__.py b/certbot/certbot/_internal/cli/__init__.py index d11a454b1..30b3fab1d 100644 --- a/certbot/certbot/_internal/cli/__init__.py +++ b/certbot/certbot/_internal/cli/__init__.py @@ -223,6 +223,13 @@ def prepare_and_parse_args(plugins: plugins_disco.PluginsRegistry, args: List[st "certificate. Not reusing private keys is the default behavior of " "Certbot. This option may be used to unset --reuse-key on an " "existing certificate.") + helpful.add( + "automation", "--new-key", + dest="new_key", action="store_true", default=flag_default("new_key"), + help="When renewing or replacing a certificate, generate a new private key, " + "even if --reuse-key is set on the existing certificate. Combining " + "--new-key and --reuse-key will result in the private key being replaced and " + "then reused in future renewals.") helpful.add( ["automation", "renew", "certonly"], diff --git a/certbot/certbot/_internal/constants.py b/certbot/certbot/_internal/constants.py index 3867d777c..5a9d97d83 100644 --- a/certbot/certbot/_internal/constants.py +++ b/certbot/certbot/_internal/constants.py @@ -74,6 +74,7 @@ CLI_DEFAULTS: Dict[str, Any] = dict( # noqa validate_hooks=True, directory_hooks=True, reuse_key=False, + new_key=False, disable_renew_updates=False, random_sleep_on_renew=True, eab_hmac_key=None, diff --git a/certbot/certbot/_internal/renewal.py b/certbot/certbot/_internal/renewal.py index 4fb2ca00a..0ba2e8108 100644 --- a/certbot/certbot/_internal/renewal.py +++ b/certbot/certbot/_internal/renewal.py @@ -336,7 +336,7 @@ def renew_cert(config: configuration.NamespaceConfig, domains: Optional[List[str domains = lineage.names() # The private key is the existing lineage private key if reuse_key is set. # Otherwise, generate a fresh private key by passing None. - if config.reuse_key: + if config.reuse_key and not config.new_key: new_key = os.path.normpath(lineage.privkey) _update_renewal_params_from_key(new_key, config) else: diff --git a/certbot/certbot/configuration.py b/certbot/certbot/configuration.py index ebeb8e98c..d5ad87599 100644 --- a/certbot/certbot/configuration.py +++ b/certbot/certbot/configuration.py @@ -300,6 +300,13 @@ class NamespaceConfig: """ return self.namespace.issuance_timeout + @property + def new_key(self) -> bool: + """This option specifies whether Certbot should generate a new private + key when replacing a certificate, even if reuse_key is set. + """ + return self.namespace.new_key + # Magic methods def __deepcopy__(self, _memo: Any) -> 'NamespaceConfig': diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 6b09f1ba2..cf523abac 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1155,7 +1155,7 @@ class MainTest(test_util.ConfigTestCase): def _test_renewal_common(self, due_for_renewal, extra_args, log_out=None, args=None, should_renew=True, error_expected=False, quiet_mode=False, expiry_date=datetime.datetime.now(), - reuse_key=False): + reuse_key=False, new_key=False): cert_path = test_util.vector_path('cert_512.pem') chain_path = os.path.normpath(os.path.join(self.config.config_dir, 'live/foo.bar/fullchain.pem')) @@ -1205,7 +1205,7 @@ class MainTest(test_util.ConfigTestCase): traceback.format_exc()) if should_renew: - if reuse_key: + if reuse_key and not new_key: # The location of the previous live privkey.pem is passed # to obtain_certificate mock_client.obtain_certificate.assert_called_once_with(['isnot.org'], @@ -1276,6 +1276,13 @@ class MainTest(test_util.ConfigTestCase): args = ["renew", "--reuse-key"] self._test_renewal_common(True, [], args=args, should_renew=True, reuse_key=True) + @mock.patch('certbot._internal.storage.RenewableCert.save_successor') + def test_new_key(self, unused_save_successor): + test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf') + args = ["renew", "--reuse-key", "--new-key"] + self._test_renewal_common(True, [], args=args, should_renew=True, reuse_key=True, + new_key=True) + @mock.patch('sys.stdin') def test_noninteractive_renewal_delay(self, stdin): stdin.isatty.return_value = False diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index 110c0d7bd..d6e2866dc 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -99,6 +99,32 @@ class RenewalTest(test_util.ConfigTestCase): assert self.config.elliptic_curve == 'secp256r1' + def test_new_key(self): + # When renewing with both reuse_key and new_key, the key should be regenerated, + # the key type, key parameters and reuse_key should be kept. + self.config.reuse_key = True + self.config.new_key = True + self.config.dry_run = True + config = configuration.NamespaceConfig(self.config) + + rc_path = test_util.make_lineage( + self.config.config_dir, 'sample-renewal.conf') + lineage = storage.RenewableCert(rc_path, config) + + le_client = mock.MagicMock() + le_client.obtain_certificate.return_value = (None, None, None, None) + + from certbot._internal import renewal + + with mock.patch('certbot._internal.renewal.hooks.renew_hook'): + renewal.renew_cert(self.config, None, le_client, lineage) + + self.assertEqual(self.config.rsa_key_size, 2048) + self.assertEqual(self.config.key_type, 'rsa') + self.assertTrue(self.config.reuse_key) + # None is passed as the existing key, i.e. the key is not actually being reused. + le_client.obtain_certificate.assert_called_with(mock.ANY, None) + @test_util.patch_display_util() @mock.patch('certbot._internal.renewal.cli.set_by_cli') def test_remove_deprecated_config_elements(self, mock_set_by_cli, unused_mock_get_utility):