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

githubwebhooks: allow infosec-prod account to access unfiltered GitHub stream #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

indygreg
Copy link
Contributor

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.

sid = "github_webhooks_all_infosec_subscribe"
effect = "Allow"
actions = [
"SNS:Subscribe",

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?


data "aws_iam_policy_document" "sns_webhooks_all" {
# Give self full access.
statement = {

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).

Copy link
Contributor Author

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.

@indygreg
Copy link
Contributor Author

I was unable to add SNS:Unsubscribe. The error message I got from AWS was esoteric: "policy statement action out of service scope." That yielded exactly 3 hits on Google. (4 once this comment is indexed.)

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.
@indygreg
Copy link
Contributor Author

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.

@gene1wood
Copy link

Yes, sorry I should have chimed in here. Indeed that makes sense. Could we instead have sns:ListSubscriptions as this will allow us to enumerate our own subscriptions and with the returned Subscription ARNs we should be able to unsubscribe (with that ARN) as we the requester are the subscribed entity.

@indygreg
Copy link
Contributor Author

I /think/ sns:ListSubscriptions is a service-global permission. I'm unable to add this permission to a specific SNS topic resource. However, sns:ListSubscriptionsByTopic is granted. I think that means your account can list all subscriptions for that specific SNS resource.

If your account wants to audit subscriptions for all SNS topics in our AWS account, then we'd grant sns:ListSubscriptions. Does it need to do this?

@gene1wood
Copy link

The key to sns:ListSubscriptions is that it should "Returns a list of the requester's subscriptions." (from the docs) meaning the subscription ARNs returned should be only those of the entity making the call, not all subscription ARNs. To get all subscriptions, not just those of the requester you'd use sns:ListSubscriptionsByTopic which "Returns a list of the subscriptions to a specific topic." and run it against each topic.

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.

If your account wants to audit subscriptions for all SNS topics in our AWS account, then we'd grant sns:ListSubscriptions. Does it need to do this?

No, no need to do that.

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.

2 participants