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

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

[Ludovic Courtès]

Hello Guix!

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.

See why?  That’s because we’re missing a semicolon in the “alias”
directive, and that directive is spit out directly as is.

To address it, we could have record types for <alias>, <root>, and all
the directives out there; it could be tedious, unless we automate it,
effectively creating a complete EDSL.

Another approach would be to have an sexp representation of the nginx
configuration language.  That’d effectively replace semicolons with
parentheses :-), but more importantly, that would allow us to not paste
strings as-is in the resulting config file.  The downside is that it’s
very much “free style” compared to records, but we could still
pattern-match the sexp to validate certain properties.

Thoughts?

Ludo’.

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

[Gábor Boskovits]

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

Hello,

Ludovic Courtès <ludo@gnu.org> ezt írta (időpont: 2019. szept. 12., Cs,
9:58):

> Hello Guix!
>
> 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.
>
> See why?  That’s because we’re missing a semicolon in the “alias”
> directive, and that directive is spit out directly as is.
>
> To address it, we could have record types for <alias>, <root>, and all
> the directives out there; it could be tedious, unless we automate it,
> effectively creating a complete EDSL.
>
> Another approach would be to have an sexp representation of the nginx
> configuration language.  That’d effectively replace semicolons with
> parentheses :-), but more importantly, that would allow us to not paste
> strings as-is in the resulting config file.  The downside is that it’s
> very much “free style” compared to records, but we could still
> pattern-match the sexp to validate certain properties.
>
>
I would most probably go for the sexp version.


> Thoughts?
>

I would like to add some more information to this, which I encountered when
trying to find a solution to the last-modified issue:

1. the nginx configuration can only be extended using server blocks, so it
is not possible to inject a location or a nested location.
2. the meaning of the nginx configuration can dependent on the order of
directives in the configuration. Either we should give
and explicit mechanism for dealing with that, or disallow such
configurations.

If you feel these points to be off topic, then I can open a separate bug
for that, but these seem to relate to the confgiuration mechanism,
and should be considered when designing the new interface. Wdyt?


>
> Ludo’.
>
>
>
>
Best regards,
g_bor

-- 
OpenPGP Key Fingerprint: 7988:3B9F:7D6A:4DBF:3719:0367:2506:A96C:CF63:0B21

[-- Attachment #2: Type: text/html, Size: 3295 bytes --]

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

[Ludovic Courtès]

Hi Gábor,

Gábor Boskovits <boskovits@gmail.com> skribis:

> I would like to add some more information to this, which I encountered when
> trying to find a solution to the last-modified issue:
>
> 1. the nginx configuration can only be extended using server blocks, so it
> is not possible to inject a location or a nested location.
> 2. the meaning of the nginx configuration can dependent on the order of
> directives in the configuration. Either we should give
> and explicit mechanism for dealing with that, or disallow such
> configurations.
>
> If you feel these points to be off topic, then I can open a separate bug
> for that, but these seem to relate to the confgiuration mechanism,
> and should be considered when designing the new interface. Wdyt?

I think it would deserve a separate issue, but I agree that extension of
<nginx-configuration> is tricky due to ordering.

Thanks for your feedback,
Ludo’.

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

[Christopher Baines]

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


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.

> See why?  That’s because we’re missing a semicolon in the “alias”
> directive, and that directive is spit out directly as is.
>
> To address it, we could have record types for <alias>, <root>, and all
> the directives out there; it could be tedious, unless we automate it,
> effectively creating a complete EDSL.
>
> Another approach would be to have an sexp representation of the nginx
> configuration language.  That’d effectively replace semicolons with
> parentheses :-), but more importantly, that would allow us to not paste
> strings as-is in the resulting config file.  The downside is that it’s
> very much “free style” compared to records, but we could still
> pattern-match the sexp to validate certain properties.

An sexp representation sounds good, although I think records will work
out better for the common and high level parts.

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

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

[Ludovic Courtès]

Hi,

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.

Yeah, this is probably doable.

I would consider it a stop-gap measure though.  Fundamentally, I think
we should make it so that, by construction, invalid (or at least
syntactically-invalid) config files cannot be produced.

> An sexp representation sounds good, although I think records will work
> out better for the common and high level parts.

The way I see it, sexps and records could be almost indistinguishable
provided some appropriate macrology.  But sexps are definitely easier to
implement.

Thanks,
Ludo’.

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

[Christopher Baines]

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


Ludovic Courtès <ludo@gnu.org> writes:

>> 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.
>
> Yeah, this is probably doable.
>
> I would consider it a stop-gap measure though.  Fundamentally, I think
> we should make it so that, by construction, invalid (or at least
> syntactically-invalid) config files cannot be produced.

Catching errors earlier is better, but being able to catch any syntactic
issues that have snuck through, as well as semantic ones when building
the configuration would be good I think. I haven't actually tested out
the NGinx configuration check functionality though, so I'm guessing
about what it does.

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

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

[Ludovic Courtès]

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