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

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

[Saku Laesvuori via Guix-patches via]

* 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

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

[Bruno Victal]

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

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

[Saku Laesvuori via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 758 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bruno Victal]

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

[bug#62642] [PATCH v2] services: certbot: Fix nginx crash when certbot is used without domains

[Saku Laesvuori via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 2247 bytes --]

* 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Saku Laesvuori via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Saku Laesvuori via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 111 bytes --]

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

- Saku

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Ludovic Courtès]

Hi Saku,

Saku Laesvuori <saku@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’.