Prevent annotations from disabling a custom lint rule

[joe-a-t]

Introduction

We have a custom ruleset that we maintain. For some of those rules, we are fine with people putting annotations on violating resources/data sources to ignore the lint rule for their use case. However, there are other situations where no one at our company under any circumstances should be allowed to violate the lint rule. Examples of this can include lint rules that prevent security vulnerabilities.

Proposal

Allow a way for the creators of rulesets to specify that a given rule cannot be disabled by annotations. This could potentially also prevent disabling via config file, but that is a bit of an open question and not as vital since config files are generally easier to control with CODEOWNERS and other mechanisms vs annotations in random .tf files.

References

• https://github.com/terraform-linters/tflint/blob/master/docs/user-guide/annotations.md
• https://github.com/terraform-linters/tflint/blob/master/docs/user-guide/config.md
• https://github.com/terraform-linters/tflint-plugin-sdk

[bendrucker]

Moving this to a discussion as I don't think this is something we'd pursue right now but want to leave it somewhere where people who want this can weigh in. The issue is this:

Examples of this can include lint rules that prevent security vulnerabilities.

This is not a primary supported use of TFLint. TFLint is a static checker, designed to catch issues early, based only on the configuration. It is not a policy enforcement tool for Terraform resources.

Policy enforcement (e.g., all S3 buckets must have a Department tag) can only be performed correctly/consistently/robustly on the plan, not the configuration. Sentinel and OPA are two of the many mature tools that support this. Since the plan is represented as JSON instead of HCL, it's much easier to integrate into generic document-oriented policy tools like OPA.

When you attempt to use static analysis for enforcement you end up with logic that is easily defeated by a bit of dynamic code, which the analyzer has to ignore if it can't handle.

The only security uses for TFLint would be in looking for risky configuration, e.g. file() calls to privileged dirs or specific resources you want to forbid.

And even then, you're thinking about ways to hard-block users from doing things, but that's a fraught path. All it takes to bypass a rule is indirecting through a module:

module "my_dangerous_thing" {
  source = "ns/dangerous-things/null
}

This could be a workaround but it could entirely innocuous. So then you need to forbid users from sourcing any un-trusted module to prevent this workaround. This type of enforcement thus becomes a cascading effort of locking down functionality to prevent workarounds. I don't want TFLint engaged in "performative security," i.e. the appearance of strong guarantees that in practice are trivially defeated.

A good place to start building your case would be to share the rules you've authored where you think you want to enforce this.

[bendrucker]

Separately, there's little need to integrate this with the TFLint lifecycle, and you could easily enforce this yourself with custom tooling. It doesn't really make sense for TFLint to run, produce issues, and then have to confront the fact that an issue is being raised whose ruleset doesn't want it being ignored.

Instead, you could just go through your configuration files up front and error if an annotation exists for a rule you don't want ignored. It doesn't really matter whether that annotation is actually suppressing an issue, it's invalid for it to be declared at all. Waiting until it's "triggered" to then ignore the issue and warn is awkward.

Certainly not as convenient as TFLint doing all the work for you but it's an option you can pursue today.

[joe-a-t]

So a couple of points to respond to in there-

First, we already have lint rules enforcing that all modules come from a trusted module source, that is one of the rules where we would like to use this new option.

As far as other use cases where a policy enforcement tool would not work, we have several of those related to extremely dangerous Terraform data sources. For example, there are a bunch of data sources that the Google provider has for getting a token for the current or a different identity like service_account_access_token. There is also the even more dangerous external data source. If your standard, automated workflow involves automatically running a terraform plan on PRs after the linter has passed, these data sources could both be used to exfiltrate the credentials that the automated workflow is running as or worse, even though terraform apply never gets run.

Yes, there are workaround that we could do with custom tooling, such as scanning the configuration files upfront. And we are pursing that course of action. But I think that, particularly for the dangerous data sources, this could be a fairly common issue people see and a more open source solution, preferably already integrated into commonly used tooling, would be extremely helpful.

[bendrucker]

Thanks, this is helpful. external_data_source is a good case.

But I think that, particularly for the dangerous data sources, this could be a fairly common issue people see...

The issue is more that these risks are numerous and hard to identify than a need for maximally strict enforcement. There is nothing inherently wrong or unsafe with using the external data source. It is only truly dangerous when executing untrusted code. Most organizations do not operate this way and effectively trust their employees, especially in a CI environment where an approver is not required to achieve execution. To the extent they are even implementing any security checks at all, they're generally going to be warnings that the user can choose to dismiss or override with an annotation.

I'd be interested to see any other examples of tools that implement similarly strict policy protections, as I can't come up with any examples. Even on the server side, GitHub allows anyone with push access to dismiss secret scanner warnings:

https://docs.github.com/en/enterprise-cloud@latest/code-security/secret-scanning/protecting-pushes-with-secret-scanning#allowing-a-blocked-secret-to-be-pushed

I've never encountered a static analysis tool that supports inline ignores but can also offers an option for rejecting ignores. tfsec certainly doesn't support it:

https://github.com/aquasecurity/tfsec#ignoring-warnings

Given the niche audience, the likelihood of the much larger task of expanding the plugin protocol to accommodate this is very low, but we could consider a contribution to the config for the core CLI. Something like this:

config {
  annotations {
    ignore {
      forbidden = ["my_org_dangerous_resource"]
    }
  }
}

If you have access to the configuration you can shut off the plugin entirely so protecting this file via branch protection with required code owner approval would be required anyway in your scenario.

[wata727]

Interesting discussion. I basically agree with Ben, but I believe TFLint should be able to support more cases than just possible errors.

Indeed, TFLint's current evaluation system design is more focused on "proving incorrectness rather than correctness", such as ignoring unknown values instead of raising errors. This does not lend itself to policy enforcement.

However, I believe TFLint should be a framework for all your desires to perform inspections on Terraform's configuration files. Parsing the Terraform language is a complex task, and developing another tool would probably be hard work. I would like to expand the plugin ecosystem to support more cases, and tflint-ruleset-opa is one of those experiments.

In this regard, I think it would be a good idea to implement rules for annotations, as Ben also suggested. The Ruby linter RuboCop also has rules that satisfy similar needs.
https://docs.rubocop.org/rubocop/1.45/cops_style.html#styledisablecopswithinsourcecodedirective

I'm not sure if this should be provided by TFLint core (maybe in tflint-ruleset-terraform, but I'm not sure if it's appropriate), but you can start as a custom rule first.

[joe-a-t]

I'm not sure if this should be provided by TFLint core (maybe in tflint-ruleset-terraform, but I'm not sure if it's appropriate), but you can start as a custom rule first.

@wata727 I wanted to get a bit of clarity on this. Are you saying that there is a way today that we can disable annotation on a specific custom rule without any changes to the core TFLint functionality? If so, how could we do that?

[wata727]

There is no way to disable annotations, but warning against annotations should make them unavailable, except for self-referential annotations.

module "my_dangerous_thing" {
  # tflint-ignore: custom_dangerous_module_source
  source = "ns/dangerous-things/null
}

1 issue(s) found:

Error: Inline disable annotation is not allowed (custom_inline_disable_annotations)

  on main.tf line 2:
   2:   # tflint-ignore: custom_dangerous_module_source

However, you probably can't warn against annotations like this:

module "my_dangerous_thing" {
  # tflint-ignore: all
  source = "ns/dangerous-things/null
}