bug#37388: <nginx-configuration> can lead to syntactically invalid configs

["Ludovic Courtès" <ludo@gnu.org>]

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

Hello!

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> It’s nice that we have <nginx-configuration> but I noticed that, unlike
>> most or all other configuration records that we have, it’s possible to
>> create an <nginx-configuration> record that leads to a syntactically
>> invalid nginx config file.
>>
>> For example, if you have a location block like this:
>>
>>         (nginx-location-configuration
>>           (uri "/manual/")
>>           (body (list "alias /srv/guix-manual")))
>>
>> Guix will silently create an invalid nginx config file, which you’ll
>> only notice once you’ve reconfigured and nginx fails to start.
>
> I wonder if some errors could be caught at build time, before attempting
> to start the service.
>
> If in the derivation to build the configuration file, nginx is run
> against the built config file with -t, that might spot errors at
> derivation build time.

Inspired, I tried the attached patch to do that.  However, that fails in
real-world situations, for example due to out-of-band references to
certificates:

--8<---------------cut here---------------start------------->8---
building /gnu/store/5k7w1l5ixg5vx1z7sdyabhgkpvvj7a5z-nginx.conf.drv...
nginx: [alert] could not open error log file: open() "run/logs/error.log" failed (2: No such file or directory)
2020/08/24 15:32:43 [warn] 7#0: the "user" directive makes sense only if the master process runs with super-user privileges, ignored in /gnu/store/c6zkj7rw37hh5a8mab9g37ca2aa33py0-unchecked-nginx.conf:1
2020/08/24 15:32:43 [emerg] 7#0: cannot load certificate "/etc/letsencrypt/live/berlin.guixsd.org/fullchain.pem": BIO_new_file() failed (SSL: error:02001002:system library:fopen:No such file or directory:fopen('/etc/letsencrypt/live/berlin.guixsd.org/fullchain.pem','r') error:2006D080:BIO routines:BIO_new_file:no such file)
nginx: configuration file /gnu/store/c6zkj7rw37hh5a8mab9g37ca2aa33py0-unchecked-nginx.conf test failed
Backtrace:
           2 (primitive-load "/gnu/store/4kb8dz6f6w5g50h8qghl35r1da0?")
In ice-9/eval.scm:
    619:8  1 (_ #f)
In guix/build/utils.scm:
    654:6  0 (invoke _ . _)

guix/build/utils.scm:654:6: In procedure invoke:
ERROR:
  1. &invoke-error:
      program: "/gnu/store/549pl4ch0zi3jjinpf1dckhxb1i0wp8f-nginx-1.19.2/sbin/nginx"
      arguments: ("-c" "/gnu/store/c6zkj7rw37hh5a8mab9g37ca2aa33py0-unchecked-nginx.conf" "-p" "run" "-t")
      exit-status: 1
      term-signal: #f
      stop-signal: #f
builder for `/gnu/store/5k7w1l5ixg5vx1z7sdyabhgkpvvj7a5z-nginx.conf.drv' failed with exit code 1
build of /gnu/store/5k7w1l5ixg5vx1z7sdyabhgkpvvj7a5z-nginx.conf.drv failed
--8<---------------cut here---------------end--------------->8---

I’m not sure what can be done.  Thoughts?

Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 2298 bytes --]

diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 3b9f9e40be..e47acfe118 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -629,7 +629,7 @@ of index files."
                  modules
                  global-directives
                  extra-content)
-   (apply mixed-text-file "nginx.conf"
+   (apply mixed-text-file "unchecked-nginx.conf"
           (flatten
            "user nginx nginx;\n"
            "pid " run-directory "/pid;\n"
@@ -662,6 +662,19 @@ of index files."
            extra-content
            "\n}\n"))))
 
+(define (validated-nginx-configuration-file nginx file)
+  "Return a copy of FILE, an nginx config file, after checking that it is
+syntactically correct."
+  (computed-file "nginx.conf"
+                 (with-imported-modules '((guix build utils))
+                   #~(begin
+                       (use-modules (guix build utils))
+
+                       (mkdir "run")
+                       (invoke #+(file-append nginx "/sbin/nginx")
+                               "-c" #$file "-p" "run" "-t")
+                       (copy-file #$file #$output)))))
+
 (define %nginx-accounts
   (list (user-group (name "nginx") (system? #t))
         (user-account
@@ -694,8 +707,10 @@ of index files."
        (mkdir-p (string-append #$run-directory "/logs"))
        ;; Check configuration file syntax.
        (system* (string-append #$nginx "/sbin/nginx")
-                "-c" #$(or file
-                           (default-nginx-config config))
+                "-c" #$(validated-nginx-configuration-file
+                        nginx
+                        (or file
+                            (default-nginx-config config)))
                 "-p" #$run-directory
                 "-t"))))
 
@@ -709,8 +724,10 @@ of index files."
            (lambda args
              #~(lambda _
                  (invoke #$nginx-binary "-c"
-                         #$(or file
-                               (default-nginx-config config))
+                         #$(validated-nginx-configuration-file
+                            nginx
+                            (or file
+                                (default-nginx-config config)))
                          #$@args)
                  (match '#$args
                    (("-s" . _) #f)