`certbot reconfigure` used production API instead of staging API about certbot HOT 5 CLOSED

I can confirm this issue: when running certbot reconfigure, it says it will "Simulate" renewal, but actually uses the production API. (Without --run-deploy-hooks, that's not necessary for this bug to hit.)
Even with a test certificate which used the staging environment, Certbot will simply override the staging server variable with the production ACME server URL.

(Not sure if the "area: cert management" is the correct label, but I couldn't find a better one.)

from certbot.

So while I think we should fix this as without a patch certbot reconfigure essentially functions like certbot certonly, I don't think we need to scramble to fix this as it appears less than 600 people have used this command with Let's Encrypt in the last 2 months.

Essentially, I think the problem is that

I started to fix that by setting dry_run if reconfigure is the "verb" during CLI parsing so this second code block runs, but then I think you also need to handle making sure the server value (or any other renewal config relevant values that dry_run implies) doesn't get changed in the renewal config unless of course the user requested these changes (to, for example, try and change the CA being used on future issuances).

Handling all this cleanly with tests is definitely doable, but due the current structure of the code, it doesn't currently seem trivial to me to fix. I also believe this problem has been around since certbot reconfigure was first introduced for what it's worth.

from certbot.

While I agree that we need great tests here, I actually think the problem shouldn't be quite that bad to solve. The problem with just running the CLI parsing logic during parsing is that then the config has those values the whole time, when really we only want them set as we go to test the new config. So if we:

1. Parse and modify the config as we currently do, without touching server
2. Make a deep copy of lineage_config (almost certainly necessary)
3. Right before actually getting the cert (between lines 1176 and 1777 below), set dry_run and server and account-related vars as in helpful.py in the copy of lineage_config, either by sticking the helpful logic in a util somewhere or just copying it
4. Get the cert, confirm it works as is currently done, but when saving it out using save_new_config_values, used the cached lineage_config

The thing to check here is that neither _init_le_client nor _get_and_save_cert modify lineage_config; I...don't think they do, but haven't checked all the way down yet. If they do, in a way that we need those changes to be saved, it gets a little more complicated, yeah, but still doable.

The other major assumption is that the logic done during helpful can be executed at that point and in that function, but I'm not seeing anything obvious where it wouldn't.

By moving the setting of dry_run and its associated values later to just where it's needed, it should be a relatively straightforward fix.

EDIT: Ok, having read through the code and called code in _init_le_client and _get_and_save_cert, I'm now largely confident that my assumptions were correct. The one case I'd want to double check is that if you have reuse_key set, we don't accidentally remove your key_type and possibly nonstandard key size/elliptic curve, because those are loaded into the config during renewal.renew_cert, but I don't think that's likely to be a problem.

from certbot.

I think your plan should work!

The thing to check here is that neither _init_le_client nor _get_and_save_cert modify lineage_config; I...don't think they do, but haven't checked all the way down yet.

This happening or that we might do it in the future scared me. I feel like there's all sorts of logic like option A implies B or ask about options C and D but default to D if noninteractive throughout Certbot, but since this is essentially using the same process as certbot renew instead of certonly or run, maybe it avoids all that.

from certbot.

I feel that. Certainly certbot is no exemplar of distinguishing between read and write operations. At minimum, maybe we should start documenting when we modify objects given as input, which we do a little but by no means universally. Separating out input collection for required values from acting on those values would also help, but that's the sort of thing that would either be a bugs-inducingly-large refactor or just cleaning it up as we go. Obviously the correct solution is to rewrite the whole thing in a functional language, but...

Anyway, the worst case scenario here should that happen in the future is we fail to update the new value when running reconfigure, meaning reconfigure can't be used for that scenario, which isn't so bad. Because yeah, it's just abstracting out what we do when we renew in a dry run, after actually doing the updates.

In practice, I think going down that stack and marking where inputs get updated in comments (copied up to their callers) would both help reassure us that we're not changing things, and remind us in the future when we go to do those updates to check on those functions' callers. But again ideally nothing significant should be happening in those functions.

Or maybe even explicitly updating function names to indicate when values are modified. This comment inspired by my nemesis when writing the code the first time, when I discovered that choose_configurator_plugins BOTH returns the chosen ones AND records it in the config. But yeah, I think the solution is less never rely on functions not changing things, and more make it explicit when they do.

from certbot.