better AWS policy layering

[tvald]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

#2890 resolved #2672 with a source_json and override_json field which enables layering of AWS policies. We leverage this mechanism to merge S3 bucket policies from a library of policies, since a bucket can only have one policy (cf #409).

Unfortunately, only two policies can be combined at a time (one each as source_json and override_json), leading to multi-step merges with temporary intermediate policies. You also have to know the name of a statement in the override_json policy in order to insert a dummy statement field, since the statement field is required even if you're combining other policies.

I propose:

1. make the statement field optional
2. change source_json and override_json to accept arrays, where priority for duplicate statements is given to later elements in the array

This makes it easy to merge any number of policies.

Affected Resource

• aws_iam_policy_document

Potential Terraform Configuration

Current:

data "aws_iam_policy_document" "tmp_merge" {
  source_json = "${data.aws_iam_policy_document.DenyIncorrectEncryptionHeader.json}"
  override_json = "${data.aws_iam_policy_document.DenyUnencryptedObjectUploads.json}"
  statement {
    sid = "DenyUnencryptedObjectUploads" # will be overridden
  }
}

data "aws_iam_policy_document" "secure_bucket" {
  policy_id = "SecureBucketPolicy"
  source_json = "${data.aws_iam_policy_document.tmp_merge.json}"
  override_json = "${data.aws_iam_policy_document.DenyUnencryptedConnections.json}"
  statement {
    sid = "DenyUnencryptedConnections" # will be overridden
  }
}

Proposed:

data "aws_iam_policy_document" "secure_bucket" {
  policy_id = "SecureBucketPolicy"
  source_json = [
    "${data.aws_iam_policy_document.DenyIncorrectEncryptionHeader.json}",
    "${data.aws_iam_policy_document.DenyUnencryptedObjectUploads.json}",
    "${data.aws_iam_policy_document.DenyUnencryptedConnections.json}",
  }
}

References

• Create a way to modify/merge changes into aws_iam_policy_document #2672
• data-source/aws_iam_policy_document: Support layering via source_json and override_json attributes #2890
• Allow multiple policies to be attached to an s3 bucket #409

[lorengordon]

I'd like to see this. statement should definitely be optional now. Though, it seemed when I tried to use both source_json and override_json on the same data source that only source_json was effective.

[skang0601]

This would be a very useful feature for my org. I'd like to start supplying a centralized repository of IAM Policy documents for other teams to consume.
Creating a bunch of IAM Policy is not very scalable due to policy attachment limits on IAM roles. If I can just supply the IAM policy documents and let users craft their own custom policies, it would be a much smoother process.

[YakDriver]

@tvald @skang0601 @lorengordon Have a look at PR #6052 to see if this will help with the overall solution.

[tvald]

@YakDriver commented in #6052:

@tvald Thanks for the input! I'm not currently working on part 2 of #5047 but wanted to get your thoughts on the design approach to part 2: a new data source (perhaps aws_iam_policy_list) with 1 main attribute (perhaps policy_json_list) and possibly a policy_id attribute. The list of policies would just be merged in order so those coming later would override earlier. The list, of course, could include aws_iam_policy_document data sources.

Sure, the approach sounds fine to me and solves this issue.

[bflad]

I'm not sure if we need to create a separate data source just for accepting lists of sources/overrides -- adding new TypeList attribute(s) that conflict with the existing attributes and performs the merging through each element of the list iteratively seems like it should work just fine.

I mention attributes plurally because it enables this scenario if something like source_jsons/source_json_list and override_jsons/override_json_list are available:

• source_json_list: Accepts multiple JSON strings and iteratively merges them. If any statement ID collisions are found, it can return an error.
• override_json_list: Accepts multiple JSON strings and iteratively merges them by overriding previous statements if there are statement ID collisions.

[tvald]

Sure, that approach also solves this issue. I have no preference between a new data source or a new attribute.

The slightly different semantics of source_json_list vs override_json_list could trip folks up, though, so it would need to be clearly documented.

[eterna2]

any updates on this?
if not what are the workaround now?

[scalen]

Just bumping this issue, as the associated PR seems to have gone stale for some reason. It still seems very useful, and has seen other interest (besides myself) in the last few hours, despite being almost a year old: #12055 (comment).

[YakDriver]

We have merged #12055 in to the Terraform AWS Provider. With this, aws_iam_policy_document provides the ability to merge multiple source and override policy documents. This is available now on the main branch and generally when version 3.28.0 is released (likely Feb. 11, 2021). If you have problems with the functionality or need further enhancements, please open a new issue. Thanks for your interest in the AWS Provider! 🎉

[ghost]

This has been released in version 3.28.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!