nginx: make enabling SSL port-specific #27426

rnhmjoj · 2017-07-16T16:40:34Z

Motivation for this change

Fix issues #27338, #27442, #25533 and possibly #23711

This PR implements @globin's suggestion about SSL:

addSSL: add a listen port on *:443 keeping *:80 and every other address/port
onlySSL: listen only on *:443
forceSSL: redirects *:80 (and every other non-ssl port) to *:443

addSSL and onlySSL are mutually exclusive while forceSSL needs addSSL.

Moreover one can freely configure the virtual host using the listen option directly.

Things done

Tested via one or more NixOS test (nixos/tests/nginx.nix)
Tested addSSL, onlySSL and forceSSL
Tested enableACME
Fits CONTRIBUTING.md.

I had to move out the default value for config.nginx.virtualHost.<name>.listen because I don't know how to access the configuration inside a submodule. Is there a way? I would like to see what's the default from man configuration.nix when possible.

cc @globin @fpletz

mention-bot · 2017-07-16T16:40:36Z

@rnhmjoj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abbradar, @roconnor and @globin to be potential reviewers.

wmertens · 2017-07-17T13:22:24Z

LGTM, but would it not be better to remain backwards-compatible by making enableSSL an alias for onlySSL? You can use visible: false to make it invisible https://github.com/NixOS/nixpkgs/blob/master/lib/options.nix#L21

rnhmjoj · 2017-07-17T13:30:24Z

Ok, a deprecation warning could be useful too. Is there a way?

edit: I have found about mkRenamedOptionModule but I can't get it right.

rnhmjoj · 2017-07-17T18:07:25Z

(mkRenamedOptionModule [ "services" "nginx" "virtualHosts" "<name>" "enableSSL" ] [ "services" "nginx" "virtualHosts" "<name>" "onySSL" ])

gives this error:

error: The option `services.nginx.virtualHosts' in `src/nixpkgs/nixos/modules/services/web-servers/nginx/default.nix' is a prefix of options in `src/nixpkgs/nixos/modules/rename.nix'.

globin · 2017-07-17T23:52:56Z

@edolstra, @nbp, do you know how to specify a mkRenamedOptionModule with submodules?

wmertens · 2017-07-19T13:33:36Z

I don't think it's possible, but you could just keep enableSSL a hidden setting that you logically OR with forceSSL to decide to force…

nbp · 2017-07-22T09:49:07Z

@globin @rnhmjoj To make it work within a submodule you would have to make a new option named warnings which accumulate the messages, and the definitions of these options (multiple instances of submodules) should be forwarded to the top-level warnings option.

https://github.com/NixOS/nixpkgs/blob/master/lib/modules.nix#L669

rnhmjoj · 2017-07-22T14:47:41Z

Sorry but I don't understand. Can you show me an example?

wmertens · 2017-07-26T15:14:24Z

👌 LGTM, let's merge?

rnhmjoj · 2017-07-26T15:35:24Z

Currently .enableSSL = true; doesn't do anything besides triggering the warning. Is it ok?

wmertens · 2017-07-26T19:07:42Z

Can you make it OR together with .sslOnly? That way there's no breakage in 17.09 and we can remove it in the release after that…

…

On Wed, Jul 26, 2017, 5:35 PM Michele Guerini Rocco < ***@***.***> wrote: Currently .enableSSL = true; doesn't do anything besides triggering the warning. Is it ok? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27426 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWln2tajWZ-84Rhd39U9VbK8qAA3vGks5sR1y9gaJpZM4OZWMq> .

rnhmjoj · 2017-07-26T20:19:42Z

Done.

fpletz

Thanks a lot! LGTM overall but see my comments.

fpletz · 2017-07-26T23:43:18Z

nixos/modules/services/web-servers/nginx/default.nix

          + ";";

+        redirectListen = filter (x: !x.ssl) defaultListen;


I think we need another assertion that there is a listen statement with and without ssl enabled. Otherwise there won't be any listen directives in either the redirect or host server block.

I'm not sure I understand. forceSSL implies addSSL so at least hostListen should have an entry. How can they be both empty?

@rnhmjoj Hmm, you can set forceSSL but not addSSL right? That would be wrong of course, but why not make addSSL be true if forceSSL is true? Or assert it at least?

There is an assert:

assertion = all (conf: with conf; forceSSL -> addSSL) (attrValues virtualHosts); message = '' Option services.nginx.virtualHosts.<name>.forceSSL requires services.nginx.virtualHosts.<name>.addSSL set to true. '';

fpletz · 2017-07-26T23:46:48Z

nixos/modules/services/web-servers/nginx/vhost-options.nix

@@ -70,12 +66,24 @@ with lib;
      '';
    };

-    enableSSL = mkOption {
+    addSSL = mkOption {
      type = types.bool;
      default = false;
      description = "Whether to enable SSL (https) support.";


Description proposal:

Whether to enable HTTPS in addition to plain HTTP. This will set defaults for <literal>listen</literal> to listen on all interfaces on the respective default ports (80, 443).

fpletz · 2017-07-26T23:52:10Z

nixos/modules/services/web-servers/nginx/vhost-options.nix

+      visible = false;
+      default = false;
+    };
+
    forceSSL = mkOption {
      type = types.bool;
      default = false;


Description proposal:

Whether to add a separate nginx server block that permanently redirects (301) all plain HTTP traffic to HTTPS. This option needs <literal>addSSL</literal> to be set to true.

fpletz · 2017-07-26T23:52:12Z

nixos/modules/services/web-servers/nginx/vhost-options.nix

+    onlySSL = mkOption {
+      type = types.bool;
+      default = false;
+      description = "Whether to enable SSL (https) support and reject plain (http) connections.";


Description proposal:

Whether to add a separate nginx server block that permanently redirects (301) all plain HTTP traffic to HTTPS. This will set defaults for <literal>listen</literal> to listen on all interfaces on port 443.

I think you mixed up this one.

wmertens · 2017-08-01T18:39:42Z

Missed that :) ok, 👍 👍 👍 from me!

…

On Tue, Aug 1, 2017, 7:26 PM Michele Guerini Rocco ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In nixos/modules/services/web-servers/nginx/default.nix <#27426 (comment)>: > + ";"; + redirectListen = filter (x: !x.ssl) defaultListen; There is an assert: assertion = all (conf: with conf; forceSSL -> addSSL) (attrValues virtualHosts); message = '' Option services.nginx.virtualHosts.<name>.forceSSL requires services.nginx.virtualHosts.<name>.addSSL set to true. ''; — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27426 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWlvsu2EqAAkYf6_wCJMO9umVtp38Aks5sT1_TgaJpZM4OZWMq> .

wmertens · 2017-08-07T14:47:49Z

Merged since no further comments, thanks!

rnhmjoj · 2017-08-07T15:01:08Z

Issues #27338 #25533 should be closed. I'm not sure about #23711.

instead of redeclaring part of the options. Backward-compatible change. This gives the same flexibility to the user as nginx itself. This also resolves the piwik module break from nginx' enableSSL introduction from NixOS#27426.

rnhmjoj force-pushed the nginx branch from 9e40656 to 7ca8fc5 Compare July 17, 2017 10:45

rnhmjoj mentioned this pull request Jul 17, 2017

Nginx module doesn't support vhosts on both regular and ssl #27442

Closed

rnhmjoj force-pushed the nginx branch from 7ca8fc5 to 940640d Compare July 17, 2017 11:30

fpletz added 0.kind: enhancement 8.has: module (update) 9.needs: changelog labels Jul 20, 2017

fpletz added this to the 17.09 milestone Jul 20, 2017

rnhmjoj force-pushed the nginx branch from 940640d to d0c6849 Compare July 22, 2017 16:21

rnhmjoj force-pushed the nginx branch from d0c6849 to d622366 Compare July 26, 2017 20:19

fpletz approved these changes Jul 26, 2017

View reviewed changes

nginx: make enabling SSL port-specific

a912a6a

rnhmjoj force-pushed the nginx branch from e5050fe to a912a6a Compare July 27, 2017 01:46

wmertens merged commit 339330b into NixOS:master Aug 7, 2017

This was referenced Aug 7, 2017

nginx.<vhost>.enableSSL also disables non-SSL HTTP connections, breaks ACME #27338

Closed

services.nginx: Support both HTTP and HTTPS in a single virtualHost #25533

Closed

wmertens mentioned this pull request Aug 7, 2017

enableACME without SSL can result in wrong certificate being served #23711

Closed

florianjacob mentioned this pull request Aug 30, 2017

nixos/piwik: Adjust to recent NixOS changes, use nginx's virtualHost instead of replicating #28749

Merged

8 tasks

rnhmjoj deleted the nginx branch September 12, 2017 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nginx: make enabling SSL port-specific #27426

nginx: make enabling SSL port-specific #27426

rnhmjoj commented Jul 16, 2017 •

edited

mention-bot commented Jul 16, 2017

wmertens commented Jul 17, 2017 •

edited

rnhmjoj commented Jul 17, 2017 •

edited

rnhmjoj commented Jul 17, 2017

globin commented Jul 17, 2017

wmertens commented Jul 19, 2017

nbp commented Jul 22, 2017 •

edited

rnhmjoj commented Jul 22, 2017

wmertens commented Jul 26, 2017

rnhmjoj commented Jul 26, 2017

wmertens commented Jul 26, 2017 via email

rnhmjoj commented Jul 26, 2017

fpletz left a comment

fpletz Jul 26, 2017

rnhmjoj Jul 27, 2017

rnhmjoj Jul 30, 2017

wmertens Aug 1, 2017

rnhmjoj Aug 1, 2017

fpletz Jul 26, 2017

rnhmjoj Jul 27, 2017

fpletz Jul 26, 2017

fpletz Jul 26, 2017

rnhmjoj Jul 27, 2017

wmertens commented Aug 1, 2017 via email

wmertens commented Aug 7, 2017

rnhmjoj commented Aug 7, 2017

nginx: make enabling SSL port-specific #27426

nginx: make enabling SSL port-specific #27426

Conversation

rnhmjoj commented Jul 16, 2017 • edited

Motivation for this change

Things done

mention-bot commented Jul 16, 2017

wmertens commented Jul 17, 2017 • edited

rnhmjoj commented Jul 17, 2017 • edited

rnhmjoj commented Jul 17, 2017

globin commented Jul 17, 2017

wmertens commented Jul 19, 2017

nbp commented Jul 22, 2017 • edited

rnhmjoj commented Jul 22, 2017

wmertens commented Jul 26, 2017

rnhmjoj commented Jul 26, 2017

wmertens commented Jul 26, 2017 via email

rnhmjoj commented Jul 26, 2017

fpletz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmertens commented Aug 1, 2017 via email

wmertens commented Aug 7, 2017

rnhmjoj commented Aug 7, 2017

rnhmjoj commented Jul 16, 2017 •

edited

wmertens commented Jul 17, 2017 •

edited

rnhmjoj commented Jul 17, 2017 •

edited

nbp commented Jul 22, 2017 •

edited