recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

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

epriestley 0308d580 49eb6403

+82 -12
+2
resources/sql/autopatches/20160518.ssh.01.activecol.sql
··· 1 + ALTER TABLE {$NAMESPACE}_auth.auth_sshkey 2 + ADD isActive BOOL;
+2
resources/sql/autopatches/20160518.ssh.02.activeval.sql
··· 1 + UPDATE {$NAMESPACE}_auth.auth_sshkey 2 + SET isActive = 1;
+2
resources/sql/autopatches/20160518.ssh.03.activekey.sql
··· 1 + ALTER TABLE {$NAMESPACE}_auth.auth_sshkey 2 + ADD UNIQUE KEY `key_activeunique` (keyIndex, isActive);
+1
scripts/ssh/ssh-auth-key.php
··· 14 14 $key = id(new PhabricatorAuthSSHKeyQuery()) 15 15 ->setViewer(PhabricatorUser::getOmnipotentUser()) 16 16 ->withKeys(array($public_key)) 17 + ->withIsActive(true) 17 18 ->executeOne(); 18 19 if (!$key) { 19 20 exit(1);
+1
scripts/ssh/ssh-auth.php
··· 6 6 7 7 $keys = id(new PhabricatorAuthSSHKeyQuery()) 8 8 ->setViewer(PhabricatorUser::getOmnipotentUser()) 9 + ->withIsActive(true) 9 10 ->execute(); 10 11 11 12 if (!$keys) {
+1
src/applications/almanac/controller/AlmanacDeviceViewController.php
··· 146 146 $keys = id(new PhabricatorAuthSSHKeyQuery()) 147 147 ->setViewer($viewer) 148 148 ->withObjectPHIDs(array($device_phid)) 149 + ->withIsActive(true) 149 150 ->execute(); 150 151 151 152 $table = id(new PhabricatorAuthSSHKeyTableView())
+1
src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php
··· 141 141 $public_key = id(new PhabricatorAuthSSHKeyQuery()) 142 142 ->setViewer($this->getViewer()) 143 143 ->withKeys(array($key_object)) 144 + ->withIsActive(true) 144 145 ->executeOne(); 145 146 146 147 if (!$public_key) {
+5
src/applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php
··· 35 35 pht('No public key exists with ID "%s".', $id)); 36 36 } 37 37 38 + if (!$key->getIsActive()) { 39 + throw new PhutilArgumentUsageException( 40 + pht('Public key "%s" is not an active key.', $id)); 41 + } 42 + 38 43 if ($key->getIsTrusted()) { 39 44 throw new PhutilArgumentUsageException( 40 45 pht('Public key with ID %s is already trusted.', $id));
+2 -1
src/applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php
··· 28 28 $viewer = $request->getUser(); 29 29 30 30 $query = id(new PhabricatorAuthSSHKeyQuery()) 31 - ->setViewer($viewer); 31 + ->setViewer($viewer) 32 + ->withIsActive(true); 32 33 33 34 $ids = $request->getValue('ids'); 34 35 if ($ids !== null) {
+1 -3
src/applications/auth/controller/PhabricatorAuthSSHKeyController.php
··· 25 25 return null; 26 26 } 27 27 28 - return id(new PhabricatorAuthSSHKey()) 29 - ->setObjectPHID($object_phid) 30 - ->attachObject($object); 28 + return PhabricatorAuthSSHKey::initializeNewSSHKey($viewer, $object); 31 29 } 32 30 33 31 }
+6 -2
src/applications/auth/controller/PhabricatorAuthSSHKeyDeleteController.php
··· 9 9 $key = id(new PhabricatorAuthSSHKeyQuery()) 10 10 ->setViewer($viewer) 11 11 ->withIDs(array($request->getURIData('id'))) 12 + ->withIsActive(true) 12 13 ->requireCapabilities( 13 14 array( 14 15 PhabricatorPolicyCapability::CAN_VIEW, ··· 27 28 $cancel_uri); 28 29 29 30 if ($request->isFormPost()) { 30 - // TODO: It would be nice to write an edge transaction here or something. 31 - $key->delete(); 31 + 32 + // TODO: Convert to transactions. 33 + $key->setIsActive(null); 34 + $key->save(); 35 + 32 36 return id(new AphrontRedirectResponse())->setURI($cancel_uri); 33 37 } 34 38
+1
src/applications/auth/controller/PhabricatorAuthSSHKeyEditController.php
··· 11 11 $key = id(new PhabricatorAuthSSHKeyQuery()) 12 12 ->setViewer($viewer) 13 13 ->withIDs(array($id)) 14 + ->withIsActive(true) 14 15 ->requireCapabilities( 15 16 array( 16 17 PhabricatorPolicyCapability::CAN_VIEW,
+4
src/applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php
··· 32 32 foreach ($handles as $phid => $handle) { 33 33 $key = $objects[$phid]; 34 34 $handle->setName(pht('SSH Key %d', $key->getID())); 35 + 36 + if (!$key->getIsActive()) { 37 + $handle->setClosed(pht('Inactive')); 38 + } 35 39 } 36 40 } 37 41
+19
src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php
··· 7 7 private $phids; 8 8 private $objectPHIDs; 9 9 private $keys; 10 + private $isActive; 10 11 11 12 public function withIDs(array $ids) { 12 13 $this->ids = $ids; ··· 26 27 public function withKeys(array $keys) { 27 28 assert_instances_of($keys, 'PhabricatorAuthSSHPublicKey'); 28 29 $this->keys = $keys; 30 + return $this; 31 + } 32 + 33 + public function withIsActive($active) { 34 + $this->isActive = $active; 29 35 return $this; 30 36 } 31 37 ··· 98 104 $key->getHash()); 99 105 } 100 106 $where[] = implode(' OR ', $sql); 107 + } 108 + 109 + if ($this->isActive !== null) { 110 + if ($this->isActive) { 111 + $where[] = qsprintf( 112 + $conn, 113 + 'isActive = %d', 114 + 1); 115 + } else { 116 + $where[] = qsprintf( 117 + $conn, 118 + 'isActive IS NULL'); 119 + } 101 120 } 102 121 103 122 return $where;
+27 -2
src/applications/auth/storage/PhabricatorAuthSSHKey.php
··· 13 13 protected $keyBody; 14 14 protected $keyComment = ''; 15 15 protected $isTrusted = 0; 16 + protected $isActive; 16 17 17 18 private $object = self::ATTACHABLE; 18 19 20 + public static function initializeNewSSHKey( 21 + PhabricatorUser $viewer, 22 + PhabricatorSSHPublicKeyInterface $object) { 23 + 24 + // You must be able to edit an object to create a new key on it. 25 + PhabricatorPolicyFilter::requireCapability( 26 + $viewer, 27 + $object, 28 + PhabricatorPolicyCapability::CAN_EDIT); 29 + 30 + $object_phid = $object->getPHID(); 31 + 32 + return id(new self()) 33 + ->setIsActive(1) 34 + ->setObjectPHID($object_phid) 35 + ->attachObject($object); 36 + } 37 + 19 38 protected function getConfiguration() { 20 39 return array( 21 40 self::CONFIG_AUX_PHID => true, ··· 26 45 'keyBody' => 'text', 27 46 'keyComment' => 'text255', 28 47 'isTrusted' => 'bool', 48 + 'isActive' => 'bool?', 29 49 ), 30 50 self::CONFIG_KEY_SCHEMA => array( 31 51 'key_object' => array( 32 52 'columns' => array('objectPHID'), 33 53 ), 34 - 'key_unique' => array( 35 - 'columns' => array('keyIndex'), 54 + 'key_active' => array( 55 + 'columns' => array('isActive', 'objectPHID'), 56 + ), 57 + // NOTE: This unique key includes a nullable column, effectively 58 + // constraining uniqueness on active keys only. 59 + 'key_activeunique' => array( 60 + 'columns' => array('keyIndex', 'isActive'), 36 61 'unique' => true, 37 62 ), 38 63 ),
+1
src/applications/conduit/controller/PhabricatorConduitAPIController.php
··· 204 204 $stored_key = id(new PhabricatorAuthSSHKeyQuery()) 205 205 ->setViewer(PhabricatorUser::getOmnipotentUser()) 206 206 ->withKeys(array($public_key)) 207 + ->withIsActive(true) 207 208 ->executeOne(); 208 209 if (!$stored_key) { 209 210 return array(
+5 -4
src/applications/people/storage/PhabricatorUser.php
··· 1291 1291 $profile->delete(); 1292 1292 } 1293 1293 1294 - $keys = id(new PhabricatorAuthSSHKey())->loadAllWhere( 1295 - 'objectPHID = %s', 1296 - $this->getPHID()); 1294 + $keys = id(new PhabricatorAuthSSHKeyQuery()) 1295 + ->setViewer($engine->getViewer()) 1296 + ->withObjectPHIDs(array($this->getPHID())) 1297 + ->execute(); 1297 1298 foreach ($keys as $key) { 1298 - $key->delete(); 1299 + $engine->destroyObject($key); 1299 1300 } 1300 1301 1301 1302 $emails = id(new PhabricatorUserEmail())->loadAllWhere(
+1
src/applications/settings/panel/PhabricatorSSHKeysSettingsPanel.php
··· 33 33 $keys = id(new PhabricatorAuthSSHKeyQuery()) 34 34 ->setViewer($viewer) 35 35 ->withObjectPHIDs(array($user->getPHID())) 36 + ->withIsActive(true) 36 37 ->execute(); 37 38 38 39 $table = id(new PhabricatorAuthSSHKeyTableView())