[Bug] Pre and post hook don't escape correctly the command

[Yivan]

Hello,

First thank you for providing us this very nice script to manage let's encrypt certificats.

I would like to submit a bug which break renewal certificats.
Here is how to reproduce it.

Please run:
/root/.acme.sh/acme.sh --force --staging --standalone --issue -d www.my-domain.com --pre-hook "echo 'always return true' || true"
-> command will be ok
Run it a second or third time: you will get a sed: -e expression #1, char 58: unknown option to s'` like:

[Fri Dec 14 15:13:41 CET 2018] Using stage ACME_DIRECTORY: https://acme-staging.api.letsencrypt.org/directory
sed: -e expression #1, char 58: unknown option to `s'
[Fri Dec 14 15:13:42 CET 2018] Run pre hook:'echo 'always return true' || true'
always return true
[Fri Dec 14 15:13:42 CET 2018] Standalone mode.
...

If you remove the || true in the hook, it work great and their is no more the sed problem.
So the problem only appear with hook finishing by || true.

This problem don't just ouput this error message when generating certs, but break the configuration generation for the domain when renewing later (some config line are missing). The cronjob will fail and running manually "/root/.acme.sh"/acme.sh --force --cron --home "/root/.acme.sh" will send a error Please specify at least one validation method: '--webroot', '--standalone', '--apache', '--nginx' or '--dns' etc. because some fields are missing in the configuration.

I hope this bug can be corrected, because actually it is not possible to use || true, and i think it reveals a deeper bug in the parser/generating configuration...

Thanks.

[Yivan]

Just to give more information with debug 2 level:

[Fri Dec 14 15:58:13 CET 2018] ACME_NEW_NONCE
[Fri Dec 14 15:58:13 CET 2018] ACME_VERSION
[Fri Dec 14 15:58:13 CET 2018] Le_NextRenewTime='1549894425'
sed: -e expression #1, char 58: unknown option to `s'
[Fri Dec 14 15:58:13 CET 2018] _on_before_issue
[Fri Dec 14 15:58:13 CET 2018] _chk_main_domain='www.my-domain.com'
[Fri Dec 14 15:58:13 CET 2018] Run pre hook:'echo 'always return true' || true'
always return true
[Fri Dec 14 15:58:13 CET 2018] 'no' contains 'no'
[Fri Dec 14 15:58:13 CET 2018] Le_LocalAddress

[Neilpang]

yes, do not use any special chars, like ' or '"' etc. It's not guaranteed.
Just use simple commands, or you can write your complex commands in to a shell script, and call it in the hooks.

[Yivan]

Thanks for your fast return.

It is just it was more convient if we could not use a script. The final command i wanted to run as prehook was:
docker stop apache || true
Because if apache is not up (for any reason), the docker command will return an error code and so the renew will be aborted by acme.sh (if prehook has error it doesn't go further).

Sure it can be prevented like you said by using a custom script running this command.
If you think the problem should be corrected on acme.sh side to support || true or maybe a new option switch do-not-stop-on-hook-errors for instance, you can leave this issue open.
If you think it should not you can close it.

Thanks.

[morungos]

Does this apply to --reloadcmd too? And is this a new effect? My deployment has been happy for a few months but I just started getting cron errors:

sed: -e expression #1, char 68: unknown option to `s'

Because this just started showing for me, and it's now running via cron, I'm not entirely sure what I need to do to fix it, although I did use a --reloadcmd with the "|| true" when I used --install-cert, so I'm suspicious it's now implanted somewhere in a config.

[Yivan]

Yes I think this problem can be the same for the reloadcmd command.
As this issue is closed, fee free to open a new one to describe your problem and link it to this one.

[Neilpang]

same as #2134

[Neilpang]

fixed, please try again.