Add Amazon SES/SNS ingress to ActionMailbox

[bobf]

Summary

Provides an ingress for Amazon SES/SNS.

Includes:

• /rails/action_mailbox/amazon/inbound_emails ingress route
• Signature verification
• Auto-subscribe
• Permitted SNS topic configuration

Tests for all of the above are included.

Other Information

This commit (f480cfabcd9000b5817b610e21466299025b12d2) from @georgeclaghorn states that SES integration was removed and will be re-written for 6.1.

I needed this sooner so I wrote a gem here: https://github.com/bobf/action_mailbox_amazon_ingress

I emailed George to discuss and he suggested a PR. I have adapted the tests from RSpec to Minitest.

Please let me know if any adjustments are needed.

[bobf]

I've just deployed an app running Rails from this branch and set up an SES/SNS topic. Confirmation and receipt both appear to be working fine.

[bobf]

Thanks for the PR! Here’s the first round of feedback.

Thanks a lot for the feedback. I have addressed all of your points. I think there is only one item outstanding (pending your feedback).

Let me know if you prefer squashed commits when the PR is ready.

[kaspth]

I still like @georgeclaghorn's idea of a separate ConfirmationsController. I also think that would force us to consider a model extraction.

Here's my take on that.

# frozen_string_literal: true

module ActionMailbox
  module Ingresses
    class Amazon::BaseController < ActionMailbox::BaseController
      before_action :set_notification

      private
        def set_notification
          @notification = SNSNotification.new params.except(:controller, :action)
        end

        class SNSNotification
          def initialize(params)
            @params = params
          end

          def subscription_confirmed?
            Net::HTTP.get_response(URI(params[:SubscribeURL])).code&.start_with?("2")
          end

          def verified?
            require "aws-sdk-sns"
            Aws::SNS::MessageVerifier.new.authentic?(params.to_json)
          end

          def topic
            params[:TopicArn]
          end

          def message_content
            params.dig(:Message, :content) if receipt?
          end

          private
            attr_reader :params

            def receipt?
              params[:Type] == "Notification" || params[:notificationType] == "Received"
            end
        end
    end

    class Amazon::ConfirmationsController < Amazon::BaseController
      def create
        if @notification.subscription_confirmed?
          head :ok
        else
          Rails.logger.error "SNS subscription confirmation request rejected."
          head :unprocessable_entity
        end
      end
    end

    class Amazon::InboundEmailsController < Amazon::BaseController
      before_action :ensure_message_content, :ensure_valid_topic, :ensure_verified

      def create
        ActionMailbox::InboundEmail.create_and_extract_message_id! @notification.message_content
      end

      private
        def ensure_message_content
          head :bad_request if @notification.message_content.blank?  
        end

        def ensure_valid_topic
          unless @notification.topic.in? Array(ActionMailbox.amazon.subscribed_topics)
            Rails.logger.warn "Ignoring unknown topic: #{@notification.topic}"
            head :unauthorized
          end
        end

        def ensure_verified
          head :unathorized unless @notification.verified?
        end
    end
  end
end

[bobf]

I still like @georgeclaghorn's idea of a separate ConfirmationsController. I also think that would force us to consider a model extraction.

Here's my take on that.

@kaspth Thanks - agree this is much nicer. One minor point is that I think @georgeclaghorn suggested separate actions and not necessarily controllers (unless I have missed a message).

I think this is better either way but wanted to highlight just in case this made any difference.

I'll update to reflect your suggested changes tomorrow if there's no further discussion needed.

[kaspth]

One minor point is that I think @georgeclaghorn suggested separate actions and not necessarily controllers (unless I have missed a message).

George suggested a separate controller in https://github.com/rails/rails/pull/39364#discussion_r428997727 that's where I got the name from 😄

[georgeclaghorn]

Sorry, I think “the different actions” in my comment may have confused. I meant “the two different actions in the separate controllers.”

[bobf]

@kaspth @georgeclaghorn Thanks both for the feedback. I have adapted Kasper's refactor and addressed a couple of issues. A couple of things I think worth noting:

• The Message param in the POST body is a JSON string that must be manually decoded. i.e. it is double-encoded. It contains the original email event data structure.
• ActionMailbox::BaseController#ingress_name (called by #ensure_configured) does not expect a controller that is not named InboundEmailsController so I have defined ActionMailbox::Ingresses::Amazon::BaseController#ingress_name which just returns :amazon. This feels kludgey but I didn't want to modify the base controller method. (Otherwise subscription confirmations fail).

Let me know if any changes are needed.

[mwesigwa]

@bobf, I'd suggest adding some form of documentation, alerting users not to enable the raw_message_delivery option when using this Ingress. I found out the hard way after spending a couple of hours trying to get this solution to work. When you enable raw message delivery for an Amazon SQS endpoint, any Amazon SNS metadata is stripped from the published message and the message is sent as is which results in the confounding error saying the topic is being ignored since it does not exist in the sent message.

Here is the Amazon documntation link that finally explained this.

[bobf]

@mwesigwa Thanks for the idea - I was tempted to select this option myself so I'm sure others will as well.

I've updated with this commit: https://github.com/rails/rails/pull/39364/commits/809f53b9576684a6fbd985d957ca22689d10138e

Let me know if you think it needs adjusting !

[mwesigwa]

The change is great! For what it's worth, I can confirm that the code worked as expected :1st_place_medal:. Thank you for implementing this.

[joshkinabrew]

Really looking forward to this being released! Thanks for your great work @bobf

[bobf]

@georgeclaghorn Is there anything here that needs changing ? I have a conflict with the main branch but would rather wait until I had some feedback before fixing in case this gets left out in the cold.

The conflict is only tiny in this case so will be no trouble to resolve but some of the tests (e.g. routing) can be a bit of a pain to deal with if things start to diverge. Let me know how you want to proceed.

[bobf]

@chrisortman Are you sure you didn't enable raw message delivery ?

Check the documentation change here: https://github.com/rails/rails/commit/809f53b9576684a6fbd985d957ca22689d10138e

If you still think it's wrong then let me know and I'll take a look !

[chrisortman]

Positive. I am using an S3 action (stores the email in S3 them sends SNS notification with bucket key) as opposed to just SNS with the message embedded. Embedded in SNS is limited to 150k and can’t handle attachments

[bobf]

@chrisortman Thanks for confirming - that's certainly interesting. I'll see if I can set up a similar scenario over the weekend and hopefully we can find a way to be compatible with both formats.

[chrisortman]

@bobf If you use terraform I could share most of the setup I'm using with you. There is an action element that can be keyed off of, but (and I acknowledge my bias here) I'm not sure how worth it it is to support the non-S3 option. Unless you're only receiving emails you are generating in another application you can't ensure you're not getting messages with stripped attachments or truncated content.

[chrisortman]

If it is helpful, I've started working through it and hoping to finish next week https://github.com/chrisortman/rails/commits/ses-action-mailbox

[bobf]

@chrisortman Thanks a lot - that's definitely helpful. I'm currently stuck doing work-work over the weekend but if I manage to get through my stack I'll certainly take a look. Appreciate your support in trying to find a solution here.

[chrisortman]

I've managed to get to a functional solution. I think the code may need some cleanup, but I'd rather take some suggestions there than guess incorrectly. I think my no_content? stuff doesn't not have too many double negatives

I also added a rake task for generating keys and signing the AWS fixtures. I think this will make it easier to add / change test cases in the future because it won't require capturing real SNS traffic.

I am using this right now, and it is working for me. But I'd really like to get this into rails proper so I don't have to run off a fork. I would also rebase & squash my changes before merging I think

[chrisortman]

I am also not sure how to share my changes. Do they need to merge into @bobf branch first in order to update this PR?

[bobf]

@chrisortman if that is the case just let me know and I'm happy to merge whatever is needed to get the functionality added.

Am too busy to do much coding on this but can do the admin no problem - let me know as needed.

[chrisortman]

Thanks @bobf would you please merge https://github.com/chrisortman/rails/tree/actionmailbox-amazon-ingress into your branch. That will update this PR with my changes.

[chrisortman]

@bobf As an alternative to me asking you to merge my branch into yours, would you consider making me a collaborator on your fork? I expect one the changes are integrated we will also want to rebase on rails:master in addition to any code style / refactoring that is asked for.

[bobf]

@chrisortman This is a great idea - I have just invited you. As you can see I'm a bit slow to get back on open source stuff at the moment !

I took a look at your code when you first shared it and it looked to be very high quality so am more than happy for you to take initiative and improve this PR. Thank you for helping.

[chrisortman]

@georgeclaghorn I think the buildkite failures are false negatives? They look like buildkite / aws errors not test failures. But I can't seem to figure out how to trigger a new build without creating any empty commit (build kite seems stuck because I have an organization and SSO is not enabled for it)

Assuming we can get past the tests, the questions I have on this are:

1. Do we want to rename the config (and probably all the classes too) option from :amazon to :amazon_ses as suggested by @mullermp ?
2. When the mail messages are delivered via S3, should we delete the S3 object after creating the inbound mail message?

[bobf]

But I can't seem to figure out how to trigger a new build without creating any empty commit

@chrisortman You can git commit --amend and git push --force to trigger a rebuild. Git commit hash is based on time as well as diff so it will change.

[mullermp]

In aws-sdk-rails, we have created an SQS ActiveJob adapter that uses either amazon or amazon_sqs as an option. In the next major version we would drop one of those - it depends on what we want to decide here. Personally I'm in favor of containing the product name so it doesn't force us to have one Amazon implementation.

[mullermp]

Consider also SES, SESv2, Pinpoint, (Lambda?) are all implementation options here so having flexibility is nice.

[chrisortman]

I think this all makes sense @mullermp. I think :amazon_<product> strikes a nice balance and gives flexibility to future implementations.

If there is a lot of drift between v1 and v2 then :amazon_<product>_v2 will make sense and it can have it's own implementation.

If changes are relatively small, we haven't dictated that it can't be an implementation decision by looking a request header or something (like how this looks at the message type to decide S3 or SNS)

I've made the changes to make it AmazonSes and :amazon_ses here. It seems like AmazonSES would have been better, but I wasn't sure where to put an inflection rule for that when it has to happen in rails core, nor that it wouldn't be a change that would break downstream stuff

[bobf]

@chrisortman I don't have any specific objection to changing from :amazon to something else but I do question wether :amazon_ses is the best option.

As you know we are specifically handling SNS messages from SES. I wonder if there are other ways we might receive these messages in future (e.g. maybe SES will be able to send directly to an endpoint, bypassing SNS). Then :amazon_ses will not have been a good choice.

So I wonder if maybe :amazon is the better option and any later mechanisms can be qualified as :amazon_whatever.

Also note that, unless I misunderstand, @mullermp 's example relates to ActiveJob whereas we are only talking about ActionMailbox. Do we really need to account for alternative implementations of delivery mechanisms for email from AWS ? Do we anticipate these being added to Rails ? It seems unlikely to me.