GNU bug report logs - #62642
[PATCH] services: certbot: Fix nginx crash when certbot is used without domains

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 62642 in the body.
You can then email your comments to 62642 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: guix-patches <at> gnu.org
Cc: Saku Laesvuori <saku <at> laesvuori.fi>
Subject: [PATCH] services: certbot: Fix nginx crash when certbot is used
 without domains
Date: Mon,  3 Apr 2023 16:32:41 +0300

* gnu/services/certbot.scm (certbot-nginx-server-configurations):
Don't return a broken nginx-server-configuration when no certificate
domains are configured.
---
 gnu/services/certbot.scm | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/gnu/services/certbot.scm b/gnu/services/certbot.scm
index 8e6784df2b..3d9d207f8a 100644
--- a/gnu/services/certbot.scm
+++ b/gnu/services/certbot.scm
@@ -173,20 +173,21 @@ (define certbot-nginx-server-configurations
   (match-lambda
     (($ <certbot-configuration> package webroot certificates email
                                 server rsa-key-size default-location)
-     (list
-      (nginx-server-configuration
-       (listen '("80" "[::]:80"))
-       (ssl-certificate #f)
-       (ssl-certificate-key #f)
-       (server-name
-        (apply append (map certificate-configuration-domains certificates)))
-       (locations
-        (filter identity
-                (list
-                 (nginx-location-configuration
-                  (uri "/.well-known")
-                  (body (list (list "root " webroot ";"))))
-                 default-location))))))))
+     (if (null? certificates) '()
+       (list
+        (nginx-server-configuration
+        (listen '("80" "[::]:80"))
+        (ssl-certificate #f)
+        (ssl-certificate-key #f)
+        (server-name
+         (apply append (map certificate-configuration-domains certificates)))
+        (locations
+         (filter identity
+                 (list
+                  (nginx-location-configuration
+                   (uri "/.well-known")
+                   (body (list (list "root " webroot ";"))))
+                  default-location)))))))))
 
 (define certbot-service-type
   (service-type (name 'certbot)

base-commit: 2cf71e725d55bc5bf1ad663b7c696516299cc8a7
-- 
2.39.2

Message #8 received at 62642 <at> debbugs.gnu.org (full text, mbox):

From: Bruno Victal <mirai <at> makinata.eu>
To: Saku Laesvuori <saku <at> laesvuori.fi>
Cc: 62642 <at> debbugs.gnu.org
Subject: Re: [bug#62642] [PATCH] services: certbot: Fix nginx crash when
 certbot is used without domains
Date: Mon, 3 Apr 2023 15:28:00 +0100

Hi Saku,

On 2023-04-03 14:32, Saku Laesvuori via Guix-patches via wrote:
> * gnu/services/certbot.scm (certbot-nginx-server-configurations):
> Don't return a broken nginx-server-configuration when no certificate
> domains are configured.

Is there a use-case for certbot without any certificate configurations provided?
IMO it looks to me that the 'certificates' field shouldn't have a default value
configured instead?


Cheers,
Bruno

Message #11 received at 62642 <at> debbugs.gnu.org (full text, mbox):

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: Bruno Victal <mirai <at> makinata.eu>
Cc: 62642 <at> debbugs.gnu.org
Subject: Re: [bug#62642] [PATCH] services: certbot: Fix nginx crash when
 certbot is used without domains
Date: Mon, 3 Apr 2023 21:06:59 +0300

[Message part 1 (text/plain, inline)]

Hi,

> Is there a use-case for certbot without any certificate configurations provided?

I was writing a service that extends certbot if a configuration option
for it is set to #t. To me it seems that it is currently impossible to
view the configuration in the service type definition, so I worked
around it by extending certbot-service-type with an empty list if the
option is set to #f.

> IMO it looks to me that the 'certificates' field shouldn't have a default value
> configured instead?

Wouldn't that mean that users who use certbot only via services that
extend it would have to configure 'certificates' to () manually and have
their nginx configuration crash if they remove the extending services
and forget to remove the certbot service?

- Saku

[signature.asc (application/pgp-signature, inline)]

Message #14 received at 62642 <at> debbugs.gnu.org (full text, mbox):

From: Bruno Victal <mirai <at> makinata.eu>
To: Saku Laesvuori <saku <at> laesvuori.fi>
Cc: 62642 <at> debbugs.gnu.org
Subject: Re: [bug#62642] [PATCH] services: certbot: Fix nginx crash when
 certbot is used without domains
Date: Tue, 4 Apr 2023 14:21:27 +0100

On 2023-04-03 19:06, Saku Laesvuori wrote:
> Hi,
> 
>> Is there a use-case for certbot without any certificate configurations provided?
> 
> I was writing a service that extends certbot if a configuration option
> for it is set to #t. To me it seems that it is currently impossible to
> view the configuration in the service type definition, so I worked
> around it by extending certbot-service-type with an empty list if the
> option is set to #f.

Right, that's a valid use case.

> 
>> IMO it looks to me that the 'certificates' field shouldn't have a default value
>> configured instead?
> 
> Wouldn't that mean that users who use certbot only via services that
> extend it would have to configure 'certificates' to () manually and have
> their nginx configuration crash if they remove the extending services
> and forget to remove the certbot service?

You're correct, having the default value set is not a problem here.

IMO, certbot should be extending the nginx service only when the 'challenge' field
is #f (ideally this should be made into a “enumerated” type, where the values range from
'http-01, 'dns-01, 'custom (as an escape hatch), ...)

Perhaps you could partition 'certificates' by whether 'challenge' is #f or not and use the
results to craft the nginx extension value instead?


Cheers,
Bruno

Message #17 received at 62642 <at> debbugs.gnu.org (full text, mbox):

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: Bruno Victal <mirai <at> makinata.eu>
Cc: 62642 <at> debbugs.gnu.org
Subject: [PATCH v2] services: certbot: Fix nginx crash when certbot is used
 without domains
Date: Tue, 4 Apr 2023 23:43:46 +0300

[Message part 1 (text/plain, inline)]

* gnu/services/certbot.scm (certbot-nginx-server-configurations):
Don't return a broken nginx-server-configuration with empty server_name
when no certificate domains are configured. Instead add a separate
server for every certificate, so 0 certificates adds 0 servers.
---
 gnu/services/certbot.scm | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/gnu/services/certbot.scm b/gnu/services/certbot.scm
index 8e6784df2b..0c45471659 100644
--- a/gnu/services/certbot.scm
+++ b/gnu/services/certbot.scm
@@ -173,20 +173,24 @@ (define certbot-nginx-server-configurations
   (match-lambda
     (($ <certbot-configuration> package webroot certificates email
                                 server rsa-key-size default-location)
-     (list
-      (nginx-server-configuration
-       (listen '("80" "[::]:80"))
-       (ssl-certificate #f)
-       (ssl-certificate-key #f)
-       (server-name
-        (apply append (map certificate-configuration-domains certificates)))
-       (locations
-        (filter identity
-                (list
-                 (nginx-location-configuration
-                  (uri "/.well-known")
-                  (body (list (list "root " webroot ";"))))
-                 default-location))))))))
+     (define (certificate->nginx-server certificate-configuration)
+       (match-record certificate-configuration <certificate-configuration> 
+         (domains challenge)
+         (nginx-server-configuration
+          (listen '("80" "[::]:80"))
+          (ssl-certificate #f)
+          (ssl-certificate-key #f)
+          (server-name domains)
+          (locations
+           (filter identity
+                   (append
+                    (if challenge
+                      '()
+                      (list (nginx-location-configuration
+                             (uri "/.well-known")
+                             (body (list (list "root " webroot ";"))))))
+                    (list default-location)))))))
+     (map certificate->nginx-server certificates))))
 
 (define certbot-service-type
   (service-type (name 'certbot)

base-commit: 2cf71e725d55bc5bf1ad663b7c696516299cc8a7
-- 
2.39.2

[signature.asc (application/pgp-signature, inline)]

Message #20 received at 62642 <at> debbugs.gnu.org (full text, mbox):

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: Bruno Victal <mirai <at> makinata.eu>
Cc: 62642 <at> debbugs.gnu.org
Subject: Re: [bug#62642] [PATCH] services: certbot: Fix nginx crash when
 certbot is used without domains
Date: Thu, 13 Apr 2023 12:00:51 +0300

[Message part 1 (text/plain, inline)]

> IMO, certbot should be extending the nginx service only when the 'challenge' field
> is #f (ideally this should be made into a “enumerated” type, where the values range from
> 'http-01, 'dns-01, 'custom (as an escape hatch), ...)
> 
> Perhaps you could partition 'certificates' by whether 'challenge' is #f or not and use the
> results to craft the nginx extension value instead?

Certbot extends nginx for two reasons:

1. serving the challenge files
2. enforcing HTTPS by redirecting requests to domains with a certificate

The v2 patch adds a separate nginx server block for each certificate and
only servers challenge files if 'challenge' is #f. This also causes an
empty list of certificates to return an empty list of nginx server
blocks and thus fixes the original issue.

- Saku Laesvuori

[signature.asc (application/pgp-signature, inline)]

Message #23 received at 62642 <at> debbugs.gnu.org (full text, mbox):

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: Bruno Victal <mirai <at> makinata.eu>, 62642 <at> debbugs.gnu.org
Subject: Re: [bug#62642] [PATCH] services: certbot: Fix nginx crash when
 certbot is used without domains
Date: Mon, 22 May 2023 14:34:52 +0300

[Message part 1 (text/plain, inline)]

Is there something blocking this patch or is it just waiting for someone
to get around to applying it?

- Saku

[signature.asc (application/pgp-signature, inline)]

Message #28 received at 62642-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Saku Laesvuori <saku <at> laesvuori.fi>
Cc: 62642-done <at> debbugs.gnu.org, Bruno Victal <mirai <at> makinata.eu>
Subject: Re: bug#62642: [PATCH] services: certbot: Fix nginx crash when
 certbot is used without domains
Date: Sun, 18 Jun 2023 23:11:56 +0200

Hi Saku,

Saku Laesvuori <saku <at> laesvuori.fi> skribis:

> * gnu/services/certbot.scm (certbot-nginx-server-configurations):
> Don't return a broken nginx-server-configuration with empty server_name
> when no certificate domains are configured. Instead add a separate
> server for every certificate, so 0 certificates adds 0 servers.

Finally applied.

Thank you, and thanks to Bruno for reviewing!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.