Merge pull request #976 from emilazy/openssh-use-links-for-authorized…

…-keys ssh: use symlinks for `authorizedKeys` options
LnL7 · Jul 10, 2024 · cf297a8 · cf297a8
2 parents fabc653 + 36a15e8
commit cf297a8
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 49 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,3 +1,18 @@
+2024-06-15
+- SECURITY NOTICE: The previous implementation of the
+  `users.users.<name>.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/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/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
@@ -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.<name>.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/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"

diff --git 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
   '';
 }