Page MenuHomePhabricator

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

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).

Event Timeline

epriestley created this task.

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).

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

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"?

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

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

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.

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.