<nginx-configuration> can lead to syntactically invalid configs

Open

Details

3 participants

Gábor Boskovits
Ludovic Courtès
Christopher Baines

Owner: unassigned

Submitted by: Ludovic Courtès

Severity: normal

Ludovic Courtès wrote on 12 Sep 2019 09:57

Recipients:(address . bug-Guix@gnu.org)

Message-ID:87d0g6q752.fsf@inria.fr

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

Gábor Boskovits wrote on 12 Sep 2019 14:49

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 37388@debbugs.gnu.org)

Message-ID:CAE4v=piOdgg0wSL2VDJYo07nMPSYYUMvYvPhGnnBVQ_nVzHc+Q@mail.gmail.com

Hello,

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

9:58):

Toggle quote (31 lines)> 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.

Toggle quote (3 lines)

> 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?

Toggle quote (6 lines)>
> Ludo’.
>
>
>
>

Best regards,

g_bor

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

Attachment: file

Ludovic Courtès wrote on 14 Sep 2019 11:48

Recipients:(name . Gábor Boskovits)(address . boskovits@gmail.com)(address . 37388@debbugs.gnu.org)

Message-ID:87lfur5hus.fsf@gnu.org

Hi Gábor,

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

Toggle quote (14 lines)> 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’.

Christopher Baines wrote on 14 Sep 2019 12:02

Recipients:(address . ludo@gnu.org)(address . 37388@debbugs.gnu.org)

Message-ID:87d0g3nqjw.fsf@cbaines.net

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

Toggle quote (14 lines)> 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.

Toggle quote (14 lines)> 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.

Ludovic Courtès wrote on 14 Sep 2019 14:26

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 37388@debbugs.gnu.org)

Message-ID:87r24j3vyk.fsf@gnu.org

Hi,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (23 lines)> 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.

Toggle quote (3 lines)

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

Christopher Baines wrote on 14 Sep 2019 17:45

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 37388@debbugs.gnu.org)

Message-ID:87a7b6op9k.fsf@cbaines.net

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

Toggle quote (13 lines)>> 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.

Ludovic Courtès wrote on 24 Aug 2020 17:35

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 37388@debbugs.gnu.org)

Message-ID:87eenw12hb.fsf@gnu.org

Hello!

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (23 lines)> 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:

Toggle snippet (24 lines)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

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

Ludo’.

Toggle diff (59 lines)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)

Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 37388@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 37388

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date