unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
* [bug#62642] [PATCH] services: certbot: Fix nginx crash when certbot is used without domains
@ 2023-04-03 13:32 Saku Laesvuori via Guix-patches via
  2023-04-03 14:28 ` Bruno Victal
  0 siblings, 1 reply; 8+ messages in thread
From: Saku Laesvuori via Guix-patches via @ 2023-04-03 13:32 UTC (permalink / raw)
  To: 62642; +Cc: Saku Laesvuori

* 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





^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [bug#62642] [PATCH] services: certbot: Fix nginx crash when certbot is used without domains
  2023-04-03 13:32 [bug#62642] [PATCH] services: certbot: Fix nginx crash when certbot is used without domains Saku Laesvuori via Guix-patches via
@ 2023-04-03 14:28 ` Bruno Victal
  2023-04-03 18:06   ` Saku Laesvuori via Guix-patches via
  0 siblings, 1 reply; 8+ messages in thread
From: Bruno Victal @ 2023-04-03 14:28 UTC (permalink / raw)
  To: Saku Laesvuori; +Cc: 62642

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#62642] [PATCH] services: certbot: Fix nginx crash when certbot is used without domains
  2023-04-03 14:28 ` Bruno Victal
@ 2023-04-03 18:06   ` Saku Laesvuori via Guix-patches via
  2023-04-04 13:21     ` Bruno Victal
  0 siblings, 1 reply; 8+ messages in thread
From: Saku Laesvuori via Guix-patches via @ 2023-04-03 18:06 UTC (permalink / raw)
  To: Bruno Victal; +Cc: 62642

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#62642] [PATCH] services: certbot: Fix nginx crash when certbot is used without domains
  2023-04-03 18:06   ` Saku Laesvuori via Guix-patches via
@ 2023-04-04 13:21     ` Bruno Victal
  2023-04-04 20:43       ` [bug#62642] [PATCH v2] " Saku Laesvuori via Guix-patches via
  2023-04-13  9:00       ` [bug#62642] " Saku Laesvuori via Guix-patches via
  0 siblings, 2 replies; 8+ messages in thread
From: Bruno Victal @ 2023-04-04 13:21 UTC (permalink / raw)
  To: Saku Laesvuori; +Cc: 62642

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#62642] [PATCH v2] services: certbot: Fix nginx crash when certbot is used without domains
  2023-04-04 13:21     ` Bruno Victal
@ 2023-04-04 20:43       ` Saku Laesvuori via Guix-patches via
  2023-06-18 21:11         ` bug#62642: [PATCH] " Ludovic Courtès
  2023-04-13  9:00       ` [bug#62642] " Saku Laesvuori via Guix-patches via
  1 sibling, 1 reply; 8+ messages in thread
From: Saku Laesvuori via Guix-patches via @ 2023-04-04 20:43 UTC (permalink / raw)
  To: Bruno Victal; +Cc: 62642

[-- 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 --]

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [bug#62642] [PATCH] services: certbot: Fix nginx crash when certbot is used without domains
  2023-04-04 13:21     ` Bruno Victal
  2023-04-04 20:43       ` [bug#62642] [PATCH v2] " Saku Laesvuori via Guix-patches via
@ 2023-04-13  9:00       ` Saku Laesvuori via Guix-patches via
  2023-05-22 11:34         ` Saku Laesvuori via Guix-patches via
  1 sibling, 1 reply; 8+ messages in thread
From: Saku Laesvuori via Guix-patches via @ 2023-04-13  9:00 UTC (permalink / raw)
  To: Bruno Victal; +Cc: 62642

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#62642] [PATCH] services: certbot: Fix nginx crash when certbot is used without domains
  2023-04-13  9:00       ` [bug#62642] " Saku Laesvuori via Guix-patches via
@ 2023-05-22 11:34         ` Saku Laesvuori via Guix-patches via
  0 siblings, 0 replies; 8+ messages in thread
From: Saku Laesvuori via Guix-patches via @ 2023-05-22 11:34 UTC (permalink / raw)
  To: Bruno Victal, 62642

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#62642: [PATCH] services: certbot: Fix nginx crash when certbot is used without domains
  2023-04-04 20:43       ` [bug#62642] [PATCH v2] " Saku Laesvuori via Guix-patches via
@ 2023-06-18 21:11         ` Ludovic Courtès
  0 siblings, 0 replies; 8+ messages in thread
From: Ludovic Courtès @ 2023-06-18 21:11 UTC (permalink / raw)
  To: Saku Laesvuori; +Cc: 62642-done, Bruno Victal

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’.




^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-06-18 21:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-03 13:32 [bug#62642] [PATCH] services: certbot: Fix nginx crash when certbot is used without domains Saku Laesvuori via Guix-patches via
2023-04-03 14:28 ` Bruno Victal
2023-04-03 18:06   ` Saku Laesvuori via Guix-patches via
2023-04-04 13:21     ` Bruno Victal
2023-04-04 20:43       ` [bug#62642] [PATCH v2] " Saku Laesvuori via Guix-patches via
2023-06-18 21:11         ` bug#62642: [PATCH] " Ludovic Courtès
2023-04-13  9:00       ` [bug#62642] " Saku Laesvuori via Guix-patches via
2023-05-22 11:34         ` Saku Laesvuori via Guix-patches via

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).