Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add policies for sns topic tagging #202

Closed

Conversation

alinabuzachis
Copy link
Collaborator

@gravesm
Copy link
Collaborator

gravesm commented Apr 21, 2022

@alinabuzachis This is fine in as much as it applies to SNS topics in our account, but the problem is that this is not enough to allow for ListTagsForResource on the third-party topic. We could add that third-party topic ARN as a resource in our policy, but because it's a cross account request, that topic would need a resource-based policy that allows us to make that request (it apparently does not). Because the community.aws PR changes make the list tags call for every request now, this effectively breaks all the existing tests against the third-party topic.

@tremble
Copy link
Contributor

tremble commented Apr 21, 2022

Tags are a bit weird, there shouldn't be a need for permissions on the other side, because you simply can't access someone else's tags, but you can attach tags that you would see to them.

@gravesm
Copy link
Collaborator

gravesm commented Apr 21, 2022

With these changes applied, I'm getting a not authorized to perform: SNS:ListTagsForResource on resource: arn:aws:sns:us-east-1:806199016981:AmazonIpSpaceChanged because no resource-based policy allows the SNS:ListTagsForResource action error

@tremble
Copy link
Contributor

tremble commented Jun 7, 2022

Oh, Yeah these tagging permissions are in the policy explicitly limited to the CI account.

In the PermitReadOnlyThirdParty stanza we'd need to add the SNS tagging permissions too.

@alinabuzachis
Copy link
Collaborator Author

Oh, Yeah these tagging permissions are in the policy explicitly limited to the CI account.

In the PermitReadOnlyThirdParty stanza we'd need to add the SNS tagging permissions too.

Done, thanks!

@alinabuzachis alinabuzachis force-pushed the sns_topic_tag_policies branch from 8c10abe to ea28667 Compare June 9, 2022 09:21
@gravesm
Copy link
Collaborator

gravesm commented Jun 9, 2022

This is still broken for me, with the same authorization error. I also tried listing tags on the third party topic as a user with full admin privileges, and this fails in the same way. I guess I don't understand how this is supposed to work. It seems to me that all the IAM permission is doing is allowing the CI account to use SNS:ListTags on that third party resource, but that resource still would have to allow us to do so. @tremble you mention it's possible to attach tags to a third party topic. Are you doing this by just calling tag_resource() with that topic's ARN?

@tremble
Copy link
Contributor

tremble commented Jun 14, 2022

@tremble you mention it's possible to attach tags to a third party topic.

I've done it with other resources before. Having had a poke about, it looks like it doesn't work for these Topics, it's probably missing a permission on the AWS side somewhere (a permission in account 806199016981).

@tremble
Copy link
Contributor

tremble commented Jun 14, 2022

https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions

When you tag public or shared resources, the tags you assign are available only to your AWS account; no other AWS account will have access to those tags. For tag-based access control to shared resources, each AWS account must assign its own set of tags to control access to the resource.

@tremble
Copy link
Contributor

tremble commented Jul 3, 2022

@alinabuzachis Looking at ansible-collections/community.aws#972 (which I just rebased), the cross-account tagging tests have been removed, so if this PR's rebased we should be able to the this and 972 merged and closed out.

@gravesm
Copy link
Collaborator

gravesm commented Feb 1, 2023

Closing this since I believe it is superseded by #260.

@gravesm gravesm closed this Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants