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: Make checkov more flexible #329

Conversation

Miscreancy
Copy link

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

Currently, the checkov hook is hard-coded to run against the entire directory - no override is possible.

This change allows the hook to run in any of three modes - whole directory (the default), against all directories which contain a changed file, and against only changed files.

It should be noted that:

  • There is a PR in place for similar but different functionality; the disparity between the two approaches I thought was sufficient to justify a separate PR as the code and logic is very different. I will post a link to this PR in that PR's discussion to raise the question as to which route should be followed.
  • Common scripts were not used deliberately; this is for two reasons. 1. Using the common::parse_cmdline function would've forced the introduction of a breaking change (having to preface all arguments with --args) and 2. Using the common::per_dir_hook function would have removed the ability to pass multiple arguments to checkov, as the code block at line 70 assumes that only $1 is an argument and the rest are files.

How can we test changes

  • Use a dummy IaC repo with multiple modules and known security issues
  • Use pre-commit try-repo $path/to/clone to confirm the default behaviour is still scanning the entire repository
  • Use manual script runs of checkov.sh from within that dummy directory to confirm that when passed --scan-change-directories (and file names), the tool only scans directories that contain a change file, and while passed --scan-change-files, the tool only scans files, as well as any other changes and logic deemed necessary.

@Miscreancy Miscreancy changed the title feat: make checkov more flexible feat: Make checkov more flexible Jan 27, 2022
Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

The code looks sensible to me, though I'll leave design appropriateness consideration and decision to @MaxymVlasov and @antonbabenko

declare -a -g FILES=()

argv=("$@")
pattern='^.*\.tf$'
Copy link
Collaborator

Choose a reason for hiding this comment

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

The any_chars_from_beginning part of pattern makes no much sense and can be dropped

Suggested change
pattern='^.*\.tf$'
pattern='\.tf$'

@MaxymVlasov
Copy link
Collaborator

MaxymVlasov commented Feb 10, 2022

There is a PR in place for similar but different functionality; the disparity between the two approaches I thought was sufficient to justify a separate PR as the code and logic is very different. I will post a link to this PR in that PR's discussion to raise the question as to which route should be followed.
... 1. Using the common::parse_cmdline function would've forced the introduction of a breaking change (having to preface all arguments with --args)...

#290 + compatibility in args parsing by replacing/changing common::parse_cmdline, but not sure it's worth it.
It could be done via #155

  1. Using the common::per_dir_hook function would have removed the ability to pass multiple arguments to checkov, as the code block at line 70 assumes that only $1 is an argument and the rest are files.

Looks like you mean this code:

# Arguments:
# args (string with array) arguments that configure wrapped tool behavior
# files (array) filenames to check
#######################################################################
function common::per_dir_hook {
local -r args="$1"
shift 1
local -a -r files=("$@")

args not arg. The answer already in var name and func description :)
It was done in this way during bash limitation

This is a string with array:

common::per_dir_hook "${ARGS[*]}" "${FILES[@]}"

That then passed as a string with array

per_dir_hook_unique_part "$args" "$dir_path"

and then - expanded to an array
terragrunt hclfmt ${args[@]}

So this argument is not vaild.

@MaxymVlasov MaxymVlasov marked this pull request as draft February 10, 2022 19:59
@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 13, 2022
@github-actions
Copy link

This PR was automatically closed because of stale in 10 days

@github-actions github-actions bot closed this Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants