Page MenuHomePhabricator

Mailgun receive support
ClosedPublic

Authored by talshiri on Jan 17 2014, 9:53 AM.
Tags
None
Referenced Files
F14056553: D7989.id.diff
Sat, Nov 16, 8:56 PM
F14046659: D7989.id18120.diff
Wed, Nov 13, 10:39 PM
F14046658: D7989.id18140.diff
Wed, Nov 13, 10:39 PM
F14046655: D7989.id18141.diff
Wed, Nov 13, 10:39 PM
F14046654: D7989.id18151.diff
Wed, Nov 13, 10:39 PM
F14046653: D7989.id18072.diff
Wed, Nov 13, 10:39 PM
F14046652: D7989.id18119.diff
Wed, Nov 13, 10:39 PM
F14046651: D7989.id.diff
Wed, Nov 13, 10:39 PM

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T4326: Mailgun adapter support
Commits
Restricted Diffusion Commit
rPa9612fac24a9: Mailgun receive support
Summary

As you've suggested, I took the SendGrid code and massaged it until it played nice with Mailgun.

btw - unless I'm missing something, it appears that the SendGrid receiver lets you spoof emails (it performs no validation on the data received).

Test Plan

Opened a task with Mailgun. Felt great.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

This looks great to me. One vaguely-substantive inline and some discussion -- fix the trigger_error() (unconventional in this codebase) and I'll pull this.

Questions:

  • Does Mailgun support SMTP? It seems a little odd to support inbound mail only, without supporting outbound mail. Do you not need/want it for your use case? I'm fine with accepting just the receive half if you don't want to deal with the (probably trickier) send half, just wanted to make sure I'm not missing anything.
  • We should mention this in the docs, but I can write that up.

it appears that the SendGrid receiver lets you spoof emails (it performs no validation on the data received).

I don't believe SendGrid supports any sort of validation on the inputs, and usually you can spoof mail by just sending the fake mail, and our mail security model (such as one exists) relies mostly on knowledge of secret email addresses. But we should probably disable this receiver if there's no SendGrid API key configured. Do you want to add that here? I think it's reasonable for both receivers to reject requests if their API keys are missing.

src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php
26–32

Just:

throw new Exception(
  pht('Mail signature is not valid. Check your Mailgun API key.'));

...or with whatever text you want. Higher levels will do reasonable things with exceptions. Avoid trigger_error() in this codebase.

Let's also reject outright if the API key isn't set, for consistency with presumed SendGrid changes.

Their send api is quite simple actually. Didn't add it because I felt a little insecure about this (never done PHP professionally).

I'll give it a try when I have some time (I'll update this revision)

as for SendGrid - they just use DKIM (might be a pain in the ass to verify), but I see you point about the secret being the email address.

Yeah, I think we'll probably add DKIM support eventually but my (maybe incomplete/limited) understanding of it is that it should go a layer below this -- we'd still accept mail from all sources, and then check DKIM no matter the source, right? I might have no idea what I'm talking about, since I haven't implemented DKIM before.

The only real reason we haven't implemented DKIM is that no one has asked for it.

Kinda. Mailgun doesn't use DKIM for the incoming mail (they just do the HMAC thing).
I should probably add a check for the timestamp, but I don't want it to cause incoming mails to mysteriously not get delivered if, say, your clock got out of sync.
Is there a way to deliver errors in a way that would be easy for the admins to see? A micro dashboard would be really nice (show amount of rejected emails etc).

You can setMessage($message) on the $received object before you save() it -- see scripts/mail/mail_handler.php for an example. These messages can be viewed with bin/mail list-inbound / bin/mail show-inbound.

We previously had a Web UI dashboard for mail, but it was very difficult to make it useful without violating policies. Particularly, we can't let web users see mail subject or body content (which may contain text they don't have permission to see in the application, or may contain things like password reset / email validation links which would let them take over another user's account) or To/From addresses (which may include object secret keys which authenticate senders), so we kind of ended up with "some mail was sent and/or received, maybe". Moving it to the CLI made it less convenient but solved all those policy/access issues.

(You can also setStatus() and maybe add new constants to MetaMTAReceivedMailStatus -- we currently never reject mail before processing it so none of the receivers are great examples of usage, but PhabricatorMetaMTAReceivedMail->processReceivedMail() might be a useful template.)

What would setStatus() do?

I was thinking about being able to update some internal counter and/or dispatch errors for admin.
So you can do something like

if (!$this->verifyMessage()) {
    notifyAdmin("Got a message that failed verification. Check mail.mailgun.api-key");
    increaseCounter("mail.mailgun.failedMessages")
    ....
}

and have it show up at the top "problems" bar the admin see.
(yeah, it could be spammy, but might be ok with some refinements)

We haven't really had any types of errors which are good fits for that yet, and I don't think this one is great either.

When an error occurs, if it's not a legitimate rejection (i.e., the mail is real, but there's just a misconfiguration), there are like 100 things which could be wrong and we can only raise an alert for a tiny number of them. If we alerted on everything, admins would be buried in a deluge of nonsense in many cases (e.g., any user on their install misconfiguring a repository, GitHub going down for a bit, etc.).

I think it's better to just have a troubleshooting process which goes end-to-end and tries to cover things exhaustively. For inbound mail, the current process is using bin/mail to examine the inbound queue (http://www.phabricator.com/docs/phabricator/article/Configuring_Inbound_Email.html). What setStatus() does is shows what happened to the mail in that interface, and in the detail interface. For example:

$ ./bin/mail show-inbound --id 109
PROPERTIES
ID: 109
Status: err:duplicate
Related PHID: 
Author PHID: 
Message ID Hash: 2EqFMXCJMqY4

MESSAGE
Ignoring email with message id hash "2EqFMXCJMqY4" that has been seen 2 times, including this message.

From the "Status" and "Message" fields, I can see that mail ID #109 was a duplicate that was ignored because it had already been received (commonly, this happens when Phabricator receives mail both directly and from a mailing list). As an administrator, I wouldn't want to get a notification about this because it's a routine condition, but if I need to troubleshoot or debug an issue I can look at the log and figure out what's going on.

By setting status/message, the mail here can be saved but marked invalid with an appropriate message.

I see what you're saying.

A counter system (a-la statsd) would be cool though, so you can at least know that there is fact SOME problem, and you can look around and see what's up (and know that you need to poke around the email queue).

talshiri updated this revision to Unknown Object (????).Jan 19 2014, 8:12 AM
  • Fixed code review comments
  • added mailgun sender
talshiri updated this revision to Unknown Object (????).Jan 19 2014, 8:14 AM
  • Fixed code review comments

One problem though: file attachments don't work right now, as it seems that HTTPFuture/HTTPSFuture doesn't have multipart support right now (am I missing something?)

(also, they do support SMTP, but having an adapter would require less config, which is always nice)

Summarizing from IRC:

  • Sorting out the attachment support is a bit of a mess because of cURL's handling of @xyz to mean "read file xyz off disk".
  • We only send mail with attachments if users configure non-inline attached patches, which is rare.
  • Since you don't actually need attachment support and I'm not sure I can quickly/easily fix HTTPSFuture to support this, retain the same security guarantees, and not break any existing code, we're just going to TODO attachment support and throw.

Some whitespace nitpicks inline if you get a chance to fix them, I can sort that in the pull otherwise. This looks great overall.

src/applications/config/option/PhabricatorMailgunConfigOptions.php
20–23

Weird indents?

src/applications/metamta/adapter/PhabricatorMailImplementationMailgunAdapter.php
44–47

As we discussed, just throw.

92

Indentation.

105

Indentation.

src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php
26–27

Indentation.

Sounds great. I'll submit an updated patch tonight.

talshiri updated this revision to Unknown Object (????).Jan 21 2014, 8:13 AM
  • Mailgun adapter now throws when you try to add an attachment.
  • Fixed indents
talshiri updated this revision to Unknown Object (????).Jan 21 2014, 8:13 AM
  • Messed up indents :p