From 5de6739201d3c5d2b6926bf425e99f363f8fbf63 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. --- doc/manual/nixops.xml | 2 +- doc/manual/overview.xml | 4 +- nix/keys.nix | 48 ++++++------------- 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 +++++- 7 files changed, 45 insertions(+), 44 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/doc/manual/nixops.xml b/doc/manual/nixops.xml index 75d03a431..5467b1045 100644 --- a/doc/manual/nixops.xml +++ b/doc/manual/nixops.xml @@ -1943,7 +1943,7 @@ input. See nixops export for examples. Description This command uploads the keys described in deployment.keys - to remote machines in the /run/keys/ directory. + to remote machines in the /etc/nixops-keys/ directory. Keys are not persisted across reboots by default. diff --git a/doc/manual/overview.xml b/doc/manual/overview.xml index 5170aad7c..c4574eb3e 100644 --- a/doc/manual/overview.xml +++ b/doc/manual/overview.xml @@ -1747,7 +1747,7 @@ deploy the new configuration. } - This will create a file /run/keys/my-secret + This will create a file /etc/nixops-keys/my-secret with the specified contents, ownership, and permissions. @@ -1786,7 +1786,7 @@ deploy the new configuration. after = [ "my-secret-key.service" ]; wants = [ "my-secret-key.service" ]; script = '' - export MY_SECRET=$(cat /run/keys/my-secret) + export MY_SECRET=$(cat /etc/nixops-keys/my-secret) run-my-program ''; }; diff --git a/nix/keys.nix b/nix/keys.nix index b6a1e6b7e..d53a46918 100644 --- a/nix/keys.nix +++ b/nix/keys.nix @@ -11,7 +11,7 @@ let The text the key should contain. So if the key name is password and foobar is set here, the contents of the file - /run/keys/password + /etc/nixops-keys/password will be foobar. ''; }; @@ -62,6 +62,7 @@ let inherit (keyOptionsType) getSubOptions; }; + keyDir = if config.deployment.storeKeysOnMachine then "/var/keys" else "/run/keys"; in { @@ -93,11 +94,11 @@ in description = '' The set of keys to be deployed to the machine. Each attribute maps a key name to a file that can be accessed as - /run/keys/name. + /etc/nixops-keys/name. Thus, { password.text = "foobar"; } causes a - file /run/keys/password to be created + file /etc/nixops-keys/password to be created with contents foobar. The directory - /run/keys is only accessible to root and + /etc/nixops-keys is only accessible to root and the keys group, so keep in mind to add any users that need to have access to a particular key to this group. @@ -115,38 +116,17 @@ 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" ] '' - 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 - '') - } + mkdir -p ${keyDir} -m 0750 + chown root:keys ${keyDir} + ln -sfn ${keyDir} /etc/nixops-keys ${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. '' - [[ -f "/run/keys/${name}" ]] && chown '${value.user}:${value.group}' "/run/keys/${name}" + [[ -f "${keyDir}/${name}" ]] && chown '${value.user}:${value.group}' "${keyDir}/${name}" '' ))} ''; @@ -162,7 +142,7 @@ in serviceConfig.RemainAfterExit = true; script = '' - while ! [ -e /run/keys/done ]; do + while ! [ -e ${keyDir}/done ]; do sleep 0.1 done ''; @@ -178,9 +158,9 @@ in path = [ pkgs.inotifyTools ]; preStart = '' (while read f; do if [ "$f" = "${name}" ]; then break; fi; done \ - < <(inotifywait -qm --format '%f' -e create /run/keys) ) & + < <(inotifywait -qm --format '%f' -e create ${keyDir}) ) & - if [[ -e "/run/keys/${name}" ]]; then + if [[ -e "${keyDir}/${name}" ]]; then echo 'flapped down' kill %1 exit 0 @@ -188,9 +168,9 @@ in wait %1 ''; script = '' - inotifywait -qq -e delete_self "/run/keys/${name}" & + inotifywait -qq -e delete_self "${keyDir}/${name}" & - if [[ ! -e "/run/keys/${name}" ]]; then + if [[ ! -e "${keyDir}/${name}" ]]; then echo 'flapped up' exit 0 fi 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..89daa4950 100644 --- a/tests/functional/test_send_keys_sends_keys.py +++ b/tests/functional/test_send_keys_sends_keys.py @@ -5,18 +5,29 @@ 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 /var/keys/secret.key") + self.check_command("rm -f /var/keys/secret.key") + self.depl.send_keys() + self.check_command("test -f /var/keys/secret.key")