Make `reconfigure` use staging server #9870

ohemorange · 2024-01-05T23:41:31Z

Creates set_test_server_options in cli_utils so that when dry_run is set after parse time, applicable options are additionally changed as well, and calls it from main.reconfigure.

Clears config.account in set_test_server_options when default server is switched to staging, so that the account id loaded during reconstitute isn't used with the staging server.

Note that this implies that if account was set on the cli and the default server is being used, the new account will not be tested during the dry run. Given that a dry run specifically implies using the staging server, there's not really a way to do that anyway. The new account will still be saved to the renewal conf file as the user requested.

Regression test failing on master:

__________________________________________________________ ReconfigureTest.test_staging_used ___________________________________________________________

self = <certbot._internal.tests.main_test.ReconfigureTest testMethod=test_staging_used>

    def test_staging_used(self):
        """ Check that we use the staging server for the dry run"""
        assert self.original_config['renewalparams']['server'] == \
            'https://acme-v02.api.letsencrypt.org/directory'
    
        new_config = self._call('--cert-name example.com --pre-hook'.split() + ['echo pre'])
    
>       assert 'staging' in self.mocks['_init_le_client'].call_args.args[0].server
E       AssertionError: assert 'staging' in 'https://acme-v02.api.letsencrypt.org/directory'
E        +  where 'https://acme-v02.api.letsencrypt.org/directory' = <certbot.configuration.NamespaceConfig object at 0x7f49e303d780>.server

certbot/certbot/_internal/tests/main_test.py:649: AssertionError

…ly modify anything else

bmw

LGTM other than a couple thoughts/suggestions/neuroses. Let me know what you think.

bmw · 2024-01-21T21:57:20Z

certbot/certbot/_internal/main.py

+    # make a deep copy of lineage_config because we're about to modify it for a test dry run
+    dry_run_lineage_config = copy.deepcopy(lineage_config)
+
+    dry_run_lineage_config.dry_run = True


What do you think about including something like the following here:

# we also set noninteractive_mode to more accurately simulate renewal (since `certbot renew` # implies noninteractive mode) and to avoid prompting the user as changes made to # dry_run_lineage_config beyond this point will not be applied to the original lineage_config dry_run_lineage_config.noninteractive_mode = True

It's probably not necessary, but my hope is it could help avoid some awkward behavior in the future when/if the functions called below change.

I ran the tests with this change and they still passed.

Trying to think of who this wouldn't work for...we're editing the config file, so even if they're running certonly instead of renew, it's reading the values from the file, not using duplicate, that's the whole point. In which case all values should be set already, and nothing should have to be read from input. I worry a little that I just haven't thought of some case. But I guess the failure there is that it doesn't for some weird case, in which case we can fix it if we get complaints, vs. what you said, about the general assurance that things we change going forward don't mess things up. That sounds good to me.

bmw · 2024-01-22T00:11:01Z

certbot/certbot/_internal/tests/main_test.py

+    def test_account_updates(self):
+        """ Even though we can't run the dry run with the new account id, it should still
+            update in the renewal conf file.
+        """


This behavior personally makes me a little nervous because it's possible certbot reconfigure succeeds and certbot renew later fails due to a problem with the given account. For example, a command like certbot reconfigure --account not_an_account succeeds in saving not_an_account in the renewal config file.

Alternatively, we could maybe include code like this in the reconfigure function:

renewalparams = orig_renewal_conf['renewalparams'] for param in ('account', 'server',): if getattr(lineage_config, param) != renewalparams.get(param): raise errors.ConfigurationError("Using reconfigure to change the " "ACME account or server is not supported.")

(I think similar problems exists with changing the server value including failing to find or pick an account for that server at renewal time.)

If we took an approach like this, I think our advice in this case would be to use the older approach described at https://eff-certbot.readthedocs.io/en/latest/using.html#certbot-v2-2-0-and-older which will obtain a real certificate, but also helps ensure that everything works at renewal time which a dry run can't really do with changes to these values.

What do you think? Overkill/too handholding or a helpful guardrail to prevent future failures?

No, that's a good point. I think if we raise an error, we should say "use certbot certonly instead". Another option would be to check the variable notify them that it won't test it, suggest that they can use certonly if they want to test it, and require confirmation if they want to go ahead with setting it anyway. It gets at a more fundamental question of if we're ok helping people make untested changes or not. If someone really wants to make the change without testing though, I suppose they can always just go in and edit the configuration file. I think I'm leaning towards the error out and provide the certonly suggestion though, what do you think?

So...reconstitute won't load the account into the config file if server is set, because of that magic set_by_user logic that considers account to have implicitly been set if server was set. My impulse here is instead of checking to see if the values changed, just check if either was set_by_user and fail based on that.

It's not super ideal, if, say, someone's set the server in cli.ini, but I'm not seeing a simple way to address this and I don't think it's a common enough need to implement a non-simple fix.

This is only a problem for --server, not --account, so we could technically do the change check for just account, but that seems even less likely to be the thing someone's trying to change.

lol, of course our integration tests do exactly that.

ok, if server was set by user and is the same as the one in the file, we can just manually load in the account from the saved config file, I guess.

This is a response to the thread current ending at https://github.com/certbot/certbot/pull/9870/files/710e17c69f048a1d0ff53f7f935993db03d17640..07a35a8c8f370cb1ff163f948eb550c9075bb8dc#r1462604621 which I think GitHub isn't always clear about when threads are continued as part of another review.

It gets at a more fundamental question of if we're ok helping people make untested changes or not. If someone really wants to make the change without testing though, I suppose they can always just go in and edit the configuration file. I think I'm leaning towards the error out and provide the certonly suggestion though, what do you think?

My opinion is that we should avoid helping people make untested changes to their renewal config files. Like you said, we can't completely stop them, but I think it's a bad idea and I wouldn't do it myself. Who wants to deal with renewal failures weeks later when you may no longer even remember what was changed?

I think erroring out is good. I like the suggestion of certbot renew slightly better than certonly because certonly blindly overwrites the previous renewal parameters with the settings used to invoke Certbot (with minor exceptions about things like changing the key type) while certbot renew merges the current settings with the old ones like certbot reconfigure does. I think this difference makes certbot certonly a bit more cumbersome because you have to respecify all values and any you forget will be dropped. I don't have that strong of a preference though so if you still prefer certonly despite this, this is fine with me.

So...reconstitute won't load the account into the config file if server is set, because of that magic set_by_user logic that considers account to have implicitly been set if server was set.

Very nice catch! I missed that we were dropping the account from the config file if server was set during my testing.

That's a good point, I'll switch to renew. Even if nothing else, we should match what our online instructions say.

bmw

I added an optional suggestion and reminder inline, but this otherwise LGTM. I'm approving this so feel free to merge as is if you wish.

bmw · 2024-01-24T22:07:47Z

certbot/certbot/_internal/tests/main_test.py

+    def test_account_updates(self):
+        """ Even though we can't run the dry run with the new account id, it should still
+            update in the renewal conf file.
+        """


This is a response to the thread current ending at https://github.com/certbot/certbot/pull/9870/files/710e17c69f048a1d0ff53f7f935993db03d17640..07a35a8c8f370cb1ff163f948eb550c9075bb8dc#r1462604621 which I think GitHub isn't always clear about when threads are continued as part of another review.

It gets at a more fundamental question of if we're ok helping people make untested changes or not. If someone really wants to make the change without testing though, I suppose they can always just go in and edit the configuration file. I think I'm leaning towards the error out and provide the certonly suggestion though, what do you think?

My opinion is that we should avoid helping people make untested changes to their renewal config files. Like you said, we can't completely stop them, but I think it's a bad idea and I wouldn't do it myself. Who wants to deal with renewal failures weeks later when you may no longer even remember what was changed?

I think erroring out is good. I like the suggestion of certbot renew slightly better than certonly because certonly blindly overwrites the previous renewal parameters with the settings used to invoke Certbot (with minor exceptions about things like changing the key type) while certbot renew merges the current settings with the old ones like certbot reconfigure does. I think this difference makes certbot certonly a bit more cumbersome because you have to respecify all values and any you forget will be dropped. I don't have that strong of a preference though so if you still prefer certonly despite this, this is fine with me.

So...reconstitute won't load the account into the config file if server is set, because of that magic set_by_user logic that considers account to have implicitly been set if server was set.

Very nice catch! I missed that we were dropping the account from the config file if server was set during my testing.

bmw · 2024-01-24T22:08:21Z

certbot/certbot/_internal/main.py

+                   "If you would like to do so, use certonly with the --force-renewal flag instead "
+                   "of reconfigure. Note that doing so will count against any rate limits. For "
+                   "more information on this method, see "
+                   "https://certbot.org/certonly-reconfiguration")


I suspect you're well aware of this but as a reminder for me/us, as of writing this, this link does not work.

Yes but thank you for the reminder, I figured I'd do it after approval before merging, so now.

bmw · 2024-01-24T22:09:21Z

certbot/certbot/_internal/tests/main_test.py

-    def test_account_updates(self):
-        """ Even though we can't run the dry run with the new account id, it should still
-            update in the renewal conf file.
+    def test_new_account_or_server_errors(self):


I love the thorough testing in this function.

bmw · 2024-01-25T22:29:44Z

certbot/certbot/_internal/main.py

@@ -1779,7 +1779,7 @@ def reconfigure(config: configuration.NamespaceConfig,
    for param in ('account', 'server',):
        if getattr(lineage_config, param) != renewalparams.get(param):
            msg = ("Using reconfigure to change the ACME account or server is not supported. "
-                   "If you would like to do so, use certonly with the --force-renewal flag instead "
+                   "If you would like to do so, use renew with the --force-renewal flag instead "
                   "of reconfigure. Note that doing so will count against any rate limits. For "
                   "more information on this method, see "
                   "https://certbot.org/certonly-reconfiguration")


I'm not sure if this is ready for another review, but just to make sure you're not waiting on me here, I'll chime in and say that I think this link should now be

Suggested change

"https://certbot.org/certonly-reconfiguration")

"https://certbot.org/renew-reconfiguration")

based on the conversations I saw.

Other than that, this PR LGTM! I'm OK with whatever order of operations between getting this link live and merging this PR that you prefer.

Weird, I thought I changed that! Thanks for catching that.

Ha, I changed it locally but didn't save the file before committing 🤦

bmw

lgtm!

ohemorange added 5 commits January 5, 2024 15:40

Make reconfigure use staging server

aa2aaf9

lint and imports

73e04d8

Merge branch 'master' into reconfigure-dry-run-fix

764d987

Merge branch 'master' into reconfigure-dry-run-fix

750209b

Unset the account if it's been set in preparation for a dry run

51abcd3

ohemorange force-pushed the reconfigure-dry-run-fix branch from d5bc7e0 to 51abcd3 Compare January 17, 2024 03:13

Add unit tests for checking we switch to staging and don't accidental…

2e81677

…ly modify anything else

ohemorange force-pushed the reconfigure-dry-run-fix branch from d1d7cb5 to 2e81677 Compare January 17, 2024 20:10

ohemorange added 2 commits January 17, 2024 12:23

add docstring

4313d3b

Add test to make sure a requested new account id is saved

d20ca69

ohemorange marked this pull request as ready for review January 17, 2024 20:38

update changelog

710e17c

zoracon requested review from a team, zoracon and bmw and removed request for a team and zoracon January 17, 2024 21:56

bmw reviewed Jan 22, 2024

View reviewed changes

ohemorange added 3 commits January 22, 2024 15:47

set noninteractive mode for dry run

7251984

error when account or server is set by the user

a6a8638

switch to checking for changed values in account and server

07a35a8

bmw previously approved these changes Jan 24, 2024

View reviewed changes

recommend using renew instead of certonly for forbidden fields

c37877a

ohemorange dismissed bmw’s stale review via c37877a January 24, 2024 23:44

bmw reviewed Jan 25, 2024

View reviewed changes

change link to renew-reconfiguration

fe69f24

bmw approved these changes Jan 25, 2024

View reviewed changes

ohemorange merged commit d33bbf3 into master Jan 26, 2024
16 checks passed

ohemorange deleted the reconfigure-dry-run-fix branch January 26, 2024 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `reconfigure` use staging server #9870

Make `reconfigure` use staging server #9870

ohemorange commented Jan 5, 2024 •

edited

bmw left a comment

bmw Jan 21, 2024

ohemorange Jan 22, 2024

bmw Jan 22, 2024

ohemorange Jan 22, 2024

ohemorange Jan 23, 2024

ohemorange Jan 23, 2024

ohemorange Jan 23, 2024

ohemorange Jan 23, 2024

ohemorange Jan 23, 2024

bmw Jan 24, 2024

ohemorange Jan 24, 2024

bmw left a comment

bmw Jan 24, 2024

bmw Jan 24, 2024

ohemorange Jan 24, 2024

bmw Jan 24, 2024

bmw Jan 25, 2024

ohemorange Jan 25, 2024

ohemorange Jan 25, 2024

bmw left a comment

	"https://certbot.org/certonly-reconfiguration")
	"https://certbot.org/renew-reconfiguration")

Make reconfigure use staging server #9870

Make reconfigure use staging server #9870

Conversation

ohemorange commented Jan 5, 2024 • edited

bmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw left a comment

Choose a reason for hiding this comment

Make `reconfigure` use staging server #9870

Make `reconfigure` use staging server #9870

ohemorange commented Jan 5, 2024 •

edited