Increase clarity of intention of CKV_TF_1

[DevOpsBoondoggles]

Describe the issue
CHK_TF_1

I think you are referring to the new CKV_TF_1 check. Check out my comment here and read the blog post #5265 (comment) @gruebel

Hi there, is it possible to maybe increase the clarity of this and say something like "Terraform Registries are vulnerable to Supply Chain Attacks". I notice this Issue has been logged multiple times as myself and others are not making the leap between the check and error message without spending time digging into these github issues.

I am assuming you are trying to say that Terraform Registries are not the best in terms of security compared to a git repo and commit hash and therefore wanting people to bypass the check as indication they understand this? ?

[gruebel]

hey @gabrielmccoll thanks fore reaching out.

This check is not about Terraform registries, it is about module sources. So, even if you use a git url with a tag reference, then you are vulnerable. So, the suggested name wouldn't help and gives you no indication of solving it.

[DevOpsBoondoggles]

Right but @gruebel my point was clearly not explained well enough.
There is currently no way to satisfy this TF CHK if you use a Terraform Registry. So I'm suggesting you add something to the message to let people know that because otherwise people will keep thinking it's just a bug and raising issues ?

[freshvolk]

This is a classic "security vs education" issue in that yes, it is undoubtedly safer for the user to directly reference a repo and a specific hash. However, if you do not provide clear instruction the users will ignore the recommendation and it will become functionally worthless given what @gabrielmccoll mentioned. I suggest a naming change to this error to indicate that terraform modules should be git sourced with explicit commit hashes only.

Security that users workaround is poor security because you have failed to make it accessible.

[verma-rajatk]

We prefer using tags
source = "git@github.com:some_repo.git//modules/some_module?ref=1.1.1"

so just skipped this issue using:
#checkov:skip=CKV_TF_1

[ricmalta]

We prefer using tags source = "git@github.com:some_repo.git//modules/some_module?ref=1.1.1"

so just skipped this issue using: #checkov:skip=CKV_TF_1

This rule must be re-evaluated as the implementation is very much opinionated by a specific use case. Using tags is a much better practice, and as @verma-rajatk that's how software is versioned, not with commit ids.
However, I believe it's fine to support both, but please don't block a good practice of tagging and use the tags.

[gruebel]

@ricmalta this is completely wrong. Did you actually read the blog post?

Tags are not a good practice in any of the current tooling, look at GitHub Actions or Docker images. Tags are mutable they are nothing more than a bookmark of a specific code state, which you can change anytime you like.

Try to argue against the OpenSSF https://github.com/ossf/scorecard/blob/main/docs/checks.md#pinned-dependencies

[DevOpsBoondoggles]

@ricmalta this is completely wrong. Did you actually read the blog post?

Tags are not a good practice in any of the current tooling, look at GitHub Actions or Docker images. Tags are mutable they are nothing more than a bookmark of a specific code state, which you can change anytime you like.

Try to argue against the OpenSSF https://github.com/ossf/scorecard/blob/main/docs/checks.md#pinned-dependencies

Just to be clear. I am not disagreeing with this what you say or claiming tags and terraform registry are better practice.

I am pointing out the check and resulting error message lead to confusion and that you would be served best by increasing the clarity of it.
People shouldn't have to come to GitHub searching issues and reading a blog post to try and understand what you're attempting to achieve ?

I also feel the fact there's follow up comments from people on this showing exactly the misunderstanding my issue raised as very likely to be happening is evidence that maybe it should be reopened and the feedback from this check could be tweaked

[gruebel]

@gabrielmccoll I have no problem to adjust the name of the check, but till now no suggestion fits and it will still raise questions.

The current name Ensure Terraform module sources use a commit hash clarifies you need to use a commit hash, when referencing a module.
Your suggestion Terraform Registries are vulnerable to Supply Chain Attacks just tells you not to use TF registry, which is not correct, you also shouldn't use git tags, plain HTTP, S3, GCS or any other private registry, if it can't enforce immutability.

If you look at the other check names, you can see we typically say things like, enable logging, use CMK, disable public access. We don't add extra information to why you should enable logging or use CMK, this is done via the guidelines and they typically come later, because of a changed release process of the documentation.

[DevOpsBoondoggles]

Okay well maybe once the documentation comes out it will help.

I'm sure you can appreciate its very counter intuitive for people to hear that package managers and versions are not secure in general and only git commit tags are.

[ricmalta]

hich you can change anytime you like.

I understand and agree with the blog-post conclusions and with you. However, this doesn't change the fact this is an opinionated implementation and will result in basically most of the implementation using tags to skip this rule entirely.

In my specific case, there are no external terraform registries being used. It's all private individual repositories for each terraform module, living in a private GitHub enterprise (kind of private registry). Example:

module "redis" {
  source = "git::https://git.OWN_DOMAIN.com/ORG_NAME/terraform-module-redis?ref=v1.1.0"
...

or

module "services_vnet" {
  source = "git::https://git.OWN_DOMAIN.com/ORG_NAME/terraform-module-network?ref=v3.0.0"
...

I posted here in this issue because the @gabrielmccoll point is precisely the issue. The rule is too generic but based on a particular use case: "Terraform Registries are vulnerable to Supply Chain Attacks" and should be used safely.

I'm sorry if my first message was rude. That was not the intention.

[DevOpsBoondoggles]

@ricmalta I believe your gut tags are also vulnerable. I think the point being made is that only hashes are immune to supply chain corruption.
Even if it's an internal registry or git repo. Someone can still swap tags. They can't swap hashes.

[laminarcode]

The error CKV_TF_1 error message needs to be improved. It should explain, or point to a document which explains, the vulnerability and solution, including a discussion on terraform registries and why/how or why not/how not to use them.

The error "Ensure Terraform module sources use a commit hash" is insufficient. A Terraform module source reference to a terraform registry does not accept a commit hash. It is therefore this check which is incorrect as it checks for a hash when it is syntactically incorrect to provide one.

If you have a security concern reference to terraform registries, please document an explanation and solution, and even better fix it within the existing language paradigm.

[sergei-ivanov]

Apart from hashicorp/terraform#31463 referenced earlier, there is a couple of additional related Terraform issues that you might want to vote for:

• Lock module versions, like providers, in .terraform.lock.hcl hashicorp/terraform#31301
• Pin modules by Commit SHA hashicorp/terraform#29867

My personal preference would be to extend the lock file to contain module versions and hashes for non-local modules. That way we would still be able to both use version ranges and perform upgrades in a controlled fashion (like we currently do with providers).