-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
feat: custom policy + hardened trust relationship #132
feat: custom policy + hardened trust relationship #132
Conversation
adding a condition on the trust policy for the codebuild project arn ensures the iam role can not be used by any other codebuild project. codebuild projects could be extremely permissive, even when least privileged so enforcing the role can only be used by the intended codebuild project limits to ability for a threat actor to quietly take control of a powerful role and do threat actory things.
@johncblandii or @srhopkins looks like you were assigned this PR. Is there anything else you need from me to get this in? we'd like to take advantage of this module, but need these changes for security reasons (#131) |
@@ -254,10 +254,22 @@ variable "logs_config" { | |||
description = "Configuration for the builds to store log data to CloudWatch or S3." | |||
} | |||
|
|||
variable "default_permissions_enabled" { |
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 we use the presence of the custom policy instead of this flag?
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.
it's possible one wants the defaults and custom policy. I used the enable variable to follow conventions elsewhere in the module.
happy to change that, if you'd prefer it to be an either or.
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.
Thanks @bt-macole - this is a really good improvement.
A couple of comments.
09aebaf
to
126c38d
Compare
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.
Thanks @bt-macole - a couple more things
description = "When 'true' default base iam permissions to get up and running with codebuild are attached. Those who want a least priveleged policy can instead set to `false` and use the `custom policy` variable." | ||
} | ||
|
||
variable "custom_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.
This one would need updating to match the variable change
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.
updated and reran tests
--- PASS: TestExamplesComplete (61.73s)
--- PASS: TestExamplesCustom (62.47s)
--- PASS: TestExamplesVPC (87.75s)
PASS
ok github.com/cloudposse/terraform-aws-codebuild 88.190s
examples/custom/main.tf
Outdated
cache_type = var.cache_type | ||
|
||
default_permissions_enabled = var.default_permissions_enabled | ||
custom_policy = [jsonencode(var.custom_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.
Update to hande list type
the default permissions are good for getting up and running, however, they are far more permissive than any least privileged policy would like, being `*` for all resources for anything in the default list + any and all additional_permissions. this allows users to still utilize the quick up and running policy, while also being able to disable it and replace it with a least privileged custom policy. the lifecycle rule ensure that users don't get confused by additional permissions and custom policy variables, failing on a plan if they attempt to use additional_permissions with default_permissions_enabled set to `false`.
534dc45
to
98576ab
Compare
I think you'll need to re-run
|
```terraform --- PASS: TestExamplesCustom (62.77s) --- PASS: TestExamplesComplete (62.87s) --- PASS: TestExamplesVPC (78.32s) PASS ok github.com/cloudposse/terraform-aws-codebuild 78.790s ```
98576ab
to
a2a68a6
Compare
hehe my bad! too used to precommit running terraform fmt and terraform-docs in our repo 🤦 |
/terratest |
Thanks @bt-macole we got there! |
what
Updated trust policy so only the codebuild project in this module can use the iam role.
Added ability to toggle on/off default permissions
Added support for attaching a custom policy
why
fe75886
9f0163f
Test Output:
references
resolves: #131