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).
Differential D7989
Mailgun receive support talshiri on Jan 17 2014, 9:53 AM. Authored by Tags None Referenced Files
Subscribers
Details
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). Opened a task with Mailgun. Felt great.
Diff Detail
Event TimelineComment Actions 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:
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.
Comment Actions 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) Comment Actions 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. Comment Actions 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. Comment Actions Kinda. Mailgun doesn't use DKIM for the incoming mail (they just do the HMAC thing). Comment Actions 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. Comment Actions (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.) Comment Actions What would setStatus() do? I was thinking about being able to update some internal counter and/or dispatch errors for admin. 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. Comment Actions 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. Comment Actions 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). Comment Actions 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?) Comment Actions (also, they do support SMTP, but having an adapter would require less config, which is always nice) Comment Actions Summarizing from IRC:
Some whitespace nitpicks inline if you get a chance to fix them, I can sort that in the pull otherwise. This looks great overall.
|