hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.77k stars 9.12k forks source link

[Enhancement]: Validate for MalformedPolicyDocument during plan step #37900

Open nicolaschambrier opened 3 months ago

nicolaschambrier commented 3 months ago

Terraform Core Version

1.5.5

AWS Provider Version

5.53.0

Affected Resource(s)

Expected Behavior

By mistake we included a aws_iam_policy statement with an empty list of resources

data "aws_iam_policy_document" "apps_ci" {
  # [...] a bunch of valid statements
  statement {
    actions = [
      "sqs:*",
    ]
    resources = var.queues_arn
  }
}

This failed at apply time, causing the policy to obviously not being applied.

Actual Behavior

It's a totally predictible error, that should have failed at plan time. It could have caused severe deployment issues due to partial apply.

Relevant Error/Panic Output Snippet

No response

Terraform Configuration Files

data "aws_iam_policy_document" "this" {
  statement {
    actions = [
      "sqs:*",
    ]
    resources = []
  }
}
resource "aws_iam_policy" "lambda_deployment_policies" {
  name        = "policy"
  policy = data.aws_iam_policy_document.this.json
}

Steps to Reproduce

Debug Output

No response

Panic Output

No response

Important Factoids

No response

References

No response

Would you like to implement a fix?

None

github-actions[bot] commented 3 months ago

Community Note

Voting for Prioritization

Volunteering to Work on This Issue

sourcefrog commented 3 months ago

I'm potentially interested in working on this. Would a PR to do this be in principle acceptable? Is there any prior art for turning optional preflight checks on and off?

AWS has a ValidatePolicy API which Terraform could call at plan time.

justinretzolk commented 3 months ago

Relates #18935

In these type of scenarios -- where we would be adding additional functionality (validations) to an existing resource -- we would consider this to be an enhancement. With that in mind, I'm going to update the title a bit to reflect that. There's not any further action required at this time, I just like to give people a heads up before making those kinds of edits.

@sourcefrog, nothing immediately comes to mind at the moment in terms of prior art here, but we're always open to PRs! One thing that immediately comes to mind is that adding calls to APIs outside of what someone should expect is something we generally wouldn't be agreeable to. The credentials being used to authenticate the AWS provider while using aws_iam_policy_document data sources may not allow access to IAM Access Analyzer, leading to unexpected errors. I suspect this is why the team hasn't implemented more thorough validations for the aws_iam_policy_document data source, but I'll leave this issue open for now so others can chime in as well.

sourcefrog commented 3 months ago

Good point about Access Analyzer potentially being blocked by an SCP or something.

Maybe we could have an option on the resource or even on the provider that opts in to plan-time validation?

sidewinder12s commented 2 months ago

Yes that was my thought was make it an optional parameter on the data resource to run the generated IAM Policy Document through Access Analyzer for policy validation only.

That or just add a data resource against IAM Access Analyzer you'd run your policy through, though that is less nice, but that might fit within the Terraform ecosystem more.