aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.65k stars 3.91k forks source link

aws-sns: `Topic.grantPublish(...)` creates identity policy assuming grantee is local to aws account. #29999

Closed wbertore closed 5 months ago

wbertore commented 6 months ago

Describe the bug

When creating an external iam user such as with User.fromArn(...) and adding it to a topic resource policy with grantPublish, the underlying constructs will create an identity policy assuming the iam user exists already in the stack.

This fails on cloudformation deployment.

Expected Behavior

It should create a resource policy on the SNS Topic and skip the identity policy if the grantee is from an external aws account.

Current Behavior

Topic.grantPublish, Grant.addToPrincipleOrResource, and User.addtoPrinciplePolicy will create a policy for the iam user, assuming it is part of the stack's aws account.

This fails on cloudformation deployment.

Reproduction Steps

Make a test app and stack:

import { App, Stack, StackProps } from 'aws-cdk-lib';
import { User } from 'aws-cdk-lib/aws-iam';
import { ITopic, Topic } from 'aws-cdk-lib/aws-sns';

const externalIamUser = 'arn:aws:iam::123456789012:user/OthersExternalIamUser';
export class TestSnsExternalIamUserStack extends Stack {
  public readonly myTopic: ITopic;

  constructor(scope: App, props: StackProps) {
    super(scope, 'TestSnsExternalIamUserStack', props);

    this.myTopic = new Topic(this, 'MyTopic');
    this.myTopic.grantPublish(User.fromUserArn(this, `OthersExternalIamUser`, externalIamUser));
  }
}

const app = new App();

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const testSnsExternalIamUserStack = new TestSnsExternalIamUserStack(app, {
  description: 'test stack for aws-cdk bug report',
  env: { account: '234567890123', region: 'us-west-2' },
});

app.synth();

synthesize the stack:

cdk synth

see cloudformation output:

Description: test stack for aws-cdk bug report
Resources:
  MyTopic86869434:
    Type: AWS::SNS::Topic
    Metadata:
      aws:cdk:path: TestSnsExternalIamUserStack/MyTopic/Resource
  MyTopicPolicy12A5EC17:
    Type: AWS::SNS::TopicPolicy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sns:Publish
            Effect: Allow
            Principal:
              AWS: arn:aws:iam::123456789012:user/OthersExternalIamUser
            Resource:
              Ref: MyTopic86869434
            Sid: "0"
        Version: "2012-10-17"
      Topics:
        - Ref: MyTopic86869434
    Metadata:
      aws:cdk:path: TestSnsExternalIamUserStack/MyTopic/Policy/Resource
  OthersExternalIamUserPolicyB3CCA1EB:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sns:Publish
            Effect: Allow
            Resource:
              Ref: MyTopic86869434
        Version: "2012-10-17"
      PolicyName: OthersExternalIamUserPolicyB3CCA1EB
      Users:
        - OthersExternalIamUser
    Metadata:
      aws:cdk:path: TestSnsExternalIamUserStack/OthersExternalIamUser/Policy/Resource
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Analytics: v2:deflate64:H4sIAAAAAAAA/0WJyw6CMBBFv4X9dKSsYO0PGHRvylCS4dEaBjSm6b/7KOjm3nPPLbDSmGfmIYraQY3cYDgvhgZ4q2sQJxgu/sYEx84l+ObJj0zPn0wzApsJw//bdITail9nsh+5cwTnW4u9HO66xCJHnfXCrObVLTxZrFO/AJt/vFuiAAAA
    Metadata:
      aws:cdk:path: TestSnsExternalIamUserStack/CDKMetadata/Default
Parameters:
  BootstrapVersion:
    Type: AWS::SSM::Parameter::Value<String>
    Default: /cdk-bootstrap/hnb659fds/version
    Description: Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]
Rules:
  CheckBootstrapVersion:
    Assertions:
      - Assert:
          Fn::Not:
            - Fn::Contains:
                - - "1"
                  - "2"
                  - "3"
                  - "4"
                  - "5"
                - Ref: BootstrapVersion
        AssertDescription: CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI.

Notice that CDK generates a Policy that references a user that doesn't exist in the cloudformation stack.

Possible Solution

Add intelligence to the grantPublish procedure or underlying calls in Grant or User to compare the stack aws account against the user aws account to skip the identity policy creation.

Additional Information/Context

I am using an internal version of cdk for my company and cannot upgrade to the latest due to company library dependencies. It's possible this is fixed in the latest version (but unlikely after reading the source code and revision history in aws-cdk).

CDK CLI Version

2.77.0

Framework Version

No response

Node.js Version

18

OS

MacOS Sonoma 14.4.1

Language

TypeScript

Language Version

5.0.4

Other information

No response

khushail commented 6 months ago

Hi @wbertore , Thanks for reaching out. It is highly suggested to use the latest CDK version. Neverthelss, Looks like with the latest cdk version 2.139 and with cdk 2.77, I am able to successfully synth and deploy the code with external user policy created .

export class GrantPublishStack extends cdk.Stack {
  public readonly mytopic : ITopic;
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    this.mytopic = new Topic(this, 'MyTopic', {
      displayName: 'MyTopic',
    });

    this.mytopic.grantPublish(User.fromUserArn(this, 'OtherExternaluser', 'arn:aws:iam::55**********:user/admin'));
  }
}

This is the synth template which has the policy for the external user mentioned in the code -


{
 "Resources": {
  "MyTopic86869434": {
   "Type": "AWS::SNS::Topic",
   "Properties": {
    "DisplayName": "MyTopic"
   },
   "Metadata": {
    "aws:cdk:path": "GrantPublishStack/MyTopic/Resource"
   }
  },
  "OtherExternaluserPolicyCD96E322": {
   "Type": "AWS::IAM::Policy",
   "Properties": {
    "PolicyDocument": {
     "Statement": [
      {
       "Action": "sns:Publish",
       "Effect": "Allow",
       "Resource": {
        "Ref": "MyTopic86869434"
       }
      }
     ],
     "Version": "2012-10-17"
    },
    "PolicyName": "OtherExternaluserPolicyCD96E322",
    "Users": [
     "admin"
    ]
   },
   "Metadata": {
    "aws:cdk:path": "GrantPublishStack/OtherExternaluser/Policy/Resource"
   }
  },
  "CDKMetadata": {
   "Type": "AWS::CDK::Metadata",
   "Properties": {
    "Analytics": "v2:deflate64:H4sIAAAAAAAA/03IPQ7CMAxA4bN0TwwpC8y9ACrdUXCC5P7YqE5BKMrdoXRhep9eDe5wgn3lX2oxDHakG+RL8jiY77pmZYXcyYPQNHf+oRjyE+SzjITv9W4qxbRRZZkxrvPfjXCgRMLFsIQIve6e7giuBlf1SmTnhRNNEdqtH+S0s/2VAAAA"
   },
   "Metadata": {
    "aws:cdk:path": "GrantPublishStack/CDKMetadata/Default"
   },
   "Condition": "CDKMetadataAvailable"
  }
 },

Snippet for the successful deployment with CDK 2.77 -

Screenshot 2024-04-30 at 1 35 39 PM

However I see some policy missing in the console, despite being successful. Diving deep to get to the root cause of what could be going wrong.

pahud commented 6 months ago

This is because iam.User.fromUserArn() does not return the principalAccount correctly.

PoC

export class DummyStack extends Stack {
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);

    const externalIamUser = 'arn:aws:iam::123456789012:user/OthersExternalIamUser';
    const externalIamRole = 'arn:aws:iam::123456789012:role/OthersExternalIamUser';
    const granteeUser = iam.User.fromUserArn(this, 'OthersExternalIamUser', externalIamUser)
    const granteeRole = iam.Role.fromRoleArn(this, 'OthersExternalIamRole', externalIamRole)
    new CfnOutput(this, 'principalAccountUser', { value: granteeUser.grantPrincipal.principalAccount! })
    new CfnOutput(this, 'principalAccountRole', { value: granteeRole.grantPrincipal.principalAccount! })
  }
}

Outputs: dummy-stack2.principalAccountRole = 123456789012 dummy-stack2.principalAccountUser =

see the different implementation between fromUserArn() and fromRoleArn().

We are getting the pricipal account with the Aws.ACCOUNT_ID which presumes always the same account and that is the root cause of this bug.

Making this a p1 bug.

pahud commented 6 months ago

By the way, it's generally not recommended using IAM user like that but IAM role is always recommended. While this is a bug we need to fix, is there any reason you have to use iam.User rather than iam.Role?

github-actions[bot] commented 5 months ago

⚠️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.

aws-cdk-automation commented 3 months ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.