Some inconsistencies between `CKV_IGNORED_DIRECTORIES` and `--skip-path`

[tpvasconcelos]

Hey! New checkov user here 👋

How I noticed the problem

While trying to run checkov's test suit on my local machine I noticed that some files were being (unexpectedly) ignored. After a little digging, I realised that this is because I cloned the repo under a path that looks something like: ~/my_git_forks/checkov.

The reason this is a problem is because the following patterns are treated as regular expressions by checkov:

checkov/checkov/secrets/utils.py

Line 10 in 42d3178

EXCLUDED_PATHS = [*ignored_directories, DEFAULT_EXTERNAL_MODULES_DIR, ".idea", ".git", "venv"]

and the .git pattern obviously excludes any paths that include the word 'git' (incl. at least one preceding character)

The more fundamental issues

I think the more fundamental problem is that checkov documents --skip-path as including regexes, documents CKV_IGNORED_DIRECTORIES to include non-regex patterns (default values arenode_modules,.terraform,.serverless instead of node_modules,\\.terraform,\\.serverless), but then treats them equally in the code by merging them in some places, and explicitly treats them as both regexes and non-regex patterns in the same line of code.

I think someone has noticed something similar in the past because I see a few p.replace(".terraform", r"\.terraform") sprinkled around the code base.

However this just seems like another inconsistency because if users set --skip-path="^.*\.terraform.*$", it will get converted internally to ^.*\\.terraform.*$, which would not ignore paths like path/to/.terraform.

How to fix it?

Since checkov allows users to pass regexes to --skip-path and regards CKV_IGNORED_DIRECTORIES as legacy, it makes sense to consistently treat all patterns as regular expressions. However, since I just started looking at this project a couple of days ago, I have no idea what the consequences of doing this are...

Some (naive?) suggestions

1. Escape the default values for CKV_IGNORED_DIRECTORIES:

checkov/checkov/common/runners/base_runner.py

Line 46 in 42d3178

IGNORED_DIRECTORIES_ENV = os.getenv("CKV_IGNORED_DIRECTORIES", "node_modules,.terraform,.serverless")

i.e.,:

-IGNORED_DIRECTORIES_ENV = os.getenv("CKV_IGNORED_DIRECTORIES", "node_modules,.terraform,.serverless")
+IGNORED_DIRECTORIES_ENV = os.getenv("CKV_IGNORED_DIRECTORIES", "node_modules,\.terraform,\.serverless")

Same needs to be done here:

checkov/checkov/common/util/env_vars_config.py

Line 44 in 42d3178

self.IGNORED_DIRECTORIES = os.getenv("CKV_IGNORED_DIRECTORIES", "node_modules,.terraform,.serverless")

2. Same here:

checkov/checkov/secrets/utils.py

Line 10 in 42d3178

EXCLUDED_PATHS = [*ignored_directories, DEFAULT_EXTERNAL_MODULES_DIR, ".idea", ".git", "venv"]

-EXCLUDED_PATHS = [*ignored_directories, DEFAULT_EXTERNAL_MODULES_DIR, ".idea", ".git", "venv"]
+EXCLUDED_PATHS = [*ignored_directories, re.escape(DEFAULT_EXTERNAL_MODULES_DIR), "\.idea", "\.git", "venv"]

3. Remove any hard-coded references to p.replace(".terraform", r"\.terraform")
4. Consistently threat all references to ignored_directories, excluded_paths, and skip_path as regex patterns. This means removing all ~path in skip_path invocations since this is not compatible with regex (e.g. "\.hidden" not in ".hidden/secret.txt")
   • As a safeguard and to avoid ~path in skip_path code being committed in the first place, the excluded_paths variable should probably be a set of re.patterns instead of strings. e.g.:

     excluded_paths = {re.compile(p) for p in self.config.skip_path}`

Other things to consider

Currently, patterns set in the form of .hidden will always exclude directories of the form .hidden-not. However, this is a problem both in the non-regex ~path in skip_path way of doing things and naive regex definitions. However, at least with regex the user can be explicit and say something like (^|.*\/)\.hidden\/?

However, this is not very user friendly and maybe there should be a way to allow users to define excluded directories in a .hidden,also_hidden flavour rather than the verbose (and non-Windown-compatible) --skip-path='(^|.*\/)\.hidden\/?' --skip-path='(^|.*\/)also_hidden\/?'

[tpvasconcelos]

I opened a PR for reference (#6738) that fixes the issue I had running the tests locally.

I doesn't address all the issues discussed above but I wanted to get started with a possible incremental solution.

[tpvasconcelos]

Hey @tsmithv11 @gruebel ! Apologies for the direct tag but you seem to be the most active maintainers of the project. I opened this issue ~5 months ago and have not gotten a reply. I completely understand if you're drowning in work and have other higher priority issues to look at first. But instead of letting this slowly die I decided to ping you in hopes to get a reaction.

Let me know if you need any help with this or if something is not clear.

Thanks in advance! 🙏

[stale]

Thanks for contributing to Checkov! We've automatically marked this issue as stale to keep our issues list tidy, because it has not had any activity for 6 months. It will be closed in 14 days if no further activity occurs. Commenting on this issue will remove the stale tag. If you want to talk through the issue or help us understand the priority and context, feel free to add a comment or join us in the Checkov slack channel at codifiedsecurity.slack.com
Thanks!

[stale]

Closing issue due to inactivity. If you feel this is in error, please re-open, or reach out to the community via slack: codifiedsecurity.slack.com Thanks!

[tpvasconcelos]

@gruebel could you re-open this issue? I'd be more than happy to contribute (see open PR: #6738)