From 82b6e15be754369f1c982ed508926a2b7422b213 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Mon, 18 Jul 2022 18:27:19 +1000 Subject: [PATCH 1/3] change default key_type from rsa to ecdsa --- .../certbot_tests/test_main.py | 22 +++++++++---------- certbot/certbot/_internal/constants.py | 2 +- certbot/tests/renewal_test.py | 4 ++-- 3 files changed, 14 insertions(+), 14 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 7bef646be..d315f93f6 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py @@ -112,7 +112,7 @@ def test_http_01(context: IntegrationTestsContext) -> None: assert_hook_execution(context.hook_probe, 'deploy') assert_saved_renew_hook(context.config_dir, certname) - assert_saved_lineage_option(context.config_dir, certname, 'key_type', 'rsa') + assert_saved_lineage_option(context.config_dir, certname, 'key_type', 'ecdsa') def test_manual_http_auth(context: IntegrationTestsContext) -> None: @@ -315,23 +315,23 @@ def test_graceful_renew_it_is_time(context: IntegrationTestsContext) -> None: def test_renew_with_changed_private_key_complexity(context: IntegrationTestsContext) -> None: """Test proper renew with updated private key complexity.""" certname = context.get_domain('renew') - context.certbot(['-d', certname, '--rsa-key-size', '4096']) + context.certbot(['-d', certname, '--key-type', 'rsa', '--rsa-key-size', '4096']) key1 = join(context.config_dir, 'archive', certname, 'privkey1.pem') - assert os.stat(key1).st_size > 3000 # 4096 bits keys takes more than 3000 bytes + assert_rsa_key(key1, 4096) assert_cert_count_for_lineage(context.config_dir, certname, 1) context.certbot(['renew']) assert_cert_count_for_lineage(context.config_dir, certname, 2) key2 = join(context.config_dir, 'archive', certname, 'privkey2.pem') - assert os.stat(key2).st_size > 3000 + assert_rsa_key(key2, 4096) context.certbot(['renew', '--rsa-key-size', '2048']) assert_cert_count_for_lineage(context.config_dir, certname, 3) key3 = join(context.config_dir, 'archive', certname, 'privkey3.pem') - assert os.stat(key3).st_size < 1800 # 2048 bits keys takes less than 1800 bytes + assert_rsa_key(key3, 2048) def test_renew_ignoring_directory_hooks(context: IntegrationTestsContext) -> None: @@ -535,24 +535,24 @@ def test_ecdsa(context: IntegrationTestsContext) -> None: def test_default_key_type(context: IntegrationTestsContext) -> None: - """Test default key type is RSA""" + """Test default key type is ECDSA""" certname = context.get_domain('renew') context.certbot([ 'certonly', '--cert-name', certname, '-d', certname ]) filename = join(context.config_dir, 'archive/{0}/privkey1.pem').format(certname) - assert_rsa_key(filename) + assert_elliptic_key(filename, SECP256R1) -def test_default_curve_type(context: IntegrationTestsContext) -> None: - """test that the curve used when not specifying any is secp256r1""" +def test_default_rsa_size(context: IntegrationTestsContext) -> None: + """test that the RSA key size used when not specifying any is 2048""" certname = context.get_domain('renew') context.certbot([ - '--key-type', 'ecdsa', '--cert-name', certname, '-d', certname + '--key-type', 'rsa', '--cert-name', certname, '-d', certname ]) key1 = join(context.config_dir, 'archive/{0}/privkey1.pem'.format(certname)) - assert_elliptic_key(key1, SECP256R1) + assert_rsa_key(key1, 2048) @pytest.mark.parametrize('curve,curve_cls,skip_servers', [ diff --git a/certbot/certbot/_internal/constants.py b/certbot/certbot/_internal/constants.py index 22bba0607..559f59f2f 100644 --- a/certbot/certbot/_internal/constants.py +++ b/certbot/certbot/_internal/constants.py @@ -61,7 +61,7 @@ CLI_DEFAULTS: Dict[str, Any] = dict( # noqa break_my_certs=False, rsa_key_size=2048, elliptic_curve="secp256r1", - key_type="rsa", + key_type="ecdsa", must_staple=False, redirect=None, auto_hsts=False, diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index d6e2866dc..9c1154c7b 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -119,8 +119,8 @@ class RenewalTest(test_util.ConfigTestCase): 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.assertEqual(self.config.elliptic_curve, 'secp256r1') + self.assertEqual(self.config.key_type, 'ecdsa') 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) From 652c06a8aeb146f77ab3e6238e574c732d8f8883 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Tue, 27 Sep 2022 13:51:16 +1000 Subject: [PATCH 2/3] fix typo in key conflict error message --- certbot/certbot/_internal/renewal.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/certbot/_internal/renewal.py b/certbot/certbot/_internal/renewal.py index 35f96a14f..df5168ea2 100644 --- a/certbot/certbot/_internal/renewal.py +++ b/certbot/certbot/_internal/renewal.py @@ -352,7 +352,7 @@ def _avoid_reuse_key_conflicts(config: configuration.NamespaceConfig, potential_conflicts = [ ("--key-type", lambda: kt != lineage.private_key_type.lower()), - ("--rsa-key-type", + ("--rsa-key-size", lambda: kt == "rsa" and config.rsa_key_size != lineage.rsa_key_size), ("--elliptic-curve", lambda: kt == "ecdsa" and lineage.elliptic_curve and \ From 5d6e067a74f174c9052b390abd1c7517a5298dc8 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Tue, 27 Sep 2022 13:51:35 +1000 Subject: [PATCH 3/3] fix tests broken by #9262 --- .../certbot_tests/test_main.py | 23 +++++++++---------- certbot/certbot/tests/util.py | 2 +- certbot/tests/main_test.py | 9 ++++---- certbot/tests/renewal_test.py | 6 ++--- 4 files changed, 20 insertions(+), 20 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 4a1cb9b0e..65eca976d 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py @@ -482,7 +482,7 @@ def test_new_key(context: IntegrationTestsContext) -> None: certname = context.get_domain('newkey') context.certbot(['--domains', certname, '--reuse-key', - '--key-type', 'rsa', '--rsa-key-size', '4096']) + '--key-type', 'ecdsa', '--elliptic-curve', 'secp384r1']) privkey1, _ = private_key(1) # renew: --new-key should replace the key, but keep reuse_key and the key type + params @@ -490,34 +490,33 @@ def test_new_key(context: IntegrationTestsContext) -> None: 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) + assert_elliptic_key(privkey2_path, SECP384R1) - # certonly: it should replace the key but the key size will change + # certonly: it should replace the key but the elliptic curve 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) + assert_elliptic_key(privkey3_path, SECP256R1) # 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]) + context.certbot(['certonly', '-d', certname, '--reuse-key', '--new-key', '--key-type', 'rsa', + '--rsa-key-size', '4096', '--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) + assert_rsa_key(privkey4_path, 4096) # 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 + context.certbot(['certonly', '-d', certname, '--key-type', 'rsa', '--reuse-key', + '--rsa-key-size', '2048']) + assert 'Unable to change the --rsa-key-size' 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_rsa_key(privkey5_path, 2048) assert privkey4 != privkey5 diff --git a/certbot/certbot/tests/util.py b/certbot/certbot/tests/util.py index dbff31a14..e758fd9d1 100644 --- a/certbot/certbot/tests/util.py +++ b/certbot/certbot/tests/util.py @@ -153,7 +153,7 @@ def load_pyopenssl_private_key(*names: str) -> crypto.PKey: return crypto.load_privatekey(loader, load_vector(*names)) -def make_lineage(config_dir: str, testfile: str, ec: bool = False) -> str: +def make_lineage(config_dir: str, testfile: str, ec: bool = True) -> str: """Creates a lineage defined by testfile. This creates the archive, live, and renewal directories if diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index b2723ced3..bec13a19d 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1208,8 +1208,9 @@ class MainTest(test_util.ConfigTestCase): mock_lineage.should_autorenew.return_value = due_for_renewal 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_lineage.private_key_type = 'ecdsa' + mock_lineage.elliptic_curve = 'secp256r1' + mock_lineage.reuse_key = reuse_key mock_certr = mock.MagicMock() mock_key = mock.MagicMock(pem='pem_key') mock_client = mock.MagicMock() @@ -1253,11 +1254,11 @@ class MainTest(test_util.ConfigTestCase): 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'], + mock_client.obtain_certificate.assert_called_once_with([mock.ANY], os.path.normpath(os.path.join( self.config.config_dir, "live/sample-renewal/privkey.pem"))) else: - mock_client.obtain_certificate.assert_called_once_with(['isnot.org'], None) + mock_client.obtain_certificate.assert_called_once_with([mock.ANY], None) else: self.assertEqual(mock_client.obtain_certificate.call_count, 0) except: diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index a61905762..10631c1c6 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -57,7 +57,7 @@ class RenewalTest(test_util.ConfigTestCase): @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.elliptic_curve = 'INVALID_VALUE' self.config.reuse_key = True self.config.dry_run = True config = configuration.NamespaceConfig(self.config) @@ -74,7 +74,7 @@ class RenewalTest(test_util.ConfigTestCase): with mock.patch('certbot._internal.renewal.hooks.renew_hook'): renewal.renew_cert(self.config, None, le_client, lineage) - assert self.config.rsa_key_size == 2048 + assert self.config.elliptic_curve == 'secp256r1' @mock.patch('certbot._internal.renewal._avoid_reuse_key_conflicts') def test_reuse_ec_key_renewal_params(self, unused_mock_avoid_reuse_conflicts): @@ -153,7 +153,7 @@ class RenewalTest(test_util.ConfigTestCase): from certbot._internal import renewal - with self.assertRaisesRegex(errors.Error, "Unable to change the --rsa-key-type"): + with self.assertRaisesRegex(errors.Error, "Unable to change the --key-type"): renewal.renew_cert(self.config, None, le_client, lineage) # ... unless --no-reuse-key is set