Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Let's Encrypt to transition http sites to https #565

merged 1 commit into from Apr 16, 2016


None yet
2 participants
Copy link

commented Apr 14, 2016

The problem. When the LE role runs for an existing http site, the site already has an Nginx conf in sites-enabled/ with a server block that doesn't include a location block for the Acme Challenge. If the LE role happens to create/load the nginx-challenge-site.conf, its server block will compete in sorting of the glob inclusion and either the Acme Challenge or the actual site won't be served till the nginx-challenge-site.conf is disabled. This means that either the Acme Challenges will fail, or the regular site will be down for a moment.

Proposed solution. This PR resolves the problem of competing server blocks by extending a strategy already in place. The already existing strategy is that when ssl is enabled, the http->https redirection section also loads the Acme Challenge location block so that LE cert renewals will pass challenge tests.

It seems relatively harmless to have the Acme Challenge location block there in the conf all the time. This PR adds the Acme Challenge location block to the non-ssl conf. As a result, when the LE role runs on an existing http site, it will use the site's existing conf (or create a new Acme Challenge conf if somehow there is no conf already). Either way, the Acme Challenge tests pass.

This creates a bit more crossover between the letsencrypt role and the wordpress-setup role, but I'm not sure it can be avoided.

The one context privileged to not have to deal with the extra Acme Challenges location block is what should be the most common: 1) ssl enabled and 2) no www redirect necessary.


This comment has been minimized.

Copy link

commented Apr 14, 2016

Working great 👍


This comment has been minimized.

Copy link

commented Apr 15, 2016

Add changelog then merge :)

@fullyint fullyint force-pushed the fullyint:letsencrypt branch to 9d9c2ab Apr 16, 2016

@fullyint fullyint merged commit 41c768d into roots:master Apr 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed

@fullyint fullyint deleted the fullyint:letsencrypt branch Apr 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.