diff --git a/certbot/certbot/_internal/cli/__init__.py b/certbot/certbot/_internal/cli/__init__.py index e50cb338a..c69bb3564 100644 --- a/certbot/certbot/_internal/cli/__init__.py +++ b/certbot/certbot/_internal/cli/__init__.py @@ -28,7 +28,8 @@ from certbot._internal.cli.cli_constants import ( ARGPARSE_PARAMS_TO_REMOVE, EXIT_ACTIONS, ZERO_ARG_ACTIONS, - VAR_MODIFIERS + VAR_MODIFIERS, + DEPRECATED_OPTIONS ) from certbot._internal.cli.cli_utils import ( @@ -471,6 +472,11 @@ def set_by_cli(var): (CLI or config file) including if the user explicitly set it to the default. Returns False if the variable was assigned a default value. """ + # We should probably never actually hit this code. But if we do, + # a deprecated option has logically never been set by the CLI. + if var in DEPRECATED_OPTIONS: + return False + detector = set_by_cli.detector # type: ignore if detector is None and helpful_parser is not None: # Setup on first run: `detector` is a weird version of config in which @@ -531,6 +537,9 @@ def option_was_set(option, value): :rtype: bool """ + # If an option is deprecated, it was effectively not set by the user. + if option in DEPRECATED_OPTIONS: + return False return set_by_cli(option) or not has_default_value(option, value) diff --git a/certbot/certbot/_internal/cli/cli_constants.py b/certbot/certbot/_internal/cli/cli_constants.py index 4bc84bfe7..dc199e152 100644 --- a/certbot/certbot/_internal/cli/cli_constants.py +++ b/certbot/certbot/_internal/cli/cli_constants.py @@ -105,3 +105,8 @@ VAR_MODIFIERS = {"account": {"server",}, "renew_hook": {"deploy_hook",}, "server": {"dry_run", "staging",}, "webroot_map": {"webroot_path",}} + +# This is a list of all CLI options that we have ever deprecated. It lets us +# opt out of the default detection, which can interact strangely with option +# deprecation. See https://github.com/certbot/certbot/issues/8540 for more info. +DEPRECATED_OPTIONS = {"manual_public_ip_logging_ok",} diff --git a/certbot/certbot/_internal/renewal.py b/certbot/certbot/_internal/renewal.py index 3a550d355..f4c7b4502 100644 --- a/certbot/certbot/_internal/renewal.py +++ b/certbot/certbot/_internal/renewal.py @@ -85,6 +85,7 @@ def _reconstitute(config, full_path): return None # Now restore specific values along with their data types, if # those elements are present. + renewalparams = _remove_deprecated_config_elements(renewalparams) try: restore_required_config_elements(config, renewalparams) _restore_plugin_configs(config, renewalparams) @@ -188,6 +189,19 @@ def restore_required_config_elements(config, renewalparams): setattr(config, item_name, value) +def _remove_deprecated_config_elements(renewalparams): + """Removes deprecated config options from the parsed renewalparams. + + :param dict renewalparams: list of parsed renewalparams + + :returns: list of renewalparams with deprecated config options removed + :rtype: dict + + """ + return {option_name: v for (option_name, v) in renewalparams.items() + if option_name not in cli.DEPRECATED_OPTIONS} + + def _restore_pref_challs(unused_name, value): """Restores preferred challenges from a renewal config file. diff --git a/certbot/certbot/tests/testdata/sample-renewal-deprecated-option.conf b/certbot/certbot/tests/testdata/sample-renewal-deprecated-option.conf new file mode 100644 index 000000000..2b777d3de --- /dev/null +++ b/certbot/certbot/tests/testdata/sample-renewal-deprecated-option.conf @@ -0,0 +1,14 @@ +# renew_before_expiry = 30 days +version = 1.11.0 +archive_dir = MAGICDIR/live/sample-renewal-deprecated-option +cert = MAGICDIR/live/sample-renewal-deprecated-option/cert.pem +privkey = MAGICDIR/live/sample-renewal-deprecated-option/privkey.pem +chain = MAGICDIR/live/sample-renewal-deprecated-option/chain.pem +fullchain = MAGICDIR/live/sample-renewal-deprecated-option/fullchain.pem + +# Options used in the renewal process +[renewalparams] +account = ffffffffffffffffffffffffffffffff +authenticator = nginx +installer = nginx +manual_public_ip_logging_ok = None diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index 4af8c6e7f..7c9c53fb4 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -1,4 +1,6 @@ """Tests for certbot._internal.renewal""" +import copy + import unittest try: @@ -98,6 +100,23 @@ class RenewalTest(test_util.ConfigTestCase): assert self.config.elliptic_curve == 'secp256r1' + @test_util.patch_get_utility() + @mock.patch('certbot._internal.renewal.cli.set_by_cli') + def test_remove_deprecated_config_elements(self, mock_set_by_cli, unused_mock_get_utility): + mock_set_by_cli.return_value = False + config = configuration.NamespaceConfig(self.config) + config.certname = "sample-renewal-deprecated-option" + + rc_path = test_util.make_lineage( + self.config.config_dir, 'sample-renewal-deprecated-option.conf') + + from certbot._internal import renewal + lineage_config = copy.deepcopy(self.config) + renewal_candidate = renewal._reconstitute(lineage_config, rc_path) + # This means that manual_public_ip_logging_ok was not modified in the config based on its + # value in the renewal conf file + self.assertTrue(isinstance(lineage_config.manual_public_ip_logging_ok, mock.MagicMock)) + class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase): """Tests for certbot._internal.renewal.restore_required_config_elements.""" diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 9ae26532f..914304cd4 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -77,6 +77,17 @@ class RelevantValuesTest(unittest.TestCase): self.assertEqual(self._call(self.values), expected_relevant_values) + @mock.patch("certbot._internal.cli.set_by_cli") + def test_deprecated_item(self, unused_mock_set_by_cli): + # deprecated items should never be relevant to store + expected_relevant_values = self.values.copy() + self.values["manual_public_ip_logging_ok"] = None + self.assertEqual(self._call(self.values), expected_relevant_values) + self.values["manual_public_ip_logging_ok"] = True + self.assertEqual(self._call(self.values), expected_relevant_values) + self.values["manual_public_ip_logging_ok"] = False + self.assertEqual(self._call(self.values), expected_relevant_values) + class BaseRenewableCertTest(test_util.ConfigTestCase): """Base class for setting up Renewable Cert tests.