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

fix(github): bug fixes in CKV_GITHUB_6, CKV_GITHUB_7, CKV_GITHUB_9 #3605

Merged
merged 8 commits into from
Oct 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions checkov/github/checks/webhooks_https_orgs.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,8 @@ def scan_entity_conf(self, conf: dict[str, Any], entity_type: str) -> tuple[Chec
secret = item_config.get('secret', '')
if re.match("^http://", url) or insecure_ssl != '0' and secret != '********': # nosec
return CheckResult.FAILED, item_config
if org_webhooks_schema.validate(conf):
marynaKK marked this conversation as resolved.
Show resolved Hide resolved
return CheckResult.PASSED, conf

return None
return CheckResult.UNKNOWN, conf


check = WebhookHttpsOrg()
3 changes: 1 addition & 2 deletions checkov/github/checks/webhooks_https_repos.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,8 @@ def scan_entity_conf(self, conf: dict[str, Any], entity_type: str) -> tuple[Chec
insecure_ssl = item_config.get('insecure_ssl', '0')
if re.match("^http://", url) or insecure_ssl != '0':
return CheckResult.FAILED, item_config
if repository_webhooks_schema.validate(conf):
marynaKK marked this conversation as resolved.
Show resolved Hide resolved
return CheckResult.PASSED, conf
return None
return CheckResult.UNKNOWN, conf


check = WebhookHttpsRepo()
51 changes: 26 additions & 25 deletions checkov/github/dal.py
Original file line number Diff line number Diff line change
@@ -1,37 +1,38 @@
from __future__ import annotations

import os
import shutil
from typing import Any
from pathlib import Path

from checkov.common.runners.base_runner import strtobool
from checkov.common.vcs.base_vcs_dal import BaseVCSDAL
from checkov.github.schemas.org_security import schema as org_security_schema


class Github(BaseVCSDAL):
github_conf_dir_path: str # noqa: CCE003 # a static attribute
github_conf_file_paths: dict[str, Path] # noqa: CCE003 # a static attribute

def __init__(self) -> None:
super().__init__()
self.github_conf_dir_path: str
self.github_org_security_file_path: str
self.github_branch_protection_rules_file_path: str
self.github_org_webhooks_file_path: str
self.github_repository_webhooks_file_path: str
self.github_repository_collaborators_file_path: str

def setup_conf_dir(self) -> None:
# files downloaded from github will be persistent in this directory
github_conf_dir_name = os.getenv('CKV_GITHUB_CONF_DIR_NAME', 'github_conf')
self.github_conf_dir_path = os.path.join(os.getcwd(), github_conf_dir_name)
self.github_org_security_file_path = os.path.join(self.github_conf_dir_path, "org_security.json")
self.github_branch_protection_rules_file_path = os.path.join(self.github_conf_dir_path,
"branch_protection_rules.json")
self.github_org_webhooks_file_path = os.path.join(self.github_conf_dir_path,
"org_webhooks.json")
self.github_repository_webhooks_file_path = os.path.join(self.github_conf_dir_path,
"repository_webhooks.json")
self.github_repository_collaborators_file_path = os.path.join(self.github_conf_dir_path,
"repository_collaborators.json")
os.environ["CKV_GITHUB_CONF_DIR_PATH"] = self.github_conf_dir_path

# if any file was left from previous run-remove it.
if os.path.isdir(self.github_conf_dir_path):
shutil.rmtree(self.github_conf_dir_path)

self.github_conf_file_paths = {
"org_security": Path(self.github_conf_dir_path) / "org_security.json",
"branch_protection_rules": Path(self.github_conf_dir_path) / "branch_protection_rules.json",
"org_webhooks": Path(self.github_conf_dir_path) / "org_webhooks.json",
"repository_webhooks": Path(self.github_conf_dir_path) / "repository_webhooks.json",
"repository_collaborators": Path(self.github_conf_dir_path) / "repository_collaborators.json"
}

def discover(self) -> None:
self.api_url = os.getenv('GITHUB_API_URL', "https://api.github.com")
Expand Down Expand Up @@ -66,17 +67,17 @@ def get_branch_protection_rules(self) -> dict[str, Any] | None:
def persist_branch_protection_rules(self) -> None:
data = self.get_branch_protection_rules()
if data:
BaseVCSDAL.persist(path=self.github_branch_protection_rules_file_path, conf=data)
BaseVCSDAL.persist(path=self.github_conf_file_paths["branch_protection_rules"], conf=data)

def persist_organization_security(self) -> None:
organization_security = self.get_organization_security()
if organization_security:
BaseVCSDAL.persist(path=self.github_org_security_file_path, conf=organization_security)
BaseVCSDAL.persist(path=self.github_conf_file_paths["org_security"], conf=organization_security)

def persist_organization_webhooks(self) -> None:
organization_webhooks = self.get_organization_webhooks()
if organization_webhooks:
BaseVCSDAL.persist(path=self.github_org_webhooks_file_path, conf=organization_webhooks)
BaseVCSDAL.persist(path=self.github_conf_file_paths["org_webhooks"], conf=organization_webhooks)

def get_organization_webhooks(self) -> dict[str, Any] | None:
data = self._request(endpoint="orgs/{}/hooks".format(self.org), allowed_status_codes=[200])
Expand All @@ -85,28 +86,28 @@ def get_organization_webhooks(self) -> dict[str, Any] | None:
return data

def get_repository_collaborators(self) -> dict[str, Any] | None:
data = self._request(endpoint="repos/{}/{}/collaborators".format(self.org, self.current_repository),
allowed_status_codes=[200])
endpoint = "repos/{}/{}/collaborators".format(self.repo_owner, self.current_repository)
data = self._request(endpoint=endpoint, allowed_status_codes=[200])
if not data:
return None
return data

def persist_repository_collaborators(self) -> None:
repository_collaborators = self.get_repository_collaborators()
if repository_collaborators:
BaseVCSDAL.persist(path=self.github_repository_collaborators_file_path, conf=repository_collaborators)
BaseVCSDAL.persist(path=self.github_conf_file_paths["repository_collaborators"], conf=repository_collaborators)

def get_repository_webhooks(self) -> dict[str, Any] | None:
data = self._request(endpoint="repos/{}/{}/hooks".format(self.org, self.current_repository),
allowed_status_codes=[200])
endpoint = "repos/{}/{}/hooks".format(self.repo_owner, self.current_repository)
data = self._request(endpoint=endpoint, allowed_status_codes=[200])
if not data:
return None
return data

def persist_repository_webhooks(self) -> None:
repository_webhooks = self.get_repository_webhooks()
if repository_webhooks:
BaseVCSDAL.persist(path=self.github_repository_webhooks_file_path, conf=repository_webhooks)
BaseVCSDAL.persist(path=self.github_conf_file_paths["repository_webhooks"], conf=repository_webhooks)

def get_organization_security(self) -> dict[str, str] | None:
if not self._organization_security:
Expand Down
3 changes: 2 additions & 1 deletion checkov/github/schemas/org_webhooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ def __init__(self) -> None:
]
},
"type": {
"type": "string"
"type": "string",
"const": "Organization"
}
},
"required": [
Expand Down
3 changes: 2 additions & 1 deletion checkov/github/schemas/repository_webhooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ def __init__(self) -> None:
"type": "object",
"properties": {
"type": {
"type": "string"
"type": "string",
"const": "Repository"
},
"id": {
"description": "Unique identifier of the webhook.",
Expand Down
3 changes: 2 additions & 1 deletion checkov/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ def run(banner: str = checkov_banner, argv: List[str] = sys.argv[1:]) -> Optiona
url = None
created_baseline_path = None

git_configuration_folders = [os.getcwd() + '/' + os.getenv('CKV_GITHUB_CONF_DIR_NAME', 'github_conf'),
default_github_dir_path = os.getcwd() + '/' + os.getenv('CKV_GITLAB_CONF_DIR_NAME', 'github_conf')
git_configuration_folders = [os.getenv("CKV_GITHUB_CONF_DIR_PATH", default_github_dir_path),
os.getcwd() + '/' + os.getenv('CKV_GITLAB_CONF_DIR_NAME', 'gitlab_conf')]

if config.directory:
Expand Down
4 changes: 2 additions & 2 deletions docs/6.Contribution/Contribute New GitHub Policies.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ If not it can be added to that file like the following example:

```python

class GitHub(BaseVCSDAL)
class GitHub(BaseVCSDAL):
...
...
def get_branch_protection_rules(self):
Expand All @@ -33,7 +33,7 @@ class GitHub(BaseVCSDAL)
def persist_branch_protection_rules(self):
data = self.get_branch_protection_rules()
if data:
BaseVCSDAL.persist(path=self.github_branch_protection_rules_file_path, conf=data)
BaseVCSDAL.persist(path=self.github_conf_file_paths["branch_protection_rules"], conf=data)

def persist_all_confs(self):
if strtobool(os.getenv("CKV_GITHUB_CONFIG_FETCH_DATA", "True")):
Expand Down
20 changes: 10 additions & 10 deletions docs/7.Scan Examples/Github.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,16 @@ Full list of github organization and repository settings related checks can be f

## GitHub scanning configuration

| Environment Variable | Default value | Description |
|-------------|---------------------------|------------------------------------------------------------------------------------------------------|
| CKV_GITHUB_CONFIG_FETCH_DATA| "True" | checkov will try to fetch GitHub configuration from API by default (unless no access token provided) |
| CKV_GITHUB_CONF_DIR_NAME | "github_conf" | checkov will create a new directory named "github_conf" under current working directory |
| GITHUB_API_URL | "https://api.github.com/" | |
| GITHUB_TOKEN | | GitHub personal access token to be used to fetch GitHub configuration |
| GITHUB_REF | refs/heads/master | Github branch for which to fetch branch protection rules configuration |
| GITHUB_ORG | | Github organization |
| GITHUB_REPOSITORY | | Github repositry for which to fetch repository configuration info |
| GITHUB_REPO_OWNER | | Github repository owner user name |
| Environment Variable | Default value | Description |
|-------------|---------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------|
| CKV_GITHUB_CONFIG_FETCH_DATA| "True" | checkov will try to fetch GitHub configuration from API by default (unless no access token provided) |
| CKV_GITHUB_CONF_DIR_NAME | "github_conf" | checkov will create a new directory named "github_conf" under current working directory |
| GITHUB_API_URL | "https://api.github.com/" | |
| GITHUB_TOKEN | | GitHub personal access token to be used to fetch GitHub configuration |
| GITHUB_REF | refs/heads/master | Github branch for which to fetch branch protection rules configuration |
| GITHUB_ORG | | Github organization |
| GITHUB_REPOSITORY | | Github repositry for which to fetch repository configuration info |
| GITHUB_REPO_OWNER | | The owner of the repository. This could be either Github repository owner user name or the organization name, in which the user is the owner. |

### Example organization security configuration

Expand Down
83 changes: 76 additions & 7 deletions tests/github/test_dal.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ def test_org_security_null_description(mocker: MockerFixture):
result = dal.get_organization_security()
assert result


@mock.patch.dict(os.environ, {"GITHUB_ORG": "simpleOrg"}, clear=True)
def test_org_security_str_description(mocker: MockerFixture):
dal = Github()
mock_data2 = {
mock_data = {
"data": {
"organization": {
"name": "Bridgecrew",
Expand All @@ -42,15 +43,83 @@ def test_org_security_str_description(mocker: MockerFixture):
}
}
}
mocker.patch("checkov.common.vcs.base_vcs_dal.BaseVCSDAL._request_graphql", return_value=mock_data2)
mocker.patch("checkov.common.vcs.base_vcs_dal.BaseVCSDAL._request_graphql", return_value=mock_data)
result = dal.get_organization_security()
assert result


@mock.patch.dict(os.environ, {"GITHUB_REPO_OWNER": "bridgecrew", "GITHUB_REPOSITORY": "main"}, clear=True)
def test_org_webhooks(mocker: MockerFixture):
dal = Github()
mock_data = [
{
"type": "Organization",
"id": 0,
"name": "web",
"active": True,
"events": [
"*"
],
"config": {
"content_type": "form",
"insecure_ssl": "0",
"url": "http://test-repo-webhook.com"
},
"updated_at": "2022-10-02T12:39:12Z",
"created_at": "2022-09-29T09:01:36Z",
"url": "",
"test_url": "",
"ping_url": "",
"deliveries_url": ""
}
]
mocker.patch("checkov.common.vcs.base_vcs_dal.BaseVCSDAL._request", return_value=mock_data)
result = dal.get_repository_webhooks()
assert result


@mock.patch.dict(os.environ, {"GITHUB_REPO_OWNER": "bridgecrew", "GITHUB_REPOSITORY": "main"}, clear=True)
def test_repository_webhooks(mocker: MockerFixture):
dal = Github()
mock_data = [
{
"type": "Repository",
"id": 0,
"name": "web",
"active": True,
"events": [
"*"
],
"config": {
"content_type": "form",
"insecure_ssl": "0",
"url": "http://test-repo-webhook.com"
},
"updated_at": "2022-10-02T12:39:12Z",
"created_at": "2022-09-29T09:01:36Z",
"url": "",
"test_url": "",
"ping_url": "",
"deliveries_url": ""
}
]
mocker.patch("checkov.common.vcs.base_vcs_dal.BaseVCSDAL._request", return_value=mock_data)
result = dal.get_repository_webhooks()
assert result


def test_validate_github_conf_paths():
# check that all the files in github_conf folder that should be updated with new data from GitHub api reply,
# are empty.In case of no reply-no old data should be left causing confusion with new retrieved data.
dal = Github()
all_github_conf_files_conf_declared = dal.github_conf_dir_path \
and dal.github_org_security_file_path and dal.github_branch_protection_rules_file_path \
and dal.github_org_webhooks_file_path and dal.github_repository_webhooks_file_path \
and dal.github_repository_collaborators_file_path
assert all_github_conf_files_conf_declared
all_github_conf_files_conf_declared = \
{"org_security", "branch_protection_rules", "org_webhooks", "repository_webhooks", "repository_collaborators"} \
- dal.github_conf_file_paths.keys()

assert all_github_conf_files_conf_declared == set()

all_files_are_empty = True
for github_conf_type, file_path in dal.github_conf_file_paths.items():
all_files_are_empty &= not os.path.isfile(file_path) or os.path.getsize(file_path) == 0

assert all_files_are_empty
4 changes: 2 additions & 2 deletions tests/github/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ def test_runner_webhooks_check(self):
runner = Runner()
runner.github.github_conf_dir_path = valid_dir_path

checks = ["CKV_GITHUB_6","CKV_GITHUB_7"]
checks = ["CKV_GITHUB_6", "CKV_GITHUB_7"]
report = runner.run(
root_folder=valid_dir_path,
runner_filter=RunnerFilter(checks=checks)
)
self.assertEqual(len(report.failed_checks), 1)
self.assertEqual(report.parsing_errors, [])
self.assertEqual(len(report.passed_checks), 3)
self.assertEqual(len(report.passed_checks), 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we lose a check here? I would expect that if we have less passing checks we'll have more failed checks or skipped checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We didn't lose a check. initially, there was an error - see bug 2 in my description.

self.assertEqual(report.skipped_checks, [])

@mock.patch.dict(os.environ, {"CKV_GITHUB_CONFIG_FETCH_DATA": "False", "PYCHARM_HOSTED": "1",
Expand Down