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

Basic support for generic json document scanning #1598

Merged
merged 18 commits into from Sep 30, 2021

Conversation

BrentSouza
Copy link
Contributor

My company is currently evaluating Bridgecrew and one of the things that we're looking for in a policy as code framework is the ability to scan arbitrary json input. The thinking here is that:

  1. It's generally easy to get some kind of configuration from various components in our stack as JSON output.
  2. It's relatively simple to write checkov custom checks against these config docs
  3. It's considerably more effort to create a fully-fledged runner implementation for each component of our stack that we want to check

Using the json parser from the terraform plan implementation, this turns out to be pretty trivial to implement with a very basic feature set. This is a very quick first pass at providing the following functionality:

  • Parse JSON documents into a relevant python dict, list, etc
  • A base JSON check that should be used to write custom checks against documents
  • Narrow the scope of a check to relevant properties within a JSON document using JMESpath-lite syntax, e.g. a.b.c[0]
  • Run checks against all items in a JSON array
  • Run checks against properties of a JSON object
  • Run checks against an entire JSON document

I've tested this locally with some checks we would like to run against our internal GitLab repositories, which can help show how this feature is useful.

Two checks we would like to run are:

  1. Users can't push changes directly into a default branch
  2. Merge Requests require at least 2 approvals

Here's an example of the MR approvals check:

from checkov.common.models.enums import CheckCategories, CheckResult
from checkov.json.base_json_check import BaseJsonCheck
from checkov.json.enums import BlockType


class MergeRequestRequiresApproval(BaseJsonCheck):
    def __init__(self):
        name = "Merge requests should require at least 2 approvals"
        id = "CKV_GITLAB_2"
        categories = [CheckCategories.CONVENTION]
        super().__init__(
            name=name,
            id=id,
            categories=categories,
            supported_entities=["approvals"],
            block_type=BlockType.OBJECT,
        )

    def scan_entity_conf(self, conf):
        if conf["approvals"]["approvals_before_merge"] < 2:
            for rule in conf["approval_rules"]:
                if rule["approvals_required"] >= 2:
                    return CheckResult.PASSED, rule
            return CheckResult.FAILED, conf
        return CheckResult.PASSED, conf["approvals"]


check = MergeRequestRequiresApproval()

Before we go much further with building this out, I wanted to get the checkov team's feedback on the concept of a generic json runner. Is this something that makes sense to include with the project?

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

)
for check, result in results.items():
result_config = result["results_configuration"]
start = result_config.start_mark.line
Copy link
Contributor

Choose a reason for hiding this comment

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

are we populating this var with a value somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is using the json parser that is already in the codebase for terraform plans which captures line numbers along with the deserialized values. The custom checks should return the passing/failing configuration along with the check status. That configuration will have the start/end mark properties as a result of the tf plan parser. Does that make sense?

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 was also wondering if you would want to do something like move the tf plan parser into common as a general JSON parser? There doesn't seem to be anything terraform specific about it, but I didn't want to move something like that without discussing it first.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it is the right thing to do. you'll need to rebase from master because we had a contribution with performance enhancement there a day ago

@BrentSouza
Copy link
Contributor Author

@schosterbarak I started pulling all of the json parsing out into common.parsers and common.parsers.json and updated arm, cloudformation, serverless, and terraform runners to use the common json parser. Please let me know if this looks ok!

@BrentSouza BrentSouza marked this pull request as ready for review September 22, 2021 15:56
Copy link
Contributor

@nimrodkor nimrodkor left a comment

Choose a reason for hiding this comment

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

Very nice @BrentSouza !

Some minor comments

checkov/json/runner.py Outdated Show resolved Hide resolved
checkov/json/runner.py Outdated Show resolved Hide resolved
checkov/terraform/plan_runner.py Outdated Show resolved Hide resolved
checkov/json/base_registry.py Outdated Show resolved Hide resolved
tests/generic_json/checks/object/PropHasValue.py Outdated Show resolved Hide resolved
@schosterbarak
Copy link
Contributor

thank you @BrentSouza don't forget to apply for hacktober fest 👍 https://bridgecrew.io/blog/happy-hacktoberfest-2021/

@schosterbarak schosterbarak merged commit 77235d1 into bridgecrewio:master Sep 30, 2021
@metahertz
Copy link
Collaborator

Epic contribution @BrentSouza!!!!

🎉

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.

None yet

4 participants