From c21a71f02489b53e5ec5c34deffc693950b69847 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 21 Oct 2016 07:23:58 -0700 Subject: [PATCH] Cache generation of the SSH authentication keyfile for sshd Summary: Ref T11469. This isn't directly related, but has been on my radar for a while: building SSH keyfiles (particular for installs with a lot of keys, like ours) can be fairly slow. At least one cluster instance is making multiple clone requests per second. While that should probably be rate limited separately, caching this should mitigate the impact of these requests. This is pretty straightforward to cache since it's exactly the same every time, and only changes when users modify SSH keys (which is rare). Test Plan: - Ran `bin/auth-ssh`, saw authfile generate. - Ran it again, saw it read from cache. - Changed an SSH key. - Ran it again, saw it regenerate. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11469 Differential Revision: https://secure.phabricator.com/D16744 --- scripts/ssh/ssh-auth.php | 134 ++++++++++-------- .../editor/PhabricatorAuthSSHKeyEditor.php | 14 ++ .../auth/query/PhabricatorAuthSSHKeyQuery.php | 2 + src/applications/cache/PhabricatorCaches.php | 17 +++ .../PhabricatorKeyValueDatabaseCache.php | 2 +- 5 files changed, 106 insertions(+), 63 deletions(-) diff --git a/scripts/ssh/ssh-auth.php b/scripts/ssh/ssh-auth.php index af6f7f7f43..b25905668b 100755 --- a/scripts/ssh/ssh-auth.php +++ b/scripts/ssh/ssh-auth.php @@ -4,74 +4,84 @@ $root = dirname(dirname(dirname(__FILE__))); require_once $root.'/scripts/__init_script__.php'; -$keys = id(new PhabricatorAuthSSHKeyQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withIsActive(true) - ->execute(); +$cache = PhabricatorCaches::getMutableCache(); +$authfile_key = PhabricatorAuthSSHKeyQuery::AUTHFILE_CACHEKEY; +$authfile = $cache->getKey($authfile_key); -if (!$keys) { - echo pht('No keys found.')."\n"; - exit(1); -} +if ($authfile === null) { + $keys = id(new PhabricatorAuthSSHKeyQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withIsActive(true) + ->execute(); -$bin = $root.'/bin/ssh-exec'; -foreach ($keys as $ssh_key) { - $key_argv = array(); - $object = $ssh_key->getObject(); - if ($object instanceof PhabricatorUser) { - $key_argv[] = '--phabricator-ssh-user'; - $key_argv[] = $object->getUsername(); - } else if ($object instanceof AlmanacDevice) { - if (!$ssh_key->getIsTrusted()) { - // If this key is not a trusted device key, don't allow SSH - // authentication. + if (!$keys) { + echo pht('No keys found.')."\n"; + exit(1); + } + + $bin = $root.'/bin/ssh-exec'; + foreach ($keys as $ssh_key) { + $key_argv = array(); + $object = $ssh_key->getObject(); + if ($object instanceof PhabricatorUser) { + $key_argv[] = '--phabricator-ssh-user'; + $key_argv[] = $object->getUsername(); + } else if ($object instanceof AlmanacDevice) { + if (!$ssh_key->getIsTrusted()) { + // If this key is not a trusted device key, don't allow SSH + // authentication. + continue; + } + $key_argv[] = '--phabricator-ssh-device'; + $key_argv[] = $object->getName(); + } else { + // We don't know what sort of key this is; don't permit SSH auth. continue; } - $key_argv[] = '--phabricator-ssh-device'; - $key_argv[] = $object->getName(); - } else { - // We don't know what sort of key this is; don't permit SSH auth. - continue; + + $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)) { + continue; + } + + $key = $ssh_key->getKeyBody(); + $key = preg_replace('@[\x00-\x20]+@', '', $key); + if (!strlen($key)) { + continue; + } + + $options = array( + 'command="'.$cmd.'"', + 'no-port-forwarding', + 'no-X11-forwarding', + 'no-agent-forwarding', + 'no-pty', + ); + $options = implode(',', $options); + + $lines[] = $options.' '.$type.' '.$key."\n"; } - $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)) { - continue; - } - - $key = $ssh_key->getKeyBody(); - $key = preg_replace('@[\x00-\x20]+@', '', $key); - if (!strlen($key)) { - continue; - } - - $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); + $ttl = phutil_units('24 hours in seconds'); + $cache->setKey($authfile_key, $authfile, $ttl); } -echo implode('', $lines); +echo $authfile; exit(0); diff --git a/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php b/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php index 1fc61ffb2c..4d04707598 100644 --- a/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php +++ b/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php @@ -191,6 +191,20 @@ final class PhabricatorAuthSSHKeyEditor return 'ssh-key-'.$object->getPHID(); } + protected function applyFinalEffects( + PhabricatorLiskDAO $object, + array $xactions) { + + // After making any change to an SSH key, drop the authfile cache so it + // is regenerated the next time anyone authenticates. + $cache = PhabricatorCaches::getMutableCache(); + $authfile_key = PhabricatorAuthSSHKeyQuery::AUTHFILE_CACHEKEY; + $cache->deleteKey($authfile_key); + + return $xactions; + } + + protected function getMailTo(PhabricatorLiskDAO $object) { return $object->getObject()->getSSHKeyNotifyPHIDs(); } diff --git a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php index 4592d794fa..6ba047d100 100644 --- a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php +++ b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php @@ -3,6 +3,8 @@ final class PhabricatorAuthSSHKeyQuery extends PhabricatorCursorPagedPolicyAwareQuery { + const AUTHFILE_CACHEKEY = 'ssh.authfile'; + private $ids; private $phids; private $objectPHIDs; diff --git a/src/applications/cache/PhabricatorCaches.php b/src/applications/cache/PhabricatorCaches.php index c9bd304ccf..a725238150 100644 --- a/src/applications/cache/PhabricatorCaches.php +++ b/src/applications/cache/PhabricatorCaches.php @@ -99,6 +99,23 @@ final class PhabricatorCaches extends Phobject { return $caches; } + public static function getMutableCache() { + static $cache; + if (!$cache) { + $caches = self::buildMutableCaches(); + $cache = self::newStackFromCaches($caches); + } + return $cache; + } + + private static function buildMutableCaches() { + $caches = array(); + + $caches[] = new PhabricatorKeyValueDatabaseCache(); + + return $caches; + } + /* -( Repository Graph Cache )--------------------------------------------- */ diff --git a/src/applications/cache/PhabricatorKeyValueDatabaseCache.php b/src/applications/cache/PhabricatorKeyValueDatabaseCache.php index 99060478c0..c6a52024fe 100644 --- a/src/applications/cache/PhabricatorKeyValueDatabaseCache.php +++ b/src/applications/cache/PhabricatorKeyValueDatabaseCache.php @@ -98,7 +98,7 @@ final class PhabricatorKeyValueDatabaseCache $this->establishConnection('w'), 'DELETE FROM %T WHERE cacheKeyHash IN (%Ls)', $this->getTableName(), - $keys); + $map); } return $this;