Skip to content

Commit

Permalink
Stop false positive generic resource templates from being tested (#179)
Browse files Browse the repository at this point in the history 
* Fix failing test_S3CrossAccountTrustRule.py::test_s3_bucket_cross_account

This root of this was quickly found by setting
`GenericResource.ALLOW_EXISTING_TYPES = False` which defaults to true.

* Fix template for test_s3_bucket_cross_account test

The `Resource` property has to be a string, a list of them, or something that
will resolve to one of them. It was previously a list of a string and a 'list
of strings'. This commit converts it to a list of something that resolves to a
string.

* Fix Wildcard Resource Rule test that was failing from a malformed template

This is not passing pydantic validation due to not having a PolicyName
property. However, this was by design, so I have kept the test and have it
test for the error.

There exists an equivalent test that has the PolicyName property, so no test
seemingly missing here by not having it.

See here https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-iam-policy.html
PolicyName is `Required: Yes` as it is in the pycfmodel too
https://github.com/Skyscanner/pycfmodel/blob/master/pycfmodel/model/resources/properties/policy.py#L16

* Fix Wildcard Resource Rule test that was failing from a malformed template

This is not passing pydantic validation due to not having a PolicyName
property. However, this was by design, so I have kept the test and have it
test for the error.

There exists an equivalent test that has the PolicyName property, so no test
seemingly missing here by not having it.

See here https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-iam-policy.html
PolicyName is `Required: Yes` as it is in the pycfmodel too
https://github.com/Skyscanner/pycfmodel/blob/master/pycfmodel/model/resources/properties/policy.py#L16

* Disallow generic resource for those resources with a known Type property in tests
  • Loading branch information
@cogpie
cogpie committed Jun 3, 2021
1 parent a8422a3 commit 0472df9
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 258 deletions.
6 changes: 6 additions & 0 deletions tests/conftest.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from pathlib import Path

import pytest
from pycfmodel.model.resources.generic_resource import GenericResource

from cfripper.config.config import Config
from cfripper.config.filter import Filter
Expand All @@ -16,6 +17,11 @@ def default_session_fixture(request):
logging.disable(logging.CRITICAL)


@pytest.fixture(scope="session", autouse=True)
def disallow_allowing_typed_generic_resources():
GenericResource.ALLOW_EXISTING_TYPES = False


@pytest.fixture
def test_files_location() -> Path:
return Path(__file__).parent / "test_files"
Expand Down
240 changes: 9 additions & 231 deletions tests/rules/test_WildcardResourceRule.py
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import pydantic
import pytest
from pycfmodel.model.resources.iam_policy import IAMPolicy

Expand All @@ -24,20 +25,6 @@ def iam_policy_with_wildcard_resource_and_wildcard_action():
).resolve()


@pytest.fixture()
def iam_policy_with_wildcard_resource_without_policy_name():
return get_cfmodel_from(
"rules/WildcardResourceRule/iam_policy_with_wildcard_resource_without_policy_name.json"
).resolve()


@pytest.fixture()
def iam_policy_with_wildcard_resource_and_wilcard_action_without_policy_name():
return get_cfmodel_from(
"rules/WildcardResourceRule/iam_policy_with_wildcard_resource_and_wilcard_action_without_policy_name.json"
).resolve()


@pytest.fixture()
def iam_policy_with_wildcard_resource_and_wildcard_action_and_condition():
return get_cfmodel_from(
Expand Down Expand Up @@ -668,222 +655,13 @@ def test_multiple_resources_with_wildcard_resources_are_detected(user_and_policy
)


def test_policy_document_with_wildcard_resource_without_policy_name_is_detected(
iam_policy_with_wildcard_resource_without_policy_name,
):
rule = WildcardResourceRule(None)
rule._config.stack_name = "stack3"
rule.all_cf_actions = set()
result = rule.invoke(iam_policy_with_wildcard_resource_without_policy_name)

assert result.valid is False
assert compare_lists_of_failures(
result.failures,
[
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:AddPermission"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:ChangeMessageVisibility"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:ChangeMessageVisibilityBatch"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:CreateQueue"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:DeleteMessage"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:DeleteMessageBatch"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:DeleteQueue"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:GetQueueAttributes"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:GetQueueUrl"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:ListDeadLetterSourceQueues"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:ListQueueTags"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:ListQueues"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:PurgeQueue"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:ReceiveMessage"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:RemovePermission"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:SendMessage"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:SendMessageBatch"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:SetQueueAttributes"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:TagQueue"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource for "sqs:UntagQueue"',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"sqs:*"},
resource_ids={"RolePolicy"},
),
],
)

def test_policy_document_with_wildcard_resource_without_policy_name_is_detected():
with pytest.raises(pydantic.ValidationError):
get_cfmodel_from("rules/WildcardResourceRule/iam_policy_with_wildcard_resource_without_policy_name.json")

def test_policy_document_with_wildcard_resource_and_wilcard_action_without_policy_name_is_detected(
iam_policy_with_wildcard_resource_and_wilcard_action_without_policy_name,
):
rule = WildcardResourceRule(None)
rule._config.stack_name = "stack3"
rule.all_cf_actions = set()
result = rule.invoke(iam_policy_with_wildcard_resource_and_wilcard_action_without_policy_name)

assert result.valid is False
assert compare_lists_of_failures(
result.failures,
[
Failure(
granularity="ACTION",
reason='"RolePolicy" is using a wildcard resource allowing all actions',
risk_value="MEDIUM",
rule="WildcardResourceRule",
rule_mode="BLOCKING",
actions={"*"},
resource_ids={"RolePolicy"},
)
],
)
def test_policy_document_with_wildcard_resource_and_wildcard_action_without_policy_name_is_detected():
with pytest.raises(pydantic.ValidationError):
get_cfmodel_from(
"rules/WildcardResourceRule/iam_policy_with_wildcard_resource_and_wilcard_action_without_policy_name.json"
)
35 changes: 14 additions & 21 deletions tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_account.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,28 +83,21 @@
},
"PolicyDocument": {
"Version": "2012-10-17",
"Statement": [
{
"Sid": "AllowAccessFromSandbox",
"Action": [
"s3:PutObject"
],
"Effect": "Allow",
"Resource": [
"",
[
"arn:aws:s3:::",
"S3Bucket",
"/*"
]
],
"Principal": {
"AWS": [
"arn:aws:iam::987654321:root"
]
}
"Statement": {
"Sid": "AllowAccessFromSandbox",
"Action": [
"s3:PutObject"
],
"Effect": "Allow",
"Resource": [
"arn:aws:s3:::S3Bucket/*"
],
"Principal": {
"AWS": [
"arn:aws:iam::987654321:root"
]
}
]
}
}
}
}
Expand Down
16 changes: 10 additions & 6 deletions ...est_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_account_from_aws_service.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,16 @@
],
"Effect": "Allow",
"Resource": [
"",
[
"arn:aws:s3:::",
"S3Bucket",
"/*"
]
{
"Fn::Join": [
"",
[
"arn:aws:s3:::",
"S3Bucket",
"/*"
]
]
}
],
"Principal": {
"AWS": [
Expand Down

0 comments on commit 0472df9

Please sign in to comment.