From 1b24b486f58afaba7d254773f536f18e40381f8b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 23 Apr 2018 13:03:59 -0700 Subject: [PATCH] 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 --- .../20180423.mail.01.properties.sql | 8 ++ src/__phutil_library_map__.php | 9 ++ .../auth/storage/PhabricatorAuthSSHKey.php | 6 -- ...ilPropertiesDestructionEngineExtension.php | 28 ++++++ ...catorMailManagementReceiveTestWorkflow.php | 4 +- .../PhabricatorMetaMTAMailPropertiesQuery.php | 51 +++++++++++ .../PhabricatorObjectMailReceiver.php | 3 +- .../PhabricatorObjectMailReceiverTestCase.php | 5 +- .../PhabricatorMailReplyHandler.php | 10 +- .../PhabricatorMetaMTAMailProperties.php | 91 +++++++++++++++++++ 10 files changed, 204 insertions(+), 11 deletions(-) create mode 100644 resources/sql/autopatches/20180423.mail.01.properties.sql create mode 100644 src/applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php create mode 100644 src/applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php create mode 100644 src/applications/metamta/storage/PhabricatorMetaMTAMailProperties.php diff --git a/resources/sql/autopatches/20180423.mail.01.properties.sql b/resources/sql/autopatches/20180423.mail.01.properties.sql new file mode 100644 index 0000000000..d4fc008023 --- /dev/null +++ b/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}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index beec5058a6..4ab77d5207 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/auth/storage/PhabricatorAuthSSHKey.php b/src/applications/auth/storage/PhabricatorAuthSSHKey.php index 2a9a6273bf..5bbb7de834 100644 --- a/src/applications/auth/storage/PhabricatorAuthSSHKey.php +++ b/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); } diff --git a/src/applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php b/src/applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php new file mode 100644 index 0000000000..4e9e0fe38f --- /dev/null +++ b/src/applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php @@ -0,0 +1,28 @@ +getPHID(); + $viewer = $engine->getViewer(); + + $properties = id(new PhabricatorMetaMTAMailPropertiesQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($object_phid)) + ->executeOne(); + if ($properties) { + $properties->delete(); + } + } + +} diff --git a/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php index 3cbe2345eb..46c444571b 100644 --- a/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php +++ b/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'; diff --git a/src/applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php new file mode 100644 index 0000000000..e6ec09f4ff --- /dev/null +++ b/src/applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php @@ -0,0 +1,51 @@ +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'; + } + +} diff --git a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php index 5985458ab4..801f7a8e6f 100644 --- a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php +++ b/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( diff --git a/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php b/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php index 2c2e0561cf..a697436d21 100644 --- a/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php +++ b/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()); } diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index f8dd784e3b..70b524b52f 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/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(); diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMailProperties.php b/src/applications/metamta/storage/PhabricatorMetaMTAMailProperties.php new file mode 100644 index 0000000000..eb454b650e --- /dev/null +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMailProperties.php @@ -0,0 +1,91 @@ + 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; + } + +}