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): allow tool name field to be customised using cli arguments #6692

Merged
merged 9 commits into from
Sep 11, 2024

Conversation

EmmaVinen
Copy link
Contributor

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

Description

An increasingly common use case for checkov is in combination with GitHub code scanning. Typically this would be done by outputting results in a SARIF format and then uploading these to code scanning using a GitHub action.
However GitHub code scanning has limited capabilities to filter code scanning alerts and one of the few parameters that can be used to distinguish different scans in the tool name field in the SARIF file.

For example a user might want to configure multiple checkov configurations using different combinations of custom checks, skipped checks and frameworks and then be able to view the results from each of these scans in the GitHub code scanning tab. At present all the scans would be uploaded with the tool name 'checkov' and there would be no easy way to distinguish the results from different scans.

This pull request adds a cli parameter --tool-name which is propagated to sarif outputs in the tool.driver.name field
Fixes # (#6691)

Local Testing

At present, the cli output tests are failing locally. I am unsure whether this is a genuine error as I can't see any obvious difference between the expected and observed output for these tests.

=============================================================== short test summary info ===============================================================
FAILED tests/sca_package_2/test_output.py::test_create_cli_cves_table - AssertionError: assert '\t/path/to/r...──────────┘\n' == '\t/path/to/r...──────────┘\n'
FAILED tests/sca_package_2/test_output.py::test_create_cli_cves_table_with_no_found_vulnerabilities - AssertionError: assert '\t/path/to/r...──────────┘\n' == '\t/path/to/r...──────────┘\n'
FAILED tests/sca_package_2/test_output.py::test_create_cli_output_no_line_numbers - AssertionError: assert '\t/requireme...──────────┘\n' == '\t/requireme...──────────┘\n'
FAILED tests/sca_package_2/test_output.py::test_create_cli_output_with_line_numbers - AssertionError: assert '\t/requireme...──────────┘\n' == '\t/requireme...──────────┘\n'
FAILED tests/sca_package_2/test_output.py::test_create_cli_output_without_license_records - AssertionError: assert '\t/requireme...──────────┘\n' == '\t/requireme...──────────┘\n'
FAILED tests/sca_package_2/test_output.py::test_create_cli_table_for_sca_package_with_dependencies - AssertionError: assert '\t/package-l...──────────┘\n' == '\t/package-l...──────────┘\n'
FAILED tests/sca_package_2/test_output.py::test_create_cli_output_without_dependencies - AssertionError: assert '\t/package.j...──────────┘\n' == '\t/package.j...──────────┘\n'
FAILED tests/sca_package_2/test_output.py::test_create_cli_table_for_package_with_diff_CVEs - AssertionError: assert '\t/package-l...──────────┘\n' == '\t/package-l...──────────┘\n'
FAILED tests/sca_package_2/test_output.py::test_create_cli_table_for_package_with_reachability_data - AssertionError: assert '\t/requireme...──────────┘\n' == '\t/requireme...──────────┘\n'
FAILED tests/sca_package_2/test_output_reports.py::test_console_output - AssertionError: assert 'sca_package ...──────────┘\n' == 'sca_package ...──────────┘\n'
FAILED tests/sca_package_2/test_output_reports.py::test_console_output_in_tty - AssertionError: assert '\x1b[34msca_...──────────┘\n' == '\x1b[34msca_...──────────┘\n'
FAILED tests/sca_image/test_output_reports.py::test_console_output - AssertionError: assert 'sca_image sc...──────────┘\n' == 'sca_image sc...──────────┘\n'
FAILED tests/sca_image/test_output_reports.py::test_console_output_in_tty - AssertionError: assert '\x1b[34msca_...──────────┘\n' == '\x1b[34msca_...──────────┘\n'
FAILED tests/policies_3d/test_output.py::test_create_simple_cli_output - assert 'Check: BC_P3...──────────┘\n' == 'Check: BC_P3...──────────┘\n'
FAILED tests/policies_3d/test_output.py::test_create_complex_cli_output - assert 'Check: BC_P3...──────────┘\n' == 'Check: BC_P3...──────────┘\n'
========================================= 15 failed, 4581 passed, 27 skipped, 5 warnings in 151.27s (0:02:31) =========================================

Checklist:

  • [x ] I have performed a self-review of my own code
  • [x ] I have commented my code, particularly in hard-to-understand areas
  • [ x] I have made corresponding changes to the documentation
  • [ x] I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes

@EmmaVinen EmmaVinen marked this pull request as ready for review August 28, 2024 21:00
Copy link
Contributor

@bo156 bo156 left a comment

Choose a reason for hiding this comment

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

Great idea and contribution :)
Please review the comments I left

checkov/main.py Outdated
@@ -2,6 +2,7 @@
from __future__ import annotations

import atexit
from email.policy import default # noqa: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

where do you use this import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't use this, seems to be a spurious addition - I have removed it.

checkov/main.py Outdated
@@ -250,7 +251,10 @@ def run(self, banner: str = checkov_banner, tool: str = checkov_tool, source_typ
"Python_implementation": platform.python_implementation()
}

logger.debug(f'Run metadata: {json.dumps(self.run_metadata, indent=2)}')
logger.debug(f'Run metadata: {json.dumps(self.run_metadata, indent=2)}') # noqa: E501
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this noqa added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

originally added because flake8 was flagging this line as too long in my IDE. I have removed the noqa and edited my IDE settings instead for consistency

@@ -557,3 +557,9 @@ def add_parser_args(self) -> None:
"resource code to OpenAI to request remediation guidance. This will use your OpenAI credits. "
"Set your number of findings that will receive enhanced guidelines using CKV_OPENAI_MAX_FINDINGS",
)
self.add(
"--tool-name",
default=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

would use Checkov here to keep the default the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the default to Checkov as recommended :)

@@ -62,4 +62,6 @@ nav_order: 2
| `--block-list-secret-scan CKV_SECRETS_SCAN_BLOCK_LIST` | List of files to filter out in the secret scanner |
| `--support` | Enable debug logs and upload the logs to the server. Requires a Prisma Cloud API key. |
| `--openai-api-key` | Add an OpenAI API key to enhance finding guidelines by sending violated policies and resource code to OpenAI to request remediation guidance. This will use your OpenAI credits. Set your number of findings that will receive enhanced guidelines using CKV_OPENAI_MAX_FINDINGS |
| `--tool-name` | Add a custom tool name to change the tool name field, this is especially useful for outputting results in SARIF format for upload to Github Code Scanning |
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call it custom-tool-name to be more precise on its meaning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I have implemented this change

Copy link
Contributor

@bo156 bo156 left a comment

Choose a reason for hiding this comment

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

Great job 💯

@matansha matansha merged commit da6e241 into bridgecrewio:main Sep 11, 2024
41 checks passed
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