GNU bug report logs - #37388
<nginx-configuration> can lead to syntactically invalid configs

Previous Next

To reply to this bug, email your comments to 37388 AT debbugs.gnu.org.

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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: bug-Guix <at> gnu.org
Subject: <nginx-configuration> can lead to syntactically invalid configs
Date: Thu, 12 Sep 2019 09:57:13 +0200

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

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

From: Gábor Boskovits <boskovits <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 37388 <at> debbugs.gnu.org
Subject: Re: bug#37388: <nginx-configuration> can lead to syntactically
 invalid configs
Date: Thu, 12 Sep 2019 14:49:10 +0200

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

Hello,

Ludovic Courtès <ludo <at> 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

[Message part 2 (text/html, inline)]

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Gábor Boskovits <boskovits <at> gmail.com>
Cc: 37388 <at> debbugs.gnu.org
Subject: Re: bug#37388: <nginx-configuration> can lead to syntactically
 invalid configs
Date: Sat, 14 Sep 2019 11:48:11 +0200

Hi Gábor,

Gábor Boskovits <boskovits <at> 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’.

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

From: Christopher Baines <mail <at> cbaines.net>
To: ludo <at> gnu.org
Cc: 37388 <at> debbugs.gnu.org
Subject: Re: bug#37388: <nginx-configuration> can lead to syntactically
 invalid configs
Date: Sat, 14 Sep 2019 11:02:59 +0100

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

Ludovic Courtès <ludo <at> 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.

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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 37388 <at> debbugs.gnu.org
Subject: Re: bug#37388: <nginx-configuration> can lead to syntactically
 invalid configs
Date: Sat, 14 Sep 2019 14:26:27 +0200

Hi,

Christopher Baines <mail <at> cbaines.net> skribis:

> Ludovic Courtès <ludo <at> 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’.

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

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 37388 <at> debbugs.gnu.org
Subject: Re: bug#37388: <nginx-configuration> can lead to syntactically
 invalid configs
Date: Sat, 14 Sep 2019 16:45:27 +0100

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

Ludovic Courtès <ludo <at> 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.

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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 37388 <at> debbugs.gnu.org
Subject: Re: bug#37388: <nginx-configuration> can lead to syntactically
 invalid configs
Date: Mon, 24 Aug 2020 17:35:12 +0200

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

Hello!

Christopher Baines <mail <at> cbaines.net> skribis:

> Ludovic Courtès <ludo <at> 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’.

[Message part 2 (text/x-patch, inline)]

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)

Previous Next

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