Manage object mailKeys automatically in Mail instead of storing them on objects

Summary:
Ref T13065. `mailKey`s are a private secret for each object. In some mail configurations, they help us ensure that inbound mail is authentic: when we send you mail, the "Reply-To" is "T123+456+abcdef".

  - The `T123` is the object you're actually replying to.
  - The `456` is your user ID.
  - The `abcdef` is a hash of your user account with the `mailKey`.

Knowing this hash effectively proves that Phabricator has sent you mail about the object before, i.e. that you legitimately control the account you're sending from. Without this, anyone could send mail to any object "From" someone else, and have comments post under their username.

To generate this hash, we need a stable secret per object. (We can't use properties like the PHID because the secret has to be legitimately secret.)

Today, we store these in `mailKey` properties on the actual objects, and manually generate them. This results in tons and tons and tons of copies of this same ~10 lines of code.

Instead, just store them in the Mail application and generate them on demand. This change also anticipates possibly adding flags like "must encrypt" and "original subject", which are other "durable metadata about mail transmission" properties we may have use cases for eventually.

Test Plan:
  - See next change for additional testing and context.
  - Sent mail about Herald rules (next change); saw mail keys generate cleanly.
  - Destroyed a Herald rule with a mail key, saw the mail properties get nuked.
  - Grepped for `getMailKey()` and converted all callsites I could which aren't the copy/pasted boilerplate present in 50 places.
  - Used `bin/mail receive-test --to T123` to test normal mail receipt of older-style objects and make sure that wasn't broken.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13065

Differential Revision: https://secure.phabricator.com/D19399

resources/sql/autopatches/20180423.mail.01.properties.sql

@@ -0,0 +1,8 @@
+CREATE TABLE {$NAMESPACE}_metamta.metamta_mailproperties (
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  objectPHID VARBINARY(64) NOT NULL,
+  mailProperties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT},
+  dateCreated INT UNSIGNED NOT NULL,
+  dateModified INT UNSIGNED NOT NULL,
+  UNIQUE KEY `key_object` (objectPHID)
+) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};

src/__phutil_library_map__.php

@@ -3346,6 +3346,7 @@ phutil_register_library_map(array(
     'PhabricatorMailOutboundRoutingSelfEmailHeraldAction' => 'applications/metamta/herald/PhabricatorMailOutboundRoutingSelfEmailHeraldAction.php',
     'PhabricatorMailOutboundRoutingSelfNotificationHeraldAction' => 'applications/metamta/herald/PhabricatorMailOutboundRoutingSelfNotificationHeraldAction.php',
     'PhabricatorMailOutboundStatus' => 'applications/metamta/constants/PhabricatorMailOutboundStatus.php',
+    'PhabricatorMailPropertiesDestructionEngineExtension' => 'applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php',
     'PhabricatorMailReceiver' => 'applications/metamta/receiver/PhabricatorMailReceiver.php',
     'PhabricatorMailReceiverTestCase' => 'applications/metamta/receiver/__tests__/PhabricatorMailReceiverTestCase.php',
     'PhabricatorMailReplyHandler' => 'applications/metamta/replyhandler/PhabricatorMailReplyHandler.php',
@@ -3403,6 +3404,8 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTAMailHasRecipientEdgeType' => 'applications/metamta/edge/PhabricatorMetaMTAMailHasRecipientEdgeType.php',
     'PhabricatorMetaMTAMailListController' => 'applications/metamta/controller/PhabricatorMetaMTAMailListController.php',
     'PhabricatorMetaMTAMailPHIDType' => 'applications/metamta/phid/PhabricatorMetaMTAMailPHIDType.php',
+    'PhabricatorMetaMTAMailProperties' => 'applications/metamta/storage/PhabricatorMetaMTAMailProperties.php',
+    'PhabricatorMetaMTAMailPropertiesQuery' => 'applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php',
     'PhabricatorMetaMTAMailQuery' => 'applications/metamta/query/PhabricatorMetaMTAMailQuery.php',
     'PhabricatorMetaMTAMailSearchEngine' => 'applications/metamta/query/PhabricatorMetaMTAMailSearchEngine.php',
     'PhabricatorMetaMTAMailSection' => 'applications/metamta/view/PhabricatorMetaMTAMailSection.php',
@@ -9038,6 +9041,7 @@ phutil_register_library_map(array(
     'PhabricatorMailOutboundRoutingSelfEmailHeraldAction' => 'PhabricatorMailOutboundRoutingHeraldAction',
     'PhabricatorMailOutboundRoutingSelfNotificationHeraldAction' => 'PhabricatorMailOutboundRoutingHeraldAction',
     'PhabricatorMailOutboundStatus' => 'Phobject',
+    'PhabricatorMailPropertiesDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension',
     'PhabricatorMailReceiver' => 'Phobject',
     'PhabricatorMailReceiverTestCase' => 'PhabricatorTestCase',
     'PhabricatorMailReplyHandler' => 'Phobject',
@@ -9106,6 +9110,11 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTAMailHasRecipientEdgeType' => 'PhabricatorEdgeType',
     'PhabricatorMetaMTAMailListController' => 'PhabricatorMetaMTAController',
     'PhabricatorMetaMTAMailPHIDType' => 'PhabricatorPHIDType',
+    'PhabricatorMetaMTAMailProperties' => array(
+      'PhabricatorMetaMTADAO',
+      'PhabricatorPolicyInterface',
+    ),
+    'PhabricatorMetaMTAMailPropertiesQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorMetaMTAMailQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorMetaMTAMailSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'PhabricatorMetaMTAMailSection' => 'Phobject',

src/applications/auth/storage/PhabricatorAuthSSHKey.php

@@ -70,12 +70,6 @@ final class PhabricatorAuthSSHKey
     return parent::save();
   }
 
-  public function getMailKey() {
-    // NOTE: We don't actually receive mail for these objects. It's OK for
-    // the mail key to be predictable until we do.
-    return PhabricatorHash::digestForIndex($this->getPHID());
-  }
-
   public function toPublicKey() {
     return PhabricatorAuthSSHPublicKey::newFromStoredKey($this);
   }

src/applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php

@@ -0,0 +1,28 @@
+<?php
+
+final class PhabricatorMailPropertiesDestructionEngineExtension
+  extends PhabricatorDestructionEngineExtension {
+
+  const EXTENSIONKEY = 'mail.properties';
+
+  public function getExtensionName() {
+    return pht('Mail Properties');
+  }
+
+  public function destroyObject(
+    PhabricatorDestructionEngine $engine,
+    $object) {
+
+    $object_phid = $object->getPHID();
+    $viewer = $engine->getViewer();
+
+    $properties = id(new PhabricatorMetaMTAMailPropertiesQuery())
+      ->setViewer($viewer)
+      ->withObjectPHIDs(array($object_phid))
+      ->executeOne();
+    if ($properties) {
+      $properties->delete();
+    }
+  }
+
+}

src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php

@@ -139,8 +139,10 @@ final class PhabricatorMailManagementReceiveTestWorkflow
         throw new Exception(pht("No such object '%s'!", $to));
       }
 
+      $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($object);
+
       $hash = PhabricatorObjectMailReceiver::computeMailHash(
-        $object->getMailKey(),
+        $mail_key,
         $user->getPHID());
 
       $header_content['to'] = $to.'+'.$user->getID().'+'.$hash.'@test.com';

src/applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php

@@ -0,0 +1,51 @@
+<?php
+
+final class PhabricatorMetaMTAMailPropertiesQuery
+  extends PhabricatorCursorPagedPolicyAwareQuery {
+
+  private $ids;
+  private $objectPHIDs;
+
+  public function withIDs(array $ids) {
+    $this->ids = $ids;
+    return $this;
+  }
+
+  public function withObjectPHIDs(array $object_phids) {
+    $this->objectPHIDs = $object_phids;
+    return $this;
+  }
+
+  public function newResultObject() {
+    return new PhabricatorMetaMTAMailProperties();
+  }
+
+  protected function loadPage() {
+    return $this->loadStandardPage($this->newResultObject());
+  }
+
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
+
+    if ($this->ids !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'id IN (%Ld)',
+        $this->ids);
+    }
+
+    if ($this->objectPHIDs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'objectPHID IN (%Ls)',
+        $this->objectPHIDs);
+    }
+
+    return $where;
+  }
+
+  public function getQueryApplicationClass() {
+    return 'PhabricatorMetaMTAApplication';
+  }
+
+}

src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php

@@ -124,7 +124,8 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
       $check_phid = $sender->getPHID();
     }
 
-    $expect_hash = self::computeMailHash($object->getMailKey(), $check_phid);
+    $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($object);
+    $expect_hash = self::computeMailHash($mail_key, $check_phid);
 
     if (!phutil_hashes_are_identical($expect_hash, $parts['hash'])) {
       throw new PhabricatorMetaMTAReceivedMailProcessingException(

src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php

@@ -98,8 +98,11 @@ final class PhabricatorObjectMailReceiverTestCase
     if ($is_bad_hash) {
       $hash = PhabricatorObjectMailReceiver::computeMailHash('x', 'y');
     } else {
+
+      $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($task);
+
       $hash = PhabricatorObjectMailReceiver::computeMailHash(
-        $task->getMailKey(),
+        $mail_key,
         $is_public ? $task->getPHID() : $user->getPHID());
     }
 

src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php

@@ -136,8 +136,11 @@ abstract class PhabricatorMailReplyHandler extends Phobject {
     // We compute a hash using the object's own PHID to prevent an attacker
     // from blindly interacting with objects that they haven't ever received
     // mail about by just sending to D1@, D2@, etc...
+
+    $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($receiver);
+
     $hash = PhabricatorObjectMailReceiver::computeMailHash(
-      $receiver->getMailKey(),
+      $mail_key,
       $receiver->getPHID());
 
     $address = "{$prefix}{$receiver_id}+public+{$hash}@{$domain}";
@@ -159,8 +162,11 @@ abstract class PhabricatorMailReplyHandler extends Phobject {
     $receiver = $this->getMailReceiver();
     $receiver_id = $receiver->getID();
     $user_id = $user->getID();
+
+    $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($receiver);
+
     $hash = PhabricatorObjectMailReceiver::computeMailHash(
-      $receiver->getMailKey(),
+      $mail_key,
       $user->getPHID());
     $domain = $this->getReplyHandlerDomain();
 

src/applications/metamta/storage/PhabricatorMetaMTAMailProperties.php

@@ -0,0 +1,91 @@
+<?php
+
+final class PhabricatorMetaMTAMailProperties
+  extends PhabricatorMetaMTADAO
+  implements PhabricatorPolicyInterface {
+
+  protected $objectPHID;
+  protected $mailProperties = array();
+
+  protected function getConfiguration() {
+    return array(
+      self::CONFIG_SERIALIZATION => array(
+        'mailProperties' => self::SERIALIZATION_JSON,
+      ),
+      self::CONFIG_COLUMN_SCHEMA => array(),
+      self::CONFIG_KEY_SCHEMA => array(
+        'key_object' => array(
+          'columns' => array('objectPHID'),
+          'unique' => true,
+        ),
+      ),
+    ) + parent::getConfiguration();
+  }
+
+  public function getMailProperty($key, $default = null) {
+    return idx($this->mailProperties, $key, $default);
+  }
+
+  public function setMailProperty($key, $value) {
+    $this->mailProperties[$key] = $value;
+    return $this;
+  }
+
+  public static function loadMailKey($object) {
+    // If this is an older object with an onboard "mailKey" property, just
+    // use it.
+    // TODO: We should eventually get rid of these and get rid of this
+    // piece of code.
+    if ($object->hasProperty('mailKey')) {
+      return $object->getMailKey();
+    }
+
+    $viewer = PhabricatorUser::getOmnipotentUser();
+    $object_phid = $object->getPHID();
+
+    $properties = id(new PhabricatorMetaMTAMailPropertiesQuery())
+      ->setViewer($viewer)
+      ->withObjectPHIDs(array($object_phid))
+      ->executeOne();
+    if (!$properties) {
+      $properties = id(new self())
+        ->setObjectPHID($object_phid);
+    }
+
+    $mail_key = $properties->getMailProperty('mailKey');
+    if ($mail_key !== null) {
+      return $mail_key;
+    }
+
+    $mail_key = Filesystem::readRandomCharacters(20);
+    $properties->setMailProperty('mailKey', $mail_key);
+
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      $properties->save();
+    unset($unguarded);
+
+    return $mail_key;
+  }
+
+
+/* -(  PhabricatorPolicyInterface  )----------------------------------------- */
+
+
+  public function getCapabilities() {
+    return array(
+      PhabricatorPolicyCapability::CAN_VIEW,
+    );
+  }
+
+  public function getPolicy($capability) {
+    switch ($capability) {
+      case PhabricatorPolicyCapability::CAN_VIEW:
+        return PhabricatorPolicies::POLICY_NOONE;
+    }
+  }
+
+  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
+    return false;
+  }
+
+}