fix: Explicitly assume with condition matching role arn

[FernandoMiguel]

Signed-off-by: Fernando Miguel github@FernandoMiguel.net

Description

Fix the self assume role using a condition

Motivation and Context

The policy can not use the role arn in the principal field until the role already exists.
And since we can't create a role without a trust policy, we have chicken-egg problem.

Using a condition to match the role arn instead of using it in the principal, we are able to work around the problem.

Breaking Changes

How Has This Been Tested?

• I have updated at least one of the examples/* to demonstrate and validate my change(s)
• I have tested and validated these changes using one or more of the provided examples/* projects

• I have executed pre-commit run -a on my pull request

tested this against one of our cluster, using my own fork branch

fixes: #282

[bryantbiggs]

if I understand the issue linked - this is still not valid, no? If a user uses role_name_prefix, even the changes in this PR do not address that scenario, correct?

[FernandoMiguel]

@bryantbiggs there is no way to use role_name_prefix cause that is only known after role being created.
in this PR i am only addressing the fact that the policy was not valid, which is now, making this module usable in our use-case with argocd.

I still think we should default role_path to "/" which is the entirely most common scenario.
and for role_name_prefix maybe a condition in the var if allow_self_assume_role is present

[FernandoMiguel]

this is a sample of the plan output

  # module.base_system.module.argocd[0].module.argocd_iam_role_sa_cross_cluster.data.aws_iam_policy_document.this[0] will be read during apply
  # (config refers to values not yet known)
 <= data "aws_iam_policy_document" "this" {
      + id   = (known after apply)
      + json = (known after apply)

      + statement {
          + actions = [
              + "sts:AssumeRole",
            ]
          + effect  = "Allow"
          + sid     = "ExplicitSelfRoleAssumption"

          + condition {
              + test     = "ArnLike"
              + values   = [
                  + (known after apply),
                ]
              + variable = "aws:PrincipalArn"
            }

          + principals {
              + identifiers = [
                  + "*",
                ]
              + type        = "AWS"
            }
        }
      + statement {
          + actions = [
              + "sts:AssumeRoleWithWebIdentity",
            ]
          + effect  = "Allow"

          + condition {
              + test     = "StringEquals"
              + values   = [
                  + "sts.amazonaws.com",
                ]
              + variable = "oidc.eks.us-east-1.amazonaws.com/id/XXXX:aud"
            }
          + condition {
              + test     = "StringEquals"
              + values   = [
                  + "system:serviceaccount:argocd-system:argocd-application-controller",
                  + "system:serviceaccount:argocd-system:argocd-server",
                ]
              + variable = "oidc.eks.us-east-1.amazonaws.com/id/XXXX:sub"
            }

          + principals {
              + identifiers = [
                  + "arn:aws:iam::XXXX:oidc-provider/oidc.eks.us-east-1.amazonaws.com/id/XXXX",
                ]
              + type        = "Federated"
            }
        }
    }

[bryantbiggs]

so a few things here:

1. The whole premise for using the manually constructed ARN of "arn:${data.aws_partition.current.partition}:iam::${data.aws_caller_identity.current.account_id}:role${var.role_path}${var.role_name}" is because of this chicken vs the egg scenario. We can't reference the IAM role ARN in the assume role policy because the role needs the policy but the policy needs the role. So we "cheat" in these scenarios and re-construct what the result will be so this should be valid today for users who use just role_name because its all static data known at plan/apply time
2. The role path most likely needs to be updated. The provider allows for null which is interpreted as "/" but with our re-construction of the ARN we have to be explicit and set the default as "/" and can't use the null trick
3. We can, and should, address the role_name_prefix here. Simply doing something like a coalesce(var.role_name, var.role_name_prefix) would suffice in the ArnLike changes here (I think). Or we can have two values, one for var.role_name and one for var.role_name_prefix which might need to end with * to match correctly

[FernandoMiguel]

Using a wildcard in the case of prefix might work, did not test that.
I can try to code the changes Friday, as I'm mostly offline tomorrow.

I entirely agree with #2

[FernandoMiguel]

@bryantbiggs I've tried to address your concerns

[FernandoMiguel]

HI team,
Can this be reviewed?

Thanks in advance.

[bryantbiggs]

please see the failed CI checks - these should all be a local

Suggested change

	values = ["arn:${local.partition}:iam::${local.account_id}:role${var.role_path}${var.role_name_condition}"]
	values = ["arn:${local.partition}:iam::${local.account_id}:role${var.role_path}${local.role_name_condition}"]

[FernandoMiguel]

iam-assumable-roles and iam-assumable-roles-with-saml don't have a single role, but 3, which makes this much harder to do.
open to suggestions to deal with the self reference block

[FernandoMiguel]

I've added some code to handle 3 roles.
but can't really support name_prefix on those, without it being a breaking change.

[FernandoMiguel]

Team, pls review.
Thanks

[bryantbiggs]

still have some pre-commit checks to fix but overall I think it looks good - @antonbabenko thoughts?

[FernandoMiguel]

@bryantbiggs pre-commit ran, and docs updated

[FernandoMiguel]

Hello folks,
Can this PR be review?
Thanks in advance.

[antonbabenko]

@FernandoMiguel Thank you very much for this PR!

[antonbabenko]

This PR is included in version 5.5.2 🎉

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.