Skip to content

Switch to ANDing Conditions keys instead of OR#188

Draft
tweedge wants to merge 2 commits intoNetflix-Skunkworks:masterfrom
tweedge:and-conditions
Draft

Switch to ANDing Conditions keys instead of OR#188
tweedge wants to merge 2 commits intoNetflix-Skunkworks:masterfrom
tweedge:and-conditions

Conversation

@tweedge
Copy link
Copy Markdown
Contributor

@tweedge tweedge commented Jan 6, 2025

Howdy! Noticed a false positive with policies that have multiple conditions, such as:

    {
        "StringLike": {
            "aws:PrincipalArn": "arn:aws:iam::*:role/*-FancyRole"
        },
        "StringEquals": {
            "aws:PrincipalOrgID": [
                "o-xxxxxxxxxx",
                "o-yyyyyyyyyy"
            ]
        }
    }

This is because is_condition_internet_accessible in statement.py checked each individual statement for public accessibility, and if any is public, it marks the statement public. The grammar for Conditions differs, and all keys are ANDed within a particular Condition, so this is not publicly accessible as both the PrincipalOrgID and PrincipalArn requirements need to be fulfilled: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-logic-multiple-context-keys-or-values.html

To address this, I've modified statement.py to check all Conditions for public accessibility, and then only return that the resource is publicly accessible if all Conditions are public. I've added a test case for this particular policy and all existing tests pass. However, most test cases do not focus on multiple Conditions, so I'm going to deep dive some more cases and ensure the change is appropriate before unmarking this as draft. I could envision cases where ex. an unmonitored action (tags?) could then cause policies to be marked as non-public simply because it has no * in it, which needs to be stamped out first.

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.

1 participant