diff --git a/checkov/terraform/checks/data/aws/GithubActionsOIDCTrustPolicy.py b/checkov/terraform/checks/data/aws/GithubActionsOIDCTrustPolicy.py index 88a5c16805c..5fe986fd91f 100644 --- a/checkov/terraform/checks/data/aws/GithubActionsOIDCTrustPolicy.py +++ b/checkov/terraform/checks/data/aws/GithubActionsOIDCTrustPolicy.py @@ -4,54 +4,83 @@ from checkov.common.util.type_forcers import force_list from checkov.terraform.checks.data.base_check import BaseDataCheck -gh_repo_regex = re.compile(r'repo:[^/]+/[^/]+') +gh_repo_regex = re.compile(r"[\w]+/.+") +gh_abusable_claims = ["workflow", "environment", "ref", "context", "head_ref", "base_ref"] class GithubActionsOIDCTrustPolicy(BaseDataCheck): def __init__(self): - name = 'Ensure GitHub Actions OIDC trust policies only allows actions from a specific known organization' + name = "Ensure GitHub Actions OIDC authorization policies only allow safe claims and claim order" id = "CKV_AWS_358" supported_data = ("aws_iam_policy_document",) categories = [CheckCategories.IAM] super().__init__(name=name, id=id, categories=categories, supported_data=supported_data) def scan_data_conf(self, conf: Dict[str, List[Any]]) -> CheckResult: - statements = force_list(conf.get('statement')) + statements = force_list(conf.get("statement")) for statement in statements: found_federated_gh_oidc = False if isinstance(statement, dict): - if statement.get('principals'): - principals = statement['principals'] + if statement.get("principals"): + principals = statement["principals"] for principal in force_list(principals): - if 'type' not in principal and 'identifiers' not in principal: + if "type" not in principal and "identifiers" not in principal: continue - principal_type = principal['type'] - principal_identifiers = principal['identifiers'] - if isinstance(principal_type, list) and len( - principal_type) and 'Federated' in principal_type and isinstance(principal_identifiers, - list): + principal_type = principal["type"] + principal_identifiers = principal["identifiers"] + if ( + isinstance(principal_type, list) + and len(principal_type) + and "Federated" in principal_type + and isinstance(principal_identifiers, list) + ): for identifier in principal_identifiers: - if isinstance(identifier, - list) and identifier[0] is not None and \ - 'oidc-provider/token.actions.githubusercontent.com' in identifier[0]: + if ( + isinstance(identifier, list) + and identifier[0] is not None + and "oidc-provider/token.actions.githubusercontent.com" in identifier[0] + ): found_federated_gh_oidc = True break if not found_federated_gh_oidc: return CheckResult.PASSED - if found_federated_gh_oidc and not statement.get('condition'): + if found_federated_gh_oidc and not statement.get("condition"): return CheckResult.FAILED found_sub_condition_variable = False found_sub_condition_value = False - for condition in statement.get('condition'): - condition_variables = condition.get('variable') - condition_values = condition.get('values') + for condition in statement.get("condition"): + condition_variables = condition.get("variable") + condition_values = condition.get("values") if isinstance(condition_variables, list): for condition_variable in condition_variables: - if condition_variable == 'token.actions.githubusercontent.com:sub': + if condition_variable == "token.actions.githubusercontent.com:sub": found_sub_condition_variable = True break - for condition_value in condition_values: - if isinstance(condition_value, list) and gh_repo_regex.search(condition_value[0]): + if isinstance(condition_values, list): + for condition_value in condition_values: + if isinstance(condition_value, list): + # First -> check if the value is a mere wildcard. If so, it's a fail + # This covers the case where the condition is ['sub':'*'] + if len(condition_value) == 1 and condition_value[0] == "*": + return CheckResult.FAILED + # Split the claims by ':' for deeper inspection + split_claims = condition_value[0].split(":") + # The assertion MUST be of the form ['{claim_name_1}:{claim_value_1}:{claim_name_2}:{claim_value_2}...'] + # If the length of the split claims is 1, it means that the assertion is ['sub':'{claim_name}'] - this is a fail + if len(split_claims) == 1: + return CheckResult.FAILED + # Second -> Check if the value is a wildcard assertion + # This covers the case where the condition is ['sub':'{claim_name}:*'] + if split_claims[1] == "*": + return CheckResult.FAILED + # Third -> Check if the value is an abusable claim + # This covers the case where the condition is ['sub':'{abusable_claim}:{any_value}'] + for abusable_claim in gh_abusable_claims: + if split_claims[0].startswith(abusable_claim): + return CheckResult.FAILED + # Fourth -> Check if the value is a repo:org/* -> this is a pass with a warning + if split_claims[0] == "repo" and not gh_repo_regex.match(split_claims[1]): + return CheckResult.FAILED found_sub_condition_value = True break if found_sub_condition_value and found_sub_condition_variable: diff --git a/tests/terraform/checks/data/aws/example_GithubActionsOIDCTrustPolicy/main.tf b/tests/terraform/checks/data/aws/example_GithubActionsOIDCTrustPolicy/main.tf index af7857db951..214f24d5c28 100644 --- a/tests/terraform/checks/data/aws/example_GithubActionsOIDCTrustPolicy/main.tf +++ b/tests/terraform/checks/data/aws/example_GithubActionsOIDCTrustPolicy/main.tf @@ -107,3 +107,109 @@ data "aws_iam_policy_document" "fail2" { } } } + +# fail for wildcard as condition +data "aws_iam_policy_document" "fail-wildcard" { + version = "2012-10-17" + + statement { + effect = "Allow" + action = [ + "sts:AssumeRoleWithWebIdentity" + ] + principals { + identifiers = ["arn:aws:iam::123456123456:oidc-provider/token.actions.githubusercontent.com"] + type = "Federated" + } + + condition { + test = "StringEquals" + values = ["*"] + variable = "token.actions.githubusercontent.com:sub" + } + } +} +# fail for abusable value as condition +data "aws_iam_policy_document" "fail-abusable" { + version = "2012-10-17" + + statement { + effect = "Allow" + action = [ + "sts:AssumeRoleWithWebIdentity" + ] + principals { + identifiers = ["arn:aws:iam::123456123456:oidc-provider/token.actions.githubusercontent.com"] + type = "Federated" + } + + condition { + test = "StringEquals" + values = ["workflow:github-actions:repo:myOrg/myRepo:ref:refs/heads/MyBranch"] + variable = "token.actions.githubusercontent.com:sub" + } + } +} +# fail for condition that asserts wildcard +data "aws_iam_policy_document" "fail-wildcard-assertion" { + version = "2012-10-17" + + statement { + effect = "Allow" + action = [ + "sts:AssumeRoleWithWebIdentity" + ] + principals { + identifiers = ["arn:aws:iam::123456123456:oidc-provider/token.actions.githubusercontent.com"] + type = "Federated" + } + + condition { + test = "StringEquals" + values = ["repo:*"] + variable = "token.actions.githubusercontent.com:sub" + } + } +} +# fail for misused "repo" condition +data "aws_iam_policy_document" "fail-misused-repo" { + version = "2012-10-17" + + statement { + effect = "Allow" + action = [ + "sts:AssumeRoleWithWebIdentity" + ] + principals { + identifiers = ["arn:aws:iam::123456123456:oidc-provider/token.actions.githubusercontent.com"] + type = "Federated" + } + + condition { + test = "StringEquals" + values = ["repo:myOrg*"] + variable = "token.actions.githubusercontent.com:sub" + } + } +} +# pass for org only "repo" condition +data "aws_iam_policy_document" "pass-org-only" { + version = "2012-10-17" + + statement { + effect = "Allow" + action = [ + "sts:AssumeRoleWithWebIdentity" + ] + principals { + identifiers = ["arn:aws:iam::123456123456:oidc-provider/token.actions.githubusercontent.com"] + type = "Federated" + } + + condition { + test = "StringEquals" + values = ["repo:myOrg/*"] + variable = "token.actions.githubusercontent.com:sub" + } + } +} \ No newline at end of file diff --git a/tests/terraform/checks/data/aws/test_GithubActionsOIDCTrustPolicy.py b/tests/terraform/checks/data/aws/test_GithubActionsOIDCTrustPolicy.py index 0eaff40f31c..46c219d7f66 100644 --- a/tests/terraform/checks/data/aws/test_GithubActionsOIDCTrustPolicy.py +++ b/tests/terraform/checks/data/aws/test_GithubActionsOIDCTrustPolicy.py @@ -19,11 +19,15 @@ def test(self): 'aws_iam_policy_document.pass1', "aws_iam_policy_document.pass2", "aws_iam_policy_document.pass3", - + "aws_iam_policy_document.pass-org-only" } failing_resources = { "aws_iam_policy_document.fail1", - "aws_iam_policy_document.fail2" + "aws_iam_policy_document.fail2", + "aws_iam_policy_document.fail-wildcard", + "aws_iam_policy_document.fail-abusable", + "aws_iam_policy_document.fail-wildcard-assertion", + "aws_iam_policy_document.fail-misused-repo" } passed_check_resources = set([c.resource for c in report.passed_checks])