Interesting. It does look like it's needed in the execution role and surprisingly not in the task role.

/test all

I'm torn about this since Secrets are things that usually have more access restrictions and having blanket access to the secret will not work with many ISO, CIS compliant companies.

We output the roles and is easy enough to add this permission to it which in many cases is a to a few specific secrets.

Maybe we can add examples on how to do this.

Just a suggestion, but we could have a separate Resource section with the specific ARNs that should be allowed access. These would be inferred from the Secrets section of the task definition. I do not know how to do this programmatically, but I suspect it would be a foreach over each secret valueFrom like

statement {
    effect    = "Allow"
    resources = ["arn:aws:secretsmanager:us-east-1:<account id>: secret :<secret name 1>",
                 "arn:aws:secretsmanager:us-east-1:<account id>: secret :<secret name 2>"
      ]

    actions = [
      "secretsmanager:GetSecretValue",
    ]
  }

Alternatively, pass a wildcard path for granting access as a variable.

secret_path=foo_app/staging/*

    effect    = "Allow"
    resources = ["arn:aws:secretsmanager:us-east-1:<account id>: secret :var.secret_path
      ]

    actions = [
      "secretsmanager:GetSecretValue",
    ]
  }

that will work, it will give you the flexibility and only permission to the specific ARNs of the secrets. @EvilPlankton could you update your PR with that?

We output the roles and is easy enough to add this permission to it which in many cases is a to a few specific secrets.

I agree more with this statement than integrating this PR. It seems a lot easier and allows this module to be less rigid. If we can easily attach a role policy outside of the module, why would we bake it in here if we don't have to?

I agree with @nitrocode and @jamengual - we should just use IAM policy attachments on the role outside of this module.

Perhaps this example would help. Like @jamengual said, we could add this to the docs to make it more clear.

module "ecs_alb_service_task" {
  source = "git::https://github.com/cloudposse/terraform-aws-ecs-alb-service-task.git?ref=master"

  ...
}

resource "aws_iam_role_policy" "ssm" {
  name   = "ssm"
  policy = data.aws_iam_policy_document.ssm.json
  role   = module.ecs_alb_service_task.task_exec_role_name
}

data "aws_iam_policy_document" "ssm" {
  statement {
    sid    = ""
    effect = "Allow"

    actions = [
      "secretsmanager:GetSecretValue",
    ]

    resources = [
      ...
    ]
  }
}

Perhaps this example would help. Like @jamengual said, we could add this to the docs to make it more clear.

module "ecs_alb_service_task" {
  source = "git::https://github.com/cloudposse/terraform-aws-ecs-alb-service-task.git?ref=master"

  ...
}

resource "aws_iam_role_policy" "ssm" {
  name   = "ssm"
  policy = data.aws_iam_policy_document.ssm.json
  role   = module.ecs_alb_service_task.task_exec_role_name
}

data "aws_iam_policy_document" "ssm" {
  statement {
    sid    = ""
    effect = "Allow"

    actions = [
      "secretsmanager:GetSecretValue",
    ]

    resources = [
      ...
    ]
  }
}

yes the example should more than enough for people to implement it, I will only suggest to add a Secret manager Example and Parameter Store example.

@EvilPlankton do you want to have a stab at updating the docs?

@EvilPlankton ping

Let us know if you're interested in taking this up @EvilPlankton and we'll re-open.