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

feat(general): Add docker to pre commit hooks #5458

Merged

Conversation

lhriley
Copy link
Contributor

@lhriley lhriley commented Aug 18, 2023

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # n/a

Description

Include a description of what makes it a violation and any relevant external links.

This PR adds support for running pre-commit hooks using the container image, rather than depending on the checkov binary or python module.

Note that due to a limitation in pre-commit I had to make a choice about what the default container tag should be. I opted for latest to reduce the overhead of maintenance. However, it is possible to override this by declaring the entry field in the pre-commit config.

Example:

---
repos:
  - repo: https://github.com/Labelbox/checkov
    rev: 76b8e380a
    hooks:
      - id: checkov_container
        entry: bridgecrew/checkov:2.4.2 -d .

Related:
pre-commit/pre-commit#2968

Fix

How does someone fix the issue in code and/or in runtime?

To test this PR, add the following to your .pre-commit-config.yaml

---
repos:
  - repo: https://github.com/Labelbox/checkov
    rev: 76b8e380a
    hooks:
      - id: checkov_container
      - id: checkov_diff_container
      - id: checkov_secrets_container
        args:
          - '--quiet'
          - '--skip-path'
          - 'dependencies/onepassword-connect/connect-helm-charts'
          - '-f' # required and must come last

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • [n/a] I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes
  • [n/a] Any dependent changes have been merged and published in downstream modules

@lhriley lhriley temporarily deployed to scan-security August 18, 2023 19:54 — with GitHub Actions Inactive
Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work, added some comments

README.md Outdated Show resolved Hide resolved
- id: checkov_container
name: Checkov
description: This hook runs checkov.
entry: bridgecrew/checkov:latest -d .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually recommend to run docker with --tty for better output handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this is how pre-commit runs docker_image hooks:

https://github.com/pre-commit/pre-commit/blob/bde292b51078357384602ebe6b9b27b1906987e5/pre_commit/languages/docker.py#L111-L146

I pushed up a commit to add --tty and the output looks like this now:

❯ pre-commit run -av checkov_secrets_container
Checkov Secrets..........................................................Passed
- hook id: checkov_secrets_container
- duration: 17.04s

secrets scan results:

Passed checks: 0, Failed checks: 0, Skipped checks: 49




❯ pre-commit run -av checkov_secrets_container
Checkov Secrets..........................................................Failed
- hook id: checkov_secrets_container
- duration: 17.16s
- exit code: 1

secrets scan results:

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

Check: CKV_SECRET_6: "Base64 High Entropy String"
	FAILED for resource: a2478cff5623ee9e46dfe2ab06e96bfe5168f732
	File: /foo/bar-values.yaml:7-8
	Guide: https://docs.paloaltonetworks.com/content/techdocs/en_US/prisma/prisma-cloud/prisma-cloud-code-security-policy-reference/secrets-policies/secrets-policy-index/git-secrets-6.html

		7 | superSecret: c2Vjcm******************************************************************************************************************************************************************************




❯ pre-commit run -a checkov_secrets_container
Checkov Secrets..........................................................Failed
- hook id: checkov_secrets_container
- exit code: 1

secrets scan results:

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

Check: CKV_SECRET_6: "Base64 High Entropy String"
	FAILED for resource: a2478cff5623ee9e46dfe2ab06e96bfe5168f732
	File: /foo/bar-values.yaml:7-8
	Guide: https://docs.paloaltonetworks.com/content/techdocs/en_US/prisma/prisma-cloud/prisma-cloud-code-security-policy-reference/secrets-policies/secrets-policy-index/git-secrets-6.html

		7 | superSecret: c2Vjcm******************************************************************************************************************************************************************************




❯ pre-commit run -a checkov_secrets_container
Checkov Secrets..........................................................Passed

@lhriley
Copy link
Contributor Author

lhriley commented Aug 24, 2023

@gruebel question about the container tag. As mentioned I picked latest to avoid maintenance overhead, however, the intended solution is to pin to the same tag as the git release / tag. That would mean updating the .pre-commit-hooks.yaml with every release cycle.

The downside to using latest is that as a user you never know which version of checkov your checks are running as.

Any thoughts on how you would want to maintain this? Should I pin to whatever tag is available today (2.4.7?) or leave it as latest?

@lhriley lhriley force-pushed the add-docker-to-pre-commit-hooks branch from 790f813 to 392b40a Compare August 24, 2023 17:42
@lhriley lhriley force-pushed the add-docker-to-pre-commit-hooks branch from 392b40a to e1ee3e9 Compare October 16, 2023 21:05
Copy link
Contributor

@JamesWoolfenden JamesWoolfenden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JamesWoolfenden
Copy link
Contributor

@gruebel question about the container tag. As mentioned I picked latest to avoid maintenance overhead, however, the intended solution is to pin to the same tag as the git release / tag. That would mean updating the .pre-commit-hooks.yaml with every release cycle.

The downside to using latest is that as a user you never know which version of checkov your checks are running as.

Any thoughts on how you would want to maintain this? Should I pin to whatever tag is available today (2.4.7?) or leave it as latest?

Least maintenance is best, going to be a pain either way if you have pinned a version from pip locally and another pinned version in pre-commit unless you can specify version as a var in pre-commit?.

@lhriley
Copy link
Contributor Author

lhriley commented Dec 5, 2023

Least maintenance is best, going to be a pain either way if you have pinned a version from pip locally and another pinned version in pre-commit unless you can specify version as a var in pre-commit?.

There are limitations based on how pre-commit is designed, but you can replace the entry field like this to specify a version:

entry: --tty bridgecrew/checkov:<version> --framework secrets --enable-secret-scan-all-files

@JamesWoolfenden
Copy link
Contributor

JamesWoolfenden commented Dec 5, 2023 via email

@lhriley
Copy link
Contributor Author

lhriley commented Dec 5, 2023

The <version> in my example is just any valid docker image tag that has been published.

@JamesWoolfenden
Copy link
Contributor

then i think its best to stick with the latest, bit of a draw back in using docker in this case.

Copy link
Collaborator

@tsmithv11 tsmithv11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but we should include how to override the latest tag in the docs. Thanks!

docs/4.Integrations/pre-commit.md Show resolved Hide resolved
docs/4.Integrations/pre-commit.md Outdated Show resolved Hide resolved
@lhriley
Copy link
Contributor Author

lhriley commented Dec 16, 2023

Thanks @tsmithv11. Suggestions merged, and branch updated from main. 👍

@JamesWoolfenden JamesWoolfenden merged commit 9fe6251 into bridgecrewio:main Jan 4, 2024
41 checks passed
JamesWoolfenden pushed a commit to nimrodkor/checkov that referenced this pull request Jan 4, 2024
* Add pre-commit hook support for container image

* Move -f into args to support passing arguments properly

* Improve argument documentation

* Update documentation to include pre-commit hooks

* Add --tty to container entries

* Update documentation per feedback

* Update docs/4.Integrations/pre-commit.md

Co-authored-by: Taylor <[email protected]>

* Update docs/4.Integrations/pre-commit.md

Co-authored-by: Taylor <[email protected]>

---------

Co-authored-by: Taylor <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants