From b833d4a32d965e6393a63b2c91b46eca2a5030d8 Mon Sep 17 00:00:00 2001 From: Emily Date: Sun, 16 Jul 2023 16:59:43 +0100 Subject: [PATCH 1/2] ssh: use symlinks for `authorizedKeys` options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As explained in the changelog and activation check, the previous implementation had a nasty security bug that made removing a user’s authorized keys effectively a no‐op. --- CHANGELOG | 15 +++++++++++++ modules/programs/ssh/default.nix | 37 ++++++++++++-------------------- modules/system/checks.nix | 23 ++++++++++++++++++++ tests/programs-ssh.nix | 8 +++---- 4 files changed, 56 insertions(+), 27 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index b307cca0..6523ef16 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,18 @@ +2024-06-15 +- SECURITY NOTICE: The previous implementation of the + `users.users..openssh.authorizedKeys.*` options would not delete + authorized keys files when the setting for a given user was removed. + + This means that if you previously stopped managing a user's authorized + SSH keys with nix-darwin, or intended to revoke their access by + removing the option, the previous set of keys could still be used to + log in as that user. + + You can check the /etc/ssh/authorized_keys.d directory to see which + keys were permitted; afterwards, please remove the directory and + re-run activation. The options continue to be supported and will now + correctly permit only the keys in your current system configuration. + 2022-08-24 - Major changes to `homebrew` module `homebrew.cleanup` was renamed to `homebrew.onActivation.cleanup`. diff --git a/modules/programs/ssh/default.nix b/modules/programs/ssh/default.nix index d1a67701..6f72369a 100644 --- a/modules/programs/ssh/default.nix +++ b/modules/programs/ssh/default.nix @@ -3,7 +3,7 @@ with lib; let - cfg = config.programs.ssh; + cfg = config.programs.ssh; knownHosts = map (h: getAttr h cfg.knownHosts) (attrNames cfg.knownHosts); @@ -81,8 +81,7 @@ let }; authKeysFiles = let - mkAuthKeyFile = u: nameValuePair "ssh/authorized_keys.d/${u.name}" { - copy = true; + mkAuthKeyFile = u: nameValuePair "ssh/nix_authorized_keys.d/${u.name}" { text = '' ${concatStringsSep "\n" u.openssh.authorizedKeys.keys} ${concatMapStrings (f: readFile f + "\n") u.openssh.authorizedKeys.keyFiles} @@ -97,28 +96,16 @@ let in { + imports = [ + (mkRemovedOptionModule [ "services" "openssh" "authorizedKeysFiles" ] "No `nix-darwin` equivalent to this NixOS option.") + ]; + options = { users.users = mkOption { type = with types; attrsOf (submodule userOptions); }; - services.openssh.authorizedKeysFiles = mkOption { - type = types.listOf types.str; - default = []; - description = '' - Specify the rules for which files to read on the host. - - This is an advanced option. If you're looking to configure user - keys, you can generally use [](#opt-users.users._name_.openssh.authorizedKeys.keys) - or [](#opt-users.users._name_.openssh.authorizedKeys.keyFiles). - - These are paths relative to the host root file system or home - directories and they are subject to certain token expansion rules. - See AuthorizedKeysFile in man sshd_config for details. - ''; - }; - programs.ssh.knownHosts = mkOption { default = {}; type = types.attrsOf (types.submodule host); @@ -148,8 +135,6 @@ in message = "knownHost ${name} must contain either a publicKey or publicKeyFile"; }); - services.openssh.authorizedKeysFiles = [ "%h/.ssh/authorized_keys" "/etc/ssh/authorized_keys.d/%u" ]; - environment.etc = authKeysFiles // { "ssh/ssh_known_hosts" = mkIf (builtins.length knownHosts > 0) { text = (flip (concatMapStringsSep "\n") knownHosts @@ -159,14 +144,20 @@ in )) + "\n"; }; "ssh/sshd_config.d/101-authorized-keys.conf" = { - text = "AuthorizedKeysFile ${toString config.services.openssh.authorizedKeysFiles}\n"; + text = '' + # sshd doesn't like reading from symbolic links, so we cat + # the file ourselves. + AuthorizedKeysCommand /bin/cat /etc/ssh/nix_authorized_keys.d/%u + # Just a simple cat, fine to use _sshd. + AuthorizedKeysCommandUser _sshd + ''; # Allows us to automatically migrate from using a file to a symlink knownSha256Hashes = [ oldAuthorizedKeysHash ]; }; }; - # Clean up .before-nix-darwin file left over from using knownSha256Hashes system.activationScripts.etc.text = '' + # Clean up .before-nix-darwin file left over from using knownSha256Hashes auth_keys_orig=/etc/ssh/sshd_config.d/101-authorized-keys.conf.before-nix-darwin if [ -e "$auth_keys_orig" ] && [ "$(shasum -a 256 $auth_keys_orig | cut -d ' ' -f 1)" = "${oldAuthorizedKeysHash}" ]; then diff --git a/modules/system/checks.nix b/modules/system/checks.nix index f0f03e80..d527aa85 100644 --- a/modules/system/checks.nix +++ b/modules/system/checks.nix @@ -202,6 +202,28 @@ let exit 2 fi ''; + + # TODO: Remove this a couple years down the line when we can assume + # that anyone who cares about security has upgraded. + oldSshAuthorizedKeysDirectory = '' + if [[ -d /etc/ssh/authorized_keys.d ]]; then + printf >&2 '\e[1;31merror: /etc/ssh/authorized_keys.d exists, aborting activation\e[0m\n' + printf >&2 'SECURITY NOTICE: The previous implementation of the\n' + printf >&2 '`users.users..openssh.authorizedKeys.*` options would not delete\n' + printf >&2 'authorized keys files when the setting for a given user was removed.\n' + printf >&2 '\n' + printf >&2 "This means that if you previously stopped managing a user's authorized\n" + printf >&2 'SSH keys with nix-darwin, or intended to revoke their access by\n' + printf >&2 'removing the option, the previous set of keys could still be used to\n' + printf >&2 'log in as that user.\n' + printf >&2 '\n' + printf >&2 'You can check the /etc/ssh/authorized_keys.d directory to see which\n' + printf >&2 'keys were permitted; afterwards, please remove the directory and\n' + printf >&2 're-run activation. The options continue to be supported and will now\n' + printf >&2 'correctly permit only the keys in your current system configuration.\n' + exit 2 + fi + ''; in { @@ -245,6 +267,7 @@ in (mkIf cfg.verifyNixChannels nixChannels) nixInstaller (mkIf cfg.verifyNixPath nixPath) + oldSshAuthorizedKeysDirectory ]; system.activationScripts.checks.text = '' diff --git a/tests/programs-ssh.nix b/tests/programs-ssh.nix index ad4f7ab6..427f71be 100644 --- a/tests/programs-ssh.nix +++ b/tests/programs-ssh.nix @@ -12,9 +12,9 @@ echo >&2 "checking for github.com in /etc/ssh/ssh_known_hosts" grep 'github.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ==' ${config.out}/etc/ssh/ssh_known_hosts - echo >&2 "checking for authorized keys for foo in /etc/ssh/authorized_keys.d/foo" - grep 'ssh-ed25519 AAAA...' ${config.out}/etc/ssh/authorized_keys.d/foo - echo >&2 "checking for authorized keys' path in /etc/ssh/sshd_config.d/101-authorized-keys.conf" - grep 'AuthorizedKeysFile %h/.ssh/authorized_keys /etc/ssh/authorized_keys.d/%u' ${config.out}/etc/ssh/sshd_config.d/101-authorized-keys.conf + echo >&2 "checking for authorized keys for foo in /etc/ssh/nix_authorized_keys.d/foo" + grep 'ssh-ed25519 AAAA...' ${config.out}/etc/ssh/nix_authorized_keys.d/foo + echo >&2 "checking for authorized keys command in /etc/ssh/sshd_config.d/101-authorized-keys.conf" + grep 'AuthorizedKeysCommand /bin/cat /etc/ssh/nix_authorized_keys.d/%u' ${config.out}/etc/ssh/sshd_config.d/101-authorized-keys.conf ''; } From 36a15e8c6c4686be29ccbf0ae0ac1d6133074615 Mon Sep 17 00:00:00 2001 From: Emily Date: Sun, 16 Jul 2023 17:02:10 +0100 Subject: [PATCH 2/2] write-text: remove support for `copy` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a huge anti‐declarative footgun; `copy` files cannot distinguish if a previous version is managed by nix-darwin, so they can’t check the hash, so they’re prone to destroying data, and copied files are not deleted when they’re removed from the system configuration, which led to a security bug. Nothing else in‐tree was using this functionality, so let’s make sure it doesn’t cause any more bugs. --- modules/lib/write-text.nix | 8 -------- modules/system/etc.nix | 19 +++++-------------- 2 files changed, 5 insertions(+), 22 deletions(-) diff --git a/modules/lib/write-text.nix b/modules/lib/write-text.nix index 2fe02aff..ddf40769 100644 --- a/modules/lib/write-text.nix +++ b/modules/lib/write-text.nix @@ -45,14 +45,6 @@ in ''; }; - copy = mkOption { - type = types.bool; - default = false; - description = '' - Whether this file should be copied instead of symlinking. - ''; - }; - knownSha256Hashes = mkOption { internal = true; type = types.listOf types.str; diff --git a/modules/system/etc.nix b/modules/system/etc.nix index 008fb1c1..bc60bef9 100644 --- a/modules/system/etc.nix +++ b/modules/system/etc.nix @@ -10,7 +10,6 @@ let }; etc = filter (f: f.enable) (attrValues config.environment.etc); - etcCopy = filter (f: f.copy) (attrValues config.environment.etc); in @@ -34,9 +33,10 @@ in '' mkdir -p $out/etc cd $out/etc - ${concatMapStringsSep "\n" (attr: "mkdir -p $(dirname '${attr.target}')") etc} - ${concatMapStringsSep "\n" (attr: "ln -s '${attr.source}' '${attr.target}'") etc} - ${concatMapStringsSep "\n" (attr: "touch '${attr.target}'.copy") etcCopy} + ${concatMapStringsSep "\n" (attr: '' + mkdir -p "$(dirname ${escapeShellArg attr.target})" + ln -s ${escapeShellArgs [ attr.source attr.target ]} + '') etc} ''; system.activationScripts.etcChecks.text = '' @@ -55,10 +55,6 @@ in etcStaticFile=/etc/static/$subPath etcFile=/etc/$subPath - if [[ -e $configFile.copy ]]; then - continue - fi - # We need to check files that exist and aren't already links to # $etcStaticFile for known hashes. if [[ @@ -109,11 +105,6 @@ in mkdir -p "$etcDir" fi - if [[ -e $etcStaticFile.copy ]]; then - cp "$etcStaticFile" "$etcFile" - continue - fi - if [[ -e $etcFile ]]; then if [[ $(readlink -- "$etcFile") == "$etcStaticFile" ]]; then continue @@ -130,7 +121,7 @@ in # Delete stale links into /etc/static. if [[ - $(readlink "$etcFile") == "$etcStaticFile" + $(readlink -- "$etcFile") == "$etcStaticFile" && ! -e $etcStaticFile ]]; then rm "$etcFile"