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

Kbroughton/gcp.iam.org folder policy changes #454

Merged

Conversation

kbroughton
Copy link
Contributor

Background

Add a detection for changes to a GCP IAM organization or folder policy.
The detection checks if the user-agent has the word "terraform" in it.
Although this could be spoofed, it will reduce noise from a lot of valid IaC changes.

Changes

gcp_audit_rules/gcp_iam_org_folder_changes.py
gcp_audit_rules/gcp_iam_org_folder_changes.yml

Testing

This has be deployed for over a month in our Panther instance
and with panther_analysis_tool

nhakmiller
nhakmiller previously approved these changes Jul 28, 2022
Copy link
Contributor

@nhakmiller nhakmiller left a comment

Choose a reason for hiding this comment

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

One small suggestion otherwise LGTM. I'll let @wey-chiang or @k-bailey or @edyesed do final approval & merge

deep_get(event,
"protoPayload",
"requestMetadata",
"callerSuppliedUserAgent").lower().find('terraform') == -1
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one thought, is there any value in still alerting in this case but using a custom severity function to set the severity to low or info? If you remove the last condition from here and added something like:

def severity(event):
  if "callerSuppliedUserAgent").lower().find('terraform') == -1:
    return 'INFO'
  return 'HIGH'

Then it would create an alert in either case, but for the terraform ones it would be at INFO severity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepted change recommendation for severity

@k-bailey
Copy link
Contributor

k-bailey commented Aug 5, 2022

Hey @kbroughton it looks like there was a minor logic bug that was creating the opposite effect from what you wanted. I adjusted it and will have someone else on the team review and merge next week. Thanks for the submission.

Test results pre change:
Screen Shot 2022-08-05 at 6 51 49 PM

@kbroughton
Copy link
Contributor Author

Any news on this one?

@edyesed edyesed merged commit 8239133 into panther-labs:master Aug 29, 2022
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