[PATCH] Multiple deploy hooks in certbot service

  • Open
  • quality assurance status badge
Details
6 participants
  • Arun Isaac
  • Carlo Zancanaro
  • Felix Lechner
  • Christopher Baines
  • Maxim Cournoyer
  • Bruno Victal
Owner
unassigned
Submitted by
Felix Lechner
Severity
normal

Debbugs page

F
F
Felix Lechner wrote on 27 Nov 2023 12:23
(address . submit@debbugs.gnu.org)
87zfyzkkt4.fsf@lease-up.com
X-Debbugs-CC: Bruno Victal <mirai@makinata.eu>

Hi,

The certbot program can accept multiple deploy hooks by repeating the
relevant option on the command line. This commit makes that capability
available to users.

Certificates are often used to secure multiple services. It is helpful
to have separate hooks for each service.

It makes the hooks easier to maintain. It's also easier that way to
re-use hooks for another certificate that may not serve to secure the
same combination of services.

Kind regards
Felix
F
F
Felix Lechner wrote on 27 Nov 2023 13:17
(no subject)
(address . control@debbugs.gnu.org)
87wmu2lwv6.fsf@lease-up.com
reassign 67497 guix-patches
tags 67497 + patch
thanks
F
F
Felix Lechner wrote on 27 Nov 2023 13:20
[PATCH 1/4] In documentation, rename %certbot-deploy-hook back to %nginx-deploy-hook..
(address . 67497@debbugs.gnu.org)
e9fdc8d35f8d57913a3a5861db7a1073d47ce729.1701120054.git.felix.lechner@lease-up.com
Bruno Victal made that change in commit fec8e513, but a nearby patch will
offer the ability to specify a list of hooks. That makes it possible to name
deploy hooks after the services they restart.

Change-Id: I128f71f2e96159eef8821e21ea03ecf0c1c0a7f4
---
doc/guix.texi | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

Toggle diff (28 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 767133cd0f..b0b1c05c73 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -32032,8 +32032,8 @@ Certificate Services
must be a @code{certbot-configuration} record as in this example:
@lisp
-(define %certbot-deploy-hook
- (program-file "certbot-deploy-hook.scm"
+(define %nginx-deploy-hook
+ (program-file "certbot-nginx-deploy-hook.scm"
(with-imported-modules '((gnu services herd))
#~(begin
(use-modules (gnu services herd))
@@ -32046,7 +32046,7 @@ Certificate Services
(list
(certificate-configuration
(domains '("example.net" "www.example.net"))
- (deploy-hook %certbot-deploy-hook))
+ (deploy-hook %nginx-deploy-hook))
(certificate-configuration
(domains '("bar.example.net")))))))
@end lisp

base-commit: 6e4914a037c8b332ab3f1149129c0bd1cea4640b
--
2.41.0
F
F
Felix Lechner wrote on 27 Nov 2023 13:20
[PATCH 2/4] In certbot documentation, call environment variables by their proper name.
(address . 67497@debbugs.gnu.org)
c31f51f5209e6dfe5df01e27698abccd38ddd2c4.1701120054.git.felix.lechner@lease-up.com
Certbot's hooks can be written in any language. in fact, they can be any kind
of executable. Environment variables are widely used to communicate values
across that type of fork(2) boundary. In the context here, it is more accurate
to talk about environment variables.

Change-Id: If0b476c3367a3108d9365d718a74faa7d9fe7530
---
doc/guix.texi | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

Toggle diff (35 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index b0b1c05c73..440a5f3efa 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -32139,24 +32139,24 @@ Certificate Services
@item @code{authentication-hook} (default: @code{#f})
Command to be run in a shell once for each certificate challenge to be
-answered. For this command, the shell variable @code{$CERTBOT_DOMAIN}
+answered. For this command, the environment variable @code{$CERTBOT_DOMAIN}
will contain the domain being authenticated, @code{$CERTBOT_VALIDATION}
contains the validation string and @code{$CERTBOT_TOKEN} contains the
file name of the resource requested when performing an HTTP-01 challenge.
@item @code{cleanup-hook} (default: @code{#f})
Command to be run in a shell once for each certificate challenge that
-have been answered by the @code{auth-hook}. For this command, the shell
+have been answered by the @code{auth-hook}. For this command, the environment
variables available in the @code{auth-hook} script are still available, and
additionally @code{$CERTBOT_AUTH_OUTPUT} will contain the standard output
of the @code{auth-hook} script.
@item @code{deploy-hook} (default: @code{#f})
Command to be run in a shell once for each successfully issued
-certificate. For this command, the shell variable
+certificate. For this command, the environment variable
@code{$RENEWED_LINEAGE} will point to the config live subdirectory (for
example, @samp{"/etc/letsencrypt/live/example.com"}) containing the new
-certificates and keys; the shell variable @code{$RENEWED_DOMAINS} will
+certificates and keys; the environment variable @code{$RENEWED_DOMAINS} will
contain a space-delimited list of renewed certificate domains (for
example, @samp{"example.com www.example.com"}.
--
2.41.0
F
F
Felix Lechner wrote on 27 Nov 2023 13:20
[PATCH 3/4] In certbot service, reduce code duplication.
(address . 67497@debbugs.gnu.org)
ed0f8c6ad1ddb4ae435d5c5cf1c8d9f72a5e41ad.1701120054.git.felix.lechner@lease-up.com
The certbot command is can only be changed with a great deal of attention. The
program branches early and constructs two separate invocations. Changes would
generally have to be made in two places. Otherwise, a new bug might be
introduced.

This commit places the conditional inquestion inside the list so that future
edits are more fool-proof.

Change-Id: I4a54f8b78ff4722688de7772d3c26a6191d6ff89
---
gnu/services/certbot.scm | 58 +++++++++++++++++++---------------------
1 file changed, 27 insertions(+), 31 deletions(-)

Toggle diff (71 lines)
diff --git a/gnu/services/certbot.scm b/gnu/services/certbot.scm
index 0c45471659..8490a69a99 100644
--- a/gnu/services/certbot.scm
+++ b/gnu/services/certbot.scm
@@ -100,37 +100,33 @@ (define certbot-command
csr authentication-hook
cleanup-hook deploy-hook)
(let ((name (or custom-name (car domains))))
- (if challenge
- (append
- (list name certbot "certonly" "-n" "--agree-tos"
- "--manual"
- (string-append "--preferred-challenges=" challenge)
- "--cert-name" name
- "--manual-public-ip-logging-ok"
- "-d" (string-join domains ","))
- (if csr `("--csr" ,csr) '())
- (if email
- `("--email" ,email)
- '("--register-unsafely-without-email"))
- (if server `("--server" ,server) '())
- (if rsa-key-size `("--rsa-key-size" ,rsa-key-size) '())
- (if authentication-hook
- `("--manual-auth-hook" ,authentication-hook)
- '())
- (if cleanup-hook `("--manual-cleanup-hook" ,cleanup-hook) '())
- (if deploy-hook `("--deploy-hook" ,deploy-hook) '()))
- (append
- (list name certbot "certonly" "-n" "--agree-tos"
- "--webroot" "-w" webroot
- "--cert-name" name
- "-d" (string-join domains ","))
- (if csr `("--csr" ,csr) '())
- (if email
- `("--email" ,email)
- '("--register-unsafely-without-email"))
- (if server `("--server" ,server) '())
- (if rsa-key-size `("--rsa-key-size" ,rsa-key-size) '())
- (if deploy-hook `("--deploy-hook" ,deploy-hook) '()))))))
+ (append
+ (list name
+ certbot
+ "certonly"
+ "-n"
+ "--agree-tos")
+ (if challenge
+ (append
+ (list "--manual"
+ (string-append "--preferred-challenges=" challenge)
+ "--manual-public-ip-logging-ok")
+ (if authentication-hook
+ (list "--manual-auth-hook" authentication-hook)
+ '())
+ (if cleanup-hook
+ (list "--manual-cleanup-hook" cleanup-hook)
+ '()))
+ (list "--webroot" "-w" webroot))
+ (list "--cert-name" name
+ "-d" (string-join domains ","))
+ (if csr (list "--csr" csr) '())
+ (if email
+ (list "--email" email)
+ (list "--register-unsafely-without-email"))
+ (if server (list "--server" server) '())
+ (if rsa-key-size (list "--rsa-key-size" rsa-key-size) '())
+ (if deploy-hook (list "--deploy-hook" deploy-hook) '())))))
certificates)))
(program-file
"certbot-command"
--
2.41.0
F
F
Felix Lechner wrote on 27 Nov 2023 13:20
[PATCH 4/4] In certbot's client configuration, offer multiple deploy-hooks.
(address . 67497@debbugs.gnu.org)
729de952f099681b99b1ffd4f3f5bed736cc6b43.1701120054.git.felix.lechner@lease-up.com
The certbot program can accept multiple deploy hooks by repeating the relevant
option on the command line. This commit makes that capability available to
users.

Certificates are often used to secure multiple services. It is helpful to have
separate hooks for each service. It makes those hooks easier to maintain. It's
also easier that way to re-use a hook for another certificate that may not
serve to secure the same combination of services.

Change-Id: I3a293daee47030d9bee7f366605aa63a14e98e38
---
doc/guix.texi | 11 ++++++-----
gnu/services/certbot.scm | 20 +++++++++++++++++---
2 files changed, 23 insertions(+), 8 deletions(-)

Toggle diff (88 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 440a5f3efa..c5cbd0275d 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -32046,7 +32046,7 @@ Certificate Services
(list
(certificate-configuration
(domains '("example.net" "www.example.net"))
- (deploy-hook %nginx-deploy-hook))
+ (deploy-hooks '(%nginx-deploy-hook)))
(certificate-configuration
(domains '("bar.example.net")))))))
@end lisp
@@ -32151,14 +32151,15 @@ Certificate Services
additionally @code{$CERTBOT_AUTH_OUTPUT} will contain the standard output
of the @code{auth-hook} script.
-@item @code{deploy-hook} (default: @code{#f})
-Command to be run in a shell once for each successfully issued
-certificate. For this command, the environment variable
+@item @code{deploy-hooks} (default: @code{'()})
+Commands to be run in a shell once for each successfully issued
+certificate. For these commands, the environment variable
@code{$RENEWED_LINEAGE} will point to the config live subdirectory (for
example, @samp{"/etc/letsencrypt/live/example.com"}) containing the new
certificates and keys; the environment variable @code{$RENEWED_DOMAINS} will
contain a space-delimited list of renewed certificate domains (for
-example, @samp{"example.com www.example.com"}.
+example, @samp{"example.com www.example.com"}. Please note that the singular
+field @code{deploy-hook} was replaced by this field in the plural.
@end table
@end deftp
diff --git a/gnu/services/certbot.scm b/gnu/services/certbot.scm
index 8490a69a99..9d5305174b 100644
--- a/gnu/services/certbot.scm
+++ b/gnu/services/certbot.scm
@@ -30,6 +30,7 @@ (define-module (gnu services certbot)
#:use-module (gnu services web)
#:use-module (gnu system shadow)
#:use-module (gnu packages tls)
+ #:use-module (guix deprecation)
#:use-module (guix i18n)
#:use-module (guix records)
#:use-module (guix gexp)
@@ -62,8 +63,11 @@ (define-record-type* <certificate-configuration>
(default #f))
(cleanup-hook certificate-cleanup-hook
(default #f))
+ ;; TODO: remove singular deploy-hook; is deprecated
(deploy-hook certificate-configuration-deploy-hook
- (default #f)))
+ (default #f))
+ (deploy-hooks certificate-configuration-deploy-hooks
+ (default '())))
(define-record-type* <certbot-configuration>
certbot-configuration make-certbot-configuration
@@ -98,7 +102,8 @@ (define certbot-command
(match-lambda
(($ <certificate-configuration> custom-name domains challenge
csr authentication-hook
- cleanup-hook deploy-hook)
+ cleanup-hook
+ deploy-hook deploy-hooks)
(let ((name (or custom-name (car domains))))
(append
(list name
@@ -126,7 +131,16 @@ (define certbot-command
(list "--register-unsafely-without-email"))
(if server (list "--server" server) '())
(if rsa-key-size (list "--rsa-key-size" rsa-key-size) '())
- (if deploy-hook (list "--deploy-hook" deploy-hook) '())))))
+
+ (if deploy-hook
+ (begin
+ (warn-about-deprecation 'deploy-hook #f
+ #:replacement 'deploy-hooks)
+ (list "--deploy-hook" deploy-hook))
+ '())
+ (append-map (lambda (hook)
+ (list "--deploy-hook" hook))
+ deploy-hooks)))))
certificates)))
(program-file
"certbot-command"
--
2.41.0
A
A
Arun Isaac wrote on 27 Nov 2023 16:24
Re: bug#67497: [PATCH] Multiple deploy hooks in certbot service
(name . bruno victal)(address . mirai@makinata.eu)
874jh6bu8c.fsf@systemreboot.net
Hi Felix,

Toggle quote (3 lines)
> Certificates are often used to secure multiple services. It is helpful
> to have separate hooks for each service.

It's already possible to write the deploy-hook as a G-expression
constructed script (using program-file) that invokes multiple hooks in
succession. Something like:

(program-file "deploy-hook"
(with-imported-modules '((guix build utils))
#~(begin
(use-modules (guix build utils))

(invoke "/some/hook")
(invoke "/some/other/hook"))))

Here /some/hook and /some/other/hook can themselves be recursively
constructed using program-file. So, do we really need a service that
explicitly accepts multiple deploy hooks?

Regards,
Arun
B
B
Bruno Victal wrote on 16 Dec 2023 12:50
(address . 67497@debbugs.gnu.org)
a224335a-b8f0-46cd-ba90-8bc51d698376@makinata.eu
Hi Felix and Arun,

On 2023-11-28 00:24, Arun Isaac wrote:
Toggle quote (12 lines)
> It's already possible to write the deploy-hook as a G-expression
> constructed script (using program-file) that invokes multiple hooks in
> succession. Something like:
>
> (program-file "deploy-hook"
> (with-imported-modules '((guix build utils))
> #~(begin
> (use-modules (guix build utils))
>
> (invoke "/some/hook")
> (invoke "/some/other/hook"))))

Indeed, and for the record mine looks like this:

Toggle snippet (11 lines)
(program-file "certbot-hook.scm"
;; source-module-closure not used here because at the time of writing
;; (gnu services herd) only uses Guile modules.
(with-imported-modules '((gnu services herd))
#~(begin
(use-modules (gnu services herd))
(with-shepherd-action 'nginx ('reload) result result)
(restart-service 'dovecot)
(restart-service 'smtpd))))

(that is, a single hook is responsible for various other shepherd
services)

Toggle quote (4 lines)
> Here /some/hook and /some/other/hook can themselves be recursively
> constructed using program-file. So, do we really need a service that
> explicitly accepts multiple deploy hooks?

As Arun pointed out, I don't think multiple deploy hooks would be
adding value here.

What would be interesting though is adding service-extensions support
for certbot-service-type. Roughly speaking, two plausible ways to
achieve this would be:

* Single deploy-hook and ungexp-splicing, i.e.:

Toggle snippet (6 lines)
;; service-extension-hooks: list of program-files
#$@(map (lambda (extension-hook)
#~(invoke #$extension-hook))
service-extension-hooks)

* Multiple --deploy-hook … behind the scenes (the deploy-hook
field in <certificate-configuration> still accepts only a single hook)

Important note, such service-extensions must account for the fact that
they are actually extensions to <certificate-configuration> objects,
i.e. they have to account for which domain(s) is the (deploy/
cleanup/authentication)-hook for.

--
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.
Attachment: OpenPGP_signature.asc
B
B
Bruno Victal wrote on 16 Dec 2023 12:58
Re: [PATCH 2/4] In certbot documentation, call environment variables by their proper name.
(name . Felix Lechner)(address . felix.lechner@lease-up.com)(address . 67497@debbugs.gnu.org)
0b64f8bb-755d-4c09-af51-871392de8262@makinata.eu
On 2023-11-27 21:20, Felix Lechner wrote:
Toggle quote (21 lines)
> Certbot's hooks can be written in any language. in fact, they can be any kind
> of executable. Environment variables are widely used to communicate values
> across that type of fork(2) boundary. In the context here, it is more accurate
> to talk about environment variables.
>
> Change-Id: If0b476c3367a3108d9365d718a74faa7d9fe7530
> ---
> doc/guix.texi | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index b0b1c05c73..440a5f3efa 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -32139,24 +32139,24 @@ Certificate Services
>
> @item @code{authentication-hook} (default: @code{#f})
> Command to be run in a shell once for each certificate challenge to be
> -answered. For this command, the shell variable @code{$CERTBOT_DOMAIN}
> +answered. For this command, the environment variable @code{$CERTBOT_DOMAIN}

[…]

Toggle quote (2 lines)
> will contain the domain being authenticated, @code{$CERTBOT_VALIDATION}

[…]

Toggle quote (2 lines)
> contains the validation string and @code{$CERTBOT_TOKEN} contains the

[…]

Toggle quote (3 lines)
> variables available in the @code{auth-hook} script are still available, and
> additionally @code{$CERTBOT_AUTH_OUTPUT} will contain the standard output

[…]

Toggle quote (7 lines)
> @code{$RENEWED_LINEAGE} will point to the config live subdirectory (for
> example, @samp{"/etc/letsencrypt/live/example.com"}) containing the new
> -certificates and keys; the shell variable @code{$RENEWED_DOMAINS} will
> +certificates and keys; the environment variable @code{$RENEWED_DOMAINS} will
> contain a space-delimited list of renewed certificate domains (for
> example, @samp{"example.com www.example.com"}.

The correct Texinfo @-command should be @env{CERTBOT_DOMAIN}, ….

Could you amend and send a v2 that addresses these issues as well?
Other than that, it LGTM.

--
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.
Attachment: OpenPGP_signature.asc
F
F
Felix Lechner wrote on 17 Dec 2023 09:46
Re: bug#67497: [PATCH] Multiple deploy hooks in certbot service
(address . 67497@debbugs.gnu.org)
875y0wrabr.fsf@lease-up.com
Hi,

Thank you both for reviewing this patch! I have to respond to several
reviews and will start with this one, because it weighed the heaviest on
me.

On Sat, Dec 16 2023, Bruno Victal wrote:

Toggle quote (3 lines)
> As Arun pointed out, I don't think multiple deploy hooks would be
> adding value here.

Your blanket opposition to this patch is incomprehensible to me from
several angles:

1. A meaningful name for a hook near the certificate declaration is more
administrator-friendly. Someone who manages several certificates, like
my twenty-one certificates [1], can see right away which services are
being restarted.

2. Arun's solution requires an extra procedure and makes the
configuration file longer without without conveying extra meaning.

3. Anyone parsing the code has to look up the definition of the hook in
order to see what it does---and probably also the definition for
'invoke', which is not standard Guile, in the Guix manual. In my view,
your code is not easy to read.

4. The bundling into one script brings no economy, because different
services generally share no code for their reloading. That was already
recognized by Certbot's upstream when the feature for multiple hooks was
added. After all, the concerns can also be combined, as you prefer, in
Certbot's own hooks, but that was apparently unpopular.

5. As a more serious downside, in your cases changing the combined hook
might inadventently reload a certificate for a service does not use
it. A grep is required to check where the cmombined hook is being
used. An extra step is required, and the propensity for errors rises.

6. In your preferred setups, the most elegant way to provide different
hooks is probably '%certbot-hook-1' and 'certbot-hook-2'. Those scripts
will then share code---likely to restart a HTTP server---for no good
reason!

7. User-friendliness is regarded as a worthwhile goal at another, more
popular Linux distribution. [2]

8. Most significantly, your use case isn't affected by this patch! The
use of combined hooks, which you prefer, is still possible should this
patch be accepted.

In summary, I do not understand what motivated you to object to this
patch, but I recognize that the opinions of reasonable people can
differ.

As a side note, I have contributed upstream, but not to the feature we
are discussing here. [3]

Toggle quote (11 lines)
> What would be interesting though is adding service-extensions support
> for certbot-service-type. Roughly speaking, two plausible ways to
> achieve this would be:
>
> * Single deploy-hook and ungexp-splicing, i.e.:
>
> [...]
>
> * Multiple --deploy-hook … behind the scenes (the deploy-hook
> field in <certificate-configuration> still accepts only a single hook)

While I very much respect Bruno's opinion and guidance on Guix services
(and genuinely appreciated this review) I do not understand what those
sentences mean.

I guess it's shame on me.

I can, however, say that I likewise fail to see an advantage in more
complexity when my patch does nearly the same thing in three lines.

Thank you!

Kind regards
Felix

A
A
Arun Isaac wrote on 18 Dec 2023 22:29
(address . 67497@debbugs.gnu.org)
8734vyu2l8.fsf@systemreboot.net
Hi Felix,

You make good points. Having multiple deploy hooks is probably in the
spirit of the certbot project and makes for more declarative
configuration. However, I still feel that combining multiple deploy
hooks into one is better /composition/, more schemy and less complexity
for the Guix certbot service. But, if others feel that multiple deploy
hooks make sense, I am very happy to accept that.

Toggle quote (3 lines)
> Your blanket opposition to this patch is incomprehensible to me from
> several angles:

And, I am not in blanket opposition to this patch. :-) I was just
contributing my two cents to the discussion. I suggested the alternative
of combining hooks just in case you had not already thought of it. I am
not invested in the certbot service. I don't even use it myself.

Regards,
Arun
C
C
Carlo Zancanaro wrote on 17 Feb 16:00 -0800
Re: [bug#67497] [PATCH] Multiple deploy hooks in certbot service
(name . Bruno Victal)(address . mirai@makinata.eu)
87v7t8w28t.fsf@zancanaro.id.au
Given this thread came up on guix-devel, and I use certbot and have
opinions, I thought I might chime in on this.

On Sat, Dec 16 2023, Bruno Victal wrote:
Toggle quote (3 lines)
> As Arun pointed out, I don't think multiple deploy hooks would be
> adding value here.

I disagree. I think adding multiple deploy hooks would be adding value,
even beyond just reducing syntactic overhead (which is valuable itself).
Having a list of hooks has two benefits that I can see:

1. A list of hooks can be introspected by Scheme code, which lets us
write code to manipulate certbot-configuration objects. In particular,
it would make it easier to write code to add hooks without accidentally
duplicating existing hooks, and it would make it possible to write code
to remove hooks. I could easily see this being used in an enhancement to
the certbot-service-type extension mechanism where multiple services
extending certbot-service-type could be combined into a single
certificate (where the domains match, and whatnot).

2. A list of hooks makes it easier to read your configuration, and makes
it obvious that we're intending to support small, specific, deploy
hooks. This is a social argument, rather than a technical one. Even with
no change in expressive power, our interfaces do communicate how we
intend for them to be used. Having a list communicates "we expect each
hook to do one thing", which feels good to me.

At the very least, even if we don't go with list of deploy-hooks, we
could get some improvement by having an official gexp helper
"invoke-all" which calls invoke on each of its gexp arguments in
turn. This would be more generally useful for gexp composition, but then
it would also be less obvious than a deploy-hooks list (unless you're
already very familiar with gexps).

Toggle quote (3 lines)
> What would be interesting though is adding service-extensions support
> for certbot-service-type.

I'm not entirely sure what you mean by this. certbot-service-type
already supports extensions?

On Sat, Dec 16 2023, Bruno Victal wrote:
Toggle quote (14 lines)
> [...] for the record mine looks like this:
>
> --8<---------------cut here---------------start------------->8---
> (program-file "certbot-hook.scm"
> ;; source-module-closure not used here because at the time of writing
> ;; (gnu services herd) only uses Guile modules.
> (with-imported-modules '((gnu services herd))
> #~(begin
> (use-modules (gnu services herd))
> (with-shepherd-action 'nginx ('reload) result result)
> (restart-service 'dovecot)
> (restart-service 'smtpd))))
> --8<---------------cut here---------------end--------------->8---

As a bit of fun: do you know the difference between this hook, and the
equivalent using multiple --deploy-hook arguments? Error handling and
logging. With multiple deploy-hooks, I believe Cerbot will always run
all of them, and will report errors for each of them individually. An
error restarting dovecot shouldn't prevent smtpd from restarting, but in
your gexp it would (although I'm not actually sure how herd reports
errors here - my point is more general than this specific example).

The more we ask people to plumb things together themselves, the more we
ask them to decide themselves about error handling and logging. Which
means that error handling and logging will be an afterthought at best.
If you want to put things in one gexp you still can, but making it the
only option leaves less room for Certbot to "add value" in handling
these things for us.

Carlo
A
A
Arun Isaac wrote on 1 Mar 13:57 -0800
Re: [PATCH] Multiple deploy hooks in certbot service
(address . 67497@debbugs.gnu.org)
87y0xoxvl0.fsf@systemreboot.net
Hi all,

This patchset LGTM. But, I would recommend that someone who actually
uses the certbot service reviews it. I do not use it myself, and would
not be able to test the service after the changes. I am therefore
tagging this issue with the help tag.

Thanks,
Arun
A
A
Arun Isaac wrote on 1 Mar 13:59 -0800
Manipulate bug 67497
(address . control@debbugs.gnu.org)
87v7ssxvi3.fsf@systemreboot.net
tags 67497 + help
thanks
M
M
Maxim Cournoyer wrote on 22 Apr 18:45 -0700
Re: [bug#67497] [PATCH] Multiple deploy hooks in certbot service
(name . Felix Lechner)(address . felix.lechner@lease-up.com)
87bjsn1wh9.fsf@gmail.com
Hi,

Arun Isaac <arunisaac@systemreboot.net> writes:

Toggle quote (7 lines)
> Hi all,
>
> This patchset LGTM. But, I would recommend that someone who actually
> uses the certbot service reviews it. I do not use it myself, and would
> not be able to test the service after the changes. I am therefore
> tagging this issue with the help tag.

Felix, could you please rebase this on master, after which I can apply
it?

--
Thanks,
Maxim
F
F
Felix Lechner wrote on 30 Apr 08:34 -0700
[PATCH v2 1/4] In documentation, rename %certbot-deploy-hook back to %nginx-deploy-hook..
(address . 67497@debbugs.gnu.org)
e2119afb4420e040d4e9f2d659a0df4ee9ca0c9c.1746026936.git.felix.lechner@lease-up.com
Bruno Victal made that change in commit fec8e513, but a nearby patch will
offer the ability to specify a list of hooks. That makes it possible to name
deploy hooks after the services they restart.

Change-Id: I128f71f2e96159eef8821e21ea03ecf0c1c0a7f4
---
doc/guix.texi | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

Toggle diff (31 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 90d90b2e1eb..b48255a16e0 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -35364,13 +35364,21 @@ Certificate Services
must be a @code{certbot-configuration} record as in this example:
@lisp
+(define %nginx-deploy-hook
+ (program-file "certbot-nginx-deploy-hook.scm"
+ (with-imported-modules '((gnu services herd))
+ #~(begin
+ (use-modules (gnu services herd))
+ (with-shepherd-action 'nginx ('reload) result result)))))
+
(service certbot-service-type
(certbot-configuration
(email "foo@@example.net")
(certificates
(list
(certificate-configuration
- (domains '("example.net" "www.example.net")))
+ (domains '("example.net" "www.example.net"))
+ (deploy-hook %nginx-deploy-hook))
(certificate-configuration
(domains '("bar.example.net")))))))
@end lisp

base-commit: bb8cc412c8fcab613c402e06ae7024d6df5c9010
--
2.49.0
F
F
Felix Lechner wrote on 30 Apr 08:34 -0700
[PATCH v2 2/4] In certbot documentation, call environment variables by their proper name.
(address . 67497@debbugs.gnu.org)
fea3a7fee3d2107ac69035a37b34c594e1250b97.1746026936.git.felix.lechner@lease-up.com
Certbot's hooks can be written in any language. in fact, they can be any kind
of executable. Environment variables are widely used to communicate values
across that type of fork(2) boundary. In the context here, it is more accurate
to talk about environment variables.

Change-Id: If0b476c3367a3108d9365d718a74faa7d9fe7530
---
doc/guix.texi | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

Toggle diff (35 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index b48255a16e0..1b0fa4f2a3a 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -35471,24 +35471,24 @@ Certificate Services
@item @code{authentication-hook} (default: @code{#f})
Command to be run in a shell once for each certificate challenge to be
-answered. For this command, the shell variable @code{$CERTBOT_DOMAIN}
+answered. For this command, the environment variable @code{$CERTBOT_DOMAIN}
will contain the domain being authenticated, @code{$CERTBOT_VALIDATION}
contains the validation string and @code{$CERTBOT_TOKEN} contains the
file name of the resource requested when performing an HTTP-01 challenge.
@item @code{cleanup-hook} (default: @code{#f})
Command to be run in a shell once for each certificate challenge that
-have been answered by the @code{auth-hook}. For this command, the shell
+have been answered by the @code{auth-hook}. For this command, the environment
variables available in the @code{auth-hook} script are still available, and
additionally @code{$CERTBOT_AUTH_OUTPUT} will contain the standard output
of the @code{auth-hook} script.
@item @code{deploy-hook} (default: @code{#f})
Command to be run in a shell once for each successfully issued
-certificate. For this command, the shell variable
+certificate. For this command, the environment variable
@code{$RENEWED_LINEAGE} will point to the config live subdirectory (for
example, @samp{"/etc/letsencrypt/live/example.com"}) containing the new
-certificates and keys; the shell variable @code{$RENEWED_DOMAINS} will
+certificates and keys; the environment variable @code{$RENEWED_DOMAINS} will
contain a space-delimited list of renewed certificate domains (for
example, @samp{"example.com www.example.com"}.
--
2.49.0
F
F
Felix Lechner wrote on 30 Apr 08:34 -0700
[PATCH v2 4/4] In certbot's client configuration, offer multiple deploy-hooks.
(address . 67497@debbugs.gnu.org)
cf51d7a8ac2a81602868c2f7e3c1fc1c143ffcc0.1746026936.git.felix.lechner@lease-up.com
The certbot program can accept multiple deploy hooks by repeating the relevant
option on the command line. This commit makes that capability available to
users.

Certificates are often used to secure multiple services. It is helpful to have
separate hooks for each service. It makes those hooks easier to maintain. It's
also easier that way to re-use a hook for another certificate that may not
serve to secure the same combination of services.

Change-Id: I3a293daee47030d9bee7f366605aa63a14e98e38
---
doc/guix.texi | 11 ++++++-----
gnu/services/certbot.scm | 18 ++++++++++++++++--
2 files changed, 22 insertions(+), 7 deletions(-)

Toggle diff (87 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 1b0fa4f2a3a..deb1f76d353 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -35378,7 +35378,7 @@ Certificate Services
(list
(certificate-configuration
(domains '("example.net" "www.example.net"))
- (deploy-hook %nginx-deploy-hook))
+ (deploy-hooks '(%nginx-deploy-hook)))
(certificate-configuration
(domains '("bar.example.net")))))))
@end lisp
@@ -35483,14 +35483,15 @@ Certificate Services
additionally @code{$CERTBOT_AUTH_OUTPUT} will contain the standard output
of the @code{auth-hook} script.
-@item @code{deploy-hook} (default: @code{#f})
-Command to be run in a shell once for each successfully issued
-certificate. For this command, the environment variable
+@item @code{deploy-hooks} (default: @code{'()})
+Commands to be run in a shell once for each successfully issued
+certificate. For these commands, the environment variable
@code{$RENEWED_LINEAGE} will point to the config live subdirectory (for
example, @samp{"/etc/letsencrypt/live/example.com"}) containing the new
certificates and keys; the environment variable @code{$RENEWED_DOMAINS} will
contain a space-delimited list of renewed certificate domains (for
-example, @samp{"example.com www.example.com"}.
+example, @samp{"example.com www.example.com"}. Please note that the singular
+field @code{deploy-hook} was replaced by this field in the plural.
@item @code{start-self-signed?} (default: @code{#t})
Whether to generate an initial self-signed certificate during system
diff --git a/gnu/services/certbot.scm b/gnu/services/certbot.scm
index 08a480ed3b1..7a67b9bd7cb 100644
--- a/gnu/services/certbot.scm
+++ b/gnu/services/certbot.scm
@@ -30,6 +30,7 @@ (define-module (gnu services certbot)
#:use-module (gnu services web)
#:use-module (gnu system shadow)
#:use-module (gnu packages tls)
+ #:use-module (guix deprecation)
#:use-module (guix i18n)
#:use-module (guix records)
#:use-module (guix gexp)
@@ -63,8 +64,11 @@ (define-record-type* <certificate-configuration>
(default #f))
(cleanup-hook certificate-cleanup-hook
(default #f))
+ ;; TODO: remove singular deploy-hook; is deprecated
(deploy-hook certificate-configuration-deploy-hook
(default #f))
+ (deploy-hooks certificate-configuration-deploy-hooks
+ (default '()))
(start-self-signed? certificate-configuration-start-self-signed?
(default #t)))
@@ -140,7 +144,8 @@ (define certbot-command
(match-lambda
(($ <certificate-configuration> custom-name domains challenge
csr authentication-hook
- cleanup-hook deploy-hook)
+ cleanup-hook
+ deploy-hook deploy-hooks)
(let ((name (or custom-name (car domains))))
(append
(list name
@@ -168,7 +173,16 @@ (define certbot-command
(list "--register-unsafely-without-email"))
(if server (list "--server" server) '())
(if rsa-key-size (list "--rsa-key-size" rsa-key-size) '())
- (if deploy-hook (list "--deploy-hook" deploy-hook) '())))))
+
+ (if deploy-hook
+ (begin
+ (warn-about-deprecation 'deploy-hook #f
+ #:replacement 'deploy-hooks)
+ (list "--deploy-hook" deploy-hook))
+ '())
+ (append-map (lambda (hook)
+ (list "--deploy-hook" hook))
+ deploy-hooks)))))
certificates)))
(program-file
"certbot-command"
--
2.49.0
F
F
Felix Lechner wrote on 30 Apr 08:34 -0700
[PATCH v2 3/4] In certbot service, reduce code duplication.
(address . 67497@debbugs.gnu.org)
e1241f73bba23e4015c84cc826d11f92d723ac6c.1746026936.git.felix.lechner@lease-up.com
The certbot command is can only be changed with a great deal of attention. The
program branches early and constructs two separate invocations. Changes would
generally have to be made in two places. Otherwise, a new bug might be
introduced.

This commit places the conditional inquestion inside the list so that future
edits are more fool-proof.

Change-Id: I4a54f8b78ff4722688de7772d3c26a6191d6ff89
---
gnu/services/certbot.scm | 60 ++++++++++++++++++----------------------
1 file changed, 27 insertions(+), 33 deletions(-)

Toggle diff (73 lines)
diff --git a/gnu/services/certbot.scm b/gnu/services/certbot.scm
index d6c7d175ff5..08a480ed3b1 100644
--- a/gnu/services/certbot.scm
+++ b/gnu/services/certbot.scm
@@ -142,39 +142,33 @@ (define certbot-command
csr authentication-hook
cleanup-hook deploy-hook)
(let ((name (or custom-name (car domains))))
- (if challenge
- (append
- (list name certbot "certonly" "-n" "--agree-tos"
- "--manual"
- (string-append "--preferred-challenges=" challenge)
- "--cert-name" name
- "--manual-public-ip-logging-ok"
- "-d" (string-join domains ","))
- (if csr `("--csr" ,csr) '())
- (if email
- `("--email" ,email)
- '("--register-unsafely-without-email"))
- (if server `("--server" ,server) '())
- (if rsa-key-size `("--rsa-key-size" ,rsa-key-size) '())
- (if authentication-hook
- `("--manual-auth-hook" ,authentication-hook)
- '())
- (if cleanup-hook `("--manual-cleanup-hook" ,cleanup-hook) '())
- (list "--deploy-hook"
- (certbot-deploy-hook name deploy-hook)))
- (append
- (list name certbot "certonly" "-n" "--agree-tos"
- "--webroot" "-w" webroot
- "--cert-name" name
- "-d" (string-join domains ","))
- (if csr `("--csr" ,csr) '())
- (if email
- `("--email" ,email)
- '("--register-unsafely-without-email"))
- (if server `("--server" ,server) '())
- (if rsa-key-size `("--rsa-key-size" ,rsa-key-size) '())
- (list "--deploy-hook"
- (certbot-deploy-hook name deploy-hook)))))))
+ (append
+ (list name
+ certbot
+ "certonly"
+ "-n"
+ "--agree-tos")
+ (if challenge
+ (append
+ (list "--manual"
+ (string-append "--preferred-challenges=" challenge)
+ "--manual-public-ip-logging-ok")
+ (if authentication-hook
+ (list "--manual-auth-hook" authentication-hook)
+ '())
+ (if cleanup-hook
+ (list "--manual-cleanup-hook" cleanup-hook)
+ '()))
+ (list "--webroot" "-w" webroot))
+ (list "--cert-name" name
+ "-d" (string-join domains ","))
+ (if csr (list "--csr" csr) '())
+ (if email
+ (list "--email" email)
+ (list "--register-unsafely-without-email"))
+ (if server (list "--server" server) '())
+ (if rsa-key-size (list "--rsa-key-size" rsa-key-size) '())
+ (if deploy-hook (list "--deploy-hook" deploy-hook) '())))))
certificates)))
(program-file
"certbot-command"
--
2.49.0
M
M
Maxim Cournoyer wrote on 1 May 07:47 -0700
Re: [PATCH v2 4/4] In certbot's client configuration, offer multiple deploy-hooks.
(name . Felix Lechner)(address . felix.lechner@lease-up.com)
87ldrg75h6.fsf@gmail.com
Hi,

Felix Lechner <felix.lechner@lease-up.com> writes:

Toggle quote (9 lines)
> The certbot program can accept multiple deploy hooks by repeating the relevant
> option on the command line. This commit makes that capability available to
> users.
>
> Certificates are often used to secure multiple services. It is helpful to have
> separate hooks for each service. It makes those hooks easier to maintain. It's
> also easier that way to re-use a hook for another certificate that may not
> serve to secure the same combination of services.

For this commit and the previous one, you can keep your nice explanatory
text, but a GNU ChangeLog must be added below, per our conventions. I
can be terse and to the point, touching only the *changes*, especially
since you already have an explanatory text.

Toggle quote (37 lines)
> Change-Id: I3a293daee47030d9bee7f366605aa63a14e98e38
> ---
> doc/guix.texi | 11 ++++++-----
> gnu/services/certbot.scm | 18 ++++++++++++++++--
> 2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 1b0fa4f2a3a..deb1f76d353 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -35378,7 +35378,7 @@ Certificate Services
> (list
> (certificate-configuration
> (domains '("example.net" "www.example.net"))
> - (deploy-hook %nginx-deploy-hook))
> + (deploy-hooks '(%nginx-deploy-hook)))
> (certificate-configuration
> (domains '("bar.example.net")))))))
> @end lisp
> @@ -35483,14 +35483,15 @@ Certificate Services
> additionally @code{$CERTBOT_AUTH_OUTPUT} will contain the standard output
> of the @code{auth-hook} script.
>
> -@item @code{deploy-hook} (default: @code{#f})
> -Command to be run in a shell once for each successfully issued
> -certificate. For this command, the environment variable
> +@item @code{deploy-hooks} (default: @code{'()})
> +Commands to be run in a shell once for each successfully issued
> +certificate. For these commands, the environment variable
> @code{$RENEWED_LINEAGE} will point to the config live subdirectory (for
> example, @samp{"/etc/letsencrypt/live/example.com"}) containing the new
> certificates and keys; the environment variable @code{$RENEWED_DOMAINS} will
> contain a space-delimited list of renewed certificate domains (for
> -example, @samp{"example.com www.example.com"}.
> +example, @samp{"example.com www.example.com"}. Please note that the singular
> +field @code{deploy-hook} was replaced by this field in the plural.

Need two space before the new sentence starts.

Toggle quote (21 lines)
>
> @item @code{start-self-signed?} (default: @code{#t})
> Whether to generate an initial self-signed certificate during system
> diff --git a/gnu/services/certbot.scm b/gnu/services/certbot.scm
> index 08a480ed3b1..7a67b9bd7cb 100644
> --- a/gnu/services/certbot.scm
> +++ b/gnu/services/certbot.scm
> @@ -30,6 +30,7 @@ (define-module (gnu services certbot)
> #:use-module (gnu services web)
> #:use-module (gnu system shadow)
> #:use-module (gnu packages tls)
> + #:use-module (guix deprecation)
> #:use-module (guix i18n)
> #:use-module (guix records)
> #:use-module (guix gexp)
> @@ -63,8 +64,11 @@ (define-record-type* <certificate-configuration>
> (default #f))
> (cleanup-hook certificate-cleanup-hook
> (default #f))
> + ;; TODO: remove singular deploy-hook; is deprecated

For standalone comments, please use complete sentences, as in:

;; TODO: Remove singular deploy-hook, which is deprecated.

Note that it's not enough to simply document it as deprecated, you must
introduce a deprecation warning when people are using it for the
'deprecation' to count as such. Since this record is using a plain Guix
record, this is usually done using a sanitizer with a maybe value that
warns when the value is set.

Toggle quote (28 lines)
> (deploy-hook certificate-configuration-deploy-hook
> (default #f))
> + (deploy-hooks certificate-configuration-deploy-hooks
> + (default '()))
> (start-self-signed? certificate-configuration-start-self-signed?
> (default #t)))
>
> @@ -140,7 +144,8 @@ (define certbot-command
> (match-lambda
> (($ <certificate-configuration> custom-name domains challenge
> csr authentication-hook
> - cleanup-hook deploy-hook)
> + cleanup-hook
> + deploy-hook deploy-hooks)
> (let ((name (or custom-name (car domains))))
> (append
> (list name
> @@ -168,7 +173,16 @@ (define certbot-command
> (list "--register-unsafely-without-email"))
> (if server (list "--server" server) '())
> (if rsa-key-size (list "--rsa-key-size" rsa-key-size) '())
> - (if deploy-hook (list "--deploy-hook" deploy-hook) '())))))
> +
> + (if deploy-hook
> + (begin
> + (warn-about-deprecation 'deploy-hook #f
> + #:replacement 'deploy-hooks)

Ah, I see you warned here, but that's going to happen at the time the
service is executed, right? Which is not as good: we'd like the
deprecation to be printed as early as possible, typically when the user
reconfigures their system. A sanitizer on the record field would
achieve that.

--
Thanks,
Maxim
C
C
Christopher Baines wrote on 26 May 08:02 -0700
tag 67497 moreinfo
(address . control@debbugs.gnu.org)
87frgr4dq0.fsf@cbaines.net
tags 67497 + moreinfo
quit
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 67497@patchwise.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 67497
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch