From 5e0f218fe480094a83905d2570413f8fed4b36a0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 20 Nov 2014 17:33:30 -0800 Subject: [PATCH] Allow device SSH keys to be trusted Summary: Ref T6240. Some discussion in that task. In instance/cluster environments, daemons need to make Conduit calls that bypass policy checks. We can't just let anyone add SSH keys with this capability to the web directly, because then an adminstrator could just add a key they own and start signing requests with it, bypassing policy checks. Add a `bin/almanac trust-key --id ` workflow for trusting keys. Only trusted keys can sign requests. Test Plan: - Generated a user key. - Generated a device key. - Trusted a device key. - Untrusted a device key. - Hit the various errors on trust/untrust. - Tried to edit a trusted key. {F236010} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T6240 Differential Revision: https://secure.phabricator.com/D10878 --- .../sql/autopatches/20141119.sshtrust.sql | 2 + src/__phutil_library_map__.php | 8 +- .../AlmanacDeviceViewController.php | 2 + .../AlmanacManagementRegisterWorkflow.php | 64 -------------- .../AlmanacManagementTrustKeyWorkflow.php | 85 +++++++++++++++++++ .../AlmanacManagementUntrustKeyWorkflow.php | 52 ++++++++++++ .../almanac/storage/AlmanacDevice.php | 4 + .../almanac/util/AlmanacConduitUtil.php | 17 ---- .../PhabricatorAuthSSHKeyEditController.php | 16 ++++ ...habricatorAuthSSHKeyGenerateController.php | 6 +- .../PhabricatorSSHPublicKeyInterface.php | 6 ++ .../auth/storage/PhabricatorAuthSSHKey.php | 2 + .../view/PhabricatorAuthSSHKeyTableView.php | 29 +++++++ .../PhabricatorConduitAPIController.php | 9 ++ .../people/storage/PhabricatorUser.php | 4 + 15 files changed, 219 insertions(+), 87 deletions(-) create mode 100644 resources/sql/autopatches/20141119.sshtrust.sql delete mode 100644 src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php create mode 100644 src/applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php create mode 100644 src/applications/almanac/management/AlmanacManagementUntrustKeyWorkflow.php delete mode 100644 src/applications/almanac/util/AlmanacConduitUtil.php diff --git a/resources/sql/autopatches/20141119.sshtrust.sql b/resources/sql/autopatches/20141119.sshtrust.sql new file mode 100644 index 0000000000..a75255b1c8 --- /dev/null +++ b/resources/sql/autopatches/20141119.sshtrust.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + ADD isTrusted BOOL NOT NULL; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index db89a79cab..5f105a5864 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -20,7 +20,6 @@ phutil_register_library_map(array( 'AlmanacBindingTransactionQuery' => 'applications/almanac/query/AlmanacBindingTransactionQuery.php', 'AlmanacBindingViewController' => 'applications/almanac/controller/AlmanacBindingViewController.php', 'AlmanacConduitAPIMethod' => 'applications/almanac/conduit/AlmanacConduitAPIMethod.php', - 'AlmanacConduitUtil' => 'applications/almanac/util/AlmanacConduitUtil.php', 'AlmanacConsoleController' => 'applications/almanac/controller/AlmanacConsoleController.php', 'AlmanacController' => 'applications/almanac/controller/AlmanacController.php', 'AlmanacCoreCustomField' => 'applications/almanac/customfield/AlmanacCoreCustomField.php', @@ -46,7 +45,8 @@ phutil_register_library_map(array( 'AlmanacInterfacePHIDType' => 'applications/almanac/phid/AlmanacInterfacePHIDType.php', 'AlmanacInterfaceQuery' => 'applications/almanac/query/AlmanacInterfaceQuery.php', 'AlmanacInterfaceTableView' => 'applications/almanac/view/AlmanacInterfaceTableView.php', - 'AlmanacManagementRegisterWorkflow' => 'applications/almanac/management/AlmanacManagementRegisterWorkflow.php', + 'AlmanacManagementTrustKeyWorkflow' => 'applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php', + 'AlmanacManagementUntrustKeyWorkflow' => 'applications/almanac/management/AlmanacManagementUntrustKeyWorkflow.php', 'AlmanacManagementWorkflow' => 'applications/almanac/management/AlmanacManagementWorkflow.php', 'AlmanacNames' => 'applications/almanac/util/AlmanacNames.php', 'AlmanacNamesTestCase' => 'applications/almanac/util/__tests__/AlmanacNamesTestCase.php', @@ -3000,7 +3000,6 @@ phutil_register_library_map(array( 'AlmanacBindingTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'AlmanacBindingViewController' => 'AlmanacServiceController', 'AlmanacConduitAPIMethod' => 'ConduitAPIMethod', - 'AlmanacConduitUtil' => 'Phobject', 'AlmanacConsoleController' => 'AlmanacController', 'AlmanacController' => 'PhabricatorController', 'AlmanacCoreCustomField' => array( @@ -3040,7 +3039,8 @@ phutil_register_library_map(array( 'AlmanacInterfacePHIDType' => 'PhabricatorPHIDType', 'AlmanacInterfaceQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'AlmanacInterfaceTableView' => 'AphrontView', - 'AlmanacManagementRegisterWorkflow' => 'AlmanacManagementWorkflow', + 'AlmanacManagementTrustKeyWorkflow' => 'AlmanacManagementWorkflow', + 'AlmanacManagementUntrustKeyWorkflow' => 'AlmanacManagementWorkflow', 'AlmanacManagementWorkflow' => 'PhabricatorManagementWorkflow', 'AlmanacNames' => 'Phobject', 'AlmanacNamesTestCase' => 'PhabricatorTestCase', diff --git a/src/applications/almanac/controller/AlmanacDeviceViewController.php b/src/applications/almanac/controller/AlmanacDeviceViewController.php index 94b7c44b8a..d6ebfa1260 100644 --- a/src/applications/almanac/controller/AlmanacDeviceViewController.php +++ b/src/applications/almanac/controller/AlmanacDeviceViewController.php @@ -161,6 +161,8 @@ final class AlmanacDeviceViewController ->setUser($viewer) ->setKeys($keys) ->setCanEdit($can_edit) + ->setShowID(true) + ->setShowTrusted(true) ->setNoDataString(pht('This device has no associated SSH public keys.')); try { diff --git a/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php b/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php deleted file mode 100644 index f48f3e8325..0000000000 --- a/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php +++ /dev/null @@ -1,64 +0,0 @@ -setName('register') - ->setSynopsis(pht('Register this host for authorized Conduit access.')) - ->setArguments(array()); - } - - public function execute(PhutilArgumentParser $args) { - $console = PhutilConsole::getConsole(); - - if (Filesystem::pathExists(AlmanacConduitUtil::getHostPrivateKeyPath())) { - throw new Exception( - 'This host already has a private key for Conduit access.'); - } - - $pair = PhabricatorSSHKeyGenerator::generateKeypair(); - list($public_key, $private_key) = $pair; - - $host = id(new AlmanacDevice()) - ->setName(php_uname('n')) - ->save(); - - id(new AlmanacProperty()) - ->setObjectPHID($host->getPHID()) - ->setName('conduitPublicOpenSSHKey') - ->setValue($public_key) - ->save(); - - id(new AlmanacProperty()) - ->setObjectPHID($host->getPHID()) - ->setName('conduitPublicOpenSSLKey') - ->setValue($this->convertToOpenSSLPublicKey($public_key)) - ->save(); - - Filesystem::writeFile( - AlmanacConduitUtil::getHostPrivateKeyPath(), - $private_key); - - Filesystem::writeFile( - AlmanacConduitUtil::getHostIDPath(), - $host->getID()); - - $console->writeOut("Registered as device %d.\n", $host->getID()); - } - - private function convertToOpenSSLPublicKey($openssh_public_key) { - $ssh_public_key_file = new TempFile(); - Filesystem::writeFile($ssh_public_key_file, $openssh_public_key); - - list($public_key, $stderr) = id(new ExecFuture( - 'ssh-keygen -e -f %s -m pkcs8', - $ssh_public_key_file))->resolvex(); - - unset($ssh_public_key_file); - - return $public_key; - } - -} diff --git a/src/applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php b/src/applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php new file mode 100644 index 0000000000..952d9c71e5 --- /dev/null +++ b/src/applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php @@ -0,0 +1,85 @@ +setName('trust-key') + ->setSynopsis(pht('Mark a public key as trusted.')) + ->setArguments( + array( + array( + 'name' => 'id', + 'param' => 'id', + 'help' => pht('ID of the key to trust.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $console = PhutilConsole::getConsole(); + + $id = $args->getArg('id'); + if (!$id) { + throw new PhutilArgumentUsageException( + pht('Specify a public key to trust with --id.')); + } + + $key = id(new PhabricatorAuthSSHKeyQuery()) + ->setViewer($this->getViewer()) + ->withIDs(array($id)) + ->executeOne(); + if (!$key) { + throw new PhutilArgumentUsageException( + pht('No public key exists with ID "%s".', $id)); + } + + if ($key->getIsTrusted()) { + throw new PhutilArgumentUsageException( + pht('Public key with ID %s is already trusted.', $id)); + } + + if (!($key->getObject() instanceof AlmanacDevice)) { + throw new PhutilArgumentUsageException( + pht('You can only trust keys associated with Almanac devices.')); + } + + $handle = id(new PhabricatorHandleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs(array($key->getObject()->getPHID())) + ->executeOne(); + + $console->writeOut( + "** %s **\n\n%s\n\n%s\n\n%s", + pht('IMPORTANT!'), + phutil_console_wrap( + pht( + 'Trusting a public key gives anyone holding the corresponding '. + 'private key complete, unrestricted access to all data in '. + 'Phabricator. The private key will be able to sign requests that '. + 'skip policy and security checks.')), + phutil_console_wrap( + pht( + 'This is an advanced feature which should normally be used only '. + 'when building a Phabricator cluster. This feature is very '. + 'dangerous if misused.')), + pht('This key is associated with device "%s".', $handle->getName())); + + $prompt = pht( + 'Really trust this key?'); + if (!phutil_console_confirm($prompt)) { + throw new PhutilArgumentUsageException( + pht('User aborted workflow.')); + } + + $key->setIsTrusted(1); + $key->save(); + + $console->writeOut( + "** %s ** %s\n", + pht('TRUSTED'), + pht('Key %s has been marked as trusted.', $id)); + } + +} diff --git a/src/applications/almanac/management/AlmanacManagementUntrustKeyWorkflow.php b/src/applications/almanac/management/AlmanacManagementUntrustKeyWorkflow.php new file mode 100644 index 0000000000..0c489605f0 --- /dev/null +++ b/src/applications/almanac/management/AlmanacManagementUntrustKeyWorkflow.php @@ -0,0 +1,52 @@ +setName('untrust-key') + ->setSynopsis(pht('Revoke trust of a public key.')) + ->setArguments( + array( + array( + 'name' => 'id', + 'param' => 'id', + 'help' => pht('ID of the key to revoke trust for.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $console = PhutilConsole::getConsole(); + + $id = $args->getArg('id'); + if (!$id) { + throw new PhutilArgumentUsageException( + pht('Specify a public key to revoke trust for with --id.')); + } + + $key = id(new PhabricatorAuthSSHKeyQuery()) + ->setViewer($this->getViewer()) + ->withIDs(array($id)) + ->executeOne(); + if (!$key) { + throw new PhutilArgumentUsageException( + pht('No public key exists with ID "%s".', $id)); + } + + if (!$key->getIsTrusted()) { + throw new PhutilArgumentUsageException( + pht('Public key with ID %s is not trusted.', $id)); + } + + $key->setIsTrusted(0); + $key->save(); + + $console->writeOut( + "** %s ** %s\n", + pht('TRUST REVOKED'), + pht('Trust has been revoked for public key %s.', $id)); + } + +} diff --git a/src/applications/almanac/storage/AlmanacDevice.php b/src/applications/almanac/storage/AlmanacDevice.php index 497091aa4f..9075c1fd7e 100644 --- a/src/applications/almanac/storage/AlmanacDevice.php +++ b/src/applications/almanac/storage/AlmanacDevice.php @@ -170,5 +170,9 @@ final class AlmanacDevice return $this->getURI(); } + public function getSSHKeyDefaultName() { + return $this->getName(); + } + } diff --git a/src/applications/almanac/util/AlmanacConduitUtil.php b/src/applications/almanac/util/AlmanacConduitUtil.php deleted file mode 100644 index 5b55f5fa26..0000000000 --- a/src/applications/almanac/util/AlmanacConduitUtil.php +++ /dev/null @@ -1,17 +0,0 @@ -getObject()->getSSHPublicKeyManagementURI($viewer); + if ($key->getIsTrusted()) { + $id = $key->getID(); + + return $this->newDialog() + ->setTitle(pht('Can Not Edit Trusted Key')) + ->appendParagraph( + pht( + 'This key is trusted. Trusted keys can not be edited. '. + 'Use %s to revoke trust before editing the key.', + phutil_tag( + 'tt', + array(), + "bin/almanac untrust-key --id {$id}"))) + ->addCancelButton($cancel_uri, pht('Okay')); + } + $token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession( $viewer, $request, diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php index a0f1ed3a7d..d055db514a 100644 --- a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php +++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php @@ -19,13 +19,15 @@ final class PhabricatorAuthSSHKeyGenerateController $cancel_uri); if ($request->isFormPost()) { + $default_name = $key->getObject()->getSSHKeyDefaultName(); + $keys = PhabricatorSSHKeyGenerator::generateKeypair(); list($public_key, $private_key) = $keys; $file = PhabricatorFile::buildFromFileDataOrHash( $private_key, array( - 'name' => 'id_rsa_phabricator.key', + 'name' => $default_name.'.key', 'ttl' => time() + (60 * 10), 'viewPolicy' => $viewer->getPHID(), )); @@ -36,7 +38,7 @@ final class PhabricatorAuthSSHKeyGenerateController $body = $public_key->getBody(); $key - ->setName('id_rsa_phabricator') + ->setName($default_name) ->setKeyType($type) ->setKeyBody($body) ->setKeyComment(pht('Generated')) diff --git a/src/applications/auth/sshkey/PhabricatorSSHPublicKeyInterface.php b/src/applications/auth/sshkey/PhabricatorSSHPublicKeyInterface.php index a69c224658..87d862bd62 100644 --- a/src/applications/auth/sshkey/PhabricatorSSHPublicKeyInterface.php +++ b/src/applications/auth/sshkey/PhabricatorSSHPublicKeyInterface.php @@ -11,4 +11,10 @@ interface PhabricatorSSHPublicKeyInterface { */ public function getSSHPublicKeyManagementURI(PhabricatorUser $viewer); + + /** + * Provide a default name for generated SSH keys. + */ + public function getSSHKeyDefaultName(); + } diff --git a/src/applications/auth/storage/PhabricatorAuthSSHKey.php b/src/applications/auth/storage/PhabricatorAuthSSHKey.php index fe04cd733e..0383e4b701 100644 --- a/src/applications/auth/storage/PhabricatorAuthSSHKey.php +++ b/src/applications/auth/storage/PhabricatorAuthSSHKey.php @@ -10,6 +10,7 @@ final class PhabricatorAuthSSHKey protected $keyIndex; protected $keyBody; protected $keyComment = ''; + protected $isTrusted = 0; private $object = self::ATTACHABLE; @@ -21,6 +22,7 @@ final class PhabricatorAuthSSHKey 'keyIndex' => 'bytes12', 'keyBody' => 'text', 'keyComment' => 'text255', + 'isTrusted' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_object' => array( diff --git a/src/applications/auth/view/PhabricatorAuthSSHKeyTableView.php b/src/applications/auth/view/PhabricatorAuthSSHKeyTableView.php index 7dc3024d1c..ee7f5756ae 100644 --- a/src/applications/auth/view/PhabricatorAuthSSHKeyTableView.php +++ b/src/applications/auth/view/PhabricatorAuthSSHKeyTableView.php @@ -5,6 +5,8 @@ final class PhabricatorAuthSSHKeyTableView extends AphrontView { private $keys; private $canEdit; private $noDataString; + private $showTrusted; + private $showID; public function setNoDataString($no_data_string) { $this->noDataString = $no_data_string; @@ -16,6 +18,16 @@ final class PhabricatorAuthSSHKeyTableView extends AphrontView { return $this; } + public function setShowTrusted($show_trusted) { + $this->showTrusted = $show_trusted; + return $this; + } + + public function setShowID($show_id) { + $this->showID = $show_id; + return $this; + } + public function setKeys(array $keys) { assert_instances_of($keys, 'PhabricatorAuthSSHKey'); $this->keys = $keys; @@ -32,9 +44,15 @@ final class PhabricatorAuthSSHKeyTableView extends AphrontView { $delete_class = 'small grey button disabled'; } + $trusted_icon = id(new PHUIIconView()) + ->setIconFont('fa-star blue'); + $untrusted_icon = id(new PHUIIconView()) + ->setIconFont('fa-times grey'); + $rows = array(); foreach ($keys as $key) { $rows[] = array( + $key->getID(), javelin_tag( 'a', array( @@ -42,6 +60,7 @@ final class PhabricatorAuthSSHKeyTableView extends AphrontView { 'sigil' => 'workflow', ), $key->getName()), + $key->getIsTrusted() ? $trusted_icon : $untrusted_icon, $key->getKeyComment(), $key->getKeyType(), phabricator_datetime($key->getDateCreated(), $viewer), @@ -60,15 +79,25 @@ final class PhabricatorAuthSSHKeyTableView extends AphrontView { ->setNoDataString($this->noDataString) ->setHeaders( array( + pht('ID'), pht('Name'), + pht('Trusted'), pht('Comment'), pht('Type'), pht('Added'), null, )) + ->setColumnVisibility( + array( + $this->showID, + true, + $this->showTrusted, + )) ->setColumnClasses( array( + '', 'wide pri', + 'center', '', '', 'right', diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php index ec040690a4..dfacbb8058 100644 --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -268,6 +268,15 @@ final class PhabricatorConduitAPIController if ($object instanceof PhabricatorUser) { $user = $object; } else { + if (!$stored_key->getIsTrusted()) { + return array( + 'ERR-INVALID-AUTH', + pht( + 'The key which signed this request is not trusted. Only '. + 'trusted keys can be used to sign API calls.'), + ); + } + throw new Exception( pht('Not Implemented: Would authenticate Almanac device.')); } diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 2d48d6e65d..a299b94e59 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -943,4 +943,8 @@ EOBODY; } } + public function getSSHKeyDefaultName() { + return 'id_rsa_phabricator'; + } + }