Skip to content

fix(terraform): CKV_AWS_358 & CKV_AWS_393 - inspect every value of a multi-value OIDC sub condition #7665

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.

Sign up for GitHub

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

Open
Dashtid wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(terraform): CKV_AWS_358 & CKV_AWS_393 - inspect every value of a multi-value OIDC sub condition #7665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 31 additions & 23 deletions checkov/terraform/checks/data/aws/GithubActionsOIDCTrustPolicy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,30 +66,38 @@ def scan_data_conf(self, conf: Dict[str, List[Any]]) -> CheckResult:
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):
# IAM evaluates multiple values of one condition key with a logical OR,
# so a single loose value admits every subject it matches no matter how
# tight the other values are. Every value must therefore be inspected;
# one safe value cannot vouch for the rest of the list.
for sub_value in condition_value:
if not isinstance(sub_value, str):
continue
# 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 sub_value == "*":
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
# Split the claims by ':' for deeper inspection
split_claims = sub_value.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
if found_sub_condition_value:
break
if found_sub_condition_value and found_sub_condition_variable:
return CheckResult.PASSED

Expand Down
25 changes: 18 additions & 7 deletions checkov/terraform/checks/resource/aws/GithubActionsOIDCTrustPolicyOnRole.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,23 +87,34 @@ def _evaluate_sub_conditions(self, condition: dict[str, Any]) -> CheckResult | N
Walk Condition operators looking for the `:sub` claim constraint.

Mirrors CKV_AWS_358's inner loop: returns FAILED on the first unsafe
value found, PASSED on the first safe value found, or None if no
`:sub` claim constraint exists in any operator (the caller then
returns FAILED, matching the data check's "Found a federated GitHub
user, but no restrictions" path).
value found, PASSED once every value of the first `:sub` constraint has
been inspected and found safe, or None if no `:sub` claim constraint
exists in any operator (the caller then returns FAILED, matching the
data check's "Found a federated GitHub user, but no restrictions"
path).

IAM evaluates multiple values of one condition key with a logical OR,
so a single loose value admits every subject it matches no matter how
tight the other values are -- one safe value cannot vouch for the rest
of the list.
"""
for operator_values in condition.values():
if not isinstance(operator_values, dict):
continue
for variable_name, values in operator_values.items():
if not isinstance(variable_name, str) or not gh_sub_condition.match(variable_name):
continue
verdict: CheckResult | None = None
for value in force_list(values):
if not isinstance(value, str):
continue
verdict = self._classify_sub_value(value)
if verdict is not None:
return verdict
value_verdict = self._classify_sub_value(value)
if value_verdict == CheckResult.FAILED:
return CheckResult.FAILED
if value_verdict == CheckResult.PASSED:
verdict = CheckResult.PASSED
if verdict is not None:
return verdict
return None

@staticmethod
Expand Down
69 changes: 68 additions & 1 deletion tests/terraform/checks/data/aws/example_GithubActionsOIDCTrustPolicy/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,4 +261,71 @@ data "aws_iam_policy_document" "pass-gh-org" {
variable = "token.actions.githubusercontent.com:aud"
}
}
}
}
# fail for a multi-value condition whose second value is the bare wildcard.
# IAM ORs the values of one condition key, so the tight first value does not
# constrain anything - the "*" admits every subject.
data "aws_iam_policy_document" "fail-multivalue-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 = "StringLike"
values = ["repo:myOrg/myRepo:ref:refs/heads/MyBranch", "*"]
variable = "token.actions.githubusercontent.com:sub"
}
}
}
# fail for a multi-value condition whose second value is an abusable claim.
# The same "workflow:..." value FAILS on its own (see fail-abusable); pairing
# it with a tight value must not hide it.
data "aws_iam_policy_document" "fail-multivalue-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 = "StringLike"
values = ["repo:myOrg/myRepo:ref:refs/heads/MyBranch", "workflow:github-actions:*"]
variable = "token.actions.githubusercontent.com:sub"
}
}
}
# pass for a multi-value condition where every value is pinned (branch + tag)
data "aws_iam_policy_document" "pass-multivalue-pinned" {
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/myRepo:ref:refs/heads/MyBranch", "repo:myOrg/myRepo:ref:refs/tags/v1.0.0"]
variable = "token.actions.githubusercontent.com:sub"
}
}
}
3 changes: 3 additions & 0 deletions tests/terraform/checks/data/aws/test_GithubActionsOIDCTrustPolicy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ def test(self):
"aws_iam_policy_document.pass-org-only",
"aws_iam_policy_document.pass_aud_first",
"aws_iam_policy_document.pass-gh-org",
"aws_iam_policy_document.pass-multivalue-pinned",
}
failing_resources = {
"aws_iam_policy_document.fail1",
Expand All @@ -30,6 +31,8 @@ def test(self):
"aws_iam_policy_document.fail-abusable",
"aws_iam_policy_document.fail-wildcard-assertion",
"aws_iam_policy_document.fail-misused-repo",
"aws_iam_policy_document.fail-multivalue-wildcard",
"aws_iam_policy_document.fail-multivalue-abusable",
}

passed_check_resources = set([c.resource for c in report.passed_checks])
Expand Down
68 changes: 68 additions & 0 deletions tests/terraform/checks/resource/aws/example_GithubActionsOIDCTrustPolicyOnRole/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,3 +300,71 @@ resource "aws_iam_role" "pass-fm-customer" {
]
})
}

# fail-multivalue-wildcard -- tight first value, bare "*" second. IAM ORs the
# values of one condition key, so the "*" admits every subject -> FAIL
resource "aws_iam_role" "fail-multivalue-wildcard" {
name = "fail-multivalue-wildcard"
assume_role_policy = jsonencode({
Version = "2012-10-17"
Statement = [
{
Effect = "Allow"
Principal = {
Federated = "arn:aws:iam::123456123456:oidc-provider/token.actions.githubusercontent.com"
}
Action = "sts:AssumeRoleWithWebIdentity"
Condition = {
StringLike = {
"token.actions.githubusercontent.com:sub" = ["repo:myOrg/myRepo:ref:refs/heads/MyBranch", "*"]
}
}
}
]
})
}

# fail-multivalue-abusable -- tight first value, abusable "workflow:..." second.
# The same value FAILS on its own (fail-abusable); pairing must not hide it -> FAIL
resource "aws_iam_role" "fail-multivalue-abusable" {
name = "fail-multivalue-abusable"
assume_role_policy = jsonencode({
Version = "2012-10-17"
Statement = [
{
Effect = "Allow"
Principal = {
Federated = "arn:aws:iam::123456123456:oidc-provider/token.actions.githubusercontent.com"
}
Action = "sts:AssumeRoleWithWebIdentity"
Condition = {
StringLike = {
"token.actions.githubusercontent.com:sub" = ["repo:myOrg/myRepo:ref:refs/heads/MyBranch", "workflow:github-actions:*"]
}
}
}
]
})
}

# pass-multivalue-pinned -- every value pinned (branch + tag) -> PASS
resource "aws_iam_role" "pass-multivalue-pinned" {
name = "pass-multivalue-pinned"
assume_role_policy = jsonencode({
Version = "2012-10-17"
Statement = [
{
Effect = "Allow"
Principal = {
Federated = "arn:aws:iam::123456123456:oidc-provider/token.actions.githubusercontent.com"
}
Action = "sts:AssumeRoleWithWebIdentity"
Condition = {
StringEquals = {
"token.actions.githubusercontent.com:sub" = ["repo:myOrg/myRepo:ref:refs/heads/MyBranch", "repo:myOrg/myRepo:ref:refs/tags/v1.0.0"]
}
}
}
]
})
}
3 changes: 3 additions & 0 deletions tests/terraform/checks/resource/aws/test_GithubActionsOIDCTrustPolicyOnRole.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ def test(self):
"aws_iam_role.pass-org-only",
"aws_iam_role.pass-gh-org",
"aws_iam_role.pass-fm-customer",
"aws_iam_role.pass-multivalue-pinned",
}
failing_resources = {
"aws_iam_role.fail1",
Expand All @@ -47,6 +48,8 @@ def test(self):
"aws_iam_role.fail-abusable",
"aws_iam_role.fail-wildcard-assertion",
"aws_iam_role.fail-misused-repo",
"aws_iam_role.fail-multivalue-wildcard",
"aws_iam_role.fail-multivalue-abusable",
}

passed_check_resources = {c.resource for c in report.passed_checks}
Expand Down