Avoid race condition when renewing certificates

[deric]

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.

[smortex]

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

[ekohl]

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?

[deric]

@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).

[ekohl]

@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]

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]

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

[ekohl]

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.