Page MenuHomePhabricator
Create Task
Maniphest T13065

Move storage for `mailKey` to the Mail application
Open, LowPublic
Actions

Description

Currently, many objects have a mailKey field, and copy/pasted logic to populate it. This key is used as a seed to generate unique addresses for each <object, user> pair so that I send mail to T123+1+abe2h3f2 and you send mail to T123+2+fo1m213n and I can't just fiddle with my "+1" and impersonate someone else.

This could much more simply be a Mail table which stores <objectPHID, mailKey> and generates new keys lazily on first use.

This would allow us to remove a substantial amount of copy/pasted code:

epriestley@orbital ~/dev/phabricator $ git grep -i mailkey
src/applications/almanac/storage/AlmanacBinding.php:  protected $mailKey;
src/applications/almanac/storage/AlmanacBinding.php:        'mailKey' => 'bytes20',
src/applications/almanac/storage/AlmanacBinding.php:    if (!$this->mailKey) {
src/applications/almanac/storage/AlmanacBinding.php:      $this->mailKey = Filesystem::readRandomCharacters(20);
src/applications/almanac/storage/AlmanacDevice.php:  protected $mailKey;
src/applications/almanac/storage/AlmanacDevice.php:        'mailKey' => 'bytes20',
src/applications/almanac/storage/AlmanacDevice.php:    if (!$this->mailKey) {
src/applications/almanac/storage/AlmanacDevice.php:      $this->mailKey = Filesystem::readRandomCharacters(20);
src/applications/almanac/storage/AlmanacNamespace.php:  protected $mailKey;
src/applications/almanac/storage/AlmanacNamespace.php:        'mailKey' => 'bytes20',
src/applications/almanac/storage/AlmanacNamespace.php:    if (!$this->mailKey) {
src/applications/almanac/storage/AlmanacNamespace.php:      $this->mailKey = Filesystem::readRandomCharacters(20);
src/applications/almanac/storage/AlmanacNetwork.php:  protected $mailKey;
src/applications/almanac/storage/AlmanacNetwork.php:        'mailKey' => 'bytes20',
src/applications/almanac/storage/AlmanacNetwork.php:    if (!$this->mailKey) {
src/applications/almanac/storage/AlmanacNetwork.php:      $this->mailKey = Filesystem::readRandomCharacters(20);
src/applications/almanac/storage/AlmanacService.php:  protected $mailKey;
src/applications/almanac/storage/AlmanacService.php:        'mailKey' => 'bytes20',
src/applications/almanac/storage/AlmanacService.php:    if (!$this->mailKey) {
...

This change needs to keep mail keys stable, so all the existing mail keys should be copied into the new table before they are deleted.

There should be a very small number of readers (2-3?) of this field, so it should be possible to build them safely like this:

  1. If the object is a lisk object with a mailKey property, continue calling getMailKey().
  2. Otherwise, read the new mail key table; use the result if a row exists.
  3. Otherwise, insert a new result into the table; use that.

Then copy and remove all the mailKey properties one by one until we can delete the chunk of code in (1).

Revisions and Commits

Restricted Differential RevisionRestricted Diffusion Commit
rP Phabricator
D21842rPb1533e5468b1 Migrate "SlowvotePoll" to modern mailkey storage
D21632rP86669ab54f97 Modernize "mailKey" for Almanac Networks
D21631rP8226b3c88082 Modernize "mailKey" on Almanac Namespaces
D21634rP3267859aeeb2 Modernize "mailKey" on Fund initiatives
D21633rPeeb009b4fa61 Modernize "mailKey" for Calendar Event
D21630rP7c0e33c34d6f Modernize "mailKey" for Almanac Bindings
D21629rPfadb2bd52b5e Modernize "mailKey" on AlamnacService
D21625rPc3e6db6f0b32 Migrate Almanac Device "mailKey" to modern storage
D19922rP1e2bc7775bc4 Remove the onboard "mailKey" from Pholio Mocks
D19670rP40d5d5c9848e Remove "mailKey" from "PhabricatorRepositoryCommit"
D19620rP50f4adef64cf Remove on-object mailkeys from Phriction
D19427rPdc510354c339 Remove explicit "mailKey" from Owners packages
D19399rP1b24b486f58a Manage object mailKeys automatically in Mail instead of storing them on objects

Related Objects

Event Timeline

epriestley triaged this task as Low priority.Feb 7 2018, 11:42 AM
epriestley created this task.
Herald added a subscriber: eadler. · View Herald TranscriptFeb 7 2018, 11:42 AM
epriestley added a comment.Feb 7 2018, 11:47 AM

If D19012/D19013 turn out badly this table should be <objectPHID, mailKey, firstSubjectWeSentMailWith> so there's some vague argument for waiting until those are in the wild for a little bit or naming the table in a more generic way (MailObjectMetadata instead of MailObjectKey).

epriestley moved this task from Backlog to Far Future on the Mail board.Feb 8 2018, 5:00 PM
epriestley added a comment.Feb 8 2018, 6:02 PM

If we make "Must Encrypt" stateful, that flag should also probably live in this table.

epriestley mentioned this in T13053: Plans: Mail Tags and Failover.Feb 8 2018, 7:35 PM
epriestley mentioned this in T13130: Plans: 2018 Week 17/18 Bonus Content.Apr 23 2018, 3:09 AM
epriestley added a revision: D19399: Manage object mailKeys automatically in Mail instead of storing them on objects.Apr 23 2018, 9:01 PM
amckinley added a subscriber: amckinley.Apr 23 2018, 9:16 PM

There should be a very small number of readers (2-3?) of this field, so it should be possible to blind them safely like this:

I'm guessing that should be "build them safely"?

epriestley added a comment.Apr 23 2018, 9:22 PM

Ah, yeah. No idea why I typed "blind" instead.

epriestley updated the task description. (Show Details)Apr 23 2018, 9:23 PM
epriestley added a commit: rP1b24b486f58a: Manage object mailKeys automatically in Mail instead of storing them on objects.Apr 25 2018, 1:47 PM
epriestley added a revision: D19427: Remove explicit "mailKey" from Owners packages.May 4 2018, 8:44 PM
epriestley added a commit: rPdc510354c339: Remove explicit "mailKey" from Owners packages.May 5 2018, 3:47 PM
amckinley added a revision: Restricted Differential Revision.Jul 20 2018, 7:00 PM
amckinley added a commit: Restricted Diffusion Commit.Jul 20 2018, 7:13 PM
epriestley added a revision: D19620: Remove on-object mailkeys from Phriction.Aug 29 2018, 3:29 PM
epriestley added a commit: rP50f4adef64cf: Remove on-object mailkeys from Phriction.Aug 29 2018, 8:43 PM
epriestley added a revision: D19670: Remove "mailKey" from "PhabricatorRepositoryCommit".Sep 14 2018, 1:27 PM
epriestley added a commit: rP40d5d5c9848e: Remove "mailKey" from "PhabricatorRepositoryCommit".Sep 15 2018, 2:54 PM
epriestley added a revision: D19922: Remove the onboard "mailKey" from Pholio Mocks.Dec 20 2018, 7:56 PM
epriestley added a commit: rP1e2bc7775bc4: Remove the onboard "mailKey" from Pholio Mocks.Dec 20 2018, 11:30 PM
epriestley added a revision: D21625: Migrate Almanac Device "mailKey" to modern storage.Mar 16 2021, 6:11 PM
epriestley added a revision: D21626: Make minor Almanac device modernization updates.Mar 16 2021, 6:18 PM
epriestley removed a revision: D21626: Make minor Almanac device modernization updates.Mar 16 2021, 9:23 PM
epriestley added a revision: D21629: Modernize "mailKey" on AlamnacService.Mar 16 2021, 10:01 PM
epriestley added a revision: D21630: Modernize "mailKey" for Almanac Bindings.Mar 16 2021, 10:05 PM
epriestley added a revision: D21631: Modernize "mailKey" on Almanac Namespaces.Mar 16 2021, 10:08 PM
epriestley added a revision: D21632: Modernize "mailKey" for Almanac Networks.Mar 16 2021, 10:11 PM
epriestley added a revision: D21633: Modernize "mailKey" for Calendar Event.Mar 16 2021, 10:19 PM
epriestley added a revision: D21634: Modernize "mailKey" on Fund initiatives.Mar 16 2021, 10:22 PM
epriestley added a comment.Mar 16 2021, 10:27 PM

After recent changes, only DifferentialRevision and ManiphestTask still have onboard mail keys.

I believe neither patch needs anything special, but they'll trigger potentially slow migrations (no install has millions of Almanac namespaces, but many have millions of revisions). Since it has only been a couple weeks (2020 Week 8) since the last heavy migration and there's no urgency here, I'm going to hold off on these last two migrations until there's some other stronger motivator for doing slow migrations.

epriestley added a commit: rPc3e6db6f0b32: Migrate Almanac Device "mailKey" to modern storage.Mar 16 2021, 10:51 PM
epriestley added a commit: rPfadb2bd52b5e: Modernize "mailKey" on AlamnacService.
epriestley added a commit: rP7c0e33c34d6f: Modernize "mailKey" for Almanac Bindings.
epriestley added a commit: rPeeb009b4fa61: Modernize "mailKey" for Calendar Event.
epriestley added a commit: rP3267859aeeb2: Modernize "mailKey" on Fund initiatives.
epriestley added a commit: rP8226b3c88082: Modernize "mailKey" on Almanac Namespaces.
epriestley added a commit: rP86669ab54f97: Modernize "mailKey" for Almanac Networks.
epriestley mentioned this in 2021 Week 13 (Late March).Mar 26 2021, 6:06 PM
epriestley added a revision: D21842: Migrate "SlowvotePoll" to modern mailkey storage.May 26 2022, 12:10 AM
epriestley added a comment.May 26 2022, 12:13 AM

After recent changes, only DifferentialRevision and ManiphestTask still have onboard mail keys.

I'm not sure what this claim was based on, I must have done something goofy with grep. There are still quite a lot of these:

src/applications/badges/storage/PhabricatorBadgesBadge.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/conpherence/storage/ConpherenceThread.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/countdown/storage/PhabricatorCountdown.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/differential/storage/DifferentialRevision.php:      $this->mailKey = Filesystem::readRandomCharacters(40);
src/applications/files/storage/PhabricatorFile.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/legalpad/storage/LegalpadDocument.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/macro/storage/PhabricatorFileImageMacro.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/maniphest/storage/ManiphestTask.php:      $this->mailKey = Filesystem::readRandomCharacters(20);
src/applications/nuance/storage/NuanceItem.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/nuance/storage/NuanceQueue.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/nuance/storage/NuanceSource.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/paste/storage/PhabricatorPaste.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/phame/storage/PhameBlog.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/phame/storage/PhamePost.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/phortune/storage/PhortuneCart.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/phurl/storage/PhabricatorPhurlURL.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/ponder/storage/PonderAnswer.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/ponder/storage/PonderQuestion.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));
src/applications/project/storage/PhabricatorProject.php:      $this->setMailKey(Filesystem::readRandomCharacters(20));

The Nuance keys could safely just be dropped without migration (these objects have never sent mail in a legitimate production system). Most of the other migrations are simple.

epriestley added a commit: rPb1533e5468b1: Migrate "SlowvotePoll" to modern mailkey storage.May 26 2022, 4:29 PM
Phabricator · Built by Phacility