Check if aws_iam_policy sets policy value from reference

[fionn]

The aws_iam_policy resource's policy argument takes a string that must match the structure of a JSON-formatted IAM policy document. Often this is done by assigning to the output of jsonencode which is passed some HCL -- however, this doesn't guarantee that the string is well-formed, only that it is JSON, and can result in errors at apply-time.

The spirit of this check is to ensure that policy values are from references. In practice, it's anticipated that these will be data.aws_iam_policy_document.foo.json objects, but it also permits locals, resources and data sources of any type.

The motivation is that I occasionally come across incorrect (or not obviously correct) jsonencode(...) inputs in code review, which this would catch automatically.

If this seems reasonable, I'll do the same for aws_iam_role's assume_role_policy argument.

[wata727]

but it also permits locals, resources and data sources of any type.

I'm not sure how useful this rule is when we're allowing variables. For example, this is also valid when using an jsonencode(...) output with an incorrect input via a variable.

[fionn]

Yeah, it's not perfect, because it doesn't recursively check the source. I did consider checking if the source is a local and then checking if that was set from a reference or not, but it seemed non-trivial.

(I also originally considered checking that the policy is a data.aws_iam_policy_document.*.foo object immediately, but that seems overly conservative.)

My argument for why it's acceptable as-is is that if there's some indirection, the problem sort of lies elsewhere? Not sure it holds up, let me know if you think we need to check all the way down.

[wata727]

The important question is whether this rule is also useful to other users. If the warning can be avoided by using local variables, there may be no incentive to enable this rule.

However, I agree that recursive checking is too complex. As a compromise, one possible criterion might be whether the expression can be resolved to a static string, since the data attribute is resolved as an unknown value in TFLint.

That said, considering that cases where external files are referenced using functions such as file or templatefile are perfectly valid, I'm not sure if this rule is always useful. You'll probably have to accept the inconvenience of not being able to use external files by enabling it.

[fionn]

whether the expression can be resolved to a static string

I think this would work well.

That said, considering that cases where external files are referenced using functions such as file or templatefile are perfectly valid, I'm not sure if this rule is always useful. You'll probably have to accept the inconvenience of not being able to use external files by enabling it.

When I started this I initially wanted to only permit data.aws_iam_policy_documents, but feel it is too aggressive for a general purpose rule that others will use. I think it's a bit tricky now because the scope of what should and should not be allowed is a little fuzzy.

I think we probably do want to permit loading from files in general, since there could be some process producing well-formed policy documents in JSON format that would be valid to read from. That said, I personally don't want to permit reading from files in any Terraform I review, every time I've come across this it would have been better to use a data.aws_iam_policy_document. So it comes down to what we think is the more generally useful behaviour.

How about, emit an issue if:

• we resolve to a static string or,
• we resolve a function call that is jsonencode?

That way we permit references and loading from files and so on.

(Totally happy to emit an issue on resolving any function call also, which would catch loading from files, if you think that's not too aggressive.)

[bendrucker]

Going any further than matching on a string or object literal seems too far.

When I started this I initially wanted to only permit data.aws_iam_policy_document

In the vast majority of cases this is preferable but this is probably only achievable with AI code review and not static analysis. A file() is valid for some policy you download or otherwise generate from some external tool. Assuming you want to vendor that result rather than fetch it on every Terraform run. Eg Datadog's setup used to involve copying this policy (now they have an API that exposes required permissions):

https://docs.datadoghq.com/integrations/guide/aws-manual-setup/?tab=roledelegation#aws-integration-iam-policy

There's also every variation of getting a policy from a remote service: data.aws_s3_bucket_object, data.http_get, data.tfe_outputs, etc., all potentially valid in the right context.

[bendrucker]

we resolve a function call that is jsonencode?

I would say it should go further, emit if jsonencode called on an object literal. If you jsonencode(var.policy) because you take a policy input as a typed object, that is valid.

[wata727]

There are probably two options:

• This rule enforces that the policy is always a value that cannot be statically resolved, such as a data source. This means that an issue will be emitted if a known value is obtained with EvaluateExpr.
  • In this case, you can prevent examples that use jsonencode to assemble JSON, but since it is not possible to reference external files using file, the rule needs to be disabled in examples like Datadog.
• This rule enforces that the policy is always a value that cannot be statically resolved, however, it makes exceptions for certain functions such as file and templatefile.

I would say it should go further, emit if jsonencode called on an object literal. If you jsonencode(var.policy) because you take a policy input as a typed object, that is valid.

Hmm, I'm not sure about this. It seems to me that there are no situations where it would be more reasonable to maintain the input type of the policy variable without using data.aws_iam_policy_document.

[fionn]

As far as I can tell, EvaluateExpr produces an error on both data source attributes and calls to functions like jsonencode. Maybe I'm misunderstanding, but it seems it's not suitable to discriminate between these cases here.

So I opted to try out the second option. With this, we could also consider making the list of permitted functions configurable.

[wata727]

JSON syntax is not HCL native syntax, so we should not assume it is hclsyntax expression here.
You can determine whether an expression can be resolved statically by evaluating it as cty.Value.
https://pkg.go.dev/github.com/terraform-linters/tflint-plugin-sdk@v0.24.0/tflint#Runner

[wata727]

Please add the fixed code shown in the above examples.

[wata727]

It's a good idea to include examples demonstrating how certain functions or unknown variables are handled exceptionally, and to keep the introductory explanation as concise as possible.

[wata727]

Since there are no configs specific to this rule, this section can be deleted.

[wata727]

I would like to make the message a little clearer. I also would like to avoid overly long messages consisting of multiple sentences, but as it is, it's probably not clear what "object" refers to.