(aws-sns): grantPublish should also grant permission to decrypt master key

[kornicameister]

What is the problem?

When calling grantPublish on topic method should add permissions to decrypt master key in order to properly send messages.

Reproduction Steps

from aws_cdk import (
    aws_sns as sns,
    aws_kms as kms,
    aws_iam as iam,
    core as cdk,
)

class Test(cdk.Stack):

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

        key = kms.Key(self, 'Key')
        topic = sns.Topic(self, 'Topic', master_key=key)
        topic.grant_publish(
            iam.Role(self, 'Role', assumed_by=iam.ServicePrincipal('ec2'))
        )

app = cdk.App()
Test(app, 'BUG')
app.synth()

What did you expect to happen?

RoleDefaultPolicy5FFB7DAB is generated with enty allowing to encrypt published message.

  RoleDefaultPolicy5FFB7DAB:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sns:Publish
            Effect: Allow
            Resource:
              Ref: TopicBFC7AF6E
          - Action:
              - kms:Encrypt
              - kms:ReEncrypt*
              - kms:GenerateDataKey*
            Effect: Allow
            Resource:
              Fn::GetAtt:
                - Key961B73FD
                - Arn
        Version: "2012-10-17"
      PolicyName: RoleDefaultPolicy5FFB7DAB
      Roles:
        - Ref: Role1ABCC5F0
    Metadata:
      aws:cdk:path: BUG/Role/DefaultPolicy/Resource

What actually happened?

RoleDefaultPolicy5FFB7DAB contains a permission to publish messages to topic but that's about it.

  RoleDefaultPolicy5FFB7DAB:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sns:Publish
            Effect: Allow
            Resource:
              Ref: TopicBFC7AF6E
        Version: "2012-10-17"
      PolicyName: RoleDefaultPolicy5FFB7DAB
      Roles:
        - Ref: Role1ABCC5F0
    Metadata:
      aws:cdk:path: BUG/Role/DefaultPolicy/Resource

CDK CLI Version

1.138.0

Framework Version

No response

Node.js Version

14.17.5

OS

MacOs BigSur

Language

Python

Language Version

3.10.1

Other information

It might not be easy to implement this because grantPublish is defined at TopicBase that does not contain reference to masterKey. Question is, can we retrieve such information for TopicBase or for topics that are being imported.

[njlynch]

It might not be easy to implement this because grantPublish is defined at TopicBase that does not contain reference to masterKey. Question is, can we retrieve such information for TopicBase or for topics that are being imported.

The solution likely would be to override grantPublish on the concrete Topic, and add the key-specific grants there.

[kornicameister]

@njlynch wouldn't it be counterintuitive that Topic does add grant but ITopic does not?

[AdamVD]

I'm going to take a shot at implementing a fix for this, but I just did a bit of research/testing and wanted to offer a correction. The set of permissions required to publish is not [kms:Encrypt, kms:ReEncrypt, kms:GenerateDataKey] which are included in kms.grantEncrypt(), but rather [kms:Decrypt, kms:GenerateDataKey*]. You can find this in the SNS docs.

I was able to verify this by assuming the role created by the following CDK code:

const role = new Role(this, 'SnsPubRole', {
  assumedBy: new AccountPrincipal(Stack.of(this).account),
});

const key = new Key(this, 'CustomKey');

const topic = new Topic(this, 'MyTopic', {
  topicName: 'mytopic',
  masterKey: key,
});

topic.grantPublish(role);
key.grant(role, 'kms:Decrypt', 'kms:GenerateDataKey*');

and then publishing a message from the CLI: aws sns publish --message "hello" --topic-arn arn:aws:sns:<region>:<account_id>:mytopic.

[kornicameister]

Won't it make it hard to figure out what's going on? I mean I would expect some consistency and what @AdamVD proposes makes thing inconsistent. I mean if re-encryption is not there in policies, like docs suggest us, shouldn't we have another ticket that deals with that issue in isolation? Not to mention that it is a bit like a breaking change since some customer may rely on grantEncrypt providing ksm:ReEncrypt*

Cheers 👍

[AdamVD]

I wasn't proposing any specific changes, just noting what the minimal KMS permissions are to publish on an encrypted topic. You're right, changing the KMS grant functions would almost certainly be breaking and not the way to go.

SQS queues have basically the same encryption requirements as topics where sending a message requires kms:Decrypt and kms:GenerateDataKey (ref). Interestingly, the queue grantSendMessages() implementation uses KMS grantEncryptDecrypt() (ref) which is sufficient but not minimal.

[kornicameister]

Well, I am all for least privileged but not my call here. 8 suppose issues to deal with a little bit too much of permissions can be dealt with later, right?

[kornicameister]

@AdamVD did you manage to cook anything up here?

[ghdoergeloh]

Problem is still there, sorry for the duplication #21892

[yamatatsu]

This issue is as same as https://github.com/aws/aws-cdk/issues/18387. I'm working on it.

[CrypticCabub]

Am curious as to if there are any updates on this?

[harrisonhjones]

A bit late to the party but wanted to chime in. I ran into this issue on a project and was able to solve it with the following code:

const kmsKeyId = (topic.node.defaultChild as CfnTopic).kmsMasterKeyId
if (kmsKeyId) {
  lambdaFunction.addToRolePolicy(
    new PolicyStatement({
      actions: ["kms:Decrypt", "kms:GenerateDataKey*"],
      effect: Effect.ALLOW,
      resources: [kmsKeyId],
    });
  );
}

I hope that helps someone!