Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3rd party: checkov] Checkov runs for all files, not just .tf #284

Closed
mj3c opened this issue Nov 17, 2021 · 7 comments
Closed

[3rd party: checkov] Checkov runs for all files, not just .tf #284

mj3c opened this issue Nov 17, 2021 · 7 comments
Labels
3rd party Issues not related to `pre-commit-terraform` hook/terraform_checkov Bash hook question

Comments

@mj3c
Copy link
Contributor

mj3c commented Nov 17, 2021

Describe the bug

I have a problem with the checkov hook running against the entire repo, checking all files, instead of only .tf files and specifically a subdirectory of them. For example, if you have this:

project
│   Dockerfile  
│   .pre-commit-config.yaml
│
└───terraform
        main.tf

And if you run pre-commit run -a, checkov will fail at any Dockerfile errors.

How can we reproduce it?

Dockerfile example that will fail checkov:

FROM ubuntu:18.04

Pre-commit config:

repos:
  - repo: git://github.com/antonbabenko/pre-commit-terraform
    rev: v1.56.0
    hooks:
      - id: checkov

Environment information

  • OS: Ubuntu 20.04
  • uname -a and/or systeminfo | Select-String "^OS" output:
Linux 5.11.0-38-generic #42~20.04.1-Ubuntu SMP Tue Sep 28 20:41:07 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
  • Tools availability and versions:
pre-commit 2.15.0
Terraform v1.0.7
Python 3.8.10
checkov 2.0.580

Additional information

I believe this is because of the following setting for the checkov hook:

  entry: checkov -d .

Even though files: is set, checkov still seems to check all files. The checkov CLI supports regex in the --skip-path argument, so perhaps this can be resolved with something like the following?

  entry: checkov -d . --skip-path '.*(?<!\.tf)$'
@mj3c mj3c added area/local_installation bug Something isn't working labels Nov 17, 2021
@yermulnik
Copy link
Collaborator

yermulnik commented Nov 17, 2021

And if you run pre-commit run -a, checkov will fail at any Dockerfile errors.

Could you please provide a snippet from the execution output to demonstrate details? Along with checkov hook config blocks from your .pre-commit-config.yaml and .pre-commit-hooks.yaml files.

As far as you can see from https://github.com/antonbabenko/pre-commit-terraform#checkov, this hook's args can be configured per individual use cases (alongside any other per-hook pre-commit config options: https://pre-commit.com/#plugins).

Even though files: is set, checkov still seems to check all files.

I lean to think that's the expected behavior for the defaults of pass_filenames: false passed to checkov hook config here: https://github.com/antonbabenko/pre-commit-terraform/blob/master/.pre-commit-hooks.yaml#L100

@mj3c
Copy link
Contributor Author

mj3c commented Nov 17, 2021

I have posted the full .pre-commit-config.yaml file in my question already.

Here is the command I execute and the output:

$ pre-commit run --files ./terraform/*

Checkov..................................................................Failed
- hook id: checkov
- exit code: 1

dockerfile scan results:

Passed checks: 1, Failed checks: 2, Skipped checks: 0

Check: CKV_DOCKER_7: "Ensure the base image uses a non latest version tag"
	PASSED for resource: /Dockerfile.
	File: /Dockerfile:1-1
	Guide: https://docs.bridgecrew.io/docs/ensure-the-base-image-uses-a-non-latest-version-tag

Check: CKV_DOCKER_2: "Ensure that HEALTHCHECK instructions have been added to container images"
	FAILED for resource: /Dockerfile.
	File: /Dockerfile:1-1
	Guide: https://docs.bridgecrew.io/docs/ensure-that-healthcheck-instructions-have-been-added-to-container-images

		1 | FROM ubuntu:18.04


Check: CKV_DOCKER_3: "Ensure that a user for the container has been created"
	FAILED for resource: /Dockerfile.
	File: /Dockerfile:1-1
	Guide: https://docs.bridgecrew.io/docs/ensure-that-a-user-for-the-container-has-been-created

		1 | FROM ubuntu:18.04

I did manage to work around this by updating the .pre-commit-config.yaml like so:

repos:
  - repo: git://github.com/antonbabenko/pre-commit-terraform
    rev: v1.56.0
    hooks:
      - id: checkov
        args:
          - --skip-path
          - .*(?<!terraform\/)$
#          - .*(?<!\.tf)$    # something like this can also do the trick

This is okay in my case, it was just not intuitive at first. I would expect it to not scan the Dockefile, or really anything but the ./terraform/* files, but maybe that's just me. I guess it is because of the default behavior (pass_filenames: false) as you mentioned.


P.S. I don't believe you can pass the -d <dir> argument to checkov hook as mentioned in the README, because it is set as a default here: https://github.com/antonbabenko/pre-commit-terraform/blob/master/.pre-commit-hooks.yaml#L98

From pre-commit docs:

entry | the entry point - the executable to run. entry can also contain arguments that will not be overridden such as entry: autopep8 -i.

@yermulnik
Copy link
Collaborator

This is okay in my case, it was just not intuitive at first.

Good to know you've worked around this.
If you have an idea of how to improve REAMDE to eliminate any confusions consumers may encounter, we'd appreciate PR(s) — realworld use cases are what in my opinion are the best building bricks for the howto's describe in README.

I would expect it to not scan the Dockefile, or really anything but the ./terraform/* files, but maybe that's just me.

Since the pre-commit-terraform is supposed to be used for the repo which contains specifically and exclusively Terraform configurations (I assume), which may have non-TF files as part of configuration (e.g. files used as templates etc including Docker files), I'd say it may be an expected behavior.

Should we close this issue at this point?

@mj3c
Copy link
Contributor Author

mj3c commented Nov 17, 2021

Yup, that also makes sense.

The confusion came from the fact that the rest of the terraform hooks seem to respect the --files <path> argument when they are checking files (you can check only a subdirectory, if you want), but checkov is an exception, and by default checks everything in the repo.

Thanks for the quick responses!

@mj3c mj3c closed this as completed Nov 17, 2021
@MaxymVlasov MaxymVlasov added 3rd party Issues not related to `pre-commit-terraform` hook/terraform_checkov Bash hook question and removed bug Something isn't working area/local_installation labels Nov 17, 2021
@MaxymVlasov MaxymVlasov changed the title Checkov runs for all files, not just .tf [3rd party: checkov] Checkov runs for all files, not just .tf Nov 17, 2021
@entscheidungsproblem
Copy link

Can this be reopened to allow the -d flag to be passed to checkov? This looks like a workaround since, as @mj3c mentioned, the -d flag currently cannot be overridden.

@MaxymVlasov
Copy link
Collaborator

@entscheidungsproblem did you try to override it using the example in https://github.com/antonbabenko/pre-commit-terraform#checkov ?

If not, #290 should add that possibility

@entscheidungsproblem
Copy link

Hi @MaxymVlasov, yes I did try to override it like the example you shared. I have a monorepo with my application code in one directory and my terraform code in another directory but checkov still runs everywhere.

- repo: https://github.com/antonbabenko/pre-commit-terraform
  rev: v1.62.2   # Get the latest from: https://github.com/antonbabenko/pre-commit-terraform/releases
  hooks:
    - id: checkov
      files: "terraform/"
      args: [
          "-d", "terraform/"
      ]

Let me know if I can provide more details, otherwise I'll follow #290, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party Issues not related to `pre-commit-terraform` hook/terraform_checkov Bash hook question
Projects
None yet
Development

No branches or pull requests

4 participants