(hitch + nixos/services/hitch): (init at 1.4.8) #39358

jflanglois · 2018-04-23T07:55:55Z

Motivation for this change

Add the Hitch TLS reverse proxy as an option for TLS termination.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[n/a] Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Mic92 · 2018-04-23T09:23:35Z

nixos/modules/misc/ids.nix

@@ -306,6 +306,7 @@
      monero = 287;
      ceph = 288;
      duplicati = 289;
+      hitch = 290;


it seems to me that hitch does not carry a lot of runtime state and all files are chowned on startup. In that case static uid/gid can be removed and nixos will use dynamic ones.

Sure I don't have a strong opinion about that. I was following Varnish's set up a bit.

I will update the PR when I'm home.

Mic92 · 2018-04-24T09:40:05Z

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

+      '';
+      postStop = ''
+        rm -rf ${cfg.ocspStaplingDir}
+      '';


I simplified the module a bit by making use of systemd's RuntimeDirectory.
What do you think?

diff --git a/nixos/modules/services/web-servers/hitch/default.nix b/nixos/modules/services/web-servers/hitch/default.nix index 54663082be1..87ee69aa7cd 100644 --- a/nixos/modules/services/web-servers/hitch/default.nix +++ b/nixos/modules/services/web-servers/hitch/default.nix @@ -6,7 +6,7 @@ let ("frontend = \"${cfg.frontend}\"") (concatMapStrings (s: "pem-file = \"${s}\"\n") cfg.pem-files) ("ciphers = \"${cfg.ciphers}\"") - ("ocsp-dir = \"${cfg.ocspStaplingDir}\"") + ("ocsp-dir = \"/run/hitch/ocsp-cache\"") "user = \"${cfg.user}\"" "group = \"${cfg.group}\"" cfg.extraConfig @@ -22,7 +22,7 @@ with lib; type = types.str; description = '' The host and port Hitch connects to when receiving - a connection + a connection in the form [HOST]:PORT ''; }; @@ -36,8 +36,8 @@ with lib; type = types.either types.str (types.listOf types.str); default = "[127.0.0.1]:443"; description = '' - This specifies the port and interface (the listen endpoint) that Hitch - binds to when listening for connections. In the form [HOST]:PORT[+CERT] + This specifies the port and interface of the listen endpoint in the + form [HOST]:PORT[+CERT]. ''; }; @@ -47,12 +47,6 @@ with lib; description = "PEM files to use"; }; - ocspStaplingDir = mkOption { - type = types.path; - default = "/var/run/hitch/ocsp-cache"; - description = "The location of the OCSP Stapling cache"; - }; - user = mkOption { type = types.str; default = "hitch"; @@ -80,16 +74,10 @@ with lib; description = "Hitch"; wantedBy = [ "multi-user.target" ]; after = [ "network.target" ]; - preStart = '' - ${pkgs.hitch}/sbin/hitch -t --config ${hitchConfig} - mkdir -p ${cfg.ocspStaplingDir} - chown -R hitch:hitch ${cfg.ocspStaplingDir} - ''; - postStop = '' - rm -rf ${cfg.ocspStaplingDir} - ''; serviceConfig = { + RuntimeDirectory = [ "hitch/ocsp-cache" ]; Type = "forking"; + ExecStartPre = "${pkgs.hitch}/sbin/hitch -t --config ${hitchConfig}"; ExecStart = "${pkgs.hitch}/sbin/hitch --daemon --config ${hitchConfig}"; ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID"; Restart = "always"; @@ -100,10 +88,7 @@ with lib; environment.systemPackages = [ pkgs.hitch ]; - users.extraUsers.hitch = { - group = "hitch"; - }; - + users.extraUsers.hitch.group = "hitch"; users.extraGroups.hitch = {}; }; }

Why do you delete ocsp directory on service stop? This is different from what upstream does.

Hmm. We can do that, but then the runtime directory will be owned by root in this case, no?

If we go that route, then we'd want to specify systemd's User and Group. I'm not sure how hitch handles being run as non-root. I'll try it out.

RuntimeDirectory = [ "hitch" "hitch/ocsp-cache" ]; maybe then?

Ah. you are right, usually the service is also started as non-root in this case.

In theory this could be circumvented by starting the service as the right user and give it ambient capability to bind privileged ports: https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/web-servers/traefik.nix#L104

This might have a security impact because before hitch would not be able to bind new ports after root privileges are dropped. At least you could remove the rm command and update the two option description I modified.

Done. I actually like the RuntimeDirectory/CacheDirectory route but I suppose we can start with this.

Mic92 · 2018-04-25T06:15:49Z

cc @Dridi @daghf if hitch would drop ambient capabilities like cap_net_bind_service it would be feasible to start it without root. There also a number of other service hardening option in systemd that could be implemented, but those are usually better maintained upstream, because without inside knowledge they are hard to maintain https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/web-servers/traefik.nix#L105

Add the Hitch TLS reverse proxy as an option for TLS termination.

Mic92 · 2018-04-26T07:54:16Z

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

+        description = "PEM files to use";
+      };
+
+      ocspStaplingDir = mkOption {


Maybe we can use RuntimeDir in future, but having this option will make taking this route impossible.
Since I think it is not particular useful, I would drop it.

Okay. Will update later.

jflanglois · 2018-04-26T20:06:59Z

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

+      };
+
+      frontend = mkOption {
+        type = types.either types.str (types.listOf types.str);


Before this gets merged I forgot to deal with the either type. I will update later today.

What do you want to change here?

It was a couple lines down, the apply.

jflanglois · 2018-04-26T20:07:59Z

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

+        description = "PEM files to use";
+      };
+
+      ocspStaplingDir = mkOption {


Okay. Will update later.

jflanglois · 2018-04-29T18:13:44Z

Whoops, sorry about the churn. I was doing some housekeeping.

Mic92 · 2018-05-01T08:44:41Z

pkgs/servers/hitch/default.nix

+  nativeBuildInputs = [ pkgconfig ];
+  buildInputs = [ docutils libev openssl ];
+
+  outputs = [ "bin" "out" "doc" "man" ];


out is empty. You can drop bin, so hitch stays in out.

Mic92

The rest looks ready to merge.

jflanglois · 2018-05-01T09:30:48Z

Removed bin. Thanks for the review!

Add the Hitch TLS reverse proxy as an option for TLS termination.

jflanglois force-pushed the new-package-hitch branch 2 times, most recently from b4a549a to a2eb953 Compare April 23, 2018 08:02

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Apr 23, 2018

jflanglois force-pushed the new-package-hitch branch 2 times, most recently from 8a6c6c5 to bcb81e3 Compare April 23, 2018 08:08

GrahamcOfBorg added 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Apr 23, 2018

jflanglois force-pushed the new-package-hitch branch from bcb81e3 to 1f7d0df Compare April 23, 2018 08:09

Mic92 reviewed Apr 23, 2018

View reviewed changes

Mic92 reviewed Apr 24, 2018

View reviewed changes

jflanglois added 3 commits April 25, 2018 22:48

(hitch + nixos/services/hitch): (init at 1.4.8)

aab22cd

Add the Hitch TLS reverse proxy as an option for TLS termination.

Code review: remove static uid and gid.

bb73d04

Code review: Adjust comments and don't remove the ocsp directory.

c1c17aa

jflanglois force-pushed the new-package-hitch branch from 177c75a to c1c17aa Compare April 26, 2018 05:48

Mic92 reviewed Apr 26, 2018

View reviewed changes

jflanglois commented Apr 28, 2018

View reviewed changes

Code review: fix options.

ae8679f

jflanglois force-pushed the new-package-hitch branch from b435d02 to ae8679f Compare April 29, 2018 18:03

jflanglois closed this Apr 29, 2018

jflanglois deleted the new-package-hitch branch April 29, 2018 18:12

jflanglois restored the new-package-hitch branch April 29, 2018 18:12

jflanglois reopened this Apr 29, 2018

Mic92 reviewed May 1, 2018

View reviewed changes

Mic92 approved these changes May 1, 2018

View reviewed changes

Code review: remove bin output.

b41f0a4

Mic92 merged commit 519b645 into NixOS:master May 1, 2018

jflanglois deleted the new-package-hitch branch May 1, 2018 10:50

Synthetica9 pushed a commit to Synthetica9/nixpkgs that referenced this pull request May 3, 2018

hitch: init at 1.4.8 + service + test (NixOS#39358)

aaf3459

Add the Hitch TLS reverse proxy as an option for TLS termination.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(hitch + nixos/services/hitch): (init at 1.4.8) #39358

(hitch + nixos/services/hitch): (init at 1.4.8) #39358

jflanglois commented Apr 23, 2018

Mic92 Apr 23, 2018

jflanglois Apr 23, 2018

jflanglois Apr 23, 2018

Mic92 Apr 24, 2018 •

edited

jflanglois Apr 25, 2018

jflanglois Apr 25, 2018

Mic92 Apr 25, 2018

Mic92 Apr 25, 2018

Mic92 Apr 25, 2018

Mic92 Apr 25, 2018

jflanglois Apr 26, 2018

Mic92 commented Apr 25, 2018

Mic92 Apr 26, 2018 •

edited

jflanglois Apr 26, 2018

jflanglois Apr 26, 2018

Mic92 Apr 30, 2018

jflanglois Apr 30, 2018

jflanglois Apr 26, 2018

jflanglois commented Apr 29, 2018

Mic92 May 1, 2018

Mic92 left a comment

jflanglois commented May 1, 2018

(hitch + nixos/services/hitch): (init at 1.4.8) #39358

(hitch + nixos/services/hitch): (init at 1.4.8) #39358

Conversation

jflanglois commented Apr 23, 2018

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Apr 24, 2018 • edited

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

Mic92 commented Apr 25, 2018

Mic92 Apr 26, 2018 • edited

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

jflanglois commented Apr 29, 2018

Choose a reason for hiding this comment

Mic92 left a comment

Choose a reason for hiding this comment

jflanglois commented May 1, 2018

Mic92 Apr 24, 2018 •

edited

Mic92 Apr 26, 2018 •

edited