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

Remove unnecessary ID from Let's Encrypt bundled cert filename #834

Merged
merged 1 commit into from
May 15, 2020

Conversation

fullyint
Copy link
Contributor

@fullyint fullyint commented May 4, 2017

#826 says it would be nice for other programs to have a stable path to the Let's Encrypt bundled cert (vs. a filename with an ID that changes).

ID ensures regenerated CSR and cert_file. The CSR uses the ID/hash here to ensure it is regenerated in response to changes in relevant conditions, and an ID in the cert_file name appeared necessary for the correct stat and age calculation for the file.

ID unnecessary in bundled cert filename. However, an ID in the bundled cert filename is not necessary, as far as I can tell. I think I just included it in #630 for filename consistency with the CSR and cert_file. So, this PR removes the ID from the bundled cert filename, for convenience of other programs that use its path. I'm not aware of any external need for the bundled cert filename to be unique with each cert renewal. Indeed, certbot considers it optional to "uniquify" cert paths: certbot/certbot#3009

Backward compatibility This PR also rsyncs existing ID-in-filename bundled certs to their non-ID counterparts. This ensures the non-ID bundled certs exist when users happen to run only the wordpress tag and not the letsencrypt tag. In such a scenario, the wordpress-site.conf files would be regenerated with non-ID filenames but such files would not exist without an rsync that runs under the same Ansible tag. (The two tasks for compatibility are the bulk of this PR, unfortunately. Removing the ID was just 2 lines of code otherwise).

Tags note. This PR also adds the wordpress-setup-nginx tag to the "Generate Lets Encrypt certificate IDs" task, this task being required for the later tasks in this PR to have access to the IDs, if a user happened to trigger these tasks via the wordpress-setup-nginx tag.

This PR also adds the nginx-includes tag to the two rsync tasks because they need to run if a user runs the nginx-includes tag, which would regenerate the wordpress-site.conf with the non-ID bundled cert filename via the "Create WordPress configuration for Nginx" task.

Playing it safe. This PR also augmented the renew-certs.py with a check for the existence of the bundled cert, in the spirit of pursuing "desired state." If the bundled cert doesn't exist, running the rest of the script is the only way it will be created. I concede that it would be a rare case for the cert_file to exist and the bundled_cert_file to not exist, but strange things happen, and this addition doesn't seem to have too big of a price for playing it safe.

@swalkinshaw
Copy link
Member

Well that was easier than I thought :)

Rest of the changes look good too 👍

@fullyint
Copy link
Contributor Author

fullyint commented May 6, 2017

I added a commit that changes the focus, while still removing the ID from the bundled cert filename.

The new focus is to expand renew-certs.py to handle a greater variety of potential starting points, system states. For example, it accommodates the possibility that users will try to run only --tags wordpress after making changes to site_hosts and/or deleting Let's Encrypt files that would require running --tags letsencrypt.

This new approach also adds a tag to the task that executes renew-certs.py, such that --tags wordpress will execute renew-certs.py to create the new non-ID-in-filename bundled cert. This is a more broadly applicable implementation than the two extra rsync tasks in this PR's former commit. I've removed those rsync tasks.

With so many edits to renew-certs.py, the diff view isn't too helpful. It's probably more worthwhile to simply view the new file instead.

@strarsis, The bundled cert's path is now stable (no ID). Is that enough, or is it a problem that the root cert filename and CSR filename will each still have changing IDs? (#286)

@strarsis
Copy link
Contributor

strarsis commented May 6, 2017

@fullyint: The CSR is rarely/never used by TLS configurations and I don't need it for my use case either.
The root cert is the CA cert of Let's Encrypt? The bundled certs already contain the CA cert, right?

@fullyint
Copy link
Contributor Author

fullyint commented May 6, 2017

My error in saying "root cert." I meant to refer to the "end-entity cert" or "leaf certificate" that Let's Encrypt issues for the server. This filename WILL have the changing ID, but I suspect other programs wouldn't use this file.

When I mentioned "bundled cert" I meant the chained certificate that browsers care about: the concatenation of our server's end-entity cert and the Let's Encrypt intermediate cert. It is the only cert file we serve and will NOT have an ID (after this PR).

@strarsis
Copy link
Contributor

strarsis commented May 6, 2017

@fullyint: Most programs require fullchain cert (=bundled) and private key, hence both files should have constant file names.

@swalkinshaw swalkinshaw force-pushed the remove-id-from-bundled-cert branch from b46823d to 91d8a35 Compare April 7, 2020 03:39
@swalkinshaw
Copy link
Member

Resurrecting this from the dead 😄 I've rebased and updated it since the renew certs scripts had significantly changed. Still needs testing.

@swalkinshaw swalkinshaw force-pushed the remove-id-from-bundled-cert branch 2 times, most recently from 94728c9 to 5de70af Compare April 11, 2020 01:04
@swalkinshaw
Copy link
Member

This should be ready to go and I've tested it on a site with Lets Encrypt.

@strarsis would you be able to try this out somehow? (without breaking a production site 😉). It's designed to be very low risk and backwards compatible though.

The CSR and cert_file use an ID in their filenames to ensure
they are regenerated in response to changes in relevant conditions.
However, an ID in the bundled cert filename serves no purpose.
This commit removes the ID from the bundled cert filename, offering
other programs a more stable and static path for the bundled cert.

This commit also rsyncs existing ID-in-filename bundled certs to their
non-ID counterparts. This ensures the non-ID certs exist when users
happen to run only the `wordpress` tag and not the `letsencrypt` tag.
In such a scenario, the wordpress-site.conf files would be regenerated
with non-ID filenames but such files would not exist without an rsync
effective under the same Ansible tag.
@swalkinshaw swalkinshaw force-pushed the remove-id-from-bundled-cert branch from 5de70af to 6968b44 Compare May 15, 2020 02:24
@swalkinshaw swalkinshaw merged commit ec053f4 into master May 15, 2020
@swalkinshaw swalkinshaw deleted the remove-id-from-bundled-cert branch May 15, 2020 22:33
@strarsis
Copy link
Contributor

@swalkinshaw: Awesome, thanks!

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.

3 participants