Improve setting permissions on /run/keys.

nix/keys.nix

@@ -2,6 +2,68 @@
 
 with pkgs.lib;
 
+let
+  keyOptionsType = types.submodule {
+    options.text = mkOption {
+      example = "super secret stuff";
+      type = types.str;
+      description = ''
+        The text the key should contain. So if the key name is
+        <replaceable>password</replaceable> and <literal>foobar</literal>
+        is set here, the contents of the file
+        <filename>/run/keys/<replaceable>password</replaceable></filename>
+        will be <literal>foobar</literal>.
+      '';
+    };
+
+    options.user = mkOption {
+      default = "root";
+      type = types.str;
+      description = ''
+        The user which will be the owner of the key file.
+      '';
+    };
+
+    options.group = mkOption {
+      default = "root";
+      type = types.str;
+      description = ''
+        The group that will be set for the key file.
+      '';
+    };
+
+    options.permissions = mkOption {
+      default = "0600";
+      example = "0640";
+      type = types.str;
+      description = ''
+        The default permissions to set for the key file, needs to be in the
+        format accepted by <citerefentry><refentrytitle>chmod</refentrytitle>
+        <manvolnum>1</manvolnum></citerefentry>.
+      '';
+    };
+  };
+
+  convertOldKeyType = key: val: let
+    warning = "Using plain strings for `deployment.keys' is"
+            + " deprecated, please use `deployment.keys.${key}.text ="
+            + " \"<value>\"` instead of `deployment.keys.${key} ="
+            + " \"<value>\"`.";
+  in if isString val then builtins.trace warning { text = val; } else val;
+
+  keyType = mkOptionType {
+    name = "string or key options";
+    check = v: isString v || keyOptionsType.check v;
+    merge = loc: defs: let
+      convert = def: def // {
+        value = convertOldKeyType (last loc) def.value;
+      };
+    in keyOptionsType.merge loc (map convert defs);
+    inherit (keyOptionsType) getSubOptions;
+  };
+
+in
+
 {
 
   ###### interface
@@ -12,7 +74,7 @@ with pkgs.lib;
       default = false;
       type = types.bool;
       description = ''
-        If true (default), secret information such as LUKS encryption
+        If true, secret information such as LUKS encryption
         keys or SSL private keys is stored on the root disk of the
         machine, allowing the machine to do unattended reboots.  If
         false, secrets are not stored; NixOps supplies them to the
@@ -24,17 +86,20 @@ with pkgs.lib;
 
     deployment.keys = mkOption {
       default = {};
-      example = { password = "foobar"; };
-      type = types.attrsOf types.str;
+      example = { password.text = "foobar"; };
+      type = types.attrsOf keyType;
+      apply = mapAttrs convertOldKeyType;
+
       description = ''
         The set of keys to be deployed to the machine.  Each attribute
-        maps a key name to a key string.  On the machine, the key can
-        be accessed as
-        <filename>/run/keys/<replaceable>name></replaceable></filename>.
-        Thus, <literal>{ password = "foobar"; }</literal> causes a
+        maps a key name to a file that can be accessed as
+        <filename>/run/keys/<replaceable>name</replaceable></filename>.
+        Thus, <literal>{ password.text = "foobar"; }</literal> causes a
         file <filename>/run/keys/password</filename> to be created
         with contents <literal>foobar</literal>.  The directory
-        <filename>/run/keys</filename> is only accessible to root.
+        <filename>/run/keys</filename> is only accessible to root and
+        the <literal>keys</literal> group.  So keep in mind to add any
+        users that need to have access to a particular key to this group.
       '';
     };
 
@@ -45,17 +110,26 @@ with pkgs.lib;
 
   config = {
 
+    warnings = mkIf config.deployment.storeKeysOnMachine [(
+      "The use of `deployment.storeKeysOnMachine' imposes a security risk " +
+      "because all keys will be put in the Nix store and thus are world-" +
+      "readable. Also, this will have an impact on services like OpenSSH, " +
+      "which require strict permissions to be set on key files, so expect " +
+      "things to break."
+    )];
+
     system.activationScripts.nixops-keys =
       ''
-        mkdir -p /run/keys -m 0700
+        mkdir -p /run/keys -m 0750
+        chown root:keys /run/keys
 
         ${optionalString config.deployment.storeKeysOnMachine
             (concatStrings (mapAttrsToList (name: value:
               let
                 # FIXME: The key file should be marked as private once
                 # https://github.com/NixOS/nix/issues/8 is fixed.
                 keyFile = pkgs.writeText name value;
-              in "ln -sfn ${keyFile} /run/keys/${name}\n")
+              in "ln -sfn ${keyFile.text} /run/keys/${name}\n")
               config.deployment.keys)
             + ''
               # FIXME: delete obsolete keys?

nixops/backends/__init__.py

@@ -19,9 +19,18 @@ def __init__(self, xml):
         self.store_keys_on_machine = xml.find("attrs/attr[@name='storeKeysOnMachine']/bool").get("value") == "true"
         self.ssh_port = int(xml.find("attrs/attr[@name='targetPort']/int").get("value"))
         self.always_activate = xml.find("attrs/attr[@name='alwaysActivate']/bool").get("value") == "true"
-        self.keys = {k.get("name"): k.find("string").get("value") for k in xml.findall("attrs/attr[@name='keys']/attrs/attr")}
         self.owners = [e.get("value") for e in xml.findall("attrs/attr[@name='owners']/list/string")]
 
+        def _extract_key_options(x):
+            opts = {}
+            for key in ('text', 'user', 'group', 'permissions'):
+                elem = x.find("attrs/attr[@name='{0}']/string".format(key))
+                if elem is not None:
+                    opts[key] = elem.get("value")
+            return opts
+
+        self.keys = {k.get("name"): _extract_key_options(k) for k in
+                     xml.findall("attrs/attr[@name='keys']/attrs/attr")}
 
 
 class MachineState(nixops.resources.ResourceState):
@@ -32,7 +41,7 @@ class MachineState(nixops.resources.ResourceState):
     ssh_port = nixops.util.attr_property("targetPort", 22, int)
     public_vpn_key = nixops.util.attr_property("publicVpnKey", None)
     store_keys_on_machine = nixops.util.attr_property("storeKeysOnMachine", True, bool)
-    keys = nixops.util.attr_property("keys", [], 'json')
+    keys = nixops.util.attr_property("keys", {}, 'json')
     owners = nixops.util.attr_property("owners", [], 'json')
 
     # Nix store path of the last global configuration deployed to this
@@ -175,14 +184,22 @@ def reboot_rescue(self, hard=False):
 
     def send_keys(self):
         if self.store_keys_on_machine: return
-        self.run_command("mkdir -m 0700 -p /run/keys")
-        for k, v in self.get_keys().items():
+        self.run_command("mkdir -m 0750 -p /run/keys"
+                         " && chown root:keys /run/keys")
+        for k, opts in self.get_keys().items():
             self.log("uploading key ‘{0}’...".format(k))
             tmp = self.depl.tempdir + "/key-" + self.name
-            f = open(tmp, "w+"); f.write(v); f.close()
-            self.run_command("rm -f /run/keys/" + k)
-            self.upload_file(tmp, "/run/keys/" + k)
-            self.run_command("chmod 600 /run/keys/" + k)
+            f = open(tmp, "w+"); f.write(opts['text']); f.close()
+            outfile = "/run/keys/" + k
+            outfile_esc = "'" + outfile.replace("'", r"'\''") + "'"
+            self.run_command("rm -f " + outfile_esc)
+            self.upload_file(tmp, outfile)
+            chmod = "chmod '{0}' " + outfile_esc
+            chown = "chown '{0}:{1}' " + outfile_esc
+            self.run_command(' && '.join([
+                chown.format(opts['user'], opts['group']),
+                chmod.format(opts['permissions'])
+            ]))
             os.remove(tmp)
         self.run_command("touch /run/keys/done")