Page MenuHomePhabricator

Plans: Mail Tags and Failover
Closed, ResolvedPublic

Description

These queued changes are adjacent and likely make sense to execute together:

See PHI54, PHI175. These issues are covered by T10448, which is likely the largest change here. That has a bunch more stuff attached to it.

See T12677. Currently, you can only configure a single outbound pathway for mail. This has caused delivery issues in the production cluster on a handful of occasions. We should upgrade this to allow configuration of zero or more mailers which can failover. We should evaluate adding Postmark support and consider making it our primary outbound mail pathway given Mailgun's uneven handling of the security incident described in T13037. This is purely upstream-motivated and doesn't solve any customer needs, except that customers love it when we change configuration and always ask for more compatibility breaks.

See PHI291. An install is interested in a way to mark security changes as sensitive so that they don't send details via email (e.g., "when tags include: security; take action: mark mail as sensitive"). Instead, users would receive a placeholder message without any content, just a link to the web UI ("This message is about a sensitive object, click to see the content: ...").

See PHI178 and PHI126. See T12689. When users resign from a revision, they should not receive mail about it even if they're a member of a non-resigned package or project. This requires propagating mail recipients deeper into the stack, and retaining richer recipient information. Subscriptions should be upgraded to support a stronger "unsubscribe" which can get you out of indirect subscriptions via projects/packages/etc. These probably mostly go down the same pathway.

See PHI125. Mail attachments are stored in JSON, and fail when they are not UTF8. Attachments should be stored in Files instead.

See T4776. We don't tell you that someone took you off the recipient list for an object. This isn't a huge deal, but is a meaningful policy concern and inconsistent with how we do things in other cases. We should plug up this hole.

See T12491. We send errors to unverified addresses, but this is inconsistent with other mail policy.

See T9141. Although this probably won't be exhaustive, these changes can rename "MetaMTA" to "Mail" where it's easy to test.


Scope creep:

See T12630. This may be an easy bug to fix.

See T11138. Maybe this is also a real bug?

See T12404. Breaking our dependency on PHPMailer would be great, but this is probably a good chunk of work and not realistic to get into scope until we learn about at least 3-4 more RCE vulnerabilities.

See T7477. This may also be outside of reasonable scope but describes a general infrastructure improvement.


Errata:

Thread-Topic: Some applications currently generate a Thread-Topic like D123 Flub the blubber. This was because I believed mail clients were likely to show the value of this header to users in some cases when I originally wrote the logic. However, I now believe that no client shows this value to users or expects it to contain a human-readable thread description or subject. More modern applications generate either D123: PHID-DREV-xxx or just PHID-DREV-xxx, and have for many years without feedback from users about unusual client behavior. I believe the only real constraint on this field is that it must retain the same unique value across all messages in an object's discussion thread. That is, this field is a bit more like a Thread-Unique-ID than a Thread-Topic.

The verbose Thread-Topic values are not appropriate for transmission with the "Must Encrypt" flag, because they may disclose sensitive information (D234 Fix issue where any user account accepts password "hunter2"). The monogram topics generally are, although it's slightly questionable because some objects (like repositories and projects) have monograms which can occasionally disclose information, and monograms are not completely immutable (repositories and projects are also examples here). The PHID-only values are appropriate for all objects and completely stable.

We should probably change all applications to use PHIDs as thread topics and remove the originalTitle field from Differential, Maniphest, and Ponder. This will cause a one-time threading break for clients.

Initially, I am keeping this header out of the "Must Encrypt" whitelist until we can make these changes. This may cause some threading issues for "Must Encrypt" messages, but they should generally resolve once we can whitelist Thread-Topic.

To/Cc Headers: If metamta.one-mail-per-recipient is disabled, real recipients will be disclosed in the mail headers. If we don't address this directly, the documentation should be updated to include this caveat.

And:

  • Proper escaping (or, at least, reasonable mangling-into-safety) for "Real Name Full of Weird Symbols" <local@domain.com> addresses.
  • Postmark inbound support.
  • In HTML mail, mail stamps in the body could be styled small and pale. There should be a little more whitespace underneath, too.
  • Postmark inbound CIDR ranges.
  • Maybe a "roles"/"disabled" option to let you disable mailers but keep them configured, or let you use a mailer for inbound-only.

Details

Commits
D19033 / rPab04d2179bf1: Add "Mute/Unmute" for subscribable objects
D19031 / rPbca9c08953bd: Add an "Acting user" field to Herald
D19030 / rP6186f0aa91b6: Briefly document mail stamps and remove obsolete header documentation
D19029 / rPbae9f459ab7f: Pass HTML bodies to push email
D19028 / rPa8f937d3138a: Only add the Mail "STAMPS" body section if there are stamps
D19027 / rP942b17a98088: Improve correctness of email address escaping in Mailgun/Postmark
D19026 / rPHUf91ef416f11f: Let PhutilEmailAddress encode addresses with complex display names somewhat…
D19025 / rP948b0ceca466: Configure a whitelist of remote addresses for Postmark inbound webhooks
D19022 / rPd0a2e3c54f9a: Fix an issue where some Differential edit pathways may not have reviewers…
D19021 / rP1cd3a593784a: When users resign from revisions, stop expanding projects/packages to include…
D19019 / rPf214abb63f9d: When a change removes recipients from an object, send them one last email
D19018 / rPdbe479f0d9de: Don't send error/exception mail to unverified addresses
D19017 / rP5792032dc9c9: Support Postmark inbound mail via webhook
D19016 / rP0986c7f6732e: Add a "View Object" button on the web mail view page
D19015 / rP085221b0d6f6: In HTML mail, make the text for mail stamps in mail bodies smaller and lighter
D19014 / rP6e5df2dd714e: Document that disabling "metamta.one-mail-per-recipient" leaks recipients for…
D19013 / rPaa74af19834b: Remove all "originalTitle"/"originalName" fields from objects
D19012 / rPf090fa7426e4: Use object PHIDs for "Thread-Topic" headers in mail
D19007 / rP1f53aa27e459: Add unit tests for mail failover behaviors when multiple mailers are configured
D19006 / rP9947eee182aa: Add some test coverage for mailers configuration
D19005 / rP994d2e8e1563: Use "cluster.mailers" if it is configured
D19004 / rP4236952cdbc0: Add a `bin/config set <key> --stdin < value.json` flag to make CLI…
D19003 / rPc868ee9c07d0: Introduce and document a new `cluster.mailers` option for configuring multiple…
D19002 / rP7f2c90fbd12b: Prepare for multiple mailers of the same type
D19000 / rP7765299f8399: Mask the sender for "Must Encrypt" mail
D18998 / rP1485debcbda2: Prepare mail transmission to support failover across multiple mailers
D19008 / rPHU57a137027d9b: Add a Postmark API client
D18999 / rP56bf0690804f: Try running Herald when performing inverse edge edits
D18997 / rP1bf64e5cbcf4: Add Differential and Herald mail stamps and some refinements
D18996 / rP7d475eb09af7: Add more mail stamps: tasks, subscribers, projects, spaces
D18995 / rP3131e733a85c: Add Editor-based mail stamps: actor, via, silent, encrypted, new, mention, self…
D18994 / rP9de54aedb578: Remove inconsistent and confusing use of the term "multiplex" in mail
D18986 / rP6d90c7ad92b3: Save mail attachments in Files, not on the actual objects
D18985 / rPeb06aca951ce: Support DestructionEngine in MetaMTAMail
D18984 / rPcbe4e68c0722: Add a Herald action to trigger "Must Encrypt" for mail
D18983 / rP7b2b5cd91e01: Add basic support for a "Must Encrypt" mail flag which prevents unsecured…

Related Objects

Mentioned In
2018 Week 6 (Early February)
T13069: Make mail stamps routable on the server, include transaction information, and replace mail tags
T13068: Refinements for "Mute Notifications"
T7477: Handle inbound email with phabricator address in the CC
T10189: Differential email headers seem to be missing
T13037: An attacker gained staff access to Mailgun and was able to read customer API keys
T12033: Large diffs can still repeatedly fail to insert
T11337: Mail v3
Mentioned Here
T13065: Move storage for `mailKey` to the Mail application
T13066: Refine inbound mail error behaviors, distinguishing between actual mail sender and acting-as sender
T13068: Refinements for "Mute Notifications"
T13069: Make mail stamps routable on the server, include transaction information, and replace mail tags
D18995: Add Editor-based mail stamps: actor, via, silent, encrypted, new, mention, self-actor, self-mention
D18873: Expand detection of protocol-relative hrefs to cover "/\evil.com" and variants
T3278: ⛄ Build a summary mode Remarkup engine for constrained text areas
D13397: MetaMTA - more progress towards a mail application
T11138: mail is not delivered if metamta.placeholder-to-recipient is null
T4776: Notify users when someone takes an action that takes them off an object's recipient list
T7477: Handle inbound email with phabricator address in the CC
T9141: Rename "MetaMTA" to "Mail"
T10448: Modularize mail tags
T12404: Implement a first-party SMTP client
T12491: Error reply emails which are generated before identifying the sender should no longer be sent, now that the "always require verification" rule is in place
T12630: Phabricator fails with a fatal PHP error if it receives a mail with no plain text part
T12677: Support multiple mail delivery services for automatic failover
T12689: Mail is still received after resigning from a revision
T13037: An attacker gained staff access to Mailgun and was able to read customer API keys

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

While it's on my mind -- these messages link to /mail/whatever/123/ right now, but that page doesn't hyperlink links in the mail body, which is a fairly significant usability issue since the workflow is: click the link to go to the mail, read the mail body, copy paste the link in the mail body into your address bar.

One way to cheat on this is just to render the body as remarkup. This is wrong: we've already rendered it as remarkup to a text rendering target, and we'd be re-rendering it. However, it will basically work: link links, turn Dxxx into an object reference, etc., and probably not break too much stuff, so it's likely an upgrade over no rendering.

A less cheaty way would be to preserve the original mail body for much longer and do the remarkup rendering at the last moment, before sending the mail. Then, re-render in the web view from the raw remarkup source. But this becomes very tricky in the general case because some mail contains complex HTML elements (like inline comments) which would also need to be stored in a structured way and then rendered just-in-time. Not insurmountable but a fairly hefty chunk of work to improve a very niche workflow.

A middle-of-the-road approach is to implement a "mail-client-like renderer" which mostly just links up hyperlinks. This would fix any bad results from re-rendering remarkup, but we necessarily end up with just an okay result, similar to viewing the mail message in any third-party client. We also have to duplicate at least a little bit of hyperlink rendering code, which has a lot of tricky security behavior (e.g., see D18873, recently).

(It's possible this "mail-client-like renderer" can just be a remarkup renderer with few active rules; see also T3278.)

Oh, great: ⛄ and ☃ are totally different emoji. Great.

An alternate attack on this is to preserve the text and link for the existing "View Revision" button as metadata (or just use the object PHID, which I think we already preserve) and re-render them into the View page. I'll likely do this in the short term so this isn't quite as aggressively bad as it is otherwise.

contains complex HTML elements (like inline comments) which would also need to be stored in a structured way and then rendered just-in-time

We could also store HTML in the database and trust it blindly, of course, but I'm wildly uneasy about this from a security viewpoint. We technically do this already, but only in very narrow cases in pure readthrough caches (remarkup cache, changeset parse cache).

Vaguely related, the subject line for this mail is currently something like:

[Application] Object Updated

Normally, it would be something like:

[Application] [Closed] Txxx: blah blah

Obviously, we have to hide the title. I think we're also correct to hide the action (like [Closed]) since it will sometimes leak information (consider learning that three [Raised Priority] actions have been applied to a security task -- this is a good hint that there's something really severe in the wild). But showing the application name is fine and doesn't disclose anything.

For most objects, we could also safely show the monogram, which would make this mail a little more useful without disclosing anything. However, I think we need to adjust applications a little bit to pass it into the mail.

alexmv added a subscriber: alexmv.Feb 2 2018, 11:39 PM

Hrrrm...

Good thing I got that top-secret message over a secure channel. Who knows what the Bad People would have one if they'd intercepted it!

When an object is updated via an "inverse edge" change (normally: when it is mentioned, or when another object is linked to it) we currently do not fire Herald rules. The corresponding block in the code has this comment:

// If we're applying inverse edge transactions, don't trigger Herald.
// From a product perspective, the current set of inverse edges (most
// often, mentions) aren't things users would expect to trigger Herald.
// From a technical perspective, objects loaded by the inverse editor may
// not have enough data to execute rules. At least for now, just stop
// Herald from executing when applying inverse edges.

With the "must encrypt" flag, this means that the "require encryption" rule never fires, so the resulting mail is sent in cleartext. This discloses the name and recipient list of the object, and additional details after the D18995 ("mail stamps") series of changes.

This pushes me toward considering the stateful model for this flag rather than the transient "if Herald says so, that mail gets flagged" model, since I think it may be an easier way forward than trying to sort out the inverse edge / Herald stuff and it's generally a sturdier model, I just didn't want to build it out if we didn't really need it for anything.

Who knows what the Bad People would have one if they'd intercepted it!

I do like how much intrigue this feature creates.

The "From" address should also probably be anonymized for flagged mail.

epriestley updated the task description. (Show Details)Feb 7 2018, 11:00 AM

See PHI178 and PHI126. See T12689. When users resign from a revision, they should not receive mail about it even if they're a member of a non-resigned package or project. This requires propagating mail recipients deeper into the stack, and retaining richer recipient information. Subscriptions should be upgraded to support a stronger "unsubscribe" which can get you out of indirect subscriptions via projects/packages/etc. These probably mostly go down the same pathway.

For the "Resigned" stuff, I think the actual implementation is that we build a list of PHIDs which targets are not permitted to expand into. So if you've resigned from a revision, projects and packages can't expand into your user PHID during target resolution, regardless of why they're targets. For example, if Alice resigns but #alice-fan-club is still a subscriber, her "Resign" should be stronger than that and still prevent expansion.

However, you should still be able to subscribe directly, and if you're in the natural target list you still get mail. If you resign and then "Subscribe" again (as an individual user) you'd reasonably expect to get mail.

For "Mute", we could try to use the UNSUBSCRIBE edge to build the "no expansion" list. However, this runs into dangerous territory with sequences of actions like this:

  • You're added as a subscriber by a mention.
  • You unsubscribe.
  • A project you're a member of is added as a reviewer.

In this case, we would not send you mail. This is probably reasonable and maybe even expected, but it's not obvious ("Why am I not getting mail?" does not lead to "Oh, because I unsubscribed 3 weeks ago." easily).

I think the only downside to a separate "Mute" action is that we end up with another action in the action list, but we can do something like this:

  • Ship "Mute".
  • Check if anyone's using it in 6-or-whatever months.
  • Remove it or turn it into a keystroke if there's little-to-no use.

So this is a separate list which just removes targets after expansion, and essentially identical to the existing getExcludeMailRecipientPHIDs() mechanism except that the delivery/routing reason should be "muted" instead of "This message is a response to another email message...".

epriestley updated the task description. (Show Details)Feb 8 2018, 1:48 PM
epriestley updated the task description. (Show Details)Feb 8 2018, 2:01 PM
epriestley closed this task as Resolved.Feb 8 2018, 7:35 PM

See T13069 for followups on mail stamps.

See T13068 for followups on "Mute Notifications".

See T13066 for followups on inbound mail error behavior.

See T13065 for followups on mail storage infrastructure.

I'll still need to vet the cluster.mailers changes more extensively with real mailers, but I'll follow up in T12677 before resolving that task.

T9141 and T12404 were unrealistically huge and didn't make it into scope. T7477 didn't make it either since inbound mail didn't get much focus from other changes.

This week's changelog will include more useful human-facing summaries of the changes and new features introduced here.

(Testing "secure" flag in webhooks. 🐑)

epriestley mentioned this in Unknown Object (Phriction Wiki Document).Feb 10 2018, 1:02 AM