payman81
commented
8 years ago Hey @dariusdamalakas - I believe this issue was fixed a while back. Let me know if this is still an issue in the latest version.
Closed gerektoolhy closed 8 years ago
payman81
commented
8 years ago Hey @dariusdamalakas - I believe this issue was fixed a while back. Let me know if this is still an issue in the latest version.
gerektoolhy
commented
8 years ago @payman81 , do you remember what the root cause / fix?
I've updated locally to latest code (master branch), and can still reproduce it. The root
{
"Version": "2012-10-17",
"Statement": [
{
"Sid": "deff9abfac2f4ea6b8f9f79d006993bc",
"Effect": "Allow",
"Principal": {
"AWS": "*"
},
"Action": "sqs:SendMessage",
"Resource": "arn:aws:sqs:eu-west-1:xxxxxxxxxx:dev-dariouso-referencetemplates",
"Condition": {
"ArnLike": {
"aws:SourceArn": "arn:aws:sns:eu-west-1:xxxxxxxxxx:dev-dariouso-eventhappened6"
}
}
},
{
"Sid": "535a6b9f9f4b442ba33eb2711ed17dcf",
"Effect": "Allow",
"Principal": {
"AWS": "*"
},
"Action": "sqs:SendMessage",
"Resource": "arn:aws:sqs:eu-west-1:xxxxxxxxxx:dev-dariouso-referencetemplates",
"Condition": {
"ArnLike": {
"aws:SourceArn": "arn:aws:sns:eu-west-1:xxxxxxxxxx:dev-dariouso-eventhappened7"
}
}
},The pattern here is that each topic gets a condition. Looking at how to overcome this. It's the AWS C# SDK which is generating these policies. I don't see currently an easy way to override this.
payman81
commented
8 years ago The policy per topic is overkill. This is what I thought was fixed. I'll pick this up and provide a fix then.
gerektoolhy
commented
8 years ago @payman81 , ok, cool. I've just had a quick look, and I don't see an easy solution here.
On my local machine, i've hooked up extra statements to override the policy that AWS is building for us by default. I've added that code to SnsTopicBase.Subscribe
Not sure how to easily pass in a func here to expose the policy for the library consumer though, if you know what the best option here let me know. The only thing I can think of is a major refactor to the initialization pipeline, which is an overkill for this issue. We could take advantage of SubscribeQueueToTopics operation in AWS3 SDK, but we need to defer initializtion, i.e. build-up what are the topics/queues, and then create them in batches where possible. This is out of scope of this issue.
gerektoolhy
commented
8 years ago P.s. to reproduce you'll need around 15 topics subscribed to single queue. Might be lower depending on the message name.
gerektoolhy
commented
8 years ago Norm Jonhson replied with suggestion to use IAmazonSQS.SetAttributes and IAmazonSimpleNoticationService.Subscribe. This will allows us to control queue policy.
gerektoolhy
commented
8 years ago @payman81 @AnthonySteele, @stuart-lang i start working on this. Thinking of adapting IVerifyAmazonQueues to allow to hook in into queue/topic creation. Potentially reworking how SnsTopicBase and SqsQueueBase because that is where subscription logic is.
payman81
commented
8 years ago @dariusdamalakas I'm trying something simpler to see if it works. I'd explicitly set the policy on queue creation with a wildcard. I think that would prevent individual policy per topic when you call 'Subscribe' later on. This way all the change will be contained within SqsByName.
gerektoolhy
commented
8 years ago @payman81 yup, that would work, however, that means from the very strict policy we'd go to allow anything policy.
payman81
commented
8 years ago This change here will produce the following
{
"Version": "2012-10-17",
"Statement": [
{
"Sid": "804e930b16e24e26b302128a14b2ee10",
"Effect": "Allow",
"Principal": {
"AWS": "*"
},
"Action": "sqs:SendMessage",
"Resource": "arn:aws:sqs:eu-west-1:507204202721:issue211-2"
}
]
}I agree this is too relaxed. But we can easily add a condition to limit subscriptions to this account/region only. I think this level of policy is acceptable.
gerektoolhy
commented
8 years ago @payman81 yeah, at least a restriction to account please. I was thinking about adding restriction like this: arn:aws:sns:eu-west-1:<accountId>:<environment>-*. Essentially this would allow only topics from environment to subscribe to queues, such as dev, tst, prd, etc.
payman81
commented
8 years ago Have a look at this spike here
This will produce the following policy.
{
"Version": "2012-10-17",
"Statement": [
{
"Sid": "527e6bf619ff45c7b068b91fd92d5adc",
"Effect": "Allow",
"Principal": {
"AWS": "*"
},
"Action": "sqs:SendMessage",
"Resource": "arn:aws:sqs:eu-west-1:507204202721:issue211-2",
"Condition": {
"ArnLike": {
"aws:SourceArn": "arn:aws:sns:eu-west-1:507204202721:*"
}
}
}
]
}@payman81 looks good! simple, and does the job. Still does not allow to customize policies, but we can say that is good enough for now. Do you need any help to finish this up?
payman81
commented
8 years ago submitted #216 to fix this. Will let you know once it's merged.
gerektoolhy
commented
8 years ago Thanks!
This is caused by reaching the limit for SQS policy. Have not spent time to dig into this issue more and think of a proper solution for addressing this. Raising this just to have this for the record.