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
merged 1 commit into from
Apr 16, 2016

Conversation

fullyint
Copy link
Contributor

The problem. When the LE role runs for an existing http site, the site already has an Nginx conf in sites-enabled/site.com.conf 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.

@swalkinshaw
Copy link
Member

Working great 👍

@swalkinshaw
Copy link
Member

Add changelog then merge :)

@fullyint fullyint merged commit 41c768d into roots:master Apr 16, 2016
@fullyint fullyint deleted the letsencrypt branch April 16, 2016 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants