{"action":"created","issue":{"url":"https://api.github.com/repos/certbot/certbot/issues/3204","repository_url":"https://api.github.com/repos/certbot/certbot","labels_url":"https://api.github.com/repos/certbot/certbot/issues/3204/labels{/name}","comments_url":"https://api.github.com/repos/certbot/certbot/issues/3204/comments","events_url":"https://api.github.com/repos/certbot/certbot/issues/3204/events","html_url":"https://github.com/certbot/certbot/pull/3204","id":161824459,"number":3204,"title":"Fix Nginx reversion","user":{"login":"bmw","id":6504915,"avatar_url":"https://avatars.githubusercontent.com/u/6504915?v=3","gravatar_id":"","url":"https://api.github.com/users/bmw","html_url":"https://github.com/bmw","followers_url":"https://api.github.com/users/bmw/followers","following_url":"https://api.github.com/users/bmw/following{/other_user}","gists_url":"https://api.github.com/users/bmw/gists{/gist_id}","starred_url":"https://api.github.com/users/bmw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bmw/subscriptions","organizations_url":"https://api.github.com/users/bmw/orgs","repos_url":"https://api.github.com/users/bmw/repos","events_url":"https://api.github.com/users/bmw/events{/privacy}","received_events_url":"https://api.github.com/users/bmw/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/certbot/certbot/labels/nginx","name":"nginx","color":"eb6420"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/certbot/certbot/milestones/22","html_url":"https://github.com/certbot/certbot/milestones/0.9.0","labels_url":"https://api.github.com/repos/certbot/certbot/milestones/22/labels","id":1805759,"number":22,"title":"0.9.0","description":"Nginx plugin","creator":{"login":"bmw","id":6504915,"avatar_url":"https://avatars.githubusercontent.com/u/6504915?v=3","gravatar_id":"","url":"https://api.github.com/users/bmw","html_url":"https://github.com/bmw","followers_url":"https://api.github.com/users/bmw/followers","following_url":"https://api.github.com/users/bmw/following{/other_user}","gists_url":"https://api.github.com/users/bmw/gists{/gist_id}","starred_url":"https://api.github.com/users/bmw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bmw/subscriptions","organizations_url":"https://api.github.com/users/bmw/orgs","repos_url":"https://api.github.com/users/bmw/repos","events_url":"https://api.github.com/users/bmw/events{/privacy}","received_events_url":"https://api.github.com/users/bmw/received_events","type":"User","site_admin":false},"open_issues":36,"closed_issues":21,"state":"open","created_at":"2016-06-02T19:22:08Z","updated_at":"2016-06-23T20:44:52Z","due_on":"2016-06-27T07:00:00Z","closed_at":null},"comments":6,"created_at":"2016-06-23T01:59:02Z","updated_at":"2016-06-23T21:41:15Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/certbot/certbot/pulls/3204","html_url":"https://github.com/certbot/certbot/pull/3204","diff_url":"https://github.com/certbot/certbot/pull/3204.diff","patch_url":"https://github.com/certbot/certbot/pull/3204.patch"},"body":"Fixes #1977.\r\n\r\nAfter auditing the code for creating/modifying/removing files, proper reverter use, etc., it turns out this was trivial. The problem occurred when the Nginx plugin was performing a challenge. Before setting up the challenge, it would create a permanent checkpoint and write out the Nginx config. It is not expected that a checkpoint is created here so when rolling back, the client always rolled back one too few checkpoints.\r\n\r\nIt turns out that no changes are made before this save point so I deleted it (although if you break after the save in `master` you'll likely see a lot of changes, but that's only because your configuration has been normalized). The reason it was even there in the first place is the Nginx plugin is modeled after the Apache plugin which makes a temporary save at this point (the save used to be permanent in Apache).\r\n\r\nUsing this PR, you should ALWAYS be able to revert your Nginx configuration to a previous state. This can be accomplished using `certbot rollback`. Furthermore, Certbot should revert in progress changes if it is interrupted. This includes hitting ctrl+c at any point during the program.\r\n\r\nNOTE: If it's interrupted when enabling a redirect but already successfully installed your cert, only the redirect changes are reverted. This is the desired behavior and is mentioned in the program output after handling this case. You can rollback all the way using `certbot rollback`.\r\n\r\nThe big caveat here is it's possible to interrupt the code rolling back your changes. For example, if you hit ctrl+c once to trigger the error handling code and then hit ctrl+c again, you'll interrupt the error handler and your changes might not be reverted. With that said, even if you interrupt the error handler, `certbot rollback` will still work.\r\n\r\nWith all that said, I encourage you to try and break this PR. I tried and couldn't but this will certainly benefit from additional testing, including the certbot-compatibility-test."},"comment":{"url":"https://api.github.com/repos/certbot/certbot/issues/comments/228193084","html_url":"https://github.com/certbot/certbot/pull/3204#issuecomment-228193084","issue_url":"https://api.github.com/repos/certbot/certbot/issues/3204","id":228193084,"user":{"login":"bmw","id":6504915,"avatar_url":"https://avatars.githubusercontent.com/u/6504915?v=3","gravatar_id":"","url":"https://api.github.com/users/bmw","html_url":"https://github.com/bmw","followers_url":"https://api.github.com/users/bmw/followers","following_url":"https://api.github.com/users/bmw/following{/other_user}","gists_url":"https://api.github.com/users/bmw/gists{/gist_id}","starred_url":"https://api.github.com/users/bmw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bmw/subscriptions","organizations_url":"https://api.github.com/users/bmw/orgs","repos_url":"https://api.github.com/users/bmw/repos","events_url":"https://api.github.com/users/bmw/events{/privacy}","received_events_url":"https://api.github.com/users/bmw/received_events","type":"User","site_admin":false},"created_at":"2016-06-23T21:41:15Z","updated_at":"2016-06-23T21:41:15Z","body":"For those that want some bedtime reading about how Certbot helps guarantee successful reversion when using plugins, here's a ton of information I've written over the past few months. I'll probably try and add some of this information to our docs/code if it's not already present. In the meantime:\r\n\r\nIf you're not already familiar with [plugin interface](https://github.com/certbot/certbot/blob/master/certbot/interfaces.py#L99), read that first. Relevant interfaces are IPlugin, IAuthenticator, and IInstaller.\r\n\r\nNext, [this comment](https://github.com/certbot/certbot/issues/1977#issuecomment-185493304) describes how Certbot uses these methods to restore your config.\r\n\r\nFurthermore, [this comment](https://github.com/certbot/certbot/pull/2510#issue-134968581) talks a bit about how the reverter works and a little about a design decision around `perform` and `cleanup`.\r\n\r\nFinally, here are my personal notes I used when writing this PR. I cleaned them up and expanded them a bit so hopefully they're more understandable:\r\n\r\n```\r\nClient assumptions:\r\n    - recovery_routine() reverts all unfinalized changes\r\n    - cleanup() reverts all changes made by perform(), even if perform is\r\n      interupted\r\n    - save() finalizes exactly one checkpoint when given a title and temporary\r\n      is set to False. Furthermore, a call to this function finalizes at most\r\n      one checkpoint and this is the only plugin method that finalizes\r\n      checkpoints. If an exception was thrown during save, a checkpoint was not \r\n      finalized\r\n\r\nWhere does Nginx make changes?\r\nBelow is a tree that starts with direct modifiers to the Nginx config and \r\nbranches based on callers. Branch ends when only callers are outside of the \r\nplugin.\r\n\r\n- nginxparser.dump\r\n    - parser.NginxParser.filedump\r\n        - configurator.NginxConfigurator.save\r\n            - tls_sni_01.perform * 2 \r\n                - configurator.perform\r\n    - tls_sni_01.NginxTlsSni01.perform\r\n- reverter.Reverter.recovery_routine\r\n    - configurator.recovery_routine\r\n    - configurator.__init__\r\n- reverter.Reverter.revert_temporary_config\r\n    - configurator.revert_challenge_config\r\n        - configurator.cleanup\r\n- reverter.Reverter.rollback_checkpoints\r\n\r\nWhere does Nginx register changes with the reverter?\r\n- All changes through configurator.NginxConfigurator.save are backed up through\r\n  add_to_checkpoint and add_to_temp_checkpoint.\r\n- Change made in perform is backed up through call to register_file_creation.\r\n\r\nProblem:\r\n- We finalize a checkpoint in perform() violating our assumption that only save\r\n  finalizes checkpoints.\r\n- Additionally, because cleanup() doesn't revert the checkpoint, cleanup()\r\n  violates the assumption that it correctly reverts changes made by\r\n  perform()\r\n\r\nSolution:\r\n- Stop finalizing checkpoints in perform(). This solves both problems\r\n  identified above.\r\n```\r\n\r\nIf you read all of this and you're not asleep yet, I recommend sleeping pills."}}
{"actor":{"display_login":"bmw"}}