Allow cross account SQS to SNS subscriptions about justsaying HOT 15 CLOSED

Which Notification Is Handled by Which Handler

Given the fictional Notification

{
  "Type" : "Notification",
  "MessageId" : "f783d045-f16a-5014-ac4d-9d5ce24a4f30",
  "TopicArn" : "arn:aws:sns:eu-west-1:928682934851:be-qa16-restaurant",
  "Subject" : "AnOrderMessage",
  "Message" : "{\"OrderId\":123}",
  "Timestamp" : "2014-12-27T20:04:48.561Z",
  "SignatureVersion" : "1",
  "Signature" : "amU8kzeSqn9Vrj0+DDAth3opLdFSufx4XtgWg15cpkXJIRS2R9L/xSCvRzgfvJLW49j2KCJpFwjOIKWO9r2YVWtkJa0ra7tHDBZoGcnnA4FFEirCuu5dEkkUcC8XqXqaNcElVsKw+mjf0ByYhT9FFOH8QOFigq9+ivYI68ygZedE0WYy3CY8vTli97O8mWu9pc9Z91MvXUxKv5WGa4FHpGmxDCOS93K/24Lpuj2yWReM79Dw085PvJcxXngrwhFKfXOrlbnfB3ofd8MW7jZY7vAnz12H/YJsihHzVN0qxwvWNh0a9pK33lsE+NSdhNIaoWM5tBdO1y4Oc3UNx8Perw==",
  "SigningCertURL" : "https://sns.eu-west-1.amazonaws.com/SimpleNotificationService-d6d679a1d18e95c2f9ffcf11f4f9e198.pem",
  "UnsubscribeURL" : "https://sns.eu-west-1.amazonaws.com/?Action=Unsubscribe&SubscriptionArn=arn:aws:sns:eu-west-1:928682934851:be-qa16-restaurant:c614f94d-7d36-4043-b301-8f4330a22dac"
}

Currently
In SqsNotificationListener.cs#Ln147 we use the Subject to determine the message type, with the correct assumption that within an account a Topic name will be unique.

When Doing Cross Account
The same topic name can exist in different accounts, so the only way to ensure uniqueness is to use the Topic ARN as the identifier

Looking at the code and requirements, my initial thought is to replace the identification of Topic by Message Name with the Topic ARN. When briefly looking at the places where Message name is used, it does feel as if Topic ARN should be a a first class citizen and this will help with cross account subscriptions.

from justsaying.

Didnt look at this in depth, but using Topic ARN sounds more correct - assuming that this is unique across account, of course.

from justsaying.

I'm going to take this on soon, here are some of my thoughts.

@jaimalchohan your summary is spot on, I see there being 2 key problems that need solving if we keep the current usage, i.e. fluent configuration, then at runtime we do some magic (create and configure) to make things work:

• Creating the policy in AccountA to allow AccountB, we can do this with ARN wildcard policies, we would need the account id only, could be set using the fluent api. We could add to JustSaying.Tools a process to add this policy to topics where we haven't yet updated JustSaying (we have a strong case for this in my team).
• Discovering the topic ARN from AccountB, having to pass in the ARN is a bit manual, and not ideal. Where ever we currently call FindTopic(...) we could now call out to multiple account and return the first we find (this would assume that we wold not support multiple accounts with the same topic name), this would require credentials to these account. Again, this would be configured by the fluent api.
  Alternatively, we could use just the account id's, so rather than using FindTopic(...), we could instead build the ARN ourselves, as the ARNs are deterministic with inputs of account id and topic name, the account id could be discovered for the current account, and then we can add other account ids explicitly from the fluent configuration. The problem with this approach is that we would assume no privileges to the other accounts, so would not be able to create or check the existence of foreign topics. I would suggest that we change the behavior of JustSaying so that the topic creation is the responsibility of the publisher, and the queue creation and subscription creation is the responsibility of the subscriber. So if a topic is yet to be created (or yet to be created in a particular account) the subscriber just creates the queue without the subscription. If the topic is later created, the subscribing service would need restarting to create the subscription to the newly created topic. This feels right IMO. An issue we might have here is with no way to check that the topic exists from another account, will we get a meaningful exception when we try to create the subscription?

Questions I have are:

• Is requiring the account id for publishers and subscribers ok? If we instead are going to require credentials for subscribers, maybe for consistency we should require credentials for publishers, we can use these to get the account id.
• Do we need to support multiple topics across accounts with the same topic name? I think yes.
• Are we now asking too much to be able to configure this all at runtime, the level of privileges we need are a one time thing, should this be split out into a tool that is run? We'd lose some simplicity (something that JustSaying is great for), the benefit is we could require less privileged at runtime and we would be less constrained in setup phase, we could have this take all account credentials which would have permission to create things, the subscriber and publisher components would only need to know about their queue and their topic, no accounts to worry about if it is all pre-configured, and a minimal set of privileges required.

This is a bit of a brain dump, so I'll summarise with what I think are the options:

1. We provide account credentials for publishing and subscribing, everything works how it does now, we create topics in all accounts when subscribing.
2. We use account ids only, we change the behavior so that topics are only created by publishers, and subscriptions are created as needed by the subscribers to all existing topics at that point in time.

My vote is for the latter.

from justsaying.

Just to save you a little time, account-id can be had from the ec2 meta-data service:

[ec2-user@ip-10-232-2-215 ~]$ curl http://169.254.169.254/latest/meta-data/iam/info
{
  "Code" : "Success",
  "LastUpdated" : "2016-06-29T14:06:18Z",
  "InstanceProfileArn" : "arn:aws:iam::123412341234:instance-profile/some-iam-path-prefix/the-name",
  "InstanceProfileId" : "<some id>"
}

from justsaying.

Also, if the environment has a service discovery tool like consul then one can store information about the account topology within it.

from justsaying.

Thanks @petemounce, already did some research and was going to use a newer addition to the awssdk, GetCallerIdentity:
http://docs.aws.amazon.com/STS/latest/APIReference/API_GetCallerIdentity.html
^ this will return the the details that match the credentials we supply, so if we go down the multiple credentials route, it works.
If v3 isn't merge when I get to it, then I'll get the current user and parse out the account id from the urn. The ec2 meta-data service is a good suggestion, but I don't think fits our usage, for example we use credentials to an aws environment when deving locally, the meta-data service won't help us there (If my understanding is correct)

from justsaying.

@petemounce I'll have a think at how to make it extensible though, so that someone could put the work in to make it work with consul, can't quite get my head around how that would work at the moment

from justsaying.

Me and @payman81 have done some investigation this afternoon. We have done some work to prove that using account ids works very well, and requires minimal change to existing users.

We came up with the following configuration additions:
Publisher (account 111111111111):

bus.WithSnsMessagePublisher<Message>().ConfigurePublisherWith(x =>
{
    x.AdditionalSubscriberAccounts = new {"222222222222", "333333333333"};
});

Subscriber (account 222222222222 & 333333333333):

bus.WithSqsTopicSubscriber()
.IntoQueue(queueName)
.ConfigureSubscriptionWith(x =>
{
    x.TopicSourceAccount = "111111111111";
})
.WithMessageHandler(...);

For subscribers, we would keep it 1-1 with topic subscriptions, so if you had multiple accounts to subscribe from, you would explicitly add another subscription.

The result of configuring the publisher would be a policy attached that does the following:

{
      "Sid": "someSid",
      "Effect": "Allow",
      "Principal": {
        "AWS": "222222222222"
      },
      "Action": [
        "SNS:Subscribe"
      ],
      "Resource": "arn:aws:sns:eu-west-1:111111111111:topicname"
    }

This could be retrospectively applied account wide using a service role policy with "Resource": "*" (if my understanding is correct), so we wouldn't need to update JustSaying across all publishers necessarily to start taking advantage of this.

Feedback please! (especially around the config naming and usability)

from justsaying.

What I like about above is that no change is needed to the fluent syntax and no breaking change is introduced.

Also, by letting the consumer to request subscription you can avoid Subscription confirmation step altogether.

from justsaying.

I find one thing confusing here - when confiruging publishers, we add extra subsriber accounts, but when configuring subscription, we don't specify extra account, but override the account.

I suggest repurposing AdditionalSubscriberAccounts to SubscriberAccounts, which would override the account where the message will be published (queue or topic).

from justsaying.

@dariusdamalakas that's a great point.
The reason I chose AdditionalSubscriberAccounts was because the behavior will be slightly different behind the scenes for the current account to an additional account, so I wanted the api to reflect that.

Currently when we subscribe (which will always be withing the same account), we will create the topic if it doesn't exist. We can't do this for additional accounts because we have no credentials. So I think we have 4 options:

• Maintain the 2 behaviors and have:
  • the config add additional accounts, which will work slightly differently
  • the config overrides as suggested, we detect if the account id is the current account and try to create the topic
  • the config overrides as suggested, and we don't try to create any of the topics.
• Unify the behavior and have publisher create topics, and subscribers create queues and subscriptions.

I like the unification approach, but I also like the minimal change to how it currently operates, which will continue to be the common case. If we don't mind not unifying, then I am favoring your suggestion of overriding the accounts, then not trying to create the topics.

from justsaying.

To be frank, i would to redesign how JustSaying is creating queues and topics. A couple of areas i'd like to improve:
a) Rework how queues / topics are created. Get rid of QueueBase and TopicBase object and introduce queue creator/topic creator
b) separate config/build stages. I.e. run the configs first, build in-memory representation of which queues and topics need to be created, then go and create. There are potential saving here in the number of API requests JustSaying does to AWS
c) Unification as you suggested. (somewhat overlaps with previous points)

This is quite a big change, and most likely out of scope of this ticket, just sharing this so that this would not get lost. Surely it would also be good to meet up and discuss this in depth how everyone sees it. I have in fact started working on point A, there's a branch manual_policy_override where i have this 70% ready, but had to park this because of other priorities. There are ISqsQueue and ISnsTopic interfaces, and IQueueCreator and ISnsTopicCreator which show where this is heading. Not finished, needs polishing and extra missing test cases.

from justsaying.

On the publish side, AdditionalSubscriberAccounts is used to open up the topic policy so consumers in other accounts can be allowed to subscribe. The topic will remain subscribable from the same account hence the property name is qualified with 'additional'. You will always create topics in the same account as the publishing side.

On the subscription side, however, you want to specify specifically which account the topic belongs to therefore you set 'TopicSourceAccount' property.

Hope that makes sense.

from justsaying.

Thanks @payman81, I got that totally backwards! This is the publisher so adding the policy is required for foreign accounts, not the stuff that I said, in fact, that was a happy coincident of not adding them as a collection for publishers.

@dariusdamalakas I think the code could do with some rationalisation, not sure what others think. I'll checkout that branch.

from justsaying.

This was shipped in v4

from justsaying.