From 1d2530a73733b6abe5001488536da37fe19a319d Mon Sep 17 00:00:00 2001 From: Kevin Cox Date: Sun, 16 Apr 2017 18:29:36 +0100 Subject: [PATCH] Stop storing keys in the nix store for storeKeysOnMachine. This patch changes storeKeysOnMachine to work roughly the same way when true as when false. The only difference is that the keys are stored in /var/keys (which is usually a real disk) as opposed to /run/keys (which is RAM). This is an improvement on the previous version of storeKeysOnMachine where the keys were stored in the Nix store and linked from /run/keys. The previous version doesn't allow setting unix permissions on the keys, meaning any process on the server can read all keys. This solution has the downside that rolling back doesn't roll back the keys, however this is consistent with how storeKeysOnMachine=false works so shouldn't be a major concern. Furthermore if someone wants to have the keys rollback with the system they can use builtins.toFile instead of the keys mechanism which works the same way that the previous storeKeysOnMachine=true worked. Additionally a symlink /etc/nixops-keys has been added that will be set to the appropriate key location to make it easier to switch between the options. --- nix/keys.nix | 36 +++++-------------- nixops/backends/__init__.py | 13 ++++--- ...nix => single_machine_secret_key_disk.nix} | 0 .../single_machine_secret_key_ram.nix | 7 ++++ tests/functional/test_send_keys_sends_keys.py | 15 ++++++-- 5 files changed, 36 insertions(+), 35 deletions(-) rename tests/functional/{single_machine_secret_key.nix => single_machine_secret_key_disk.nix} (100%) create mode 100644 tests/functional/single_machine_secret_key_ram.nix diff --git a/nix/keys.nix b/nix/keys.nix index b6a1e6b7e..360b4db24 100644 --- a/nix/keys.nix +++ b/nix/keys.nix @@ -115,41 +115,21 @@ in 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 = stringAfter [ "users" "groups" ] + system.activationScripts.nixops-keys = stringAfter [ "users" "groups" ] ( '' 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.text; - in "ln -sfn ${keyFile} /run/keys/${name}\n") - config.deployment.keys) - + '' - # FIXME: delete obsolete keys? - touch /run/keys/done - '') - } - - ${concatStringsSep "\n" (flip mapAttrsToList config.deployment.keys (name: value: - # Make sure each key has correct ownership, since the configured owning - # user or group may not have existed when first uploaded. - '' + ${concatStrings (flip mapAttrsToList config.deployment.keys (name: value: + optionalString config.deployment.storeKeysOnMachine '' + ln -snf "/var/keys/${name}" "/run/keys/${name}" + '' + '' [[ -f "/run/keys/${name}" ]] && chown '${value.user}:${value.group}' "/run/keys/${name}" '' ))} - ''; + '' + lib.optionalString config.deployment.storeKeysOnMachine '' + ln -snf /var/keys/done /run/keys/done + ''); systemd.services = ( { nixops-keys = diff --git a/nixops/backends/__init__.py b/nixops/backends/__init__.py index 193bd7e8a..f2e08d9b4 100644 --- a/nixops/backends/__init__.py +++ b/nixops/backends/__init__.py @@ -199,6 +199,10 @@ def reboot_rescue(self, hard=False): self.warn("machine ‘{0}’ doesn't have a rescue" " system.".format(self.name)) + @property + def key_dir(self): + return "/var/keys" if self.store_keys_on_machine else "/run/keys" + def send_keys(self): if self.state == self.RESCUE: # Don't send keys when in RESCUE state, because we're most likely @@ -206,14 +210,13 @@ def send_keys(self): # so keys will probably end up being written to DISK instead of # into memory. return - if self.store_keys_on_machine: return - self.run_command("mkdir -m 0750 -p /run/keys" - " && chown root:keys /run/keys") + key_dir = self.key_dir + self.run_command("mkdir -pm 0750 {0} && chown root:keys {0}".format(key_dir)) 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(opts['text']); f.close() - outfile = "/run/keys/" + k + outfile = os.path.join(key_dir, k) outfile_esc = "'" + outfile.replace("'", r"'\''") + "'" self.run_command("rm -f " + outfile_esc) self.upload_file(tmp, outfile) @@ -237,7 +240,7 @@ def send_keys(self): ) ) os.remove(tmp) - self.run_command("touch /run/keys/done") + self.run_command("touch {}/done".format(key_dir)) def get_keys(self): return self.keys diff --git a/tests/functional/single_machine_secret_key.nix b/tests/functional/single_machine_secret_key_disk.nix similarity index 100% rename from tests/functional/single_machine_secret_key.nix rename to tests/functional/single_machine_secret_key_disk.nix diff --git a/tests/functional/single_machine_secret_key_ram.nix b/tests/functional/single_machine_secret_key_ram.nix new file mode 100644 index 000000000..6f0bfc722 --- /dev/null +++ b/tests/functional/single_machine_secret_key_ram.nix @@ -0,0 +1,7 @@ +{ + machine.deployment = { + storeKeysOnMachine = false; + + keys."secret.key" = "12345"; + }; +} diff --git a/tests/functional/test_send_keys_sends_keys.py b/tests/functional/test_send_keys_sends_keys.py index 53d2a1f6c..b46a82714 100644 --- a/tests/functional/test_send_keys_sends_keys.py +++ b/tests/functional/test_send_keys_sends_keys.py @@ -5,16 +5,27 @@ parent_dir = path.dirname(__file__) -secret_key_spec = '%s/single_machine_secret_key.nix' % (parent_dir) +secret_key_ram_spec = '%s/single_machine_secret_key_ram.nix' % (parent_dir) +secret_key_disk_spec = '%s/single_machine_secret_key_disk.nix' % (parent_dir) class TestSendKeysSendsKeys(single_machine_test.SingleMachineTest): _multiprocess_can_split_ = True def setup(self): super(TestSendKeysSendsKeys,self).setup() - self.depl.nix_exprs = self.depl.nix_exprs + [ secret_key_spec ] def run_check(self): + self.depl.nix_exprs = self.depl.nix_exprs + [ secret_key_ram_spec ] + + self.depl.deploy() + self.check_command("test -f /run/keys/secret.key") + self.check_command("rm -f /run/keys/secret.key") + self.depl.send_keys() + self.check_command("test -f /run/keys/secret.key") + + def run_check(self): + self.depl.nix_exprs = self.depl.nix_exprs + [ secret_key_disk_spec ] + self.depl.deploy() self.check_command("test -f /run/keys/secret.key") self.check_command("rm -f /run/keys/secret.key")