error out when --reuse-key conflicts with other flags (#9262)

* error out when --reuse-key conflicts with other flags

* add unit test

* add integration tests

* lint

6 files changed, 141 insertions, 13 deletions

diff --git a/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py b/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py
index 3563b30af..a1e814405 100644
--- a/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py
+++ b/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py
@@ -33,8 +33,8 @@ def assert_elliptic_key(key: str, curve: Type[EllipticCurve]) -> None:
 
     key = load_pem_private_key(data=privkey1, password=None, backend=default_backend())
 
-    assert isinstance(key, EllipticCurvePrivateKey)
-    assert isinstance(key.curve, curve)
+    assert isinstance(key, EllipticCurvePrivateKey), f"should be an EC key but was {type(key)}"
+    assert isinstance(key.curve, curve), f"should have curve {curve} but was {key.curve}"
 
 
 def assert_rsa_key(key: str, key_size: Optional[int] = None) -> 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 7bef646be..64ec45178 100644
--- a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py
+++ b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py
@@ -507,6 +507,19 @@ def test_new_key(context: IntegrationTestsContext) -> None:
     assert_saved_lineage_option(context.config_dir, certname, 'reuse_key', 'True')
     assert_elliptic_key(privkey4_path, SECP256R1)
 
+    # certonly: it should not be possible to change a key parameter without --new-key
+    with pytest.raises(subprocess.CalledProcessError) as error:
+        context.certbot(['certonly', '-d', certname, '--reuse-key',
+                         '--elliptic-curve', 'secp384r1'])
+    assert 'Unable to change the --elliptic-curve' in error.value.stderr
+
+    # certonly: not specifying --key-type should keep the existing key type (non-interactively).
+    # TODO: when ECDSA is made default key type, the key types must be inverted
+    context.certbot(['certonly', '-d', certname, '--no-reuse-key'])
+    privkey5, privkey5_path = private_key(5)
+    assert_elliptic_key(privkey5_path, SECP256R1)
+    assert privkey4 != privkey5
+
 
 def test_incorrect_key_type(context: IntegrationTestsContext) -> None:
     with pytest.raises(subprocess.CalledProcessError):
diff --git a/certbot/certbot/_internal/renewal.py b/certbot/certbot/_internal/renewal.py
index bd84901e1..35f96a14f 100644
--- a/certbot/certbot/_internal/renewal.py
+++ b/certbot/certbot/_internal/renewal.py
@@ -324,12 +324,57 @@ def _avoid_invalidating_lineage(config: configuration.NamespaceConfig,
                     "unless you use the --break-my-certs flag!")
 
 
+def _avoid_reuse_key_conflicts(config: configuration.NamespaceConfig,
+                               lineage: storage.RenewableCert) -> None:
+    """Don't allow combining --reuse-key with any flags that would conflict
+    with key reuse (--key-type, --rsa-key-size, --elliptic-curve), unless
+    --new-key is also set.
+    """
+    # If --no-reuse-key is set, no conflict
+    if cli.set_by_cli("reuse_key") and not config.reuse_key:
+        return
+
+    # If reuse_key is not set on the lineage and --reuse-key is not
+    # set on the CLI, no conflict.
+    if not lineage.reuse_key and not config.reuse_key:
+        return
+
+    # If --new-key is set, no conflict
+    if config.new_key:
+        return
+
+    kt = config.key_type.lower()
+
+    # The remaining cases where conflicts are present:
+    # - --key-type is set on the CLI and doesn't match the stored private key
+    # - It's an RSA key and --rsa-key-size is set and doesn't match
+    # - It's an ECDSA key and --eliptic-curve is set and doesn't match
+    potential_conflicts = [
+        ("--key-type",
+         lambda: kt != lineage.private_key_type.lower()),
+        ("--rsa-key-type",
+         lambda: kt == "rsa" and config.rsa_key_size != lineage.rsa_key_size),
+        ("--elliptic-curve",
+         lambda: kt == "ecdsa" and lineage.elliptic_curve and \
+                 config.elliptic_curve.lower() != lineage.elliptic_curve.lower())
+    ]
+
+    for conflict in potential_conflicts:
+        if conflict[1]():
+            raise errors.Error(
+                f"Unable to change the {conflict[0]} of this certificate because --reuse-key "
+                "is set. To stop reusing the private key, specify --no-reuse-key. "
+                "To change the private key this one time and then reuse it in future, "
+                "add --new-key.")
+
+
 def renew_cert(config: configuration.NamespaceConfig, domains: Optional[List[str]],
                le_client: client.Client, lineage: storage.RenewableCert) -> None:
     """Renew a certificate lineage."""
     renewal_params = lineage.configuration["renewalparams"]
     original_server = renewal_params.get("server", cli.flag_default("server"))
     _avoid_invalidating_lineage(config, lineage, original_server)
+    _avoid_reuse_key_conflicts(config, lineage)
     if not domains:
         domains = lineage.names()
     # The private key is the existing lineage private key if reuse_key is set.
diff --git a/certbot/certbot/_internal/storage.py b/certbot/certbot/_internal/storage.py
index 567073acf..978295cce 100644
--- a/certbot/certbot/_internal/storage.py
+++ b/certbot/certbot/_internal/storage.py
@@ -12,10 +12,12 @@ from typing import List
 from typing import Mapping
 from typing import Optional
 from typing import Tuple
+from typing import Union
 
 import configobj
 from cryptography.hazmat.backends import default_backend
 from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateKey
+from cryptography.hazmat.primitives.asymmetric.ec import EllipticCurvePrivateKey
 from cryptography.hazmat.primitives.serialization import load_pem_private_key
 import parsedatetime
 import pkg_resources
@@ -569,6 +571,12 @@ class RenewableCert(interfaces.RenewableCert):
             return util.is_staging(self.server)
         return False
 
+    @property
+    def reuse_key(self) -> bool:
+        """Returns whether this certificate is configured to reuse its private key"""
+        return "reuse_key" in self.configuration["renewalparams"] and \
+               self.configuration["renewalparams"].as_bool("reuse_key")
+
     def _check_symlinks(self) -> None:
         """Raises an exception if a symlink doesn't exist"""
         for kind in ALL_FOUR:
@@ -1115,22 +1123,47 @@ class RenewableCert(interfaces.RenewableCert):
             target, values)
         return cls(new_config.filename, cli_config)
 
-    @property
-    def private_key_type(self) -> str:
-        """
-        :returns: The type of algorithm for the private, RSA or ECDSA
-        :rtype: str
-        """
+    def _private_key(self) -> Union[RSAPrivateKey, EllipticCurvePrivateKey]:
         with open(self.configuration["privkey"], "rb") as priv_key_file:
             key = load_pem_private_key(
                 data=priv_key_file.read(),
                 password=None,
                 backend=default_backend()
             )
+            return key
+
+    @property
+    def private_key_type(self) -> str:
+        """
+        :returns: The type of algorithm for the private, RSA or ECDSA
+        :rtype: str
+        """
+        key = self._private_key()
         if isinstance(key, RSAPrivateKey):
             return "RSA"
-        else:
-            return "ECDSA"
+        return "ECDSA"
+
+    @property
+    def rsa_key_size(self) -> Optional[int]:
+        """
+        :returns: If the private key is an RSA key, its size.
+        :rtype: int
+        """
+        key = self._private_key()
+        if isinstance(key, RSAPrivateKey):
+            return key.key_size
+        return None
+
+    @property
+    def elliptic_curve(self) -> Optional[str]:
+        """
+        :returns: If the private key is an elliptic key, the name of its curve.
+        :rtype: str
+        """
+        key = self._private_key()
+        if isinstance(key, EllipticCurvePrivateKey):
+            return key.curve.name
+        return None
 
     def save_successor(self, prior_version: int, new_cert: bytes, new_privkey: bytes,
                        new_chain: bytes, cli_config: configuration.NamespaceConfig) -> int:
diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py
index e26b19357..b2723ced3 100644
--- a/certbot/tests/main_test.py
+++ b/certbot/tests/main_test.py
@@ -1209,6 +1209,7 @@ class MainTest(test_util.ConfigTestCase):
         mock_lineage.has_pending_deployment.return_value = False
         mock_lineage.names.return_value = ['isnot.org']
         mock_lineage.private_key_type = 'RSA'
+        mock_lineage.rsa_key_size = 2048
         mock_certr = mock.MagicMock()
         mock_key = mock.MagicMock(pem='pem_key')
         mock_client = mock.MagicMock()
diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py
index d6e2866dc..a33332e30 100644
--- a/certbot/tests/renewal_test.py
+++ b/certbot/tests/renewal_test.py
@@ -55,7 +55,8 @@ class RenewalTest(test_util.ConfigTestCase):
         self.assertEqual(self.config.webroot_map, {})
         self.assertEqual(self.config.webroot_path, ['/var/www/test'])
 
-    def test_reuse_key_renewal_params(self):
+    @mock.patch('certbot._internal.renewal._avoid_reuse_key_conflicts')
+    def test_reuse_key_renewal_params(self, unused_mock_avoid_reuse_conflicts):
         self.config.rsa_key_size = 'INVALID_VALUE'
         self.config.reuse_key = True
         self.config.dry_run = True
@@ -75,7 +76,8 @@ class RenewalTest(test_util.ConfigTestCase):
 
         assert self.config.rsa_key_size == 2048
 
-    def test_reuse_ec_key_renewal_params(self):
+    @mock.patch('certbot._internal.renewal._avoid_reuse_key_conflicts')
+    def test_reuse_ec_key_renewal_params(self, unused_mock_avoid_reuse_conflicts):
         self.config.elliptic_curve = 'INVALID_CURVE'
         self.config.reuse_key = True
         self.config.dry_run = True
@@ -99,7 +101,9 @@ class RenewalTest(test_util.ConfigTestCase):
 
         assert self.config.elliptic_curve == 'secp256r1'
 
-    def test_new_key(self):
+    @mock.patch('certbot._internal.renewal.cli.set_by_cli')
+    def test_new_key(self, mock_set_by_cli):
+        mock_set_by_cli.return_value = False
         # 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
@@ -125,6 +129,38 @@ class RenewalTest(test_util.ConfigTestCase):
         # 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)
 
+    @mock.patch('certbot._internal.renewal.hooks.renew_hook')
+    @mock.patch('certbot._internal.renewal.cli.set_by_cli')
+    def test_reuse_key_conflicts(self, mock_set_by_cli, unused_mock_renew_hook):
+        mock_set_by_cli.return_value = False
+
+        # When renewing with reuse_key and a conflicting key parameter (size, curve)
+        # an error should be raised ...
+        self.config.reuse_key = True
+        self.config.key_type = "rsa"
+        self.config.rsa_key_size = 4096
+        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)
+        lineage.configuration["renewalparams"]["reuse_key"] = True
+
+        le_client = mock.MagicMock()
+        le_client.obtain_certificate.return_value = (None, None, None, None)
+
+        from certbot._internal import renewal
+
+        with self.assertRaisesRegex(errors.Error, "Unable to change the --rsa-key-type"):
+            renewal.renew_cert(self.config, None, le_client, lineage)
+
+        # ... unless --no-reuse-key is set
+        mock_set_by_cli.side_effect = lambda var: var == "reuse_key"
+        self.config.reuse_key = False
+        renewal.renew_cert(self.config, None, le_client, lineage)
+
     @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):