ungleich-gitea commented

Owner

Tangentially related to: !974 and !873

Fixing this would also fix ungleich-public/cdist-contrib#2

Description of the issues

I detected a renewal hook did not run, even though the cert itself did get renewed, upon inspection I see:

# cat /etc/cron.d/certbot  | grep -v '^#'
SHELL=/bin/sh
PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin

0 */12 * * * root test -x /usr/bin/certbot -a \! -d /run/systemd/system && perl -e 'sleep int(rand(43200))' && certbot -q renew

# crontab -l | grep certb
47 0 * * * /usr/bin/certbot renew -q  --renew-hook "service nginx reload" # __cron/letsencrypt-certbot

Race condition due to two renewal jobs

So there is a bit of a race condition: on the /etc/cron.d/certbot entry, there is a random sleep of anything between 0 to 12h, if that happens to be less than 47 minutes (about 6.53% chances), that job will renew the certificate but not apply the hook as expected.

Misleading expectation of per-certificate hook

__letsencrypt_cert example.com \
    --admin-email root@example.com \
    --automatic-renewal \
    --renew-hook "service nginx reload" \
    --webroot /data/letsencrypt/root

__letsencrypt_cert smtp.example.com \
    --admin-email root@example.com \
    --automatic-renewal \
    --renew-hook "service postfix reload" \
    --webroot /data/letsencrypt/root

I would expect this to:

Work
Apply the different hooks to each certificate

Instead (not tested) this execution would fail due to discordant arguments for the __cron/letsencrypt-certbot object.

Conclusion

!974 and !873 are not sufficient to fix these issues, we should instead rely on per-realm/certificate configuration, using following.

I intend to implement this with a clear migration path for current installations rendering those MRs redundant by:

Using:
https://certbot.eff.org/docs/using.html?highlight=hook#pre-and-post-validation-hooks

Deprecating --renew-hook, having it work as an alias for --deploy-hook (see below).
Adding optional arguments: --deploy-hook, --pre-hook, --post-hook
These would create a ${ETC_DIR}/letsencrypt/renewal-hooks/{deploy,pre,post}/${__object_id}.cdist.sh script with the proper permissions.
And contents similar to:

#!/bin/sh -e

if echo "${CERTBOT_ALL_DOMAINS}" | tr ',' '\n' | grep "^${__object_id}$"; then
    ${HOOK_STRING_CONTENTS}
fi

Tangentially related to: !974 and !873 Fixing this would also fix ungleich-public/cdist-contrib#2 # Description of the issues I detected a renewal hook did not run, even though the cert itself did get renewed, upon inspection I see: ``` # cat /etc/cron.d/certbot | grep -v '^#' SHELL=/bin/sh PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin 0 */12 * * * root test -x /usr/bin/certbot -a \! -d /run/systemd/system && perl -e 'sleep int(rand(43200))' && certbot -q renew # crontab -l | grep certb 47 0 * * * /usr/bin/certbot renew -q --renew-hook "service nginx reload" # __cron/letsencrypt-certbot ``` ## Race condition due to two renewal jobs So there is a bit of a race condition: on the `/etc/cron.d/certbot` entry, there is a random sleep of anything between 0 to 12h, if that happens to be less than 47 minutes (about 6.53% chances), that job will renew the certificate but not apply the hook as expected. ## Misleading expectation of per-certificate hook ``` __letsencrypt_cert example.com \ --admin-email root@example.com \ --automatic-renewal \ --renew-hook "service nginx reload" \ --webroot /data/letsencrypt/root __letsencrypt_cert smtp.example.com \ --admin-email root@example.com \ --automatic-renewal \ --renew-hook "service postfix reload" \ --webroot /data/letsencrypt/root ``` I would expect this to: 1. Work 2. Apply the different hooks to each certificate Instead (not tested) this execution would fail due to discordant arguments for the `__cron/letsencrypt-certbot` object. # Conclusion !974 and !873 are not sufficient to fix these issues, we should instead rely on per-realm/certificate configuration, using following. I intend to implement this with a clear migration path for current installations rendering those MRs redundant by: Using: https://certbot.eff.org/docs/using.html?highlight=hook#pre-and-post-validation-hooks 1. Deprecating `--renew-hook`, having it work as an alias for `--deploy-hook` (see below). 2. Adding `optional` arguments: `--deploy-hook`, `--pre-hook`, `--post-hook` 3. These would create a `${ETC_DIR}/letsencrypt/renewal-hooks/{deploy,pre,post}/${__object_id}.cdist.sh` script with the proper permissions. And contents similar to: ``` #!/bin/sh -e if echo "${CERTBOT_ALL_DOMAINS}" | tr ',' '\n' | grep "^${__object_id}$"; then ${HOOK_STRING_CONTENTS} fi ```

evilham was assigned by ungleich-gitea

2021-11-20 11:24:47 +00:00

ungleich-gitea closed this issue

2021-11-20 11:24:48 +00:00

ungleich-gitea commented

2021-11-20 15:25:32 +00:00

Author

Owner

mentioned in issue cdist-contrib#2

ungleich-gitea commented

2021-11-20 15:25:34 +00:00

Author

Owner

mentioned in commit b3a9c907ad

mentioned in commit b3a9c907ad3811676e75145e8678a55b8bbf41d3

ungleich-gitea commented

2021-11-20 15:25:35 +00:00

Author

Owner

mentioned in merge request !977

ungleich-gitea commented

2021-11-20 15:25:37 +00:00

Author

Owner

mentioned in commit bc145bbc27

mentioned in commit bc145bbc27a45e382adb744e6774fb803f4fda0a

ungleich-gitea commented

2021-11-20 15:25:38 +00:00

Author

Owner

I also propose deprecating --automatic-renewals, it makes no sense and is a maintenance nightmare. If an OS needs it (like Alpine and, from what I can tell Arch), we can enable autorenewal system-wide, which is what was happening now anyway.

I also propose deprecating `--automatic-renewals`, it makes no sense and is a maintenance nightmare. If an OS needs it (like Alpine and, from what I can tell Arch), we can enable autorenewal system-wide, which is what was happening now anyway.

ungleich-gitea commented

2021-11-20 15:25:39 +00:00

Author

Owner

I don't have much experience with Alpine, can someone confirm that it does not have an in-distro mechanism for automatic renewals?

ungleich-gitea commented

2021-11-20 15:25:41 +00:00

Author

Owner

Sounds like the proper solution to me. We can even deprecate --renew-hook and remove it in the next version change of cdist.

ungleich-gitea commented

2021-11-20 15:25:42 +00:00

Author

Owner

@nico / @ander / @steven at your scale you have probably hit this, does the proposal sound good? Should I work on it?

[__letscrypt_cert] Hooks are wrongly implemented #13

Description of the issues

Race condition due to two renewal jobs

Misleading expectation of per-certificate hook

Conclusion