From abddd5e00c4fdab3c6c5cac583be9494c22fced2 Mon Sep 17 00:00:00 2001 From: Ramon Date: Tue, 19 Apr 2022 11:41:23 +0200 Subject: [PATCH] FIX: Effect on Statement is required (#101) * Effect on Statement is required * add PR number to changelog * update cloudformation_actions.py * update cloudformation_actions.py * PR suggestions Co-authored-by: Ramon --- CHANGELOG.md | 6 ++ pycfmodel/cloudformation_actions.py | 57 ++++++++++++++++--- .../resources/properties/policy_document.py | 12 ++-- .../model/resources/properties/statement.py | 10 ++-- setup.py | 2 +- .../properties/test_policy_document.py | 16 +++++- tests/resources/properties/test_statement.py | 31 ++++++---- 7 files changed, 106 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb63a6ac..247853f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,12 @@ # Change Log All notable changes to this project will be documented in this file. +## 0.19.1 +### Fixes +- `Effect` on `Statement` is **required**. [#101](https://github.com/Skyscanner/pycfmodel/pull/97) +### Updates +- Update `CLOUDFORMATION_ACTIONS`. + ## 0.19.0 ### Improvements - Able to parse PolicyDocument that are implicit in string properties. [#97](https://github.com/Skyscanner/pycfmodel/pull/97) diff --git a/pycfmodel/cloudformation_actions.py b/pycfmodel/cloudformation_actions.py index f9d4de18..3d041acd 100644 --- a/pycfmodel/cloudformation_actions.py +++ b/pycfmodel/cloudformation_actions.py @@ -483,20 +483,24 @@ "apprunner:AssociateCustomDomain", "apprunner:CreateAutoScalingConfiguration", "apprunner:CreateConnection", + "apprunner:CreateObservabilityConfiguration", "apprunner:CreateService", "apprunner:CreateVpcConnector", "apprunner:DeleteAutoScalingConfiguration", "apprunner:DeleteConnection", + "apprunner:DeleteObservabilityConfiguration", "apprunner:DeleteService", "apprunner:DeleteVpcConnector", "apprunner:DescribeAutoScalingConfiguration", "apprunner:DescribeCustomDomains", + "apprunner:DescribeObservabilityConfiguration", "apprunner:DescribeOperation", "apprunner:DescribeService", "apprunner:DescribeVpcConnector", "apprunner:DisassociateCustomDomain", "apprunner:ListAutoScalingConfigurations", "apprunner:ListConnections", + "apprunner:ListObservabilityConfigurations", "apprunner:ListOperations", "apprunner:ListServices", "apprunner:ListTagsForResource", @@ -1014,7 +1018,7 @@ "billingconductor:ListPricingRules", "billingconductor:ListPricingRulesAssociatedToPricingPlan", "billingconductor:ListResourcesAssociatedToCustomLineItem", - "billingconductor:ListTagsResource", + "billingconductor:ListTagsForResource", "billingconductor:TagResource", "billingconductor:UntagResource", "billingconductor:UpdateBillingGroup", @@ -1116,13 +1120,17 @@ "chatbot:CreateSlackChannelConfiguration", "chatbot:DeleteChimeWebhookConfiguration", "chatbot:DeleteSlackChannelConfiguration", + "chatbot:DeleteSlackUserIdentity", "chatbot:DeleteSlackWorkspaceAuthorization", "chatbot:DescribeChimeWebhookConfigurations", "chatbot:DescribeSlackChannelConfigurations", "chatbot:DescribeSlackChannels", + "chatbot:DescribeSlackUserIdentities", "chatbot:DescribeSlackWorkspaces", + "chatbot:GetAccountPreferences", "chatbot:GetSlackOauthParameters", "chatbot:RedeemSlackOauthCode", + "chatbot:UpdateAccountPreferences", "chatbot:UpdateChimeWebhookConfiguration", "chatbot:UpdateSlackChannelConfiguration", "chime:AcceptDelegate", @@ -2788,6 +2796,7 @@ "datasync:CreateAgent", "datasync:CreateLocationEfs", "datasync:CreateLocationFsxLustre", + "datasync:CreateLocationFsxOpenZfs", "datasync:CreateLocationFsxWindows", "datasync:CreateLocationHdfs", "datasync:CreateLocationNfs", @@ -2801,6 +2810,7 @@ "datasync:DescribeAgent", "datasync:DescribeLocationEfs", "datasync:DescribeLocationFsxLustre", + "datasync:DescribeLocationFsxOpenZfs", "datasync:DescribeLocationFsxWindows", "datasync:DescribeLocationHdfs", "datasync:DescribeLocationNfs", @@ -3062,6 +3072,7 @@ "devicefarm:UpdateUpload", "devicefarm:UpdateVPCEConfiguration", "devops-guru:AddNotificationChannel", + "devops-guru:DeleteInsight", "devops-guru:DescribeAccountHealth", "devops-guru:DescribeAccountOverview", "devops-guru:DescribeAnomaly", @@ -4558,6 +4569,7 @@ "events:CreateApiDestination", "events:CreateArchive", "events:CreateConnection", + "events:CreateEndpoint", "events:CreateEventBus", "events:CreatePartnerEventSource", "events:DeactivateEventSource", @@ -4565,12 +4577,14 @@ "events:DeleteApiDestination", "events:DeleteArchive", "events:DeleteConnection", + "events:DeleteEndpoint", "events:DeleteEventBus", "events:DeletePartnerEventSource", "events:DeleteRule", "events:DescribeApiDestination", "events:DescribeArchive", "events:DescribeConnection", + "events:DescribeEndpoint", "events:DescribeEventBus", "events:DescribeEventSource", "events:DescribePartnerEventSource", @@ -4582,6 +4596,7 @@ "events:ListApiDestinations", "events:ListArchives", "events:ListConnections", + "events:ListEndpoints", "events:ListEventBuses", "events:ListEventSources", "events:ListPartnerEventSourceAccounts", @@ -4605,6 +4620,7 @@ "events:UpdateApiDestination", "events:UpdateArchive", "events:UpdateConnection", + "events:UpdateEndpoint", "evidently:BatchEvaluateFeature", "evidently:CreateExperiment", "evidently:CreateFeature", @@ -4687,11 +4703,13 @@ "fis:UntagResource", "fis:UpdateExperimentTemplate", "fms:AssociateAdminAccount", + "fms:AssociateThirdPartyFirewall", "fms:DeleteAppsList", "fms:DeleteNotificationChannel", "fms:DeletePolicy", "fms:DeleteProtocolsList", "fms:DisassociateAdminAccount", + "fms:DisassociateThirdPartyFirewall", "fms:GetAdminAccount", "fms:GetAppsList", "fms:GetComplianceDetail", @@ -4699,6 +4717,7 @@ "fms:GetPolicy", "fms:GetProtectionStatus", "fms:GetProtocolsList", + "fms:GetThirdPartyFirewallAssociationStatus", "fms:GetViolationDetails", "fms:ListAppsLists", "fms:ListComplianceStatus", @@ -4706,6 +4725,7 @@ "fms:ListPolicies", "fms:ListProtocolsLists", "fms:ListTagsForResource", + "fms:ListThirdPartyFirewallFirewallPolicies", "fms:PutAppsList", "fms:PutNotificationChannel", "fms:PutPolicy", @@ -6083,6 +6103,7 @@ "iot:ListJobTemplates", "iot:ListJobs", "iot:ListManagedJobTemplates", + "iot:ListMetricValues", "iot:ListMitigationActions", "iot:ListNamedShadowsForThing", "iot:ListOTAUpdates", @@ -7905,6 +7926,34 @@ "mgn:UpdateReplicationConfiguration", "mgn:UpdateReplicationConfigurationTemplate", "mgn:UpdateSourceServerReplicationType", + "migrationhub-orchestrator:CreateWorkflow", + "migrationhub-orchestrator:CreateWorkflowStep", + "migrationhub-orchestrator:CreateWorkflowStepGroup", + "migrationhub-orchestrator:DeleteWorkflow", + "migrationhub-orchestrator:DeleteWorkflowStep", + "migrationhub-orchestrator:DeleteWorkflowStepGroup", + "migrationhub-orchestrator:GetMessage", + "migrationhub-orchestrator:GetTemplate", + "migrationhub-orchestrator:GetTemplateStep", + "migrationhub-orchestrator:GetTemplateStepGroup", + "migrationhub-orchestrator:GetWorkflow", + "migrationhub-orchestrator:GetWorkflowStep", + "migrationhub-orchestrator:GetWorkflowStepGroup", + "migrationhub-orchestrator:ListPlugins", + "migrationhub-orchestrator:ListTemplateStepGroups", + "migrationhub-orchestrator:ListTemplateSteps", + "migrationhub-orchestrator:ListTemplates", + "migrationhub-orchestrator:ListWorkflowStepGroups", + "migrationhub-orchestrator:ListWorkflowSteps", + "migrationhub-orchestrator:ListWorkflows", + "migrationhub-orchestrator:RegisterPlugin", + "migrationhub-orchestrator:RetryWorkflowStep", + "migrationhub-orchestrator:SendMessage", + "migrationhub-orchestrator:StartWorkflow", + "migrationhub-orchestrator:StopWorkflow", + "migrationhub-orchestrator:UpdateWorkflow", + "migrationhub-orchestrator:UpdateWorkflowStep", + "migrationhub-orchestrator:UpdateWorkflowStepGroup", "migrationhub-strategy:GetAntiPattern", "migrationhub-strategy:GetApplicationComponentDetails", "migrationhub-strategy:GetApplicationComponentStrategies", @@ -8939,7 +8988,6 @@ "rds:CopyDBParameterGroup", "rds:CopyDBSnapshot", "rds:CopyOptionGroup", - "rds:CreateCustomAvailabilityZone", "rds:CreateCustomDBEngineVersion", "rds:CreateDBCluster", "rds:CreateDBClusterEndpoint", @@ -8957,7 +9005,6 @@ "rds:CreateGlobalCluster", "rds:CreateOptionGroup", "rds:CrossRegionCommunication", - "rds:DeleteCustomAvailabilityZone", "rds:DeleteCustomDBEngineVersion", "rds:DeleteDBCluster", "rds:DeleteDBClusterEndpoint", @@ -8973,12 +9020,10 @@ "rds:DeleteDBSubnetGroup", "rds:DeleteEventSubscription", "rds:DeleteGlobalCluster", - "rds:DeleteInstallationMedia", "rds:DeleteOptionGroup", "rds:DeregisterDBProxyTargets", "rds:DescribeAccountAttributes", "rds:DescribeCertificates", - "rds:DescribeCustomAvailabilityZones", "rds:DescribeDBClusterBacktracks", "rds:DescribeDBClusterEndpoints", "rds:DescribeDBClusterParameterGroups", @@ -9007,7 +9052,6 @@ "rds:DescribeEvents", "rds:DescribeExportTasks", "rds:DescribeGlobalClusters", - "rds:DescribeInstallationMedia", "rds:DescribeOptionGroupOptions", "rds:DescribeOptionGroups", "rds:DescribeOrderableDBInstanceOptions", @@ -9022,7 +9066,6 @@ "rds:DownloadDBLogFilePortion", "rds:FailoverDBCluster", "rds:FailoverGlobalCluster", - "rds:ImportInstallationMedia", "rds:ListTagsForResource", "rds:ModifyCertificates", "rds:ModifyCurrentDBClusterCapacity", diff --git a/pycfmodel/model/resources/properties/policy_document.py b/pycfmodel/model/resources/properties/policy_document.py index 8d761fa9..9e74f362 100644 --- a/pycfmodel/model/resources/properties/policy_document.py +++ b/pycfmodel/model/resources/properties/policy_document.py @@ -35,6 +35,10 @@ def _statement_as_list(self) -> List[Statement]: return [self.Statement] return self.Statement + @staticmethod + def _is_statement_effect_allow(statement_effect: str) -> bool: + return statement_effect.lower() == "allow" + def statements_with(self, pattern: Pattern) -> List[Statement]: """ Finds all statements which have at least one resource with the pattern. @@ -60,7 +64,7 @@ def allowed_actions_with(self, pattern: Pattern) -> List[Statement]: return [ statement for statement in self._statement_as_list() - if statement.actions_with(pattern) and statement.Effect == "Allow" + if statement.actions_with(pattern) and self._is_statement_effect_allow(statement.Effect) ] def allowed_principals_with(self, pattern: Pattern) -> List[str]: @@ -75,7 +79,7 @@ def allowed_principals_with(self, pattern: Pattern) -> List[str]: """ principals = set() for statement in self._statement_as_list(): - if statement.Effect == "Allow": + if self._is_statement_effect_allow(statement.Effect): principals.update(statement.principals_with(pattern)) return list(principals) @@ -91,7 +95,7 @@ def non_whitelisted_allowed_principals(self, whitelist: List[str]) -> List[str]: """ principals = set() for statement in self._statement_as_list(): - if statement.Effect == "Allow": + if self._is_statement_effect_allow(statement.Effect): principals.update(statement.non_whitelisted_principals(whitelist)) return list(principals) @@ -130,6 +134,6 @@ def get_allowed_actions(self) -> List[str]: """ actions = set() for statement in self._statement_as_list(): - if statement.Effect.lower() == "allow": + if self._is_statement_effect_allow(statement.Effect): actions.update(statement.get_expanded_action_list()) return sorted(actions) diff --git a/pycfmodel/model/resources/properties/statement.py b/pycfmodel/model/resources/properties/statement.py index ce42b0e4..0057418d 100644 --- a/pycfmodel/model/resources/properties/statement.py +++ b/pycfmodel/model/resources/properties/statement.py @@ -25,7 +25,7 @@ class Principal(Property): class Statement(Property): """ - Contains information about an attached policy. + Contains information about an statement of a policy document. Properties: @@ -43,7 +43,7 @@ class Statement(Property): """ Sid: Optional[ResolvableStr] = None - Effect: Optional[ResolvableStr] = None + Effect: ResolvableStr Principal: Optional[PrincipalTypes] = None NotPrincipal: Optional[PrincipalTypes] = None Action: Optional[ResolvableStrOrList] = None @@ -53,9 +53,11 @@ class Statement(Property): Condition: Optional[StatementCondition] = None @validator("Effect") - def capitalize_if_str(cls, v: ResolvableStr): + def allowed_values_for_effect_and_capitalized(cls, v: ResolvableStr): if isinstance(v, str): - return v.capitalize() + v = v.capitalize() + if v not in ["Allow", "Deny"]: + raise ValueError("Effect on Statement must either be Allow or Deny") return v def get_action_list(self, include_action=True, include_not_action=True) -> List[ResolvableStr]: diff --git a/setup.py b/setup.py index a1de52ca..13bc50fe 100644 --- a/setup.py +++ b/setup.py @@ -21,7 +21,7 @@ setup( name="pycfmodel", - version="0.19.0", + version="0.19.1", description="A python model for CloudFormation scripts", author="Skyscanner Product Security", author_email="security@skyscanner.net", diff --git a/tests/resources/properties/test_policy_document.py b/tests/resources/properties/test_policy_document.py index 1b5a2a12..e644c891 100644 --- a/tests/resources/properties/test_policy_document.py +++ b/tests/resources/properties/test_policy_document.py @@ -1,6 +1,7 @@ import re from ipaddress import IPv4Network +import pytest from pytest import fixture from pycfmodel.cloudformation_actions import CLOUDFORMATION_ACTIONS @@ -34,7 +35,7 @@ def policy_document_multi_statement(): "Action": ["sts:AssumeRole"], }, { - "Effect": "bar", + "Effect": "Allow", "Principal": {"Service": ["ec2.amazonaws.com"], "AWS": "arn:aws:iam::324320755747:root"}, "Action": ["sts:AssumeRole"], }, @@ -179,7 +180,7 @@ def test_one_statement(policy_document_one_statement): def test_multi_statements(policy_document_multi_statement): assert policy_document_multi_statement.Statement[0].Effect == "Allow" - assert policy_document_multi_statement.Statement[1].Effect == "Bar" + assert policy_document_multi_statement.Statement[1].Effect == "Allow" def test_star_resource(policy_document_star_resource): @@ -337,6 +338,17 @@ def policy_document_condition_with_source_vpce(): ) +@pytest.mark.parametrize( + "statement_effect, is_allow", + [ + ("Allow", True), + ("Deny", False), + ], +) +def test_policy_document_chan_check_if_the_statement_effect_is_allow_or_deny(statement_effect: str, is_allow: bool): + assert PolicyDocument._is_statement_effect_allow(statement_effect=statement_effect) == is_allow + + def test_policy_document_condition_with_source_ip(policy_document_condition_with_source_ip: PolicyDocument): assert policy_document_condition_with_source_ip.Statement[0].Condition.IpAddress == { "aws:SourceIp": [IPv4Network("116.202.65.160/32"), IPv4Network("116.202.68.32/27")] diff --git a/tests/resources/properties/test_statement.py b/tests/resources/properties/test_statement.py index f6c4c8d0..1bee5b2e 100644 --- a/tests/resources/properties/test_statement.py +++ b/tests/resources/properties/test_statement.py @@ -22,62 +22,68 @@ def statement_4(): def statement_principal_1(): - return Statement(**{"Principal": {"AWS": "arn:aws:iam::123456789012:root"}}) + return Statement(**{"Effect": "Allow", "Principal": {"AWS": "arn:aws:iam::123456789012:root"}}) def statement_principal_2(): return Statement( **{ + "Effect": "Allow", "Principal": { "AWS": ["arn:aws:iam::AWS-account-ID:user/user-name-1", "arn:aws:iam::AWS-account-ID:user/UserName2"] - } + }, } ) def statement_principal_3(): - return Statement(**{"Principal": {"Federated": "cognito-identity.amazonaws.com"}}) + return Statement(**{"Effect": "Allow", "Principal": {"Federated": "cognito-identity.amazonaws.com"}}) def statement_principal_4(): - return Statement(**{"Principal": "arn:aws:iam::123456789012:root"}) + return Statement(**{"Effect": "Allow", "Principal": "arn:aws:iam::123456789012:root"}) def statement_principal_5(): return Statement( - **{"Principal": ["arn:aws:iam::AWS-account-ID:user/user-name-1", "arn:aws:iam::AWS-account-ID:user/UserName2"]} + **{ + "Effect": "Allow", + "Principal": ["arn:aws:iam::AWS-account-ID:user/user-name-1", "arn:aws:iam::AWS-account-ID:user/UserName2"], + } ) def statement_not_principal_1(): - return Statement(**{"NotPrincipal": {"AWS": "arn:aws:iam::123456789012:root"}}) + return Statement(**{"Effect": "Allow", "NotPrincipal": {"AWS": "arn:aws:iam::123456789012:root"}}) def statement_not_principal_2(): return Statement( **{ + "Effect": "Allow", "NotPrincipal": { "AWS": ["arn:aws:iam::AWS-account-ID:user/user-name-1", "arn:aws:iam::AWS-account-ID:user/UserName2"] - } + }, } ) def statement_not_principal_3(): - return Statement(**{"NotPrincipal": {"Federated": "cognito-identity.amazonaws.com"}}) + return Statement(**{"Effect": "Allow", "NotPrincipal": {"Federated": "cognito-identity.amazonaws.com"}}) def statement_not_principal_4(): - return Statement(**{"NotPrincipal": "arn:aws:iam::123456789012:root"}) + return Statement(**{"Effect": "Allow", "NotPrincipal": "arn:aws:iam::123456789012:root"}) def statement_not_principal_5(): return Statement( **{ + "Effect": "Allow", "NotPrincipal": [ "arn:aws:iam::AWS-account-ID:user/user-name-1", "arn:aws:iam::AWS-account-ID:user/UserName2", - ] + ], } ) @@ -87,6 +93,11 @@ def test_capitalize_effect(): assert statement.Effect == "Allow" +def test_statement_effect_raises_error_if_it_is_not_an_allowed_value(): + with pytest.raises(ValueError): + Statement(**{"Effect": "another string"}) + + @pytest.mark.parametrize( "statement, expected_output", [