bug#37388: can lead to syntactically invalid configs
Hello! Christopher Baines skribis: > Ludovic Courtès writes: > >> It’s nice that we have but I noticed that, unlike >> most or all other configuration records that we have, it’s possible to >> create an 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. : 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’. 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" . _)
bug#37388: can lead to syntactically invalid configs
Ludovic Courtès 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 Description: PGP signature
bug#37388: can lead to syntactically invalid configs
Hi, Christopher Baines skribis: > Ludovic Courtès writes: > >> It’s nice that we have but I noticed that, unlike >> most or all other configuration records that we have, it’s possible to >> create an 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: can lead to syntactically invalid configs
Ludovic Courtès writes: > It’s nice that we have but I noticed that, unlike > most or all other configuration records that we have, it’s possible to > create an 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 , , 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 Description: PGP signature
bug#37388: can lead to syntactically invalid configs
Hi Gábor, Gábor Boskovits 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 is tricky due to ordering. Thanks for your feedback, Ludo’.
bug#37388: can lead to syntactically invalid configs
Hello, Ludovic Courtès ezt írta (időpont: 2019. szept. 12., Cs, 9:58): > Hello Guix! > > It’s nice that we have but I noticed that, unlike > most or all other configuration records that we have, it’s possible to > create an 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 , , 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
bug#37388: can lead to syntactically invalid configs
Hello Guix! It’s nice that we have but I noticed that, unlike most or all other configuration records that we have, it’s possible to create an 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 , , 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’.