Implement email sending via SES #3580

dstufft · 2018-04-07T21:10:38Z

This implements sending emails via Amazon SES. It has the following features:

Overall it still needs some cleanups as well (tests need written, some hardcoded region values, error handling is broken, etc). However it has all the basic functionality!

Here's some screenshots of the admin interface:

dstufft · 2018-04-07T23:46:59Z

Note: Before we deploy this we will need to change the environment variables in our deploys, the new mechanism uses MAIL_BACKEND=<backend> foo=bar format that the storage and docs and such use.

dstufft · 2018-04-08T01:25:09Z

I've hardcoded the number of transient bounces that an email address can have before it gets flagged for re-verification to 5. I'm not sure if that's a good number or not or if that should be generally configurable or not.

Whenever we get a soft bounce that doesn't occur after a delivery (i.e. not an OOTO auto-responder) increment the number of transient bounces by 1.
Whenever we get a successful email delivery, reset the number of transient bounces to 0.
Whenever the number of transient bounces is greater than 5, treat it as if the email had hard bounced and un-verify the email address.
Whenever an email gets verified (either the first time, or any subsequent times) we reset the number of transient bounces back to 0.

alex · 2018-04-08T03:19:45Z

warehouse/utils/sns.py

+
+    def _validate_topic(self, topic):
+        comparer = functools.partial(hmac.compare_digest, topic)
+        if not all(map(comparer, self.topics)):


I don't understand this code at all. As far as I can tell the only time this could be valid is if all the items in topics are the same. Is this supposed to be an any or something?

It looks like you only ever pass a single topic, maybe only support one?

alex · 2018-04-08T03:20:24Z

warehouse/utils/sns.py

+
+        self._validate_topic(message["TopicArn"])
+        self._validate_timestamp(message["Timestamp"])
+        self._validate_signature(message)


Shouldn't you validate the signature first thing, cryptographic doom principle and all?

alex · 2018-04-08T03:21:36Z

warehouse/utils/sns.py

+        # Before we do anything, we need to verify that the URL for the
+        # signature matches what we expect.
+        cert_host = urllib.parse.urlparse(cert_url).netloc
+        if _signing_url_host_re.search(cert_host) is None:


Prefer .match() to .search(). It doesn't matter in this case since your regexp is anchored, but better safe than sorry.

I'm torn! I generally always use .search() because .match() messes with my mental model of how regular expressions work. In this case, .match() is probably better because I don't know that it ever makes sense to have this not anchored at the beginning.

alex · 2018-04-08T03:22:14Z

warehouse/utils/sns.py

+
+    def _get_data_to_sign(self, message):
+        if message["Type"] == "Notification":
+            parts = self._get_parts_to_sign_notiifcation(message)


This is not how you spell notification

dstufft · 2018-04-08T03:44:39Z

To switch to SES, you need a configuration like:

MAIL_BACKEND=warehouse.email.services.SESEmailSender [email protected] region=us-west-2 topic=...

That assumes you have the other environment variables setup to configure authentication for AWS.

dstufft · 2018-04-08T18:37:34Z

Note: After this merges and deploys, pypi/infra#5 will need to be merged and applied to wire the SNS topic up to Warehouse's web hook handler. It will likely take two runs of terraform for this to work, since we need to know the expected TopicArn before we can subscribe to it.

ewdurbin

No blockers in my sight, just a couple questions.

ewdurbin · 2018-04-10T11:52:14Z

warehouse/email/ses/tasks.py

+
+
+@tasks.task(ignore_result=True, acks_late=True)
+def cleanup(request):


I think it might behoove us to cleanup unsuccessful deliveries on a slower pace than successful deliveries. I'd suggest we keep 30-90days of failures for debugging as it's quite possible for someone to not notice that things have gone awry in the short term. Particularly for email notifications for things that they did not initiate.

The alternative to holding onto logs for longer period is to persist some kind of summary as to why the email address was unverified, rather than just that it was.

Either option works for me, but the latter actually has the benefit of persisting for a much longer period than even 30-90 days which is nice.

When a user finally reaches out, we can at least tell them what the "last straw" for their previously verified address was.

To be clear, the boolean fields stored on accounts_email are probably enough to continue forward but may not capture all the necessary information to help a user get things back up and running. Also we may need more insight for certain spam greylisting/blacklisting situations.

Set it up so failures are kept for 90 days, and success are kept for 14 days.

ewdurbin · 2018-04-10T11:54:26Z

warehouse/routes.py

@@ -209,6 +209,9 @@ def includeme(config):
    )
    config.add_route("packaging.file", files_url)

+    # SES Webhooks
+    config.add_route("ses.hook", "/_/ses-hook/", domain=warehouse)


I've not used SES -> SNS -> webhook for this in the past, can we ensure we have the appropriate DeliveryPolicies in place to give these webhooks a fighting chance?

ewdurbin · 2018-04-10T11:59:12Z

warehouse/email/ses/views.py

+    require_csrf=False,
+    header="x-amz-sns-message-type:Notification",
+)
+def notification(request):


This route needs to lean towards failing hard to make Retry logic simple with SNS webhooks.

SNS webhook retry conditions:

HTTP status in the range 500-599.

HTTP status outside the range 200-599.

Looks like we should be OK, as the explicit HTTPBadRequest responses are indeed for requests that are successfully ignored, but something to keep in mind.

ewdurbin · 2018-04-10T11:59:32Z

warehouse/migrations/versions/7f0d1b5af8c7_add_tables_for_storing_ses_information.py

+        ),
+        sa.Column("message_id", sa.Text(), nullable=False),
+        sa.Column("from", sa.Text(), nullable=False),
+        sa.Column("to", sa.Text(), nullable=False),


should we index this to help with admin search?

I've added an index now!

dstufft force-pushed the ses branch from 9a60a66 to 333478a Compare April 8, 2018 00:56

alex reviewed Apr 8, 2018

View reviewed changes

dstufft force-pushed the ses branch from 4306c4f to 7794122 Compare April 8, 2018 17:54

dstufft changed the title ~~[WIP] Implement email sending via SES~~ Implement email sending via SES Apr 8, 2018

dstufft requested a review from ewdurbin April 8, 2018 17:56

dstufft mentioned this pull request Apr 8, 2018

Switch from using a SQS Queue to SNS Web Hooks pypi/infra#5

Merged

dstufft force-pushed the ses branch from 12eae0e to ce5d553 Compare April 9, 2018 11:25

dstufft mentioned this pull request Apr 9, 2018

Provide feedback to users about email delivery issues #3590

Closed

ewdurbin approved these changes Apr 10, 2018

View reviewed changes

dstufft added 17 commits April 10, 2018 22:18

Implement email sending via SES

e2dce9f

Handle searching emails for a specific to address

ccff099

Switch to allowing the mail sender to be configured

eee9287

Remove hardcoded regions and debug code

cfa5e28

After 5 transient bounces, unverify an email

5e89e4b

Cleanup the imports

b40bfe5

Verify the SNS messages are valid

3cde805

Don't violate the cryptographic doom principle

b17ab22

Don't be too clever (and ultimately screw it up)

42d7f1e

I am good at spelling

8acde5f

Switch to .match() instead of .search()

8ae59aa

Use .fullmatch() instead of .match()

0a29b68

Fix existing tests

b55d918

Verify that the URL is a HTTPS URL

00ed39b

Add tests

30cb469

Schedule the periodic SES cleanup task to run daily

bfc07fe

Rework the EmailStatus interface

762b6eb

dstufft added 4 commits April 10, 2018 22:18

Track complaint vs delivery failure independently

be48cf2

Use an Enum to mark why an email was unverified

5743db5

Add an index on EmailMessage.to

18cc10e

Keep delivery failures longer than successfu deliveries

6e8509e

dstufft force-pushed the ses branch from 4dede1c to 6e8509e Compare April 11, 2018 02:43

dstufft added 2 commits April 10, 2018 23:28

Switch to using enums

1e341de

Merge branch 'master' into ses

74ec7f7

dstufft merged commit 6602e85 into pypi:master Apr 11, 2018

dstufft deleted the ses branch April 11, 2018 06:57

dstufft mentioned this pull request Apr 12, 2018

expose mail deliverability in PyPI admin #3176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement email sending via SES #3580

Implement email sending via SES #3580

dstufft commented Apr 7, 2018 •

edited

Loading

dstufft commented Apr 7, 2018

dstufft commented Apr 8, 2018

alex Apr 8, 2018

alex Apr 8, 2018

alex Apr 8, 2018

dstufft Apr 8, 2018

alex Apr 8, 2018

dstufft commented Apr 8, 2018

dstufft commented Apr 8, 2018

ewdurbin left a comment •

edited

Loading

ewdurbin Apr 10, 2018

ewdurbin Apr 10, 2018

dstufft Apr 11, 2018

ewdurbin Apr 10, 2018

ewdurbin Apr 10, 2018

ewdurbin Apr 10, 2018

dstufft Apr 11, 2018



		@tasks.task(ignore_result=True, acks_late=True)
		def cleanup(request):

Implement email sending via SES #3580

Implement email sending via SES #3580

Conversation

dstufft commented Apr 7, 2018 • edited Loading

dstufft commented Apr 7, 2018

dstufft commented Apr 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstufft commented Apr 8, 2018

dstufft commented Apr 8, 2018

ewdurbin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstufft commented Apr 7, 2018 •

edited

Loading

ewdurbin left a comment •

edited

Loading