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

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,

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