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

Avoid race condition when renewing certificates #310

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

deric
Copy link
Contributor

@deric deric commented Mar 30, 2023

Pull Request (PR) description

This PR makes sure that only single certificate can be processed at the same time. Make sure to obtain exclusive lock before executing renewal command.

flock command is used to obtain exclusive lock (with 30 seconds timeout).

This Pull Request (PR) fixes the following issues

Resolve errors like this, when randomized cron job time is not enough:

Another instance of Certbot is already running.

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it needs some work. I added in-line comments regarding the concerns I have reviewing the change. Feel free to ask if you need more details! Thank you

templates/renew-script.sh.erb Outdated Show resolved Hide resolved
templates/renew-script.sh.erb Outdated Show resolved Hide resolved
files/certbot_lock.sh Outdated Show resolved Hide resolved
files/certbot_lock.sh Outdated Show resolved Hide resolved
@deric deric force-pushed the lock branch 2 times, most recently from 40e6fb7 to 4210f4c Compare March 31, 2023 11:52
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still question why this module has such a complex renewal process. Certbot itself has hooks to run after certain points. Why can't it rely on that and use certbot renew? That's already by default running in cron / systemd timer in many Linux packages and makes this module much less invasive.

Quoting https://eff-certbot.readthedocs.io/en/stable/using.html#renewing-certificates

You can also specify hooks by placing files in subdirectories of Certbot’s configuration directory. Assuming your configuration directory is /etc/letsencrypt, any executable files found in /etc/letsencrypt/renewal-hooks/pre, /etc/letsencrypt/renewal-hooks/deploy, and /etc/letsencrypt/renewal-hooks/post will be run as pre, deploy, and post hooks respectively when any certificate is renewed with the renew subcommand. These hooks are run in alphabetical order and are not run for other subcommands. (The order the hooks are run is determined by the byte value of the characters in their filenames and is not dependent on your locale.)

https://eff-certbot.readthedocs.io/en/stable/using.html#id5 also says it already has locks in place.

Doesn't this move the whole module in the wrong direction by inventing more custom code?

Only single certificate can be processed at the same time. Make sure to
obtain exclusive lock before executing renewal command.
@deric
Copy link
Contributor Author

deric commented Aug 7, 2023

@ekohl Yes, there are locking mechanisms in cerbot, but the process will end with an error instead of waiting for releasing the lock.

I agree it's pretty complex. Part of the problem is that:

$cron_minute = fqdn_rand(60, $title)
$cron_hour     = fqdn_rand(24, $title)

doesn't provide much flexibility. With ~20-30 certificates you can easily run into collisions. If you have only a few certificates you should be fine (or you could define those times manually).

@deric deric requested a review from smortex August 7, 2023 13:51
@ekohl
Copy link
Member

ekohl commented Aug 8, 2023

@deric my whole suggestion was to use the command certbot renew instead of one cronjob per certificate. certbot can figure out what needs renewel on its own. It's a big refactor for the whole module, but it would greatly simplify things.

@bastelfreak
Copy link
Member

I agree with @ekohl . In the past I also had one systemd timer per certificate but that doesn't make much sense. certbot itself is really flexible with different hooks and can deal multiple certificates.

@deric
Copy link
Contributor Author

deric commented Aug 9, 2023

@ekohl You're right, reducing per-site cron jobs to a single cron job would be definitely a better option.

@ekohl
Copy link
Member

ekohl commented Aug 9, 2023

There's even an additional advantage: if multiple certificates are renewed at the same time you can reload/restart services which consume multiple certificates (Apache, nginx) only once instead of for each certificate.

@kenyon kenyon added the bug Something isn't working label Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants