Skip to content

feat: Split the actions and conditions of the assume role by principal #85

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,8 @@ For automated tests of the complete example using [bats](https://github.com/bats
| Name | Description | Type | Default | Required |
|------|-------------|------|---------|:--------:|
| <a name="input_additional_tag_map"></a> [additional\_tag\_map](#input\_additional\_tag\_map) | Additional key-value pairs to add to each map in `tags_as_list_of_maps`. Not added to `tags` or `id`.<br/>This is for some rare cases where resources want additional configuration of tags<br/>and therefore take a list of maps with tag key, value, and additional configuration. | `map(string)` | `{}` | no |
| <a name="input_assume_role_actions"></a> [assume\_role\_actions](#input\_assume\_role\_actions) | The IAM action to be granted by the AssumeRole policy | `list(string)` | <pre>[<br/> "sts:AssumeRole",<br/> "sts:TagSession"<br/>]</pre> | no |
| <a name="input_assume_role_conditions"></a> [assume\_role\_conditions](#input\_assume\_role\_conditions) | List of conditions for the assume role policy | <pre>list(object({<br/> test = string<br/> variable = string<br/> values = list(string)<br/> }))</pre> | `[]` | no |
| <a name="input_assume_role_actions"></a> [assume\_role\_actions](#input\_assume\_role\_actions) | Map of principal type to list of actions for the assume role policy. E.g. { AWS = ["sts:AssumeRole"], Federated = ["sts:AssumeRoleWithSAML"] } | `map(list(string))` | <pre>{<br/> "AWS": [<br/> "sts:AssumeRole",<br/> "sts:TagSession"<br/> ]<br/>}</pre> | no |
| <a name="input_assume_role_conditions"></a> [assume\_role\_conditions](#input\_assume\_role\_conditions) | Map of principal type to list of conditions for the assume role policy. E.g. { AWS = [], Federated = [...] } | <pre>map(list(object({<br/> test = string<br/> variable = string<br/> values = list(string)<br/> })))</pre> | `{}` | no |
| <a name="input_assume_role_policy"></a> [assume\_role\_policy](#input\_assume\_role\_policy) | A JSON assume role policy document. If set, this will be used as the assume role policy and the principals, assume\_role\_conditions, and assume\_role\_actions variables will be ignored. | `string` | `null` | no |
| <a name="input_attributes"></a> [attributes](#input\_attributes) | ID element. Additional attributes (e.g. `workers` or `cluster`) to add to `id`,<br/>in the order they appear in the list. New attributes are appended to the<br/>end of the list. The elements of the list are joined by the `delimiter`<br/>and treated as a single ID element. | `list(string)` | `[]` | no |
| <a name="input_context"></a> [context](#input\_context) | Single object for setting entire context at once.<br/>See description of individual variables for details.<br/>Leave string and numeric variables as `null` to use default value.<br/>Individual variable settings (non-null) override settings in context object,<br/>except for attributes, tags, and additional\_tag\_map, which are merged. | `any` | <pre>{<br/> "additional_tag_map": {},<br/> "attributes": [],<br/> "delimiter": null,<br/> "descriptor_formats": {},<br/> "enabled": true,<br/> "environment": null,<br/> "id_length_limit": null,<br/> "label_key_case": null,<br/> "label_order": [],<br/> "label_value_case": null,<br/> "labels_as_tags": [<br/> "unset"<br/> ],<br/> "name": null,<br/> "namespace": null,<br/> "regex_replace_chars": null,<br/> "stage": null,<br/> "tags": {},<br/> "tenant": null<br/>}</pre> | no |
Expand Down Expand Up @@ -213,10 +213,12 @@ For automated tests of the complete example using [bats](https://github.com/bats
| Name | Description |
|------|-------------|
| <a name="output_arn"></a> [arn](#output\_arn) | The Amazon Resource Name (ARN) specifying the role |
| <a name="output_assume_role_policy"></a> [assume\_role\_policy](#output\_assume\_role\_policy) | The assume role policy document in JSON format. |
| <a name="output_id"></a> [id](#output\_id) | The stable and unique string identifying the role |
| <a name="output_instance_profile"></a> [instance\_profile](#output\_instance\_profile) | Name of the ec2 profile (if enabled) |
| <a name="output_name"></a> [name](#output\_name) | The name of the IAM role created |
| <a name="output_policy"></a> [policy](#output\_policy) | Role policy document in json format. Outputs always, independent of `enabled` variable |

<!-- markdownlint-restore -->


Expand Down Expand Up @@ -272,7 +274,6 @@ Check out these related projects.
>
> <a href="https://cpco.io/commercial-support?utm_source=github&utm_medium=readme&utm_campaign=cloudposse/terraform-aws-iam-role&utm_content=commercial_support"><img alt="Request Quote" src="https://img.shields.io/badge/request%20quote-success.svg?style=for-the-badge"/></a>
> </details>

## ✨ Contributing

This project is under active development, and we encourage contributions from our community.
Expand Down Expand Up @@ -329,9 +330,7 @@ regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at

https://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
Expand Down
14 changes: 14 additions & 0 deletions examples/complete/fixtures.us-east-2.tfvars
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,17 @@ principals = {
}

inline_policy_enabled = true

assume_role_conditions = {
AWS = [
{
test = "StringEquals"
variable = "sts:ExternalId"
values = ["test-external-id"]
}
]
}

assume_role_actions = {
AWS = ["sts:AssumeRole", "sts:TagSession"]
}
5 changes: 5 additions & 0 deletions examples/complete/outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,8 @@ output "user_unique_id" {
value = module.bucket.user_unique_id
description = "The user unique ID assigned by AWS"
}

output "assume_role_policy" {
value = module.role.assume_role_policy
description = "The assume role policy document in JSON format."
}
4 changes: 2 additions & 2 deletions main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ data "aws_iam_policy_document" "assume_role" {

statement {
effect = "Allow"
actions = var.assume_role_actions
actions = lookup(var.assume_role_actions, element(keys(var.principals), count.index))

principals {
type = element(keys(var.principals), count.index)
identifiers = var.principals[element(keys(var.principals), count.index)]
}

dynamic "condition" {
for_each = var.assume_role_conditions
for_each = lookup(var.assume_role_conditions, element(keys(var.principals), count.index), [])
content {
test = condition.value.test
variable = condition.value.variable
Expand Down
7 changes: 6 additions & 1 deletion outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,9 @@ output "policy" {
output "instance_profile" {
description = "Name of the ec2 profile (if enabled)"
value = join("", aws_iam_instance_profile.default.*.name)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like tflint is failing above. Could you also change

output "instance_profile" {
  description = "Name of the ec2 profile (if enabled)"
- value       = join("", aws_iam_instance_profile.default.*.name)
+ value       = join("", aws_iam_instance_profile.default[*].name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the variables, except the new one, follow the .*. pattern. Before I change more things in this PR, I would like to understand whether this approach is preferred over #86

Copy link
Member

Choose a reason for hiding this comment

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

Yea makes sense to me. Unfortunately because the file didn't have a newline at the end of the file, tflint detects it as a change to the above block too which threw the error. If it's not corrected then the terratest will fail.

The two approaches do not have to be mutually exclusive. You can have both the object based inputs and the override. In the short term, 86 is preferable since it doesn't break the map. In the long run, when contributors are ready to release (could be today, I cannot say) a new major version of this module, this PR could be included or be the reason for it. Many times the contributors want to bundle a number of breaking changes when releasing a major version all at once so they don't have to release multiple major versions rapidly.

cc @Benbentwo

Copy link
Member

Choose a reason for hiding this comment

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

I'm leaning towards #86 for now, I will take another look at this PR shortly thereafter. Not breaking the map is good for now as stated by @nitrocode and I should be able to get a good review of this PR in soon.


output "assume_role_policy" {
value = join("", data.aws_iam_policy_document.assume_role_aggregated[*].json)
description = "The assume role policy document in JSON format."
}
6 changes: 6 additions & 0 deletions test/src/examples_complete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ func TestExamplesComplete(t *testing.T) {
expectedroleName := "eg-test-iam-role-test-" + randId
// Verify we're getting back the outputs we expect
assert.Equal(t, expectedroleName, roleName)

// Run `terraform output` to get the value of an output variable
assumeRolePolicy := terraform.Output(t, terraformOptions, "assume_role_policy")
// Verify the assume role policy contains the expected condition for sts:ExternalId
assert.Contains(t, assumeRolePolicy, "sts:ExternalId")
assert.Contains(t, assumeRolePolicy, "test-external-id")
}

// Test the Terraform module in examples/complete doesn't attempt to create resources with enabled=false.
Expand Down
14 changes: 7 additions & 7 deletions variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,19 @@ variable "policy_description" {
}

variable "assume_role_actions" {
type = list(string)
default = ["sts:AssumeRole", "sts:TagSession"]
description = "The IAM action to be granted by the AssumeRole policy"
type = map(list(string))
description = "Map of principal type to list of actions for the assume role policy. E.g. { AWS = [\"sts:AssumeRole\"], Federated = [\"sts:AssumeRoleWithSAML\"] }"
default = { AWS = ["sts:AssumeRole", "sts:TagSession"] }
}

variable "assume_role_conditions" {
type = list(object({
type = map(list(object({
test = string
variable = string
values = list(string)
}))
description = "List of conditions for the assume role policy"
default = []
})))
description = "Map of principal type to list of conditions for the assume role policy. E.g. { AWS = [], Federated = [...] }"
default = {}
}

variable "instance_profile_enabled" {
Expand Down