Skip to content
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.

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

sns: topic enforceSsl does not work #30456

Closed
mrgum opened this issue Jun 5, 2024 · 6 comments
Closed

sns: topic enforceSsl does not work #30456

mrgum opened this issue Jun 5, 2024 · 6 comments
Assignees
Labels
@aws-cdk/aws-sns Related to Amazon Simple Notification Service bug This issue is a bug.

Comments

@mrgum
Copy link

mrgum commented Jun 5, 2024

Describe the bug

I'm using to use enforceSsl from https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_sns/Topic.html introduced in 2.129.0 #29144

But it does not add the policy it should. the synth is the same with or without the property, with it set as true or false.

Its like it makes no difference

Expected Behavior

policy added to topic

Current Behavior

nothing, no effect at all

Reproduction Steps

import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as sns from 'aws-cdk-lib/aws-sns';

export class TopicTsStack extends cdk.Stack {
constructor(scope: Construct, id: string, props?: cdk.StackProps) {
super(scope, id, props);

const queue = new sns.Topic(this, 'Topic', {
  enforceSSL: true
});

}
}

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.129.0 (build d5ab0df)

Framework Version

No response

Node.js Version

v18.17.1

OS

mac

Language

TypeScript, Python

Language Version

TypeScript Version 4.6.2

Other information

i've tried updating cdk etc no change, same using python rather than typescript

@mrgum mrgum added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 5, 2024
@github-actions github-actions bot added the @aws-cdk/aws-sns Related to Amazon Simple Notification Service label Jun 5, 2024
@ashishdhingra ashishdhingra self-assigned this Jun 5, 2024
@ashishdhingra
Copy link
Contributor

@mrgum Good morning. Thanks for reporting the issue. Per Enforce encryption of data in transit when publishing to a topic, enforceSSL flag enforces SSL when creating a topic policy. Do you would need to create a topic policy for this flag to take effect. Using the example code in above link:

import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as sns from 'aws-cdk-lib/aws-sns';
import * as iam from 'aws-cdk-lib/aws-iam';

export class TestStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const topic = new sns.Topic(this, 'MyTopic', {
      enforceSSL: true
    });

    topic.addToResourcePolicy(new iam.PolicyStatement({
      principals: [new iam.ServicePrincipal('s3.amazonaws.com')],
      actions: ['sns:Publish'],
      resources: [topic.topicArn],
    }));
  }
}

and running cdk synth produces the below CloudFormation template:

Resources:
  MyTopic86869434:
    Type: AWS::SNS::Topic
    Metadata:
      aws:cdk:path: TestStack/MyTopic/Resource
  MyTopicPolicy12A5EC17:
    Type: AWS::SNS::TopicPolicy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sns:Publish
            Effect: Allow
            Principal:
              Service: s3.amazonaws.com
            Resource:
              Ref: MyTopic86869434
            Sid: "0"
          - Action: sns:Publish
            Condition:
              Bool:
                aws:SecureTransport: "false"
            Effect: Deny
            Principal: "*"
            Resource:
              Ref: MyTopic86869434
            Sid: AllowPublishThroughSSLOnly
        Version: "2012-10-17"
      Topics:
        - Ref: MyTopic86869434
    Metadata:
      aws:cdk:path: TestStack/MyTopic/Policy/Resource
...
...

Notice the Deny action when aws:SecureTransport is false.

Thanks,
Ashish

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 5, 2024
@mrgum
Copy link
Author

mrgum commented Jun 6, 2024

Thank you, that explains it and works with typescript. Oddly the same thing in python does not work

class TopicStack(Stack):

    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        topic = sns.Topic(self, "topic", enforce_ssl=True)

        topic_policy_document = iam.PolicyDocument(
            statements=[
                iam.PolicyStatement(
                    actions=["sns:Publish"],
                    principals=[iam.ServicePrincipal("s3.amazonaws.com")],
                    resources=[topic.topic_arn],
                ),
            ]
        )
        sns.TopicPolicy(
            self,
            "TopicPolicy",
            topics=[topic],
            policy_document=topic_policy_document,
        )

gives

Resources:
  topic69831491:
    Type: AWS::SNS::Topic
    Metadata:
      aws:cdk:path: TopicStack/topic/Resource
  TopicPolicyA24B096F:
    Type: AWS::SNS::TopicPolicy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sns:Publish
            Effect: Allow
            Principal:
              Service: s3.amazonaws.com
            Resource:
              Ref: topic69831491
        Version: "2012-10-17"
      Topics:
        - Ref: topic69831491

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 6, 2024
@ashishdhingra
Copy link
Contributor

@mrgum Good afternoon. The equivalent Python code for the one shared in #30456 (comment):

from aws_cdk import (
    Stack,
    aws_sns as sns,
    aws_iam as iam
)
from constructs import Construct

class PythonStack(Stack):

    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        topic = sns.Topic(self, "MyTopic", enforce_ssl=True)
        topic.add_to_resource_policy(
            iam.PolicyStatement(
                principals=[iam.ServicePrincipal("s3.amazonaws.com")],
                actions=["sns:Publish"],
                resources=[topic.topic_arn]
            )
        )

produces the expected CloudFormation stack when running cdk synth:

Resources:
  MyTopic86869434:
    Type: AWS::SNS::Topic
    Metadata:
      aws:cdk:path: PythonStack/MyTopic/Resource
  MyTopicPolicy12A5EC17:
    Type: AWS::SNS::TopicPolicy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sns:Publish
            Effect: Allow
            Principal:
              Service: s3.amazonaws.com
            Resource:
              Ref: MyTopic86869434
            Sid: "0"
          - Action: sns:Publish
            Condition:
              Bool:
                aws:SecureTransport: "false"
            Effect: Deny
            Principal: "*"
            Resource:
              Ref: MyTopic86869434
            Sid: AllowPublishThroughSSLOnly
        Version: "2012-10-17"
      Topics:
        - Ref: MyTopic86869434
    Metadata:
      aws:cdk:path: PythonStack/MyTopic/Policy/Resource
...
...

When creating creating a topic policy, the enforceSSL attribute needs to be set at TopicPolicy level as per example at Enforce encryption of data in transit when publishing to a topic. The below code (taken from the example):

from aws_cdk import (
    Stack,
    aws_sns as sns,
    aws_iam as iam
)
from constructs import Construct

class PythonStack(Stack):

    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        topic = sns.Topic(self, "myTopic");
        policyDocument = iam.PolicyDocument(
            assign_sids=True,
            statements=[
                iam.PolicyStatement(
                    actions=["sns:Publish"],
                    principals=[iam.ServicePrincipal('s3.amazonaws.com')],
                    resources=[topic.topic_arn]
            )]
        )

        topicPolicy = sns.TopicPolicy(self, "Policy", 
                                      topics=[topic],
                                      policy_document=policyDocument,
                                      enforce_ssl=True
        )

produces the below expected CloudFormation template when running cdk synth:

Resources:
  myTopicDE69997A:
    Type: AWS::SNS::Topic
    Metadata:
      aws:cdk:path: PythonStack/myTopic/Resource
  Policy23B91518:
    Type: AWS::SNS::TopicPolicy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sns:Publish
            Effect: Allow
            Principal:
              Service: s3.amazonaws.com
            Resource:
              Ref: myTopicDE69997A
            Sid: "0"
          - Action: sns:Publish
            Condition:
              Bool:
                aws:SecureTransport: "false"
            Effect: Deny
            Principal: "*"
            Resource:
              Ref: myTopicDE69997A
            Sid: AllowPublishThroughSSLOnly
        Version: "2012-10-17"
      Topics:
        - Ref: myTopicDE69997A
...
...

Please let me know if this makes the guidance more clearer. 😄

Thanks,
Ashish

@ashishdhingra ashishdhingra added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 6, 2024
@mrgum
Copy link
Author

mrgum commented Jun 7, 2024

Hello Ashish,

Yes that makes it clearer thank you.

I wonder if it would be possible to warning when enforce_ssl is used in the Topic in the way I was when it has no effect?

Thanks

Neil

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 7, 2024
@ashishdhingra
Copy link
Contributor

Hello Ashish,

Yes that makes it clearer thank you.

I wonder if it would be possible to warning when enforce_ssl is used in the Topic in the way I was when it has no effect?

Thanks

Neil

@mrgum I'm unsure if we could selectively add any warning since the Python code is transformed to TypeScript by the JSII layer, which is then synthesized to CloudFormation template. The Python library generation is automated and hence it might not be feasible to add this warning.

@ashishdhingra ashishdhingra closed this as not planned Won't fix, can't repro, duplicate, stale Jun 7, 2024
Copy link

github-actions bot commented Jun 7, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-sns Related to Amazon Simple Notification Service bug This issue is a bug.
Projects
None yet
Development

No branches or pull requests

2 participants