Bug 1444814

Summary: certbot: error: argument --pre-hook: expected one argument
Product: [Fedora] Fedora EPEL Reporter: Brian J. Murrell <brian>
Component: certbotAssignee: James Hogarth <james.hogarth>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: urgent Docs Contact:
Priority: unspecified    
Version: epel7CC: alexey, bmw, igeorgex, itamar, james.hogarth, mharris, nb, nick, rbu, rhbz
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-09-11 14:39:54 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
patch to the unit file using ${} syntax none

Description Brian J. Murrell 2017-04-24 11:22:52 UTC
Description of problem:
When I try to run the certbot-renew service I get:

Apr 24 07:15:23 server certbot[26600]: usage:
Apr 24 07:15:23 server certbot[26600]: certbot [SUBCOMMAND] [options] [-d DOMAIN] [-d DOMAIN] ...
Apr 24 07:15:23 server certbot[26600]: Certbot can obtain and install HTTPS/TLS/SSL certificates.  By default,
Apr 24 07:15:23 server certbot[26600]: it will attempt to use a webserver both for obtaining and installing the
Apr 24 07:15:23 server certbot[26600]: cert.
Apr 24 07:15:23 server certbot[26600]: certbot: error: argument --pre-hook: expected one argument
Apr 24 07:15:23 server systemd[1]: certbot-renew.service: main process exited, code=exited, status=2/INVALIDARGUMENT
Apr 24 07:15:23 server systemd[1]: Failed to start This service automatically renews any certbot certificates found.
Apr 24 07:15:23 server systemd[1]: Unit certbot-renew.service entered failed state.
Apr 24 07:15:23 server systemd[1]: certbot-renew.service failed.

Version-Release number of selected component (if applicable):
certbot-0.12.0-4.el7.noarch

How reproducible:
100%

Steps to Reproduce:
1. systemctl start certbot-renew.service

Actual results:
Apr 24 07:15:23 server certbot[26600]: usage:
Apr 24 07:15:23 server certbot[26600]: certbot [SUBCOMMAND] [options] [-d DOMAIN] [-d DOMAIN] ...
Apr 24 07:15:23 server certbot[26600]: Certbot can obtain and install HTTPS/TLS/SSL certificates.  By default,
Apr 24 07:15:23 server certbot[26600]: it will attempt to use a webserver both for obtaining and installing the
Apr 24 07:15:23 server certbot[26600]: cert.
Apr 24 07:15:23 server certbot[26600]: certbot: error: argument --pre-hook: expected one argument
Apr 24 07:15:23 server systemd[1]: certbot-renew.service: main process exited, code=exited, status=2/INVALIDARGUMENT
Apr 24 07:15:23 server systemd[1]: Failed to start This service automatically renews any certbot certificates found.
Apr 24 07:15:23 server systemd[1]: Unit certbot-renew.service entered failed state.
Apr 24 07:15:23 server systemd[1]: certbot-renew.service failed.

Expected results:
Should run without error

Additional info:
I suppose there is something going wrong with the quoting in the sysconfig file.  I have not worked out what yet though.

Comment 1 Brian J. Murrell 2017-05-01 13:55:38 UTC
Any ideas here?  This is preventing (automatic) renewals from happening and I don't want my certs to expire.

Comment 2 Anatole Denis 2017-05-02 21:01:25 UTC
The quoting done in the sysconfig file is incorrect and should be done in the systemd unit file itself. As indicated in https://www.freedesktop.org/software/systemd/man/systemd.service.html#Command%20lines

>  Double quotes ("…") and single quotes ('…') may be used, in which case 
> everything until the next matching quote becomes part of the same argument. 
> Quotes themselves are removed

The default variables containing only quotes (PRE_HOOK="''") are expanded into nothing

I'll attach a patch file correcting the systemd unit by using the ${} syntax to force forming a single argument from the hooks variables, which seems to work even with empty variables.

As a workaround, I'm having success by setting "/bin/true" as the XXX_HOOK parameters in the sysconfig file for hooks that I'm not using, with the current unit file

Comment 3 Anatole Denis 2017-05-02 21:02:39 UTC
Created attachment 1275745 [details]
patch to the unit file using ${} syntax

Comment 4 Brian J. Murrell 2017-05-04 10:34:07 UTC
The result of that patch, when applied here manually seems to yield better results.

Thanks much!

Comment 5 Mike A. Harris 2017-05-05 15:13:04 UTC
I can reproduce the issue and confirm the workaround mentioned in https://bugzilla.redhat.com/show_bug.cgi?id=1444814#c2 appears to work.

Comment 6 JM 2017-05-08 15:46:55 UTC
Same problem here, the patch from comment #3 works and solved the problem.

Comment 7 James Hogarth 2017-05-12 14:56:19 UTC
Thanks for the investigation work, not sure then why it worked on my testing here but then that was on Fedora and I, obviously wrongly, assumed the same behaviour on systemd for them both.

This issue I have is that I wanted to include the --pre-hook (and post etc) in the certbot renew ExecStart directly, but with "" in sysconfig there was no value provided to that which then gets an error.

When I tested with "''" in sysconfig then '' was provided as the value which then worked.

I'll do another verify on AWS on both fedora and centos and then issue the update to fix this by default as part of 0.14.0

Comment 8 James Hogarth 2017-05-12 16:48:09 UTC
Right after doing some testing with the certbot 0.14.0 build in AWS trying to have --post-hook in the ExecStart by default just isn't going to work ...

Leave it empty and it complains it needs an argument, make the argument '' and it complains there is no executable it can run.

So what I'm going to move to is having the line:

ExecStart=/usr/bin/certbot renew $PRE_HOOK $POST_HOOK $RENEW_HOOK $CERTBOT_ARGS

Then in sysconfig the various hooks will be populated as:

POST_HOOK="" 

To make use of the hook, and this will be a comment you can just uncomment in sysconfig, the full argument will need to be set

POST_HOOK="--post-hook 'systemctl restart httpd'"

I just tested this setup and it worked correctly.

Please pay attention to your configs to merge it okay and make it happy.

Comment 9 Anatole Denis 2017-05-12 17:24:29 UTC
That seems counter-intuitive and error-prone. At that point I would consider just dropping the *_HOOK args and letting people put whatever they need in the CERTBOT_ARGS variable.

What do you think of using the same logic as for the current workaround and having /bin/true as the default value instead ? This also has the benefit of allowing the use of the ${VARIABLE} syntax sparing us from having to remember to escape things in the sysconfig file.

Comment 10 James Hogarth 2017-05-12 21:39:04 UTC
I did think about dropping the *_HOOK env vars ... but I liked the clarity of having them there ... plus it would be even more of a drastic change from 0.12.0

I'm good with defaulting them to /bin/true though as the workaround to make it all happy.

I'll add that to a fresh build over the weekend and include it as part of the 0.14.x package.

Comment 11 Fedora Update System 2017-05-17 11:30:30 UTC
certbot-0.14.1-2.fc26 python-acme-0.14.1-1.fc26 python-certbot-apache-0.14.1-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-64270716a0

Comment 12 Fedora Update System 2017-05-17 11:31:35 UTC
certbot-0.14.1-2.fc26 python-acme-0.14.1-1.fc26 python-certbot-apache-0.14.1-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-64270716a0

Comment 13 Fedora Update System 2017-05-17 11:33:59 UTC
certbot-0.14.1-2.fc25 python-acme-0.14.1-1.fc25 python-certbot-apache-0.14.1-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-f1071b956e

Comment 14 Fedora Update System 2017-05-17 11:34:38 UTC
certbot-0.14.1-2.fc25 python-acme-0.14.1-1.fc25 python-certbot-apache-0.14.1-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-f1071b956e

Comment 15 Fedora Update System 2017-05-17 11:36:28 UTC
certbot-0.14.1-2.fc24 python-acme-0.14.1-1.fc24 python-certbot-apache-0.14.1-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-c651872919

Comment 16 Fedora Update System 2017-05-17 11:37:15 UTC
certbot-0.14.1-2.fc24 python-acme-0.14.1-1.fc24 python-certbot-apache-0.14.1-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-c651872919

Comment 17 Fedora Update System 2017-05-17 11:38:39 UTC
certbot-0.14.1-2.el7 python-acme-0.14.1-1.el7 python-certbot-apache-0.14.1-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-acdfbe8cfa

Comment 18 Fedora Update System 2017-05-17 11:39:10 UTC
certbot-0.14.1-2.el7 python-acme-0.14.1-1.el7 python-certbot-apache-0.14.1-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-acdfbe8cfa

Comment 19 James Hogarth 2017-05-17 11:39:44 UTC
Apologies for the delay but there was breakage shown in the tests with augeas 1.7+ 

I'd appreciate if you can test the new 0.14.1 build and if it meets your expectations provide karma on the builds so we can get it out to branches ASAP

Comment 20 Fedora Update System 2017-05-17 19:07:13 UTC
certbot-0.14.1-2.fc26, python-acme-0.14.1-1.fc26, python-certbot-apache-0.14.1-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-64270716a0

Comment 21 Fedora Update System 2017-05-17 21:30:35 UTC
certbot-0.14.1-2.el7, python-acme-0.14.1-1.el7, python-certbot-apache-0.14.1-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-acdfbe8cfa

Comment 22 Fedora Update System 2017-05-17 23:07:03 UTC
certbot-0.14.1-2.fc24, python-acme-0.14.1-1.fc24, python-certbot-apache-0.14.1-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-c651872919

Comment 23 Fedora Update System 2017-05-17 23:11:34 UTC
certbot-0.14.1-2.fc25, python-acme-0.14.1-1.fc25, python-certbot-apache-0.14.1-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-f1071b956e

Comment 24 Brad Warren 2017-05-18 15:55:48 UTC
I'm the technical lead on Certbot and there's actually another substantial problem that's made worse by the changes here. If you run something like:
```
certbot certonly --standalone --pre-hook "systemctl stop nginx" --post-hook "systemctl start nginx"
```
These values get saved in /etc/letsencrypt/renewal/domain.conf like:
```
...
[renewalparams]
authenticator = standalone
post_hook = systemctl start nginx
pre_hook = systemctl stop nginx
...
```
Normally when you run `certbot renew`, these hooks are reused when it comes time to renew that certificate, however, if your certificates are renewed by https://src.fedoraproject.org/cgit/rpms/certbot.git/tree/certbot-renew-systemd.service?id=ef6c3a40fba22c36585912c447c8ef0c1db3650d, these hooks are ignored and /etc/letsencrypt/renewal/domain.conf becomes:
```
...
[renewalparams]
authenticator = standalone
post_hook = /bin/true
pre_hook = /bin/true
renew_hook = /bin/true
...
```
Previously, certbot-renew.service would fail about an invalid argument but if the user had scripted their own renewal, it can actually break things by overwriting their hooks they may have been relying on. It also removes the ability for users to define different hooks for each certificate.

I'm happy to open another bug report about this but I was directed here and this is definitely related. The easiest course of action is to remove the *_HOOK vars entirely but there are other options as well.

Comment 25 Anatole Denis 2017-05-18 16:48:36 UTC
This seems to be a problem indeed for users that have setup hooks when first getting a certificate.

However, neither certbot --help certonly or the online docs mention this behavior, or even that the --*-hook options are available for any other command than renew. The closest I found to a hint that this would work is this paragraph documenting the renew command: 

> For more fine-grained control, you can renew individual lineages with the
> `certonly` subcommand. Hooks are available to run commands before and
> after renewal; see https://certbot.eff.org/docs/using.html#renewal for
> more information on these.

And also this issue: https://github.com/certbot/certbot/issues/4103

So it seems reasonable to assume, at least for now, that no hooks have been setup by most users when getting the certificates in the first place, since they wouldn't know it is possible. So overriding them wouldn't be an issue.

As a user, I would argue for the complete renewal of the sysconfig file anyway, and leave only a $CERTBOT_ARGS variable in the systemd unit file to be overriden by a systemd partial unit if the user wants special parameters.

Maybe it could be migrated with a postinst scriptlet that would move any hooks defined by /etc/sysconfig/certbot into the config files in /etc/letsencrypt (without overriding existing ones), to ensure compatibility when the variables are removed ?

In any case, you should probably be documenting that behavior, it is quite useful to have hooks by domain !

Comment 26 James Hogarth 2017-05-18 23:21:14 UTC
So, I've given this a lot of thought and I do like splitting out and specifying the hooks in sysconfig for the considerable number of situations that a global hook makes sense and it looks cleaner with the different sections so far easier to read.

However I would like to be able to support per certificate configuration of hooks, as I can see a lot of potential in that.

In light of the upstream developer feedback highlighting the known issues I've made my decision and the package is going to work as follows, now that I've carried out some testing.

The sysconfig put in place will have it split out as previous, but the values of PRE_HOOK, POST_HOOK and RENEW_HOOK will default to "" so that there's no argument at all provided to certbot renew by default, which permits the per certificate configuration.

The examples in the sysconfig that can be uncommented will include the --post-hook etc to make it clear they need to be included.

There will also be a note in the sysconfig pointing out per certificate config for hooks is possible.

See here for what I mean: https://src.fedoraproject.org/cgit/rpms/certbot.git/tree/certbot-sysconfig-certbot

I have tested this and it does work as expected.

I am keeping with the sysconfig file as that's what the vast majority of people expect on a Red Hat style system and requiring that people use systemctl edit to add a [Service] override for Environment=CERTBOT_ARGS="--post-hook 'foo'" is not very intuitive to most.

I feel that this will provide maximum flexibility and the greatest ease of use.

I do apologise for the need to update any configurations when this goes stable, but this interface will remain stable going forwards unless something really unexpected happens.

I hope you understand and appreciate my reasoning on this.

Comment 27 Fedora Update System 2017-05-19 16:58:18 UTC
certbot-0.14.1-3.el7 python-acme-0.14.1-1.el7 python-certbot-apache-0.14.1-1.el7 python-certbot-nginx-0.14.1-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-acdfbe8cfa

Comment 28 Fedora Update System 2017-05-19 17:00:24 UTC
certbot-0.14.1-3.fc26 python-acme-0.14.1-1.fc26 python-certbot-apache-0.14.1-1.fc26 python-certbot-nginx-0.14.1-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-64270716a0

Comment 29 Fedora Update System 2017-05-19 17:02:37 UTC
certbot-0.14.1-3.fc24 python-acme-0.14.1-1.fc24 python-certbot-apache-0.14.1-1.fc24 python-certbot-nginx-0.14.1-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-c651872919

Comment 30 Fedora Update System 2017-05-19 17:03:43 UTC
certbot-0.14.1-3.fc25 python-acme-0.14.1-1.fc25 python-certbot-apache-0.14.1-1.fc25 python-certbot-nginx-0.14.1-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-f1071b956e

Comment 31 Alexey Pavlyuts 2017-05-19 23:54:17 UTC
The problem is that 'certbot renew' handles all and any installed cert

Therefore put a hook to systemd is a good idea inly if you have just one use of all the serts.

But if you have, for example, http and email servers - you have to run different hooks on different certificates. It's not a case for systemd.

IMHO, the approach to handle it:

1. Keep just one CERTBOT_ARGS in syscinfig. This is pretty enough to pass any extra paraveters.

2. Clearly describe certbot behaviour on relation between cert generation hooks and renewal config in certbot's configuration. To explain that cortbot remembers the last hook in /etc/letsencrypt/renewal/domain.conf and the hook SHOULD be set there and MAY be overridden in sysconfig. Think, it's useful to remove hook refresh on 'renew' action just for simple reason: the probabilty we need to use different hook on renewal seems very low.

Comment 32 Fedora Update System 2017-05-20 08:16:34 UTC
certbot-0.14.1-3.fc26, python-acme-0.14.1-1.fc26, python-certbot-apache-0.14.1-1.fc26, python-certbot-nginx-0.14.1-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-64270716a0

Comment 33 Fedora Update System 2017-05-20 14:31:20 UTC
certbot-0.14.1-3.fc24, python-acme-0.14.1-1.fc24, python-certbot-apache-0.14.1-1.fc24, python-certbot-nginx-0.14.1-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-c651872919

Comment 34 Fedora Update System 2017-05-20 21:02:32 UTC
certbot-0.14.1-3.el7, python-acme-0.14.1-1.el7, python-certbot-apache-0.14.1-1.el7, python-certbot-nginx-0.14.1-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-acdfbe8cfa

Comment 35 Fedora Update System 2017-05-20 22:31:38 UTC
certbot-0.14.1-3.fc25, python-acme-0.14.1-1.fc25, python-certbot-apache-0.14.1-1.fc25, python-certbot-nginx-0.14.1-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-f1071b956e

Comment 36 Fedora Update System 2017-05-30 21:26:40 UTC
certbot-0.14.1-3.fc24, python-acme-0.14.1-1.fc24, python-certbot-apache-0.14.1-1.fc24, python-certbot-nginx-0.14.1-1.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 37 Fedora Update System 2017-05-31 09:02:31 UTC
certbot-0.14.1-3.fc25, python-acme-0.14.1-1.fc25, python-certbot-apache-0.14.1-1.fc25, python-certbot-nginx-0.14.1-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 38 Fedora Update System 2017-06-06 06:24:42 UTC
certbot-0.14.1-3.el7, python-acme-0.14.1-1.el7, python-certbot-apache-0.14.1-1.el7, python-certbot-nginx-0.14.1-1.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

Comment 39 Fedora Update System 2017-06-09 18:51:52 UTC
certbot-0.14.1-3.fc26, python-acme-0.14.1-1.fc26, python-certbot-apache-0.14.1-1.fc26, python-certbot-nginx-0.14.1-1.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.