Page MenuHomePhabricator

[keyserver] Use SMTP (via Postmark) instead of `sendmail` for email subscriptions
ClosedPublic

Authored by atul on Jun 17 2022, 11:41 PM.
Tags
None
Referenced Files
F3384005: D4291.id14211.diff
Thu, Nov 28, 6:56 PM
Unknown Object (File)
Sun, Nov 10, 12:10 AM
Unknown Object (File)
Sat, Nov 9, 10:50 PM
Unknown Object (File)
Sat, Nov 9, 10:04 PM
Unknown Object (File)
Sat, Nov 9, 8:28 PM
Unknown Object (File)
Sat, Nov 9, 8:26 PM
Unknown Object (File)
Sat, Nov 9, 8:24 PM
Unknown Object (File)
Sat, Nov 9, 8:14 PM
Subscribers

Details

Summary

The SubscriptionForm on the landing page broke when we moved the keyserver to EC2 because the image didn't include sendmail. After installing sendmail it appeared that things were working and the requests were succeeding, but the emails didn't end up getting delivered reliably. There's additional configuration that's necessary to get the sendmail approach working reliably/consistently.

Instead, in this diff we move from the sendmail transport to using SMTP via nodemailer with Postmark as the "host."

These are some resources I referenced to possibly save reviewer some googling:

Test Plan
  1. Change the to field in sendmail.sendMail to my Comm email account
  2. Navigate to localhost/commlanding/
  3. Put a valid email in the SubscriptionForm input
  4. Click "Subscribe for updates"
  5. Observe that the email shows up in Postmark
  6. Ensure that the email showed up in my inbox

Diff Detail

Repository
rCOMM Comm
Branch
june17 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 17 2022, 11:45 PM
Harbormaster failed remote builds in B9797: Diff 13560!
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 17 2022, 11:53 PM
Harbormaster failed remote builds in B9798: Diff 13561!
atul requested review of this revision.Jun 17 2022, 11:57 PM
atul added inline comments.
keyserver/src/emails/sendmail.js
21

"Tl;dr Use port 587 if you can, 465 if you can’t, 25 if you must.

Port 587 is technically correct, the best kind of correct. However, many ESPs have adopted implicit TLS on port 465. While you can send email over port 25 and 2525, it’s much more secure to have the messages encrypted. This makes port 587 the preferred option for sending, with port 465 as a close second."


"This is the default mail submission port. When users submit an email to be routed by a proper mail server, this is the one that will provide best results.
Port 587, coupled with TLS encryption, ensures that email is submitted securely and following the guidelines set out by the IETF. All Mailgun customers should consider using port 587 as their default SMTP port unless you're explicitly blocked by your upstream network or hosting provider."

22

"Setting secure to false does not mean that you would not use an encrypted connection. Most SMTP servers allow connection upgrade via STARTTLS command but to use this you have to connect using plaintext first"

"secure – if true the connection will use TLS when connecting to server. If false (the default) then TLS is used if server supports the STARTTLS extension. In most cases set this value to true if you are connecting to port 465. For port 587 or 25 keep it false"

27

"requireTLS – if this is true and secure is false then Nodemailer tries to use STARTTLS even if the server does not advertise support for it. If the connection can not be encrypted then message is not sent"

keyserver/src/emails/subscribe-email-updates.js
26

Using @squadcal.org resulted in the following:

ErrorCode: '400', Message: 'The 'From' address you supplied (no-reply@squadcal.org) is not a Sender Signature on your account. Please add and confirm this address in order to be able to use it in the 'From' field of your messages.'.

Figured @comm.app makes more sense anyways.

ashoat requested changes to this revision.Jun 18 2022, 9:47 AM
ashoat added inline comments.
.buildkite/eslint_flow_jest.yml
5 ↗(On Diff #13562)

Then you won't need this

keyserver/src/emails/sendmail.js
5 ↗(On Diff #13562)

Use importJSON

This revision now requires changes to proceed.Jun 18 2022, 9:47 AM
keyserver/src/emails/sendmail.js
5 ↗(On Diff #13562)

Also let's make sure to type this

keyserver/src/emails/subscribe-email-updates.js
26 ↗(On Diff #13562)

We'll need to validate no-reply@comm.app on Postmark for this, but we can't validate the full comm.app domain since Google Workspace is in charge of that. I'm sure there's a way to configure this (either authorizing just no-reply@comm.app with Postmark somehow, or maybe there's a way to have a "dual tenant") but it probably isn't easy.

It looks like you tested this in your dev environment, though... so presumably things work?

What's the status of this work? It's been delayed quite a bit and has an extremely noticeable external effect. We should be prioritizing this over all active Dev Ops work

partially address feedback

keyserver/src/emails/sendmail.js
25–28 ↗(On Diff #14185)

This isn't great because we're reading the API key from the file every single time someone submits the form

29 ↗(On Diff #14185)

This is not great because this will crash for developers if they don't have the postmark API keys locally. We should fallback on sendmail if __DEV__ AND !postmark

rebase to include CI changes

fallback on sendmail if isDev && ~postmark

keyserver/src/emails/subscribe-email-updates.js
26 ↗(On Diff #13562)

It looks like you tested this in your dev environment, though... so presumably things work?

Yeah, things are working in my local dev environment.

We'll need to validate no-reply@comm.app on Postmark for this, but we can't validate the full comm.app domain since Google Workspace is in charge of that.

Here's what I'm seeing on the "Sender Signatures" page:

aksljd.png (446×2 px, 291 KB)

"You're able to send from any email address on this domain."
So it looks like it's good to go?

Nice, yeah that looks good to go!

keyserver/src/emails/sendmail.js
26–47 ↗(On Diff #14187)

Should we cache this "transport" object so we never have to create a new one? Or is that a bad idea?

41–44 ↗(On Diff #14187)

Seems weird to duplicate the apiToken here, but I guess that's how Postmark wants it?

This revision is now accepted and ready to land.Jul 5 2022, 12:11 PM
keyserver/src/emails/sendmail.js
26–47 ↗(On Diff #14187)

Yeah I think that'd be a good idea. What would be the best way to do that?

Would something like this work?

1310f924.png (1×1 px, 445 KB)

41–44 ↗(On Diff #14187)

Yeah that's how it is in their docs so stuck with that

Yup that should work, I think!

Was checking the nodemailer docs to see if there are any warnings against leaving a transport object lying around for a while. Couldn't find anything (might be good to do some more digging on your end), but I did find this option for a connection pool. Not sure if it's a better solution here but it might be?

introduce cachedTransport

I don't think we want to use the pooled SMTP option. It seems like it's only really relevant if there's a large volume of emails to be sent, which isn't the case at the moment.

Was checking the nodemailer docs to see if there are any warnings against leaving a transport object lying around for a while

Will look into this for the single connection option, but we'll definitely have extra resource usage with the pooled option because it keeps maxConnections number of connections open: "If transporter uses pooling then connections are kept open even if there is nothing to be sent." Whereas with single connection option it "would connect to a SMTP server separately for every single message."

So my understanding is that "Single connection" is a better approach for us than "Pooled connections."

Will look into whether leaving "Single connection" object around is an anti-pattern, but I doubt it is.

Sounds good to me, "would connect to a SMTP server separately for every single message." seems like all the confirmation we need for leaving the object around, and I buy your reasoning for why a connection pool doesn't make sense for us here.