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

sns_topic - Fix Permission Issue for Cross Account Subscriptions #1418

Merged
merged 3 commits into from
Feb 3, 2023
Merged

sns_topic - Fix Permission Issue for Cross Account Subscriptions #1418

merged 3 commits into from
Feb 3, 2023

Conversation

ichekaldin
Copy link
Contributor

SUMMARY

sns_topic currently fails with the following error if it has any cross account subscriptions:

Couldn't get subscription attributes for subscription arn:aws:sns:us-east-1:123412341234:my-sns-topic-name:555950dc-7c5f-416c-8f8e-e8f38eabfa54: An error occurred (AuthorizationError) when calling the GetSubscriptionAttributes operation: Not authorized to access this subscription

This happens, for example, when a Lambda function in account A is subscribed to an SNS topic in account B, as described here.

I believe this was caused by #640.

I am not sure how to write a test for this specific situation as it would require multiple AWS accounts.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

sns_topic

ADDITIONAL INFORMATION
- community.aws.sns_topic:
    name: my-sns-topic-in-account-123412341234
    subscriptions:
      - endpoint: "arn:aws:lambda:us-east-1:567856785678:function:my-lambda-function-in-account-567856785678"
        protocol: lambda
    state: present

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage plugins plugin (any type) labels Aug 19, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 12s
✔️ build-ansible-collection SUCCESS in 5m 27s
✔️ ansible-test-sanity-docker-devel SUCCESS in 12m 34s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 25s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 48s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 02s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 42s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 8m 19s
✔️ ansible-test-splitter SUCCESS in 2m 33s
✔️ integration-community.aws-1 SUCCESS in 6m 47s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@tremble tremble added the backport-4 PR should be backported to the stable-4 branch label Aug 25, 2022
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this.

I think it would be better to base the continue on "attributes" not being set rather than RawMessageDelivery not being set. The current logic is likely to cause confusion further down the line if additional attributes are added.

@ichekaldin
Copy link
Contributor Author

I think it would be better to base the continue on "attributes" not being set rather than RawMessageDelivery not being set. The current logic is likely to cause confusion further down the line if additional attributes are added.

Agreed. I think my latest commit does the trick.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 16s
✔️ build-ansible-collection SUCCESS in 5m 30s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 10s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 45s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 05s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 12s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 5m 56s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 5m 09s
✔️ ansible-test-splitter SUCCESS in 2m 40s
✔️ integration-community.aws-1 SUCCESS in 7m 04s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@markuman markuman added the backport-5 PR should be backported to the stable-5 branch label Oct 18, 2022
@markuman markuman requested a review from tremble October 18, 2022 05:12
@tremble tremble added the mergeit Merge the PR (SoftwareFactory) label Feb 3, 2023
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

✔️ ansible-galaxy-importer SUCCESS in 4m 25s
✔️ build-ansible-collection SUCCESS in 5m 52s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 05s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 13s (non-voting)
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 9m 49s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 07s
✔️ ansible-test-sanity-docker-stable-2.14 SUCCESS in 8m 54s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 6m 41s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 5m 51s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 37s
✔️ ansible-test-units-amazon-aws-python310 SUCCESS in 8m 18s
✔️ ansible-test-changelog SUCCESS in 2m 16s
✔️ ansible-test-splitter SUCCESS in 2m 40s
✔️ integration-community.aws-1 SUCCESS in 6m 23s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
⚠️ integration-community.aws-19 SKIPPED
⚠️ integration-community.aws-20 SKIPPED
⚠️ integration-community.aws-21 SKIPPED
⚠️ integration-community.aws-22 SKIPPED

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit de21c4b into ansible-collections:main Feb 3, 2023
@patchback
Copy link

patchback bot commented Feb 3, 2023

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/de21c4bdda68d6c9f1c14c7d4d0d8604b06929a6/pr-1418

Backported as #1700

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Feb 3, 2023
sns_topic - Fix Permission Issue for Cross Account Subscriptions

SUMMARY

sns_topic currently fails with the following error if it has any cross account subscriptions:
Couldn't get subscription attributes for subscription arn:aws:sns:us-east-1:123412341234:my-sns-topic-name:555950dc-7c5f-416c-8f8e-e8f38eabfa54: An error occurred (AuthorizationError) when calling the GetSubscriptionAttributes operation: Not authorized to access this subscription

This happens, for example, when a Lambda function in account A is subscribed to an SNS topic in account B, as described here.
I believe this was caused by #640.
I am not sure how to write a test for this specific situation as it would require multiple AWS accounts.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

sns_topic
ADDITIONAL INFORMATION

- community.aws.sns_topic:
    name: my-sns-topic-in-account-123412341234
    subscriptions:
      - endpoint: "arn:aws:lambda:us-east-1:567856785678:function:my-lambda-function-in-account-567856785678"
        protocol: lambda
    state: present

Reviewed-by: Mark Chappell <None>
(cherry picked from commit de21c4b)
@patchback
Copy link

patchback bot commented Feb 3, 2023

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/de21c4bdda68d6c9f1c14c7d4d0d8604b06929a6/pr-1418

Backported as #1701

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Feb 3, 2023
sns_topic - Fix Permission Issue for Cross Account Subscriptions

SUMMARY

sns_topic currently fails with the following error if it has any cross account subscriptions:
Couldn't get subscription attributes for subscription arn:aws:sns:us-east-1:123412341234:my-sns-topic-name:555950dc-7c5f-416c-8f8e-e8f38eabfa54: An error occurred (AuthorizationError) when calling the GetSubscriptionAttributes operation: Not authorized to access this subscription

This happens, for example, when a Lambda function in account A is subscribed to an SNS topic in account B, as described here.
I believe this was caused by #640.
I am not sure how to write a test for this specific situation as it would require multiple AWS accounts.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

sns_topic
ADDITIONAL INFORMATION

- community.aws.sns_topic:
    name: my-sns-topic-in-account-123412341234
    subscriptions:
      - endpoint: "arn:aws:lambda:us-east-1:567856785678:function:my-lambda-function-in-account-567856785678"
        protocol: lambda
    state: present

Reviewed-by: Mark Chappell <None>
(cherry picked from commit de21c4b)
@tremble
Copy link
Contributor

tremble commented Feb 3, 2023

Thanks @ichekaldin, I'm sorry this stalled out for so long.

softwarefactory-project-zuul bot pushed a commit that referenced this pull request Feb 3, 2023
…) (#1700)

[PR #1418/de21c4bd backport][stable-4] sns_topic - Fix Permission Issue for Cross Account Subscriptions

This is a backport of PR #1418 as merged into main (de21c4b).
SUMMARY

sns_topic currently fails with the following error if it has any cross account subscriptions:
Couldn't get subscription attributes for subscription arn:aws:sns:us-east-1:123412341234:my-sns-topic-name:555950dc-7c5f-416c-8f8e-e8f38eabfa54: An error occurred (AuthorizationError) when calling the GetSubscriptionAttributes operation: Not authorized to access this subscription

This happens, for example, when a Lambda function in account A is subscribed to an SNS topic in account B, as described here.
I believe this was caused by #640.
I am not sure how to write a test for this specific situation as it would require multiple AWS accounts.

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

sns_topic
ADDITIONAL INFORMATION



- community.aws.sns_topic:
    name: my-sns-topic-in-account-123412341234
    subscriptions:
      - endpoint: "arn:aws:lambda:us-east-1:567856785678:function:my-lambda-function-in-account-567856785678"
        protocol: lambda
    state: present

Reviewed-by: Mark Chappell <None>
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Feb 3, 2023
…) (#1701)

[PR #1418/de21c4bd backport][stable-5] sns_topic - Fix Permission Issue for Cross Account Subscriptions

This is a backport of PR #1418 as merged into main (de21c4b).
SUMMARY

sns_topic currently fails with the following error if it has any cross account subscriptions:
Couldn't get subscription attributes for subscription arn:aws:sns:us-east-1:123412341234:my-sns-topic-name:555950dc-7c5f-416c-8f8e-e8f38eabfa54: An error occurred (AuthorizationError) when calling the GetSubscriptionAttributes operation: Not authorized to access this subscription

This happens, for example, when a Lambda function in account A is subscribed to an SNS topic in account B, as described here.
I believe this was caused by #640.
I am not sure how to write a test for this specific situation as it would require multiple AWS accounts.

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

sns_topic
ADDITIONAL INFORMATION



- community.aws.sns_topic:
    name: my-sns-topic-in-account-123412341234
    subscriptions:
      - endpoint: "arn:aws:lambda:us-east-1:567856785678:function:my-lambda-function-in-account-567856785678"
        protocol: lambda
    state: present

Reviewed-by: Mark Chappell <None>
@ichekaldin ichekaldin deleted the sns_topic_fix_cross_account_subscriptions branch February 3, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-4 PR should be backported to the stable-4 branch backport-5 PR should be backported to the stable-5 branch bug This issue/PR relates to a bug community_review mergeit Merge the PR (SoftwareFactory) module module plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants