Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
May 1, 2018

Conversation

jflanglois
Copy link
Contributor

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update the PR when I'm home.

'';
postStop = ''
rm -rf ${cfg.ocspStaplingDir}
'';
Copy link
Member

@Mic92 Mic92 Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Mic92
Copy link
Member

Mic92 commented Apr 25, 2018

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

description = "PEM files to use";
};

ocspStaplingDir = mkOption {
Copy link
Member

@Mic92 Mic92 Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Will update later.

};

frontend = mkOption {
type = types.either types.str (types.listOf types.str);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you want to change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a couple lines down, the apply.

description = "PEM files to use";
};

ocspStaplingDir = mkOption {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Will update later.

@jflanglois jflanglois closed this Apr 29, 2018
@jflanglois jflanglois deleted the new-package-hitch branch April 29, 2018 18:12
@jflanglois jflanglois restored the new-package-hitch branch April 29, 2018 18:12
@jflanglois jflanglois reopened this Apr 29, 2018
@jflanglois
Copy link
Contributor Author

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

nativeBuildInputs = [ pkgconfig ];
buildInputs = [ docutils libev openssl ];

outputs = [ "bin" "out" "doc" "man" ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest looks ready to merge.

@jflanglois
Copy link
Contributor Author

Removed bin. Thanks for the review!

@Mic92 Mic92 merged commit 519b645 into NixOS:master May 1, 2018
@jflanglois 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
Add the Hitch TLS reverse proxy as an option for TLS termination.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants