-
Notifications
You must be signed in to change notification settings - Fork 10
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
githubwebhooks: allow infosec-prod account to access unfiltered GitHub stream #72
base: master
Are you sure you want to change the base?
Conversation
50482f1
to
6b869cc
Compare
githubwebhooks/iam-roles.tf
Outdated
sid = "github_webhooks_all_infosec_subscribe" | ||
effect = "Allow" | ||
actions = [ | ||
"SNS:Subscribe", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also grant ListSubscriptions and Unsubscribe so we can determine our subscription ARN and unsubscribe if needed?
githubwebhooks/iam-roles.tf
Outdated
|
||
data "aws_iam_policy_document" "sns_webhooks_all" { | ||
# Give self full access. | ||
statement = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too familiar with TerraForm but if this is you granting your own account rights via a resource policy (AKA SNS Topic Policy), I'd recommend against it unless there's a good reason. I'd recommend governing access to your SNS topics in your own account with an identity-based policy (aka an IAM policy).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I loaded up the AWS web console, the topic had a default policy which contained the rules I implemented here. Not sure why that is or if anything is relying on it.
We already have a separate IAM policy allowing the Lambda function to SNS:Publish to this topic. That's the only thing that matters. So I'll remove this.
If we lock ourselves out of the topic, we can restore access via an IAM policy elsewhere.
6b869cc
to
f7d7791
Compare
I was unable to add I /think/ the error means that we can't grant this action to an entire account: only a service. So if we changed the principal to a service within the infosec AWS account rather than the account itself, it may work. Maybe we could wildcard the principal to all instances of a particular subscriber type? |
…b stream This was requested by @gene1wood so infosec can monitor GitHub activity.
f7d7791
to
f652f8d
Compare
The latest PR is rebased on master and is the active configuration. We should probably merge it so Git repo state is in sync with terraform state in production. |
Yes, sorry I should have chimed in here. Indeed that makes sense. Could we instead have |
I /think/ If your account wants to audit subscriptions for all SNS topics in our AWS account, then we'd grant |
The key to Lemme do this to avoid ambiguity, I'll run a couple tests today to validate the exact permissions we need and update the ticket then.
No, no need to do that. |
This was requested by @gene1wood.
There is a default policy on SNS topics. The first portion of the
added policy copies what was present on the topic.