From 6e8f58e3f618c887df10c7640c3c148fc62cbf8c Mon Sep 17 00:00:00 2001 From: alexzorin Date: Thu, 3 Mar 2022 03:55:17 +1100 Subject: [PATCH] improve handling and ux of unexpected key type migration (#9200) * improve handling and ux of unexpected key type migration * update unit tests * update integration tests * if --cert-name and --key-type are set, dont prompt --- .../certbot_tests/test_main.py | 43 ++++++++++++------- certbot/certbot/_internal/main.py | 40 +++++++++++++---- certbot/tests/main_test.py | 38 ++++++++++++---- 3 files changed, 87 insertions(+), 34 deletions(-) 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 146ba58bb..21f400d37 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py @@ -540,35 +540,46 @@ def test_renew_with_ec_keys(context: IntegrationTestsContext) -> None: '--key-type', 'ecdsa', '--elliptic-curve', 'secp256r1', '--force-renewal', '-d', certname, ]) - key1 = join(context.config_dir, "archive", certname, 'privkey1.pem') assert 200 < os.stat(key1).st_size < 250 # ec keys of 256 bits are ~225 bytes assert_elliptic_key(key1, SECP256R1) assert_cert_count_for_lineage(context.config_dir, certname, 1) context.certbot(['renew', '--elliptic-curve', 'secp384r1']) - assert_cert_count_for_lineage(context.config_dir, certname, 2) key2 = join(context.config_dir, 'archive', certname, 'privkey2.pem') - assert_elliptic_key(key2, SECP384R1) assert 280 < os.stat(key2).st_size < 320 # ec keys of 384 bits are ~310 bytes + assert_elliptic_key(key2, SECP384R1) - # 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 - ]) + # When running non-interactively, if --key-type is unspecified but the default value differs + # to the lineage key type, Certbot should keep the lineage key type. The curve will still + # change to the default value, in order to stay consistent with the behavior of certonly. + context.certbot(['certonly', '--force-renewal', '-d', certname]) + assert_cert_count_for_lineage(context.config_dir, certname, 3) + key3 = join(context.config_dir, 'archive', certname, 'privkey3.pem') + assert 200 < os.stat(key3).st_size < 250 # ec keys of 256 bits are ~225 bytes + assert_elliptic_key(key3, SECP256R1) + + # When running non-interactively, specifying a different --key-type requires user confirmation + # with both --key-type and --cert-name. + with pytest.raises(subprocess.CalledProcessError) as error: + context.certbot(['certonly', '--force-renewal', '-d', certname, + '--key-type', 'rsa']) + assert 'Please provide both --cert-name and --key-type' in error.value.stderr + + context.certbot(['certonly', '--force-renewal', '-d', certname, + '--key-type', 'rsa', '--cert-name', certname]) + assert_cert_count_for_lineage(context.config_dir, certname, 4) + key4 = join(context.config_dir, 'archive', certname, 'privkey4.pem') + assert_rsa_key(key4) # We expect that the previous behavior of requiring both --cert-name and # --key-type to be set to not apply to the renew subcommand. - context.certbot(['renew', '--force-renewal', '--key-type', 'rsa']) - assert_cert_count_for_lineage(context.config_dir, certname, 3) - key3 = join(context.config_dir, 'archive', certname, 'privkey3.pem') - assert_rsa_key(key3) + context.certbot(['renew', '--force-renewal', '--key-type', 'ecdsa']) + assert_cert_count_for_lineage(context.config_dir, certname, 5) + key5 = join(context.config_dir, 'archive', certname, 'privkey5.pem') + assert 200 < os.stat(key5).st_size < 250 # ec keys of 256 bits are ~225 bytes + assert_elliptic_key(key5, SECP256R1) def test_ocsp_must_staple(context: IntegrationTestsContext) -> None: diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 15a7239f6..9d63be23a 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -156,17 +156,39 @@ def _handle_unexpected_key_type_migration(config: configuration.NamespaceConfig, :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() - new_key_type = config.key_type.upper() - cur_key_type = cert.private_key_type.upper() + if new_key_type == cur_key_type: + return - 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 to confirm the change you are trying to make.') - msg = msg.format(cert.lineagename, cur_key_type, new_key_type) - raise errors.Error(msg) + # If both --key-type and --cert-name are provided, we consider the user's intent to + # be unambiguous: to change the key type of this lineage. + is_confirmed_via_cli = cli.set_by_cli("key_type") and cli.set_by_cli("certname") + + # Failing that, we interactively prompt the user to confirm the change. + if is_confirmed_via_cli or display_util.yesno( + f'An {cur_key_type} certificate named {cert.lineagename} already exists. Do you want to ' + f'update its key type to {new_key_type}?', + yes_label='Update key type', no_label='Keep existing key type', + default=False, force_interactive=False, + ): + return + + # If --key-type was set on the CLI but the user did not confirm the key type change using + # one of the two above methods, their intent is ambiguous. Error out. + if cli.set_by_cli("key_type"): + raise errors.Error( + 'Are you trying to change the key type of the certificate named ' + f'{cert.lineagename} from {cur_key_type} to {new_key_type}? Please provide ' + 'both --cert-name and --key-type on the command line to confirm the change ' + 'you are trying to make.' + ) + + # The mismatch between the lineage's key type and config.key_type is caused by Certbot's + # default value. The user is not asking for a key change: keep the key type of the existing + # lineage. + config.key_type = cur_key_type.lower() def _handle_subset_cert_request(config: configuration.NamespaceConfig, diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 3813e4d50..d75b6ad58 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -70,30 +70,50 @@ class TestHandleCerts(unittest.TestCase): self.assertEqual(ret, ("renew", mock_lineage)) self.assertTrue(mock_handle_migration.called) + @mock.patch("certbot._internal.main.display_util.yesno") @mock.patch("certbot._internal.main.cli.set_by_cli") - def test_handle_unexpected_key_type_migration(self, mock_set): + def test_handle_unexpected_key_type_migration(self, mock_set, mock_yesno): config = mock.Mock() - config.key_type = "rsa" cert = mock.Mock() - cert.private_key_type = "ecdsa" + # If the key types do not differ, it should be a no-op. + config.key_type = "rsa" + cert.private_key_type = "rsa" + main._handle_unexpected_key_type_migration(config, cert) + mock_yesno.assert_not_called() + self.assertEqual(config.key_type, cert.private_key_type) + + # If the user confirms the change interactively, the key change should proceed silently. + cert.private_key_type = "ecdsa" + mock_yesno.return_value = True + main._handle_unexpected_key_type_migration(config, cert) + self.assertEqual(mock_set.call_count, 2) + self.assertEqual(config.key_type, "rsa") + + # User does not interactively confirm the key type change. + mock_yesno.return_value = False + + # If --key-type and --cert-name are both set, the key type change should proceed silently. mock_set.return_value = True main._handle_unexpected_key_type_migration(config, cert) + self.assertEqual(config.key_type, "rsa") + # If neither --key-type nor --cert-name are set, Certbot should keep the old key type. mock_set.return_value = False - with self.assertRaises(errors.Error) as raised: - main._handle_unexpected_key_type_migration(config, cert) - self.assertIn("Please provide both --cert-name and --key-type", str(raised.exception)) + main._handle_unexpected_key_type_migration(config, cert) + self.assertEqual(config.key_type, "ecdsa") + # If --key-type is set and --cert-name isn't, Certbot should error. + config.key_type = "rsa" mock_set.side_effect = lambda var: var != "certname" with self.assertRaises(errors.Error) as raised: main._handle_unexpected_key_type_migration(config, cert) self.assertIn("Please provide both --cert-name and --key-type", str(raised.exception)) + # If --key-type is not set, Certbot should keep the old key type. 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.assertIn("Please provide both --cert-name and --key-type", str(raised.exception)) + main._handle_unexpected_key_type_migration(config, cert) + self.assertEqual(config.key_type, "ecdsa") class RunTest(test_util.ConfigTestCase):