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

Author: epriestley

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;

resources/sql/autopatches/20141107.ssh.2.keyhash.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_auth.auth_sshkey
+  DROP COLUMN keyHash;

resources/sql/autopatches/20141107.ssh.3.keyindex.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_auth.auth_sshkey
+  ADD COLUMN keyIndex BINARY(12);

resources/sql/autopatches/20141107.ssh.4.keymig.php

@@ -0,0 +1,50 @@
+<?php
+
+$table = new PhabricatorAuthSSHKey();
+$conn_w = $table->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";

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;

resources/sql/autopatches/20141107.ssh.6.indexkey.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_auth.auth_sshkey
+  ADD UNIQUE KEY `key_unique` (keyIndex);

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;

scripts/ssh/ssh-auth.php

@@ -34,9 +34,15 @@
 
   $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.'"',

src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php

@@ -73,21 +73,18 @@ protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
     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);
     }

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 @@ public function attachObject($object) {
   }
 
 
+
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
 

src/applications/auth/storage/PhabricatorAuthSSHPublicKey.php

@@ -13,6 +13,15 @@ private function __construct() {
     // <internal>
   }
 
+  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 @@ public function getComment() {
     return $this->comment;
   }
 
+  public function getHash() {
+    $body = $this->getBody();
+    $body = trim($body);
+    $body = rtrim($body, '=');
+    return PhabricatorHash::digestForIndex($body);
+  }
+
 }

src/applications/people/storage/PhabricatorUser.php

@@ -897,7 +897,7 @@ public function destroyObjectPermanently(
       }
 
       $keys = id(new PhabricatorAuthSSHKey())->loadAllWhere(
-        'userPHID = %s',
+        'objectPHID = %s',
         $this->getPHID());
       foreach ($keys as $key) {
         $key->delete();

src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php

@@ -44,8 +44,7 @@ public function processRequest(AphrontRequest $request) {
       $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 @@ public function processRequest(AphrontRequest $request) {
         return new Aphront404Response();
       }
     } else {
-      $key = new PhabricatorAuthSSHKey();
-      $key->setUserPHID($user->getPHID());
+      $key = id(new PhabricatorAuthSSHKey())
+        ->setObjectPHID($user->getPHID());
     }
 
     if ($delete) {
@@ -89,7 +88,6 @@ public function processRequest(AphrontRequest $request) {
 
           $key->setKeyType($type);
           $key->setKeyBody($body);
-          $key->setKeyHash(md5($body));
           $key->setKeyComment($comment);
 
           $e_key = null;
@@ -309,11 +307,10 @@ private function processGenerate(AphrontRequest $request) {
       $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();