From e10e549a95a3a3ca9af66f3eeb1ae5e1797546d5 Mon Sep 17 00:00:00 2001
From: alexzorin <alex@zorin.id.au>
Date: Wed, 29 Mar 2023 02:44:19 +1100
Subject: [PATCH] renewal: fix key_type not being preserved on <v1.25.0 renewal
 configs (#9636)

Fixes #9635.
---
 .../certbot_tests/test_main.py                | 21 +++++++++++++++++++
 certbot/CHANGELOG.md                          |  4 ++++
 certbot/certbot/_internal/renewal.py          |  8 +++++++
 certbot/tests/main_test.py                    |  4 ++--
 certbot/tests/renewal_test.py                 | 11 ++++++++++
 5 files changed, 46 insertions(+), 2 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 6df79c7f9..8ed1fbf1e 100644
--- a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py
+++ b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py
@@ -936,3 +936,24 @@ def test_preferred_chain(context: IntegrationTestsContext) -> None:
         with open(conf_path, 'r') as f:
             assert f'preferred_chain = {requested}' in f.read(), \
                    'Expected preferred_chain to be set in renewal config'
+
+
+def test_ancient_rsa_key_type_preserved(context: IntegrationTestsContext) -> None:
+    certname = context.get_domain('newname')
+    context.certbot(['certonly', '-d', certname, '--key-type', 'rsa'])
+    assert_saved_lineage_option(context.config_dir, certname, 'key_type', 'rsa')
+
+    # Remove `key_type = rsa` from the renewal config to emulate a <v1.25.0 Certbot certificate.
+    conf_path = join(context.config_dir, 'renewal', f'{certname}.conf')
+    conf_contents: str = ''
+    with open(conf_path) as f:
+        conf_contents = f.read()
+    conf_contents = conf_contents.replace('key_type = rsa', '')
+    with open(conf_path, 'w') as f:
+        f.write(conf_contents)
+
+    context.certbot(['renew', '--cert-name', certname, '--force-renewal'])
+
+    assert_saved_lineage_option(context.config_dir, certname, 'key_type', 'rsa')
+    key2 = join(context.config_dir, 'archive/{0}/privkey2.pem'.format(certname))
+    assert_rsa_key(key2, 2048)
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 3348369f3..7fd4800e8 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -21,6 +21,10 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
+* Fixed `renew` sometimes not preserving the key type of RSA certificates.
+  * Users who upgraded from Certbot <v1.25.0 to Certbot >=v2.0.0 may
+    have had their RSA certificates inadvertently changed to ECDSA certificates. If desired,
+    the key type may be changed back to RSA. See the [User Guide](https://eff-certbot.readthedocs.io/en/stable/using.html#changing-a-certificate-s-key-type).
 * Deprecated flags were inadvertently not printing warnings since v1.16.0. This is now fixed.
 
 More details about these changes can be found on our GitHub repo.
diff --git a/certbot/certbot/_internal/renewal.py b/certbot/certbot/_internal/renewal.py
index 39f704f2f..2b329dfd8 100644
--- a/certbot/certbot/_internal/renewal.py
+++ b/certbot/certbot/_internal/renewal.py
@@ -87,6 +87,14 @@ def reconstitute(config: configuration.NamespaceConfig,
         logger.error("Renewal configuration file %s does not specify "
                        "an authenticator. Skipping.", full_path)
         return None
+
+    # Prior to Certbot v1.25.0, the default value of key_type (rsa) was not persisted to the
+    # renewal params. If the option is absent, it means the certificate was an RSA key.
+    # Restoring the option here is necessary to preserve the certificate key_type if
+    # the user has upgraded directly from Certbot <v1.25.0 to >=v2.0.0, where the default
+    # key_type was changed to ECDSA. See https://github.com/certbot/certbot/issues/9635.
+    renewalparams["key_type"] = renewalparams.get("key_type", "rsa")
+
     # Now restore specific values along with their data types, if
     # those elements are present.
     renewalparams = _remove_deprecated_config_elements(renewalparams)
diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py
index 6220ce537..8dd4deba1 100644
--- a/certbot/tests/main_test.py
+++ b/certbot/tests/main_test.py
@@ -1472,13 +1472,13 @@ class MainTest(test_util.ConfigTestCase):
         self._test_renewal_common(True, [], args=args, should_renew=True)
 
     def test_reuse_key(self):
-        test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf')
+        test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf', ec=False)
         args = ["renew", "--dry-run", "--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_reuse_key_no_dry_run(self, unused_save_successor):
-        test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf')
+        test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf', ec=False)
         args = ["renew", "--reuse-key"]
         self._test_renewal_common(True, [], args=args, should_renew=True, reuse_key=True)
 
diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py
index 0bb915345..f11b01603 100644
--- a/certbot/tests/renewal_test.py
+++ b/certbot/tests/renewal_test.py
@@ -177,6 +177,17 @@ class RenewalTest(test_util.ConfigTestCase):
         # value in the renewal conf file
         assert isinstance(lineage_config.manual_public_ip_logging_ok, mock.MagicMock)
 
+    @mock.patch('certbot._internal.renewal.cli.set_by_cli')
+    def test_absent_key_type_restored(self, mock_set_by_cli):
+        mock_set_by_cli.return_value = False
+
+        rc_path = test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf', ec=False)
+
+        from certbot._internal import renewal
+        lineage_config = copy.deepcopy(self.config)
+        renewal.reconstitute(lineage_config, rc_path)
+        assert lineage_config.key_type == 'rsa'
+
 
 class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase):
     """Tests for certbot._internal.renewal.restore_required_config_elements."""