From b3975c5dc83c5d82a23db079df42d70efe79530e Mon Sep 17 00:00:00 2001 From: "Kenta Goto (k.goto)" <24818752+go-to-k@users.noreply.github.com> Date: Sat, 1 Feb 2025 07:43:05 +0900 Subject: [PATCH] fix(sns): topic policy is not created even if enforceSSL enabled (#31569) ### Issue # (if applicable) Closes #31558. ### Reason for this change SNS topic policy is not created even if `enforceSSL` is enabled, until calling `addToResourcePolicy` method. But, originally, the policy should be created without calling the `addToResourcePolicy` method. ### Description of changes The topic policy is created first if the `enforceSSL` is enabled. ### Description of how you validated changes Unit and integ tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: GZ --- ...efaultTestDeployAssert005CA6BA.assets.json | 2 +- .../SNSTopicPolicyStack.assets.json | 6 +- .../SNSTopicPolicyStack.template.json | 40 ++++- .../cdk.out | 2 +- .../integ.json | 2 +- .../manifest.json | 18 ++- .../tree.json | 137 +++++++++++++----- .../aws-sns/test/integ.sns-topic-policy.ts | 4 + .../aws-cdk-lib/aws-sns/lib/topic-base.ts | 35 +++-- packages/aws-cdk-lib/aws-sns/lib/topic.ts | 4 + packages/aws-cdk-lib/aws-sns/test/sns.test.ts | 45 +++++- 11 files changed, 233 insertions(+), 62 deletions(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyIntegDefaultTestDeployAssert005CA6BA.assets.json b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyIntegDefaultTestDeployAssert005CA6BA.assets.json index 3a6d49824aa28..edccf882f98ca 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyIntegDefaultTestDeployAssert005CA6BA.assets.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyIntegDefaultTestDeployAssert005CA6BA.assets.json @@ -1,5 +1,5 @@ { - "version": "36.0.0", + "version": "38.0.1", "files": { "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": { "source": { diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyStack.assets.json b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyStack.assets.json index 106268b0f976a..3eb5f2e84aff0 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyStack.assets.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyStack.assets.json @@ -1,7 +1,7 @@ { - "version": "36.0.0", + "version": "38.0.1", "files": { - "14fdbf9da76c6d2a9fd9d7ecce1e6fb899d1bd67a967d68ab86428209e729203": { + "a2e03faff837f5b6f5c47d6e7e6ca3c90397e9be8cc83d53fa17416fdf27756c": { "source": { "path": "SNSTopicPolicyStack.template.json", "packaging": "file" @@ -9,7 +9,7 @@ "destinations": { "current_account-current_region": { "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}", - "objectKey": "14fdbf9da76c6d2a9fd9d7ecce1e6fb899d1bd67a967d68ab86428209e729203.json", + "objectKey": "a2e03faff837f5b6f5c47d6e7e6ca3c90397e9be8cc83d53fa17416fdf27756c.json", "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}" } } diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyStack.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyStack.template.json index c6af60e302553..ea27eacf3365e 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyStack.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/SNSTopicPolicyStack.template.json @@ -59,6 +59,20 @@ "Properties": { "PolicyDocument": { "Statement": [ + { + "Action": "sns:Publish", + "Condition": { + "Bool": { + "aws:SecureTransport": "false" + } + }, + "Effect": "Deny", + "Principal": "*", + "Resource": { + "Ref": "TopicAddPolicy7DB03706" + }, + "Sid": "AllowPublishThroughSSLOnly" + }, { "Action": "sns:Publish", "Effect": "Allow", @@ -68,8 +82,26 @@ "Resource": { "Ref": "TopicAddPolicy7DB03706" }, - "Sid": "0" - }, + "Sid": "1" + } + ], + "Version": "2012-10-17" + }, + "Topics": [ + { + "Ref": "TopicAddPolicy7DB03706" + } + ] + } + }, + "TopicWithSSLC879A4EA": { + "Type": "AWS::SNS::Topic" + }, + "TopicWithSSLPolicy3E7ECD75": { + "Type": "AWS::SNS::TopicPolicy", + "Properties": { + "PolicyDocument": { + "Statement": [ { "Action": "sns:Publish", "Condition": { @@ -80,7 +112,7 @@ "Effect": "Deny", "Principal": "*", "Resource": { - "Ref": "TopicAddPolicy7DB03706" + "Ref": "TopicWithSSLC879A4EA" }, "Sid": "AllowPublishThroughSSLOnly" } @@ -89,7 +121,7 @@ }, "Topics": [ { - "Ref": "TopicAddPolicy7DB03706" + "Ref": "TopicWithSSLC879A4EA" } ] } diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/cdk.out b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/cdk.out index 1f0068d32659a..c6e612584e352 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/cdk.out +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/cdk.out @@ -1 +1 @@ -{"version":"36.0.0"} \ No newline at end of file +{"version":"38.0.1"} \ No newline at end of file diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/integ.json b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/integ.json index 99eb1a0d311f1..b452e755ae02f 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/integ.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/integ.json @@ -1,5 +1,5 @@ { - "version": "36.0.0", + "version": "38.0.1", "testCases": { "SNSTopicPolicyInteg/DefaultTest": { "stacks": [ diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/manifest.json b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/manifest.json index 1c70ea54b89c0..d71c2248ddb9d 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/manifest.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/manifest.json @@ -1,5 +1,5 @@ { - "version": "36.0.0", + "version": "38.0.1", "artifacts": { "SNSTopicPolicyStack.assets": { "type": "cdk:asset-manifest", @@ -16,9 +16,10 @@ "templateFile": "SNSTopicPolicyStack.template.json", "terminationProtection": false, "validateOnSynth": false, + "notificationArns": [], "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", - "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/14fdbf9da76c6d2a9fd9d7ecce1e6fb899d1bd67a967d68ab86428209e729203.json", + "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/a2e03faff837f5b6f5c47d6e7e6ca3c90397e9be8cc83d53fa17416fdf27756c.json", "requiresBootstrapStackVersion": 6, "bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version", "additionalDependencies": [ @@ -58,6 +59,18 @@ "data": "TopicAddPolicyAEA24A5A" } ], + "/SNSTopicPolicyStack/TopicWithSSL/Resource": [ + { + "type": "aws:cdk:logicalId", + "data": "TopicWithSSLC879A4EA" + } + ], + "/SNSTopicPolicyStack/TopicWithSSL/Policy/Resource": [ + { + "type": "aws:cdk:logicalId", + "data": "TopicWithSSLPolicy3E7ECD75" + } + ], "/SNSTopicPolicyStack/BootstrapVersion": [ { "type": "aws:cdk:logicalId", @@ -88,6 +101,7 @@ "templateFile": "SNSTopicPolicyIntegDefaultTestDeployAssert005CA6BA.template.json", "terminationProtection": false, "validateOnSynth": false, + "notificationArns": [], "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json", diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/tree.json b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/tree.json index f65edb84ed63b..a6773db4f1cc6 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/tree.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.js.snapshot/tree.json @@ -23,14 +23,14 @@ } }, "constructInfo": { - "fqn": "aws-cdk-lib.aws_sns.CfnTopic", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } } }, "constructInfo": { - "fqn": "aws-cdk-lib.aws_sns.Topic", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } }, "TopicPolicy": { @@ -81,14 +81,14 @@ } }, "constructInfo": { - "fqn": "aws-cdk-lib.aws_sns.CfnTopicPolicy", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } } }, "constructInfo": { - "fqn": "aws-cdk-lib.aws_sns.TopicPolicy", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } }, "TopicAddPolicy": { @@ -106,8 +106,8 @@ } }, "constructInfo": { - "fqn": "aws-cdk-lib.aws_sns.CfnTopic", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } }, "Policy": { @@ -122,6 +122,20 @@ "aws:cdk:cloudformation:props": { "policyDocument": { "Statement": [ + { + "Action": "sns:Publish", + "Condition": { + "Bool": { + "aws:SecureTransport": "false" + } + }, + "Effect": "Deny", + "Principal": "*", + "Resource": { + "Ref": "TopicAddPolicy7DB03706" + }, + "Sid": "AllowPublishThroughSSLOnly" + }, { "Action": "sns:Publish", "Effect": "Allow", @@ -131,8 +145,63 @@ "Resource": { "Ref": "TopicAddPolicy7DB03706" }, - "Sid": "0" - }, + "Sid": "1" + } + ], + "Version": "2012-10-17" + }, + "topics": [ + { + "Ref": "TopicAddPolicy7DB03706" + } + ] + } + }, + "constructInfo": { + "fqn": "constructs.Construct", + "version": "10.3.0" + } + } + }, + "constructInfo": { + "fqn": "constructs.Construct", + "version": "10.3.0" + } + } + }, + "constructInfo": { + "fqn": "constructs.Construct", + "version": "10.3.0" + } + }, + "TopicWithSSL": { + "id": "TopicWithSSL", + "path": "SNSTopicPolicyStack/TopicWithSSL", + "children": { + "Resource": { + "id": "Resource", + "path": "SNSTopicPolicyStack/TopicWithSSL/Resource", + "attributes": { + "aws:cdk:cloudformation:type": "AWS::SNS::Topic", + "aws:cdk:cloudformation:props": {} + }, + "constructInfo": { + "fqn": "constructs.Construct", + "version": "10.3.0" + } + }, + "Policy": { + "id": "Policy", + "path": "SNSTopicPolicyStack/TopicWithSSL/Policy", + "children": { + "Resource": { + "id": "Resource", + "path": "SNSTopicPolicyStack/TopicWithSSL/Policy/Resource", + "attributes": { + "aws:cdk:cloudformation:type": "AWS::SNS::TopicPolicy", + "aws:cdk:cloudformation:props": { + "policyDocument": { + "Statement": [ { "Action": "sns:Publish", "Condition": { @@ -143,7 +212,7 @@ "Effect": "Deny", "Principal": "*", "Resource": { - "Ref": "TopicAddPolicy7DB03706" + "Ref": "TopicWithSSLC879A4EA" }, "Sid": "AllowPublishThroughSSLOnly" } @@ -152,48 +221,48 @@ }, "topics": [ { - "Ref": "TopicAddPolicy7DB03706" + "Ref": "TopicWithSSLC879A4EA" } ] } }, "constructInfo": { - "fqn": "aws-cdk-lib.aws_sns.CfnTopicPolicy", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } } }, "constructInfo": { - "fqn": "aws-cdk-lib.aws_sns.TopicPolicy", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } } }, "constructInfo": { - "fqn": "aws-cdk-lib.aws_sns.Topic", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } }, "BootstrapVersion": { "id": "BootstrapVersion", "path": "SNSTopicPolicyStack/BootstrapVersion", "constructInfo": { - "fqn": "aws-cdk-lib.CfnParameter", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } }, "CheckBootstrapVersion": { "id": "CheckBootstrapVersion", "path": "SNSTopicPolicyStack/CheckBootstrapVersion", "constructInfo": { - "fqn": "aws-cdk-lib.CfnRule", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } } }, "constructInfo": { - "fqn": "aws-cdk-lib.Stack", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } }, "SNSTopicPolicyInteg": { @@ -220,22 +289,22 @@ "id": "BootstrapVersion", "path": "SNSTopicPolicyInteg/DefaultTest/DeployAssert/BootstrapVersion", "constructInfo": { - "fqn": "aws-cdk-lib.CfnParameter", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } }, "CheckBootstrapVersion": { "id": "CheckBootstrapVersion", "path": "SNSTopicPolicyInteg/DefaultTest/DeployAssert/CheckBootstrapVersion", "constructInfo": { - "fqn": "aws-cdk-lib.CfnRule", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } } }, "constructInfo": { - "fqn": "aws-cdk-lib.Stack", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } } }, @@ -260,8 +329,8 @@ } }, "constructInfo": { - "fqn": "aws-cdk-lib.App", - "version": "0.0.0" + "fqn": "constructs.Construct", + "version": "10.3.0" } } } \ No newline at end of file diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.ts index 9aa9b9c73b58b..bb90221b5b1dd 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.ts +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-sns/test/integ.sns-topic-policy.ts @@ -41,6 +41,10 @@ topicAddPolicy.addToResourcePolicy(new PolicyStatement({ resources: [topicAddPolicy.topicArn], })); +new Topic(stack, 'TopicWithSSL', { + enforceSSL: true, +}); + new IntegTest(app, 'SNSTopicPolicyInteg', { testCases: [stack], stackUpdateWorkflow: false, diff --git a/packages/aws-cdk-lib/aws-sns/lib/topic-base.ts b/packages/aws-cdk-lib/aws-sns/lib/topic-base.ts index 224f801d40450..779d2f1e82fce 100644 --- a/packages/aws-cdk-lib/aws-sns/lib/topic-base.ts +++ b/packages/aws-cdk-lib/aws-sns/lib/topic-base.ts @@ -133,25 +133,42 @@ export abstract class TopicBase extends Resource implements ITopic { * Adds a statement to the IAM resource policy associated with this topic. * * If this topic was created in this stack (`new Topic`), a topic policy - * will be automatically created upon the first call to `addToResourcePolicy`. If - * the topic is imported (`Topic.import`), then this is a no-op. + * will be automatically created upon the first call to `addToResourcePolicy`. + * However, if `enforceSSL` is set to `true`, the policy has already been created + * before the first call to this method. + * + * If the topic is imported (`Topic.import`), then this is a no-op. */ public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { - if (!this.policy && this.autoCreatePolicy) { - this.policy = new TopicPolicy(this, 'Policy', { topics: [this] }); - } + this.createTopicPolicy(); if (this.policy) { this.policy.document.addStatements(statement); - - if (this.enforceSSL) { - this.policy.document.addStatements(this.createSSLPolicyDocument()); - } return { statementAdded: true, policyDependable: this.policy }; } return { statementAdded: false }; } + /** + * Adds a SSL policy to the topic resource policy. + */ + protected addSSLPolicy(): void { + this.createTopicPolicy(); + + if (this.policy) { + this.policy.document.addStatements(this.createSSLPolicyDocument()); + } + } + + /** + * Creates a topic policy for this topic. + */ + protected createTopicPolicy(): void { + if (!this.policy && this.autoCreatePolicy) { + this.policy = new TopicPolicy(this, 'Policy', { topics: [this] }); + } + } + /** * Adds a statement to enforce encryption of data in transit when publishing to the topic. * diff --git a/packages/aws-cdk-lib/aws-sns/lib/topic.ts b/packages/aws-cdk-lib/aws-sns/lib/topic.ts index a5c0849fb5af6..5ca9aa169a099 100644 --- a/packages/aws-cdk-lib/aws-sns/lib/topic.ts +++ b/packages/aws-cdk-lib/aws-sns/lib/topic.ts @@ -356,6 +356,10 @@ export class Topic extends TopicBase { this.topicName = this.getResourceNameAttribute(resource.attrTopicName); this.fifo = props.fifo || false; this.contentBasedDeduplication = props.contentBasedDeduplication || false; + + if (this.enforceSSL) { + this.addSSLPolicy(); + } } private renderLoggingConfigs(): CfnTopic.LoggingConfigProperty[] { diff --git a/packages/aws-cdk-lib/aws-sns/test/sns.test.ts b/packages/aws-cdk-lib/aws-sns/test/sns.test.ts index 225f6d7caeda1..6db7b6e243761 100644 --- a/packages/aws-cdk-lib/aws-sns/test/sns.test.ts +++ b/packages/aws-cdk-lib/aws-sns/test/sns.test.ts @@ -205,6 +205,37 @@ describe('Topic', () => { }); test('can enforce ssl when creating the topic', () => { + // GIVEN + const stack = new cdk.Stack(); + new sns.Topic(stack, 'Topic', { + enforceSSL: true, + }); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::SNS::TopicPolicy', { + PolicyDocument: { + Version: '2012-10-17', + Statement: [ + { + 'Sid': 'AllowPublishThroughSSLOnly', + 'Action': 'sns:Publish', + 'Effect': 'Deny', + 'Resource': { + 'Ref': 'TopicBFC7AF6E', + }, + 'Condition': { + 'Bool': { + 'aws:SecureTransport': 'false', + }, + }, + 'Principal': '*', + }, + ], + }, + }); + }); + + test('can enforce ssl with addToResourcePolicy method', () => { // GIVEN const stack = new cdk.Stack(); const topic = new sns.Topic(stack, 'Topic', { @@ -223,13 +254,6 @@ describe('Topic', () => { PolicyDocument: { Version: '2012-10-17', Statement: [ - { - 'Sid': '0', - 'Action': 'sns:*', - 'Effect': 'Allow', - 'Principal': { 'AWS': 'arn' }, - 'Resource': '*', - }, { 'Sid': 'AllowPublishThroughSSLOnly', 'Action': 'sns:Publish', @@ -244,6 +268,13 @@ describe('Topic', () => { }, 'Principal': '*', }, + { + 'Sid': '1', + 'Action': 'sns:*', + 'Effect': 'Allow', + 'Principal': { 'AWS': 'arn' }, + 'Resource': '*', + }, ], }, });