GNU bug report logs - #29467
[PATCH] web: Don't error about missing ssl related files.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 29467 in the body.
You can then email your comments to 29467 AT debbugs.gnu.org in the normal way.

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

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

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH] web: Don't error about missing ssl related files.
Date: Mon, 27 Nov 2017 08:26:20 +0000

Erroring here prevents doing things like building a system using nginx on a
different machine from where it's intended to be deployed, or creating
containers and VMs that use the ssl-certificate parts of the nginx
configuration, without also getting these files to exist.

* gnu/services/web.scm (emit-nginx-server-config): Don't error on missing ssl
  related files.
---
 gnu/services/web.scm | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 9d713003c..1af32278c 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -191,16 +191,6 @@ of index files."
             (syntax-parameterize ((<> (identifier-syntax x*)))
               (list tail ...))
             '())))
-    (for-each
-     (match-lambda
-      ((record-key . file)
-       (if (and file (not (file-exists? file)))
-           (error
-            (simple-format
-             #f
-             "~A in the nginx configuration for the server with name \"~A\" does not exist" record-key server-name)))))
-     `(("ssl-certificate"     . ,ssl-certificate)
-       ("ssl-certificate-key" . ,ssl-certificate-key)))
     (list
      "    server {\n"
      (and/l http-port  "      listen " (number->string <>) ";\n")
-- 
2.14.2

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

From: julien lepiller <julien <at> lepiller.eu>
To: guix-patches <at> gnu.org
Subject: Re: [bug#29467] [PATCH] web: Don't error about missing ssl related
 files.
Date: Mon, 27 Nov 2017 10:22:48 +0100

Le 2017-11-27 09:26, Christopher Baines a écrit :
> Erroring here prevents doing things like building a system using nginx 
> on a
> different machine from where it's intended to be deployed, or creating
> containers and VMs that use the ssl-certificate parts of the nginx
> configuration, without also getting these files to exist.
> 
> * gnu/services/web.scm (emit-nginx-server-config): Don't error on 
> missing ssl
>   related files.
> ---
>  gnu/services/web.scm | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
> index 9d713003c..1af32278c 100644
> --- a/gnu/services/web.scm
> +++ b/gnu/services/web.scm
> @@ -191,16 +191,6 @@ of index files."
>              (syntax-parameterize ((<> (identifier-syntax x*)))
>                (list tail ...))
>              '())))
> -    (for-each
> -     (match-lambda
> -      ((record-key . file)
> -       (if (and file (not (file-exists? file)))
> -           (error
> -            (simple-format
> -             #f
> -             "~A in the nginx configuration for the server with name
> \"~A\" does not exist" record-key server-name)))))
> -     `(("ssl-certificate"     . ,ssl-certificate)
> -       ("ssl-certificate-key" . ,ssl-certificate-key)))
>      (list
>       "    server {\n"
>       (and/l http-port  "      listen " (number->string <>) ";\n")

Hi, when configuring nginx for the first time, users will probably 
forget to
configure ssl properly. The default is to enable ssl and find 
certificates in
/etc/nginx. When these files don't exist, nginx will fail to start and 
at least
one user complained it was hard to debug. This code was introduced to 
prevent
such a mistake.

Maybe we should set the default to #f (but then users would have to 
configure
more fields to enable https). Maybe we should add a configuration option 
like
warn-only? (default to #f) to only warn about missing files. Or maybe 
there's
a way to show nginx that another service is providing that file?

I agree there is an issue, but your patch feels like a regression to me 
for the
documented use-cases. WDYT?

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: julien lepiller <julien <at> lepiller.eu>
Cc: 29467 <at> debbugs.gnu.org
Subject: Re: [bug#29467] [PATCH] web: Don't error about missing ssl related
 files.
Date: Tue, 05 Dec 2017 12:14:12 +0100

Hi,

julien lepiller <julien <at> lepiller.eu> skribis:

> Le 2017-11-27 09:26, Christopher Baines a écrit :
>> Erroring here prevents doing things like building a system using
>> nginx on a
>> different machine from where it's intended to be deployed, or creating
>> containers and VMs that use the ssl-certificate parts of the nginx
>> configuration, without also getting these files to exist.
>>
>> * gnu/services/web.scm (emit-nginx-server-config): Don't error on
>> missing ssl
>>   related files.
>> ---
>>  gnu/services/web.scm | 10 ----------
>>  1 file changed, 10 deletions(-)
>>
>> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
>> index 9d713003c..1af32278c 100644
>> --- a/gnu/services/web.scm
>> +++ b/gnu/services/web.scm
>> @@ -191,16 +191,6 @@ of index files."
>>              (syntax-parameterize ((<> (identifier-syntax x*)))
>>                (list tail ...))
>>              '())))
>> -    (for-each
>> -     (match-lambda
>> -      ((record-key . file)
>> -       (if (and file (not (file-exists? file)))

There’s another problem: ‘file-exists?’ checks the current machine,
under the current root file system.  That check doesn’t work if you do
“guix system init config.scm /some/other/root”, or if you create a
container, or with the envisioned “guix system reconfigure --remote”.

> Hi, when configuring nginx for the first time, users will probably
> forget to
> configure ssl properly. The default is to enable ssl and find
> certificates in
> /etc/nginx. When these files don't exist, nginx will fail to start and
> at least
> one user complained it was hard to debug. This code was introduced to
> prevent
> such a mistake.

Yes, I agree that it’s nice to have early error reports.

> Maybe we should set the default to #f (but then users would have to
> configure
> more fields to enable https). Maybe we should add a configuration
> option like
> warn-only? (default to #f) to only warn about missing files. Or maybe
> there's
> a way to show nginx that another service is providing that file?

Good questions.

We cannot check for file existence at configuration time for the reasons
above.

We cannot check for file existence at build time because certificates
may be part of the machine’s state; they are typically managed in a
stateful fashion, outside of GuixSD.

So the only option we’re left with is checking at run time, when we
start the service.  But that’s something nginx already does, I think?

As for the default, I would be in favor of setting it to #f, because I
can’t really think of a default that would work for everyone.

WDYT?

Ludo’.

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

From: julien lepiller <julien <at> lepiller.eu>
To: 29467 <at> debbugs.gnu.org
Subject: Re: [bug#29467] [PATCH] web: Don't error about missing ssl related
 files.
Date: Tue, 05 Dec 2017 12:23:39 +0100

Le 2017-12-05 12:14, ludo <at> gnu.org a écrit :
> Hi,
> 
> julien lepiller <julien <at> lepiller.eu> skribis:
> 
>> Le 2017-11-27 09:26, Christopher Baines a écrit :
>>> Erroring here prevents doing things like building a system using
>>> nginx on a
>>> different machine from where it's intended to be deployed, or 
>>> creating
>>> containers and VMs that use the ssl-certificate parts of the nginx
>>> configuration, without also getting these files to exist.
>>> 
>>> * gnu/services/web.scm (emit-nginx-server-config): Don't error on
>>> missing ssl
>>>   related files.
>>> ---
>>>  gnu/services/web.scm | 10 ----------
>>>  1 file changed, 10 deletions(-)
>>> 
>>> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
>>> index 9d713003c..1af32278c 100644
>>> --- a/gnu/services/web.scm
>>> +++ b/gnu/services/web.scm
>>> @@ -191,16 +191,6 @@ of index files."
>>>              (syntax-parameterize ((<> (identifier-syntax x*)))
>>>                (list tail ...))
>>>              '())))
>>> -    (for-each
>>> -     (match-lambda
>>> -      ((record-key . file)
>>> -       (if (and file (not (file-exists? file)))
> 
> There’s another problem: ‘file-exists?’ checks the current machine,
> under the current root file system.  That check doesn’t work if you do
> “guix system init config.scm /some/other/root”, or if you create a
> container, or with the envisioned “guix system reconfigure --remote”.
> 
>> Hi, when configuring nginx for the first time, users will probably
>> forget to
>> configure ssl properly. The default is to enable ssl and find
>> certificates in
>> /etc/nginx. When these files don't exist, nginx will fail to start and
>> at least
>> one user complained it was hard to debug. This code was introduced to
>> prevent
>> such a mistake.
> 
> Yes, I agree that it’s nice to have early error reports.
> 
>> Maybe we should set the default to #f (but then users would have to
>> configure
>> more fields to enable https). Maybe we should add a configuration
>> option like
>> warn-only? (default to #f) to only warn about missing files. Or maybe
>> there's
>> a way to show nginx that another service is providing that file?
> 
> Good questions.
> 
> We cannot check for file existence at configuration time for the 
> reasons
> above.
> 
> We cannot check for file existence at build time because certificates
> may be part of the machine’s state; they are typically managed in a
> stateful fashion, outside of GuixSD.
> 
> So the only option we’re left with is checking at run time, when we
> start the service.  But that’s something nginx already does, I think?
> 
> As for the default, I would be in favor of setting it to #f, because I
> can’t really think of a default that would work for everyone.
> 
> WDYT?

Having it default to #f is fine with me. Nginx does this check at 
runtime
and will refuse to start if these files are missing. Keeping https-port
to 443 and certificates to #f means it will not be able to establish a
connection to the client, but the http website will be available. So 
just
setting the key and the certificate to #f by default should be OK.

> 
> Ludo’.

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: julien lepiller <julien <at> lepiller.eu>
Cc: Christopher Baines <mail <at> cbaines.net>, 29467 <at> debbugs.gnu.org
Subject: Re: [bug#29467] [PATCH] web: Don't error about missing ssl related
 files.
Date: Fri, 08 Dec 2017 10:41:36 +0100

Hi,

julien lepiller <julien <at> lepiller.eu> skribis:

> Le 2017-12-05 12:14, ludo <at> gnu.org a écrit :

[...]

>> We cannot check for file existence at configuration time for the
>> reasons
>> above.
>>
>> We cannot check for file existence at build time because certificates
>> may be part of the machine’s state; they are typically managed in a
>> stateful fashion, outside of GuixSD.
>>
>> So the only option we’re left with is checking at run time, when we
>> start the service.  But that’s something nginx already does, I think?
>>
>> As for the default, I would be in favor of setting it to #f, because I
>> can’t really think of a default that would work for everyone.
>>
>> WDYT?
>
> Having it default to #f is fine with me. Nginx does this check at
> runtime
> and will refuse to start if these files are missing. Keeping https-port
> to 443 and certificates to #f means it will not be able to establish a
> connection to the client, but the http website will be available. So
> just
> setting the key and the certificate to #f by default should be OK.

OK, sounds good.

Chris, can you make this change?

Thanks,
Ludo’.

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

From: Christopher Baines <mail <at> cbaines.net>
To: 29467 <at> debbugs.gnu.org
Subject: [PATCH 1/2] web: Don't error about missing ssl related files.
Date: Sat,  9 Dec 2017 09:31:13 +0000

Erroring here prevents doing things like building a system using nginx on a
different machine from where it's intended to be deployed, or creating
containers and VMs that use the ssl-certificate parts of the nginx
configuration, without also getting these files to exist.

* gnu/services/web.scm (emit-nginx-server-config): Don't error on missing ssl
  related files.
---
 gnu/services/web.scm | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 9d713003c..1af32278c 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -191,16 +191,6 @@ of index files."
             (syntax-parameterize ((<> (identifier-syntax x*)))
               (list tail ...))
             '())))
-    (for-each
-     (match-lambda
-      ((record-key . file)
-       (if (and file (not (file-exists? file)))
-           (error
-            (simple-format
-             #f
-             "~A in the nginx configuration for the server with name \"~A\" does not exist" record-key server-name)))))
-     `(("ssl-certificate"     . ,ssl-certificate)
-       ("ssl-certificate-key" . ,ssl-certificate-key)))
     (list
      "    server {\n"
      (and/l http-port  "      listen " (number->string <>) ";\n")
-- 
2.14.2

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

From: Christopher Baines <mail <at> cbaines.net>
To: 29467 <at> debbugs.gnu.org
Subject: [PATCH 2/2] services: web: Remove default certificate and key files
 for nginx.
Date: Sat,  9 Dec 2017 09:31:14 +0000

If nginx is configured with a ssl-certificate file, and ssl-certificate-key,
it will fail to start unless these exist. To avoid this happening, change the
default to #f.

* gnu/services/web.scm (<nginx-server-configuration>)
  [ssl-certificate,ssl-certificate-key]: Set the defaults to #f.
* gnu/tests/web.scm (%nginx-servers): Remove redundant
  nginx-server-configuration fields.
* doc/guix.texi (Web Services): Update examples and documentation.
---
 doc/guix.texi        | 20 ++++----------------
 gnu/services/web.scm |  4 ++--
 gnu/tests/web.scm    |  5 +----
 3 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 23ccfa2f6..35f895bb4 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -14813,10 +14813,7 @@ A simple example configuration is given below.
            (server-blocks
              (list (nginx-server-configuration
                      (server-name '("www.example.com"))
-                     (root "/srv/http/www.example.com")
-                     (https-port #f)
-                     (ssl-certificate #f)
-                     (ssl-certificate-key #f))))))
+                     (root "/srv/http/www.example.com"))))))
 @end example
 
 In addition to adding server blocks to the service configuration
@@ -14826,9 +14823,6 @@ blocks, as in this example:
 @example
 (simple-service 'my-extra-server nginx-service-type
                 (list (nginx-server-configuration
-                        (https-port #f)
-                        (ssl-certificate #f)
-                        (ssl-certificate-key #f)
                         (root "/srv/http/extra-website")
                         (try-files (list "$uri" "$uri/index.html")))))
 @end example
@@ -14873,10 +14867,7 @@ HTTPS.
            (server-blocks
              (list (nginx-server-configuration
                      (server-name '("www.example.com"))
-                     (root "/srv/http/www.example.com")
-                     (https-port #f)
-                     (ssl-certificate #f)
-                     (ssl-certificate-key #f))))))
+                     (root "/srv/http/www.example.com"))))))
 @end example
 
 @item @code{upstream-blocks} (default: @code{'()})
@@ -14899,9 +14890,6 @@ requests with two servers.
       (list (nginx-server-configuration
               (server-name '("www.example.com"))
               (root "/srv/http/www.example.com")
-              (https-port #f)
-              (ssl-certificate #f)
-              (ssl-certificate-key #f)
               (locations
                 (list
                   (nginx-location-configuration
@@ -14965,11 +14953,11 @@ Nginx will send the list of files in the directory.
 A list of files whose existence is checked in the specified order.
 @code{nginx} will use the first file it finds to process the request.
 
-@item @code{ssl-certificate} (default: @code{"/etc/nginx/cert.pem"})
+@item @code{ssl-certificate} (default: @code{#f})
 Where to find the certificate for secure connections.  Set it to @code{#f} if
 you don't have a certificate or you don't want to use HTTPS.
 
-@item @code{ssl-certificate-key} (default: @code{"/etc/nginx/key.pem"})
+@item @code{ssl-certificate-key} (default: @code{#f})
 Where to find the private key for secure connections.  Set it to @code{#f} if
 you don't have a key or you don't want to use HTTPS.
 
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 1af32278c..51cd9da1d 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -102,9 +102,9 @@
   (try-files           nginx-server-configuration-try-files
                        (default '()))
   (ssl-certificate     nginx-server-configuration-ssl-certificate
-                       (default "/etc/nginx/cert.pem"))
+                       (default #f))
   (ssl-certificate-key nginx-server-configuration-ssl-certificate-key
-                       (default "/etc/nginx/key.pem"))
+                       (default #f))
   (server-tokens?      nginx-server-configuration-server-tokens?
                        (default #f)))
 
diff --git a/gnu/tests/web.scm b/gnu/tests/web.scm
index 3fa272c67..de7ab3cd6 100644
--- a/gnu/tests/web.scm
+++ b/gnu/tests/web.scm
@@ -45,10 +45,7 @@
   ;; Server blocks.
   (list (nginx-server-configuration
          (root "/srv")
-         (http-port 8042)
-         (https-port #f)
-         (ssl-certificate #f)
-         (ssl-certificate-key #f))))
+         (http-port 8042))))
 
 (define %nginx-os
   ;; Operating system under test.
-- 
2.14.2

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

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: julien lepiller <julien <at> lepiller.eu>, 29467 <at> debbugs.gnu.org
Subject: Re: [bug#29467] [PATCH] web: Don't error about missing ssl related
 files.
Date: Sat, 09 Dec 2017 09:37:19 +0000

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

Ludovic Courtès writes:

> Hi,
>
> julien lepiller <julien <at> lepiller.eu> skribis:
>
>> Le 2017-12-05 12:14, ludo <at> gnu.org a écrit:
>
> [...]
>
>>> We cannot check for file existence at configuration time for the
>>> reasons
>>> above.
>>>
>>> We cannot check for file existence at build time because certificates
>>> may be part of the machine’s state; they are typically managed in a
>>> stateful fashion, outside of GuixSD.
>>>
>>> So the only option we’re left with is checking at run time, when we
>>> start the service.  But that’s something nginx already does, I think?
>>>
>>> As for the default, I would be in favor of setting it to #f, because I
>>> can’t really think of a default that would work for everyone.
>>>
>>> WDYT?
>>
>> Having it default to #f is fine with me. Nginx does this check at
>> runtime
>> and will refuse to start if these files are missing. Keeping https-port
>> to 443 and certificates to #f means it will not be able to establish a
>> connection to the client, but the http website will be available. So
>> just
>> setting the key and the certificate to #f by default should be OK.
>
> OK, sounds good.
>
> Chris, can you make this change?

Yep, I've send some updated patches.

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

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Christopher Baines <mail <at> cbaines.net>
Cc: 29467 <at> debbugs.gnu.org
Subject: Re: [bug#29467] [PATCH 1/2] web: Don't error about missing ssl
 related files.
Date: Mon, 11 Dec 2017 14:26:22 +0100

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

> Erroring here prevents doing things like building a system using nginx on a
> different machine from where it's intended to be deployed, or creating
> containers and VMs that use the ssl-certificate parts of the nginx
> configuration, without also getting these files to exist.
>
> * gnu/services/web.scm (emit-nginx-server-config): Don't error on missing ssl
>   related files.

LGTM!

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Christopher Baines <mail <at> cbaines.net>
Cc: 29467 <at> debbugs.gnu.org
Subject: Re: [bug#29467] [PATCH 2/2] services: web: Remove default certificate
 and key files for nginx.
Date: Mon, 11 Dec 2017 14:26:33 +0100

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

> If nginx is configured with a ssl-certificate file, and ssl-certificate-key,
> it will fail to start unless these exist. To avoid this happening, change the
> default to #f.
>
> * gnu/services/web.scm (<nginx-server-configuration>)
>   [ssl-certificate,ssl-certificate-key]: Set the defaults to #f.
> * gnu/tests/web.scm (%nginx-servers): Remove redundant
>   nginx-server-configuration fields.
> * doc/guix.texi (Web Services): Update examples and documentation.

LGTM, thanks!

Ludo'.

Message #37 received at 29467-done <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 29467-done <at> debbugs.gnu.org
Subject: Re: [bug#29467] [PATCH 2/2] services: web: Remove default certificate
 and key files for nginx.
Date: Mon, 11 Dec 2017 20:41:04 +0000

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

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

> Christopher Baines <mail <at> cbaines.net> skribis:
>
>> If nginx is configured with a ssl-certificate file, and ssl-certificate-key,
>> it will fail to start unless these exist. To avoid this happening, change the
>> default to #f.
>>
>> * gnu/services/web.scm (<nginx-server-configuration>)
>>   [ssl-certificate,ssl-certificate-key]: Set the defaults to #f.
>> * gnu/tests/web.scm (%nginx-servers): Remove redundant
>>   nginx-server-configuration fields.
>> * doc/guix.texi (Web Services): Update examples and documentation.
>
> LGTM, thanks!

Great, I've now pushed these two patches.

Thanks for reviewing :)

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

Previous Next

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