From 0308d580d7df4d1c4597d9f6f42bf76b3c298c9d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 18 May 2016 09:32:50 -0700 Subject: [PATCH] Deactivate SSH keys instead of destroying them completely Summary: Ref T10917. Currently, when you delete an SSH key, we really truly delete it forever. This isn't very consistent with other applications, but we built this stuff a long time ago before we were as rigorous about retaining data and making it auditable. In partiular, destroying data isn't good for auditing after security issues, since it means we can't show you logs of any changes an attacker might have made to your keys. To prepare to improve this, stop destoying data. This will allow later changes to become transaction-oriented and show normal transaction logs. The tricky part here is that we have a `UNIQUE KEY` on the public key part of the key. Instead, I changed this to `UNIQUE (key, isActive)`, where `isActive` is a nullable boolean column. This works because MySQL does not enforce "unique" if part of the key is `NULL`. So you can't have two rows with `("A", 1)`, but you can have as many rows as you want with `("A", null)`. This lets us keep the "each key may only be active for one user/object" rule without requiring us to delete any data. Test Plan: - Ran schema changes. - Viewed public keys. - Tried to add a duplicate key, got rejected (already associated with another object). - Deleted SSH key. - Verified that the key was no longer actually deleted from the database, just marked inactive (in future changes, I'll update the UI to be more clear about this). - Uploaded a new copy of the same public key, worked fine (no duplicate key rejection). - Tried to upload yet another copy, got rejected. - Generated a new keypair. - Tried to upload a duplicate to an Almanac device, got rejected. - Generated a new pair for a device. - Trusted a device key. - Untrusted a device key. - "Deleted" a device key. - Tried to trust a deleted device key, got "inactive" message. - Ran `bin/ssh-auth`, got good output with unique keys. - Ran `cat ~/.ssh/id_rsa.pub | ./bin/ssh-auth-key`, got good output with one key. - Used `auth.querypublickeys` Conduit method to query keys, got good active keys. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10917 Differential Revision: https://secure.phabricator.com/D15943 --- .../autopatches/20160518.ssh.01.activecol.sql | 2 ++ .../autopatches/20160518.ssh.02.activeval.sql | 2 ++ .../autopatches/20160518.ssh.03.activekey.sql | 2 ++ scripts/ssh/ssh-auth-key.php | 1 + scripts/ssh/ssh-auth.php | 1 + .../AlmanacDeviceViewController.php | 1 + .../AlmanacManagementRegisterWorkflow.php | 1 + .../AlmanacManagementTrustKeyWorkflow.php | 5 ++++ ...torAuthQueryPublicKeysConduitAPIMethod.php | 3 +- .../PhabricatorAuthSSHKeyController.php | 4 +-- .../PhabricatorAuthSSHKeyDeleteController.php | 8 +++-- .../PhabricatorAuthSSHKeyEditController.php | 1 + .../phid/PhabricatorAuthSSHKeyPHIDType.php | 4 +++ .../auth/query/PhabricatorAuthSSHKeyQuery.php | 19 ++++++++++++ .../auth/storage/PhabricatorAuthSSHKey.php | 29 +++++++++++++++++-- .../PhabricatorConduitAPIController.php | 1 + .../people/storage/PhabricatorUser.php | 9 +++--- .../panel/PhabricatorSSHKeysSettingsPanel.php | 1 + 18 files changed, 82 insertions(+), 12 deletions(-) create mode 100644 resources/sql/autopatches/20160518.ssh.01.activecol.sql create mode 100644 resources/sql/autopatches/20160518.ssh.02.activeval.sql create mode 100644 resources/sql/autopatches/20160518.ssh.03.activekey.sql diff --git a/resources/sql/autopatches/20160518.ssh.01.activecol.sql b/resources/sql/autopatches/20160518.ssh.01.activecol.sql new file mode 100644 index 0000000000..09c3e16df1 --- /dev/null +++ b/resources/sql/autopatches/20160518.ssh.01.activecol.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + ADD isActive BOOL; diff --git a/resources/sql/autopatches/20160518.ssh.02.activeval.sql b/resources/sql/autopatches/20160518.ssh.02.activeval.sql new file mode 100644 index 0000000000..c70f91492c --- /dev/null +++ b/resources/sql/autopatches/20160518.ssh.02.activeval.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_auth.auth_sshkey + SET isActive = 1; diff --git a/resources/sql/autopatches/20160518.ssh.03.activekey.sql b/resources/sql/autopatches/20160518.ssh.03.activekey.sql new file mode 100644 index 0000000000..a6775edf92 --- /dev/null +++ b/resources/sql/autopatches/20160518.ssh.03.activekey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + ADD UNIQUE KEY `key_activeunique` (keyIndex, isActive); diff --git a/scripts/ssh/ssh-auth-key.php b/scripts/ssh/ssh-auth-key.php index 80c553e563..0c23a20edf 100755 --- a/scripts/ssh/ssh-auth-key.php +++ b/scripts/ssh/ssh-auth-key.php @@ -14,6 +14,7 @@ try { $key = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withKeys(array($public_key)) + ->withIsActive(true) ->executeOne(); if (!$key) { exit(1); diff --git a/scripts/ssh/ssh-auth.php b/scripts/ssh/ssh-auth.php index 5fa5891f49..af6f7f7f43 100755 --- a/scripts/ssh/ssh-auth.php +++ b/scripts/ssh/ssh-auth.php @@ -6,6 +6,7 @@ require_once $root.'/scripts/__init_script__.php'; $keys = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withIsActive(true) ->execute(); if (!$keys) { diff --git a/src/applications/almanac/controller/AlmanacDeviceViewController.php b/src/applications/almanac/controller/AlmanacDeviceViewController.php index 000c8f8971..086ba21087 100644 --- a/src/applications/almanac/controller/AlmanacDeviceViewController.php +++ b/src/applications/almanac/controller/AlmanacDeviceViewController.php @@ -146,6 +146,7 @@ final class AlmanacDeviceViewController $keys = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer($viewer) ->withObjectPHIDs(array($device_phid)) + ->withIsActive(true) ->execute(); $table = id(new PhabricatorAuthSSHKeyTableView()) diff --git a/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php b/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php index 0493068eb4..ebe992469f 100644 --- a/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php +++ b/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php @@ -141,6 +141,7 @@ final class AlmanacManagementRegisterWorkflow $public_key = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer($this->getViewer()) ->withKeys(array($key_object)) + ->withIsActive(true) ->executeOne(); if (!$public_key) { diff --git a/src/applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php b/src/applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php index 81ece51b72..c0bbc59ff0 100644 --- a/src/applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php +++ b/src/applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php @@ -35,6 +35,11 @@ final class AlmanacManagementTrustKeyWorkflow pht('No public key exists with ID "%s".', $id)); } + if (!$key->getIsActive()) { + throw new PhutilArgumentUsageException( + pht('Public key "%s" is not an active key.', $id)); + } + if ($key->getIsTrusted()) { throw new PhutilArgumentUsageException( pht('Public key with ID %s is already trusted.', $id)); diff --git a/src/applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php b/src/applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php index be91af7863..ae7c7f1391 100644 --- a/src/applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php +++ b/src/applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php @@ -28,7 +28,8 @@ final class PhabricatorAuthQueryPublicKeysConduitAPIMethod $viewer = $request->getUser(); $query = id(new PhabricatorAuthSSHKeyQuery()) - ->setViewer($viewer); + ->setViewer($viewer) + ->withIsActive(true); $ids = $request->getValue('ids'); if ($ids !== null) { diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyController.php index 86cf81778f..d529621b33 100644 --- a/src/applications/auth/controller/PhabricatorAuthSSHKeyController.php +++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyController.php @@ -25,9 +25,7 @@ abstract class PhabricatorAuthSSHKeyController return null; } - return id(new PhabricatorAuthSSHKey()) - ->setObjectPHID($object_phid) - ->attachObject($object); + return PhabricatorAuthSSHKey::initializeNewSSHKey($viewer, $object); } } diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyDeleteController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyDeleteController.php index 6c18e211cc..22d2d029bf 100644 --- a/src/applications/auth/controller/PhabricatorAuthSSHKeyDeleteController.php +++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyDeleteController.php @@ -9,6 +9,7 @@ final class PhabricatorAuthSSHKeyDeleteController $key = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer($viewer) ->withIDs(array($request->getURIData('id'))) + ->withIsActive(true) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, @@ -27,8 +28,11 @@ final class PhabricatorAuthSSHKeyDeleteController $cancel_uri); if ($request->isFormPost()) { - // TODO: It would be nice to write an edge transaction here or something. - $key->delete(); + + // TODO: Convert to transactions. + $key->setIsActive(null); + $key->save(); + return id(new AphrontRedirectResponse())->setURI($cancel_uri); } diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyEditController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyEditController.php index d09d52cc14..31920696d7 100644 --- a/src/applications/auth/controller/PhabricatorAuthSSHKeyEditController.php +++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyEditController.php @@ -11,6 +11,7 @@ final class PhabricatorAuthSSHKeyEditController $key = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer($viewer) ->withIDs(array($id)) + ->withIsActive(true) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, diff --git a/src/applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php b/src/applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php index 10ab2fdfb6..3672861cdc 100644 --- a/src/applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php +++ b/src/applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php @@ -32,6 +32,10 @@ final class PhabricatorAuthSSHKeyPHIDType foreach ($handles as $phid => $handle) { $key = $objects[$phid]; $handle->setName(pht('SSH Key %d', $key->getID())); + + if (!$key->getIsActive()) { + $handle->setClosed(pht('Inactive')); + } } } diff --git a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php index f68969d0e8..4592d794fa 100644 --- a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php +++ b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php @@ -7,6 +7,7 @@ final class PhabricatorAuthSSHKeyQuery private $phids; private $objectPHIDs; private $keys; + private $isActive; public function withIDs(array $ids) { $this->ids = $ids; @@ -29,6 +30,11 @@ final class PhabricatorAuthSSHKeyQuery return $this; } + public function withIsActive($active) { + $this->isActive = $active; + return $this; + } + public function newResultObject() { return new PhabricatorAuthSSHKey(); } @@ -100,6 +106,19 @@ final class PhabricatorAuthSSHKeyQuery $where[] = implode(' OR ', $sql); } + if ($this->isActive !== null) { + if ($this->isActive) { + $where[] = qsprintf( + $conn, + 'isActive = %d', + 1); + } else { + $where[] = qsprintf( + $conn, + 'isActive IS NULL'); + } + } + return $where; } diff --git a/src/applications/auth/storage/PhabricatorAuthSSHKey.php b/src/applications/auth/storage/PhabricatorAuthSSHKey.php index 3e77c1bca1..aae7c8b238 100644 --- a/src/applications/auth/storage/PhabricatorAuthSSHKey.php +++ b/src/applications/auth/storage/PhabricatorAuthSSHKey.php @@ -13,9 +13,28 @@ final class PhabricatorAuthSSHKey protected $keyBody; protected $keyComment = ''; protected $isTrusted = 0; + protected $isActive; private $object = self::ATTACHABLE; + public static function initializeNewSSHKey( + PhabricatorUser $viewer, + PhabricatorSSHPublicKeyInterface $object) { + + // You must be able to edit an object to create a new key on it. + PhabricatorPolicyFilter::requireCapability( + $viewer, + $object, + PhabricatorPolicyCapability::CAN_EDIT); + + $object_phid = $object->getPHID(); + + return id(new self()) + ->setIsActive(1) + ->setObjectPHID($object_phid) + ->attachObject($object); + } + protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -26,13 +45,19 @@ final class PhabricatorAuthSSHKey 'keyBody' => 'text', 'keyComment' => 'text255', 'isTrusted' => 'bool', + 'isActive' => 'bool?', ), self::CONFIG_KEY_SCHEMA => array( 'key_object' => array( 'columns' => array('objectPHID'), ), - 'key_unique' => array( - 'columns' => array('keyIndex'), + 'key_active' => array( + 'columns' => array('isActive', 'objectPHID'), + ), + // NOTE: This unique key includes a nullable column, effectively + // constraining uniqueness on active keys only. + 'key_activeunique' => array( + 'columns' => array('keyIndex', 'isActive'), 'unique' => true, ), ), diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php index c4f3e65c35..690f6cc1da 100644 --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -204,6 +204,7 @@ final class PhabricatorConduitAPIController $stored_key = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withKeys(array($public_key)) + ->withIsActive(true) ->executeOne(); if (!$stored_key) { return array( diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 2c9202ef67..b504936825 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -1291,11 +1291,12 @@ final class PhabricatorUser $profile->delete(); } - $keys = id(new PhabricatorAuthSSHKey())->loadAllWhere( - 'objectPHID = %s', - $this->getPHID()); + $keys = id(new PhabricatorAuthSSHKeyQuery()) + ->setViewer($engine->getViewer()) + ->withObjectPHIDs(array($this->getPHID())) + ->execute(); foreach ($keys as $key) { - $key->delete(); + $engine->destroyObject($key); } $emails = id(new PhabricatorUserEmail())->loadAllWhere( diff --git a/src/applications/settings/panel/PhabricatorSSHKeysSettingsPanel.php b/src/applications/settings/panel/PhabricatorSSHKeysSettingsPanel.php index 0faf620041..d97b9c9002 100644 --- a/src/applications/settings/panel/PhabricatorSSHKeysSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorSSHKeysSettingsPanel.php @@ -33,6 +33,7 @@ final class PhabricatorSSHKeysSettingsPanel extends PhabricatorSettingsPanel { $keys = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer($viewer) ->withObjectPHIDs(array($user->getPHID())) + ->withIsActive(true) ->execute(); $table = id(new PhabricatorAuthSSHKeyTableView()) -- 2.51.2