From 6df278bea84a2c8a66752bf4b591d7f3d4432f89 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 8 Aug 2018 10:07:00 -0700
Subject: [PATCH] In "bin/ssh-auth", cache a structure instead of a flat file
 because paths may change at runtime

Summary:
Fixes T12397. Ref T13164. See PHI801.

Several installs have hit various use cases where the path on disk where Phabricator lives changes at runtime. Currently, `bin/ssh-auth` caches a flat file which includes the path to `bin/ssh-exec`, so this may fall out of date if `phabricator/` moves.

These use cases have varying strengths of legitimacy, but "we're migrating to a new set of hosts and the pool is half old machines and half new machines" seems reasonably compelling and not a problem entirely of one's own making.

Test Plan:
  - Compared output on `master` to output after change, found them byte-for-byte identical.
  - Moved `phabricator/` to `phabricator2/`, ran `bin/ssh-auth`, got updated output.
  - Added a new SSH key, saw it appear in the output.
  - Grepped for `AUTHFILE_CACHEKEY` (no hits).
  - Dropped the cache, verified that the file regenerates cleanly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164, T12397

Differential Revision: https://secure.phabricator.com/D19568
---
 scripts/ssh/ssh-auth.php                      | 89 +++++++++++++------
 .../auth/query/PhabricatorAuthSSHKeyQuery.php |  4 +-
 2 files changed, 64 insertions(+), 29 deletions(-)

diff --git a/scripts/ssh/ssh-auth.php b/scripts/ssh/ssh-auth.php
index b25905668b..3c4f3f2b33 100755
--- a/scripts/ssh/ssh-auth.php
+++ b/scripts/ssh/ssh-auth.php
@@ -2,13 +2,27 @@
 <?php
 
 $root = dirname(dirname(dirname(__FILE__)));
-require_once $root.'/scripts/__init_script__.php';
+require_once $root.'/scripts/init/init-script.php';
+
+// NOTE: We are caching a datastructure rather than the flat key file because
+// the path on disk to "ssh-exec" is arbitrarily mutable at runtime. See T12397.
 
 $cache = PhabricatorCaches::getMutableCache();
-$authfile_key = PhabricatorAuthSSHKeyQuery::AUTHFILE_CACHEKEY;
-$authfile = $cache->getKey($authfile_key);
+$authstruct_key = PhabricatorAuthSSHKeyQuery::AUTHSTRUCT_CACHEKEY;
+$authstruct_raw = $cache->getKey($authstruct_key);
 
-if ($authfile === null) {
+$authstruct = null;
+
+if (strlen($authstruct_raw)) {
+  try {
+    $authstruct = phutil_json_decode($authstruct_raw);
+  } catch (Exception $ex) {
+    // Ignore any issues with the cached data; we'll just rebuild the
+    // structure below.
+  }
+}
+
+if ($authstruct === null) {
   $keys = id(new PhabricatorAuthSSHKeyQuery())
     ->setViewer(PhabricatorUser::getOmnipotentUser())
     ->withIsActive(true)
@@ -19,7 +33,7 @@ if ($authfile === null) {
     exit(1);
   }
 
-  $bin = $root.'/bin/ssh-exec';
+  $key_list = array();
   foreach ($keys as $ssh_key) {
     $key_argv = array();
     $object = $ssh_key->getObject();
@@ -42,18 +56,7 @@ if ($authfile === null) {
     $key_argv[] = '--phabricator-ssh-key';
     $key_argv[] = $ssh_key->getID();
 
-    $cmd = csprintf('%s %Ls', $bin, $key_argv);
-
-    $instance = PhabricatorEnv::getEnvConfig('cluster.instance');
-    if (strlen($instance)) {
-      $cmd = csprintf('PHABRICATOR_INSTANCE=%s %C', $instance, $cmd);
-    }
-
-    // This is additional escaping for the SSH 'command="..."' string.
-    $cmd = addcslashes($cmd, '"\\');
-
     // Strip out newlines and other nonsense from the key type and key body.
-
     $type = $ssh_key->getKeyType();
     $type = preg_replace('@[\x00-\x20]+@', '', $type);
     if (!strlen($type)) {
@@ -66,22 +69,54 @@ if ($authfile === null) {
       continue;
     }
 
-    $options = array(
-      'command="'.$cmd.'"',
-      'no-port-forwarding',
-      'no-X11-forwarding',
-      'no-agent-forwarding',
-      'no-pty',
+    $key_list[] = array(
+      'argv' => $key_argv,
+      'type' => $type,
+      'key' => $key,
     );
-    $options = implode(',', $options);
-
-    $lines[] = $options.' '.$type.' '.$key."\n";
   }
 
-  $authfile = implode('', $lines);
+  $authstruct = array(
+    'keys' => $key_list,
+  );
+
+  $authstruct_raw = phutil_json_encode($authstruct);
   $ttl = phutil_units('24 hours in seconds');
-  $cache->setKey($authfile_key, $authfile, $ttl);
+  $cache->setKey($authstruct_key, $authstruct_raw, $ttl);
 }
 
+$bin = $root.'/bin/ssh-exec';
+$instance = PhabricatorEnv::getEnvConfig('cluster.instance');
+
+$lines = array();
+foreach ($authstruct['keys'] as $key_struct) {
+  $key_argv = $key_struct['argv'];
+  $key = $key_struct['key'];
+  $type = $key_struct['type'];
+
+  $cmd = csprintf('%s %Ls', $bin, $key_argv);
+
+  if (strlen($instance)) {
+    $cmd = csprintf('PHABRICATOR_INSTANCE=%s %C', $instance, $cmd);
+  }
+
+  // This is additional escaping for the SSH 'command="..."' string.
+  $cmd = addcslashes($cmd, '"\\');
+
+  $options = array(
+    'command="'.$cmd.'"',
+    'no-port-forwarding',
+    'no-X11-forwarding',
+    'no-agent-forwarding',
+    'no-pty',
+  );
+  $options = implode(',', $options);
+
+  $lines[] = $options.' '.$type.' '.$key."\n";
+}
+
+$authfile = implode('', $lines);
+
 echo $authfile;
+
 exit(0);
diff --git a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php
index 77b666ea44..d8474085b2 100644
--- a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php
+++ b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php
@@ -3,7 +3,7 @@
 final class PhabricatorAuthSSHKeyQuery
   extends PhabricatorCursorPagedPolicyAwareQuery {
 
-  const AUTHFILE_CACHEKEY = 'ssh.authfile';
+  const AUTHSTRUCT_CACHEKEY = 'ssh.authstruct';
 
   private $ids;
   private $phids;
@@ -13,7 +13,7 @@ final class PhabricatorAuthSSHKeyQuery
 
   public static function deleteSSHKeyCache() {
     $cache = PhabricatorCaches::getMutableCache();
-    $authfile_key = self::AUTHFILE_CACHEKEY;
+    $authfile_key = self::AUTHSTRUCT_CACHEKEY;
     $cache->deleteKey($authfile_key);
   }