exit_hook is called at invalid_challenge and not at the end of the cron command #439

New Issue

adam · 2025-12-29T01:25:11+01:00

adam commented

2025-12-29 01:25:11 +01:00

Originally created by @kepi on GitHub (Oct 22, 2019).

Originally assigned to: @lukas2511 on GitHub.

TL;DR: exit_hook is called after every invalid challenge instead of end of the cron run.

I think that during fix of #630 in 1c77730 regression was introduced.

exit_hook has this description (as stated in #630 too):

This hook is called at the end of the cron command and can be used to
do some final (cleanup or other) tasks.

Problem is that when running with keep going option, exit_hook is now called after first validation error. We have similar setup as described in #630 - in startup_hook we are starting simple httpd server which serves well known requests. In exit_hook we are sending SIGQUIT to it.

We had same problem as in #630 - http server was not killed and remained running sometimes. But in our case it wasn't big deal. Much bigger problem is current behavior - after first error https server is killed in exit_hook and all other requests are failing (as we are using keep going option). As we had one failing domain near the start of couple hundred long domains file... fortunately we have monitoring and couple days to fix the situation :)

Originally created by @kepi on GitHub (Oct 22, 2019). Originally assigned to: @lukas2511 on GitHub. TL;DR: `exit_hook` is called after every invalid challenge instead of end of the cron run. I think that during fix of #630 in 1c77730 regression was introduced. **exit_hook** has this description (as stated in #630 too): > This hook is called at the end of the cron command and can be used to > do some final (cleanup or other) tasks. Problem is that when running with *keep going* option, `exit_hook` is now called after first validation error. We have similar setup as described in #630 - in `startup_hook` we are starting simple httpd server which serves well known requests. In `exit_hook` we are sending SIGQUIT to it. We had same problem as in #630 - http server was not killed and remained running sometimes. But in our case it wasn't big deal. Much bigger problem is current behavior - after first error https server is killed in `exit_hook` and all other requests are failing (as we are using keep going option). As we had one failing domain near the start of couple hundred long domains file... fortunately we have monitoring and couple days to fix the situation :)

adam closed this issue

2025-12-29 01:25:11 +01:00

adam commented

2025-12-29 01:25:12 +01:00

@aderumier commented on GitHub (Oct 23, 2019):

Hi,

I have updated recently, and I have also keep going not working anymore.

@aderumier commented on GitHub (Oct 23, 2019): Hi, I have updated recently, and I have also keep going not working anymore.

adam commented

2025-12-29 01:25:12 +01:00

@usev6 commented on GitHub (Apr 14, 2020):

I'm also using --keep-going and I'm afraid this problem prevents an update to https://github.com/dehydrated-io/dehydrated/commit/1c77730373 and newer for me.

I think that #630 raised a valid concern: exit_hook was not called anymore if _exiterr was called first -- thus breaking the ability to do cleanup tasks via this hook.

On the other hand, the possibility to have exit_hook called more than once looks like a bug to me (and breaks --keep-going badly). Before https://github.com/dehydrated-io/dehydrated/commit/1c77730373 there was only one place that called exit_hook and that was at the end of command_sign_domains -- exactly as advertised.

This hook is called at the end of the cron command and can be used to
do some final (cleanup or other) tasks.

IMHO calling a hook in _exiterr and passing it the actual error message is a Good Thing. But re-using the existing hook that was especially useful in combination with --keep-going leads to problems.

I don't see a clean solution. Maybe one could set a global variable (as a flag) before the call to sign_domain if --keep-going was used. (If I'm not mistaken only stuff in sign_domain seems to be protected -- other calls to _exiterr do end the script and should call exit_hook.) _exiterr could then look for that global variable to decide whether exit_hook should be called or not. Something along this line (totally untested):

diff --git a/dehydrated b/dehydrated
index fd1b27a..52d3dd3 100755
--- a/dehydrated
+++ b/dehydrated
@@ -444,7 +444,7 @@ _sed() {
 # Print error message and exit with error
 _exiterr() {
   echo "ERROR: ${1}" >&2
-  [[ -n "${HOOK:-}" ]] && "${HOOK}" "exit_hook" "${1}" || true
+  [[ "${skip_exit_hook:-no}" = "no" ]] && [[ -n "${HOOK:-}" ]] && "${HOOK}" "exit_hook" "${1}" || true
   exit 1
 }
 
@@ -1369,8 +1369,10 @@ command_sign_domains() {
       update_ocsp="yes"
       [[ -z "${csr}" ]] || printf "%s" "${csr}" > "${certdir}/cert-${timestamp}.csr"
       if [[ "${PARAM_KEEP_GOING:-}" = "yes" ]]; then
+        skip_exit_hook="yes"
         sign_domain "${certdir}" ${timestamp} ${domain} ${morenames} &
         wait $! || true
+        skip_exit_hook="no"
       else
         sign_domain "${certdir}" ${timestamp} ${domain} ${morenames}
       fi

@usev6 commented on GitHub (Apr 14, 2020): I'm also using ```--keep-going``` and I'm afraid this problem prevents an update to https://github.com/dehydrated-io/dehydrated/commit/1c77730373 and newer for me. I think that #630 raised a valid concern: ```exit_hook``` was not called anymore if ```_exiterr``` was called first -- thus breaking the ability to do cleanup tasks via this hook. On the other hand, the possibility to have ```exit_hook``` called more than once looks like a bug to me (and breaks ```--keep-going``` badly). Before https://github.com/dehydrated-io/dehydrated/commit/1c77730373 there was only one place that called ```exit_hook``` and that was at the end of ```command_sign_domains``` -- exactly as advertised. > This hook is called at the end of the cron command and can be used to do some final (cleanup or other) tasks. IMHO calling a hook in ```_exiterr``` and passing it the actual error message is a Good Thing. But re-using the existing hook that was especially useful in combination with ```--keep-going``` leads to problems. I don't see a clean solution. Maybe one could set a global variable (as a flag) before the call to ```sign_domain``` if ```--keep-going``` was used. (If I'm not mistaken only stuff in ```sign_domain``` seems to be protected -- other calls to ```_exiterr``` *do* end the script and *should* call ```exit_hook```.) ```_exiterr``` could then look for that global variable to decide whether ```exit_hook``` should be called or not. Something along this line (totally untested): ``` diff --git a/dehydrated b/dehydrated index fd1b27a..52d3dd3 100755 --- a/dehydrated +++ b/dehydrated @@ -444,7 +444,7 @@ _sed() { # Print error message and exit with error _exiterr() { echo "ERROR: ${1}" >&2 - [[ -n "${HOOK:-}" ]] && "${HOOK}" "exit_hook" "${1}" || true + [[ "${skip_exit_hook:-no}" = "no" ]] && [[ -n "${HOOK:-}" ]] && "${HOOK}" "exit_hook" "${1}" || true exit 1 } @@ -1369,8 +1369,10 @@ command_sign_domains() { update_ocsp="yes" [[ -z "${csr}" ]] || printf "%s" "${csr}" > "${certdir}/cert-${timestamp}.csr" if [[ "${PARAM_KEEP_GOING:-}" = "yes" ]]; then + skip_exit_hook="yes" sign_domain "${certdir}" ${timestamp} ${domain} ${morenames} & wait $! || true + skip_exit_hook="no" else sign_domain "${certdir}" ${timestamp} ${domain} ${morenames} fi ```

adam commented

2025-12-29 01:25:12 +01:00

@kepi commented on GitHub (Apr 27, 2020):

@lukas2511 can we please get your opinion? I believe this is critical issue, even if only for some use cases.

I'm willing to try fix this and send pull request but I don't want to spend time with approach witch will not be according to project direction. Thanks.

@kepi commented on GitHub (Apr 27, 2020): @lukas2511 can we please get your opinion? I believe this is critical issue, even if only for some use cases. I'm willing to try fix this and send pull request but I don't want to spend time with approach witch will not be according to project direction. Thanks.

adam commented

2025-12-29 01:25:13 +01:00

@lukas2511 commented on GitHub (Apr 27, 2020):

@kepi i'll have a look at this either today or tomorrow 👍 sorry for not responding earlier

@lukas2511 commented on GitHub (Apr 27, 2020): @kepi i'll have a look at this either today or tomorrow :+1: sorry for not responding earlier

adam commented

2025-12-29 01:25:13 +01:00

@lukas2511 commented on GitHub (Apr 28, 2020):

This should be fixed now. I basically took 1:1 the suggested patch by @usev6. Not the prettiest solution but it works.

@lukas2511 commented on GitHub (Apr 28, 2020): This should be fixed now. I basically took 1:1 the suggested patch by @usev6. Not the prettiest solution but it works.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/dehydrated#439