From bf17b12daf4339f17e3b1fc168d23572c6a016f3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 7 Nov 2014 15:34:44 -0800 Subject: [PATCH] Standardize SSH key storage Summary: Ref T5833. This fixes a few weird things with this table: - A bunch of columns were nullable for no reason. - We stored an MD5 hash of the key (unusual) but never used it and callers were responsible for manually populating it. - We didn't perform known-key-text lookups by using an index. Test Plan: - Ran migrations. - Faked duplicate keys, saw them clean up correctly. - Added new keys. - Generated new keys. - Used `bin/auth-ssh` and `bin/auth-ssh-key`. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5833 Differential Revision: https://secure.phabricator.com/D10805 --- .../autopatches/20141107.ssh.1.colname.sql | 2 + .../autopatches/20141107.ssh.2.keyhash.sql | 2 + .../autopatches/20141107.ssh.3.keyindex.sql | 2 + .../sql/autopatches/20141107.ssh.4.keymig.php | 50 +++++++++++++++++++ .../autopatches/20141107.ssh.5.indexnull.sql | 2 + .../autopatches/20141107.ssh.6.indexkey.sql | 2 + .../autopatches/20141107.ssh.7.colnull.sql | 23 +++++++++ scripts/ssh/ssh-auth.php | 6 +++ .../auth/query/PhabricatorAuthSSHKeyQuery.php | 9 ++-- .../auth/storage/PhabricatorAuthSSHKey.php | 42 +++++++++------- .../storage/PhabricatorAuthSSHPublicKey.php | 16 ++++++ .../people/storage/PhabricatorUser.php | 2 +- .../panel/PhabricatorSettingsPanelSSHKeys.php | 11 ++-- 13 files changed, 136 insertions(+), 33 deletions(-) create mode 100644 resources/sql/autopatches/20141107.ssh.1.colname.sql create mode 100644 resources/sql/autopatches/20141107.ssh.2.keyhash.sql create mode 100644 resources/sql/autopatches/20141107.ssh.3.keyindex.sql create mode 100644 resources/sql/autopatches/20141107.ssh.4.keymig.php create mode 100644 resources/sql/autopatches/20141107.ssh.5.indexnull.sql create mode 100644 resources/sql/autopatches/20141107.ssh.6.indexkey.sql create mode 100644 resources/sql/autopatches/20141107.ssh.7.colnull.sql diff --git a/resources/sql/autopatches/20141107.ssh.1.colname.sql b/resources/sql/autopatches/20141107.ssh.1.colname.sql new file mode 100644 index 0000000000..af34fd7277 --- /dev/null +++ b/resources/sql/autopatches/20141107.ssh.1.colname.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + CHANGE userPHID objectPHID VARBINARY(64) NOT NULL; diff --git a/resources/sql/autopatches/20141107.ssh.2.keyhash.sql b/resources/sql/autopatches/20141107.ssh.2.keyhash.sql new file mode 100644 index 0000000000..71c11edb16 --- /dev/null +++ b/resources/sql/autopatches/20141107.ssh.2.keyhash.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + DROP COLUMN keyHash; diff --git a/resources/sql/autopatches/20141107.ssh.3.keyindex.sql b/resources/sql/autopatches/20141107.ssh.3.keyindex.sql new file mode 100644 index 0000000000..8df02acfe5 --- /dev/null +++ b/resources/sql/autopatches/20141107.ssh.3.keyindex.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + ADD COLUMN keyIndex BINARY(12); diff --git a/resources/sql/autopatches/20141107.ssh.4.keymig.php b/resources/sql/autopatches/20141107.ssh.4.keymig.php new file mode 100644 index 0000000000..2388a282cc --- /dev/null +++ b/resources/sql/autopatches/20141107.ssh.4.keymig.php @@ -0,0 +1,50 @@ +establishConnection('w'); + +echo "Updating SSH public key indexes...\n"; + +$keys = new LiskMigrationIterator($table); +foreach ($keys as $key) { + $id = $key->getID(); + + echo "Updating key {$id}...\n"; + + try { + $hash = $key->toPublicKey()->getHash(); + } catch (Exception $ex) { + echo "Key has bad format! Removing key.\n"; + queryfx( + $conn_w, + 'DELETE FROM %T WHERE id = %d', + $table->getTableName(), + $id); + continue; + } + + $collision = queryfx_all( + $conn_w, + 'SELECT * FROM %T WHERE keyIndex = %s AND id < %d', + $table->getTableName(), + $hash, + $key->getID()); + if ($collision) { + echo "Key is a duplicate! Removing key.\n"; + queryfx( + $conn_w, + 'DELETE FROM %T WHERE id = %d', + $table->getTableName(), + $id); + continue; + } + + queryfx( + $conn_w, + 'UPDATE %T SET keyIndex = %s WHERE id = %d', + $table->getTableName(), + $hash, + $key->getID()); +} + +echo "Done.\n"; diff --git a/resources/sql/autopatches/20141107.ssh.5.indexnull.sql b/resources/sql/autopatches/20141107.ssh.5.indexnull.sql new file mode 100644 index 0000000000..c011ecc9fc --- /dev/null +++ b/resources/sql/autopatches/20141107.ssh.5.indexnull.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + CHANGE keyIndex keyIndex BINARY(12) NOT NULL; diff --git a/resources/sql/autopatches/20141107.ssh.6.indexkey.sql b/resources/sql/autopatches/20141107.ssh.6.indexkey.sql new file mode 100644 index 0000000000..967c813beb --- /dev/null +++ b/resources/sql/autopatches/20141107.ssh.6.indexkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + ADD UNIQUE KEY `key_unique` (keyIndex); diff --git a/resources/sql/autopatches/20141107.ssh.7.colnull.sql b/resources/sql/autopatches/20141107.ssh.7.colnull.sql new file mode 100644 index 0000000000..3182c2d9c7 --- /dev/null +++ b/resources/sql/autopatches/20141107.ssh.7.colnull.sql @@ -0,0 +1,23 @@ +UPDATE {$NAMESPACE}_auth.auth_sshkey + SET name = '' WHERE name IS NULL; + +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + CHANGE name name VARCHAR(255) COLLATE {$COLLATE_TEXT} NOT NULL; + +UPDATE {$NAMESPACE}_auth.auth_sshkey + SET keyType = '' WHERE keyType IS NULL; + +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + CHANGE keyType keyType VARCHAR(255) COLLATE {$COLLATE_TEXT} NOT NULL; + +UPDATE {$NAMESPACE}_auth.auth_sshkey + SET keyBody = '' WHERE keyBody IS NULL; + +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + CHANGE keyBody keyBody LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL; + +UPDATE {$NAMESPACE}_auth.auth_sshkey + SET keyComment = '' WHERE keyComment IS NULL; + +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + CHANGE keyComment keyComment VARCHAR(255) COLLATE {$COLLATE_TEXT} NOT NULL; diff --git a/scripts/ssh/ssh-auth.php b/scripts/ssh/ssh-auth.php index d84a1dded5..f4f3de9e67 100755 --- a/scripts/ssh/ssh-auth.php +++ b/scripts/ssh/ssh-auth.php @@ -34,9 +34,15 @@ foreach ($keys as $ssh_key) { $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.'"', diff --git a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php index 35f4eca4bb..d2963234f7 100644 --- a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php +++ b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php @@ -73,21 +73,18 @@ final class PhabricatorAuthSSHKeyQuery if ($this->objectPHIDs !== null) { $where[] = qsprintf( $conn_r, - 'userPHID IN (%Ls)', + 'objectPHID IN (%Ls)', $this->objectPHIDs); } if ($this->keys !== null) { - // TODO: This could take advantage of a better key, and the hashing - // scheme for this table is a bit nonstandard and questionable. - $sql = array(); foreach ($this->keys as $key) { $sql[] = qsprintf( $conn_r, - '(keyType = %s AND keyBody = %s)', + '(keyType = %s AND keyIndex = %s)', $key->getType(), - $key->getBody()); + $key->getHash()); } $where[] = implode(' OR ', $sql); } diff --git a/src/applications/auth/storage/PhabricatorAuthSSHKey.php b/src/applications/auth/storage/PhabricatorAuthSSHKey.php index eace711311..c789c5ffd9 100644 --- a/src/applications/auth/storage/PhabricatorAuthSSHKey.php +++ b/src/applications/auth/storage/PhabricatorAuthSSHKey.php @@ -4,43 +4,45 @@ final class PhabricatorAuthSSHKey extends PhabricatorAuthDAO implements PhabricatorPolicyInterface { - protected $userPHID; + protected $objectPHID; protected $name; protected $keyType; + protected $keyIndex; protected $keyBody; - protected $keyHash; - protected $keyComment; + protected $keyComment = ''; private $object = self::ATTACHABLE; - public function getObjectPHID() { - return $this->getUserPHID(); - } - public function getConfiguration() { return array( self::CONFIG_COLUMN_SCHEMA => array( - 'keyHash' => 'bytes32', - 'keyComment' => 'text255?', - - // T6203/NULLABILITY - // These seem like they should not be nullable. - 'name' => 'text255?', - 'keyType' => 'text255?', - 'keyBody' => 'text?', + 'name' => 'text255', + 'keyType' => 'text255', + 'keyIndex' => 'bytes12', + 'keyBody' => 'text', + 'keyComment' => 'text255', ), self::CONFIG_KEY_SCHEMA => array( - 'userPHID' => array( - 'columns' => array('userPHID'), + 'key_object' => array( + 'columns' => array('objectPHID'), ), - 'keyHash' => array( - 'columns' => array('keyHash'), + 'key_unique' => array( + 'columns' => array('keyIndex'), 'unique' => true, ), ), ) + parent::getConfiguration(); } + public function save() { + $this->setKeyIndex($this->toPublicKey()->getHash()); + return parent::save(); + } + + public function toPublicKey() { + return PhabricatorAuthSSHPublicKey::newFromStoredKey($this); + } + public function getEntireKey() { $parts = array( $this->getKeyType(), @@ -60,6 +62,8 @@ final class PhabricatorAuthSSHKey } + + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/auth/storage/PhabricatorAuthSSHPublicKey.php b/src/applications/auth/storage/PhabricatorAuthSSHPublicKey.php index 78325157cb..6ed5ff20df 100644 --- a/src/applications/auth/storage/PhabricatorAuthSSHPublicKey.php +++ b/src/applications/auth/storage/PhabricatorAuthSSHPublicKey.php @@ -13,6 +13,15 @@ final class PhabricatorAuthSSHPublicKey extends Phobject { // } + public static function newFromStoredKey(PhabricatorAuthSSHKey $key) { + $public_key = new PhabricatorAuthSSHPublicKey(); + $public_key->type = $key->getKeyType(); + $public_key->body = $key->getKeyBody(); + $public_key->comment = $key->getKeyComment(); + + return $public_key; + } + public static function newFromRawKey($entire_key) { $entire_key = trim($entire_key); if (!strlen($entire_key)) { @@ -83,4 +92,11 @@ final class PhabricatorAuthSSHPublicKey extends Phobject { return $this->comment; } + public function getHash() { + $body = $this->getBody(); + $body = trim($body); + $body = rtrim($body, '='); + return PhabricatorHash::digestForIndex($body); + } + } diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 2fc686461b..90f9892ec5 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -897,7 +897,7 @@ EOBODY; } $keys = id(new PhabricatorAuthSSHKey())->loadAllWhere( - 'userPHID = %s', + 'objectPHID = %s', $this->getPHID()); foreach ($keys as $key) { $key->delete(); diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php b/src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php index a2bf1bac89..de300477ed 100644 --- a/src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php +++ b/src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php @@ -44,8 +44,7 @@ final class PhabricatorSettingsPanelSSHKeys $this->getPanelURI()); $id = nonempty($edit, $delete); - - if ($id) { + if ($id && (int)$id) { $key = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer($viewer) ->withIDs(array($id)) @@ -59,8 +58,8 @@ final class PhabricatorSettingsPanelSSHKeys return new Aphront404Response(); } } else { - $key = new PhabricatorAuthSSHKey(); - $key->setUserPHID($user->getPHID()); + $key = id(new PhabricatorAuthSSHKey()) + ->setObjectPHID($user->getPHID()); } if ($delete) { @@ -89,7 +88,6 @@ final class PhabricatorSettingsPanelSSHKeys $key->setKeyType($type); $key->setKeyBody($body); - $key->setKeyHash(md5($body)); $key->setKeyComment($comment); $e_key = null; @@ -309,11 +307,10 @@ final class PhabricatorSettingsPanelSSHKeys $body = $public_key->getBody(); $key = id(new PhabricatorAuthSSHKey()) - ->setUserPHID($user->getPHID()) + ->setObjectPHID($user->getPHID()) ->setName('id_rsa_phabricator') ->setKeyType($type) ->setKeyBody($body) - ->setKeyHash(md5($body)) ->setKeyComment(pht('Generated')) ->save();