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

[pablotp]

what

With this PR, you can now specify both actions and conditions per principal type, just as you can already specify principals. This allows you to generate policies with multiple statements, each with its own principal(s), action(s), and condition(s).

Breaking change

You now specify both assume_role_actions and assume_role_conditions as maps, keyed by principal type:

    # Old
    assume_role_actions = ["sts:AssumeRole", "sts:TagSession"]
    assume_role_conditions = [
      {
        test     = "StringEquals"
        variable = "sts:ExternalId"
        values   = ["example"]
      }
    ]

    # New (for AWS principal)
    assume_role_actions = {
      AWS = ["sts:AssumeRole", "sts:TagSession"]
    }
    assume_role_conditions = {
      AWS = [
        {
          test     = "StringEquals"
          variable = "sts:ExternalId"
          values   = ["example"]
        }
      ]
    }

why

Previously, the module only supported a single list of actions and a single set of conditions for all assume role policy statements. This made it impossible to generate assume role policies where different principals (e.g., AWS, Federated) require different actions and/or different conditions in their respective statements.
AWS IAM supports policies with multiple statements, each with different principals, actions, and conditions. For example, you may need to allow certain AWS roles to assume a role with sts:AssumeRole, while also allowing a federated identity provider to assume the same role with sts:AssumeRoleWithWebIdentity and a specific condition.

Example of an assume policy that couldn't be generated before

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": [
          "arn:aws:iam::111122223333:role/app-prod-ci-agent",
          "AROAEXAMPLEID1",
          "arn:aws:iam::444455556666:role/ci-agent",
          "arn:aws:iam::111122223333:role/app-prod-use1-mz-4-ci-agent"
        ]
      },
      "Action": "sts:AssumeRole"
    },
    {
      "Effect": "Allow",
      "Principal": {
        "Federated": "arn:aws:iam::444455556666:oidc-provider/oidc.eks.us-east-1.amazonaws.com/id/EXAMPLEOIDC"
      },
      "Action": "sts:AssumeRoleWithWebIdentity",
      "Condition": {
        "StringEquals": {
          "oidc.eks.us-east-1.amazonaws.com/id/EXAMPLEOIDC:sub": "system:serviceaccount:ci:ci-agent"
        }
      }
    }
  ]
}

With the previous implementation, you could not generate the second statement with a different action (sts:AssumeRoleWithWebIdentity) and a different condition for the Federated principal.

references

• Backwards compatible alternative: feat: Allow overriding the whole assume policy #86
• This feature will then be used by the Atmos component (chore: Update the iam-role module to v1.0.0 cloudposse-terraform-components/aws-iam-role#27).

[nitrocode]

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)

[pablotp]

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

[nitrocode]

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

[Benbentwo]

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.

[mergify]

💥 This pull request now has conflicts. Could you fix it @pablotp? 🙏