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

aws-dynamodb: addToResourcePolicy has no effect #30793

Open
greg5123334 opened this issue Jul 9, 2024 · 3 comments · May be fixed by #31516
Open

aws-dynamodb: addToResourcePolicy has no effect #30793

greg5123334 opened this issue Jul 9, 2024 · 3 comments · May be fixed by #31516
Labels
@aws-cdk/aws-dynamodb Related to Amazon DynamoDB bug This issue is a bug. effort/small Small work item – less than a day of effort p3

Comments

@greg5123334
Copy link
Contributor

Describe the bug

DynamoDB's TableV2 addToResourcePolicy method is not taking effect.

Expected Behavior

statements should be added to existing policy document. and in the absence of an existing policy document, one should be created on first call of addToResourcePolicy as documented:

A resource policy will be automatically created upon the first call to addToResourcePolicy.

Current Behavior

addToResourcePolicy has no effect on changesets.

Reproduction Steps

1. Initial deploy WITHOUT policy nor statement

    // table with resource policy
    const table = new dynamodb.TableV2(this, "TableTestV2-1", {
      partitionKey: {
        name: "id",
        type: dynamodb.AttributeType.STRING,
      },
      removalPolicy: RemovalPolicy.DESTROY,
      deletionProtection: false,
    });

2. Include addToResourcePolicy WITHOUT policy

Diff

    const statement_one = new iam.PolicyStatement({
      actions: ["dynamodb:GetItem"],
      principals: [new iam.AccountRootPrincipal()],
      resources: ["*"],
    });

    // table with resource policy
    const table = new dynamodb.TableV2(this, "TableTestV2-1", {
      partitionKey: {
        name: "id",
        type: dynamodb.AttributeType.STRING,
      },
      removalPolicy: RemovalPolicy.DESTROY,
      deletionProtection: false,
    });
    table.addToResourcePolicy(statement_one);

cdk diff

Stack SandboxStack
Hold on while we create a read-only change set to get a diff with accurate replacement information (use --no-change-set to use a less accurate but faster template-only diff)
There were no differences

✨  Number of stacks with differences: 0

Deploy first addToResourcePolicy

aws dynamodb get-resource-policy --resource-arn arn:aws:dynamodb:eu-west-1:000000000000:table/SandboxStack-TableTestV215EEA02B7-FUG73UWOHO6C
An error occurred (PolicyNotFoundException) when calling the GetResourcePolicy operation: Resource-based policy not found for the provided ResourceArn: arn:aws:dynamodb:eu-west-1:000000000000:table/SandboxStack-TableTestV215EEA02B7-FUG73UWOHO6C

NO POLICY ADDED!!

3. Add policy via resourcePolicy prop

    const statement_one = new iam.PolicyStatement({
      actions: ["dynamodb:GetItem"],
      principals: [new iam.AccountRootPrincipal()],
      resources: ["*"],
    });

    // resource policy document
    const policy = new iam.PolicyDocument({
      statements: [statement_one],
    });

    // table with resource policy
    const table = new dynamodb.TableV2(this, "TableTestV2-1", {
      partitionKey: {
        name: "id",
        type: dynamodb.AttributeType.STRING,
      },
      removalPolicy: RemovalPolicy.DESTROY,
      deletionProtection: false,
      resourcePolicy: policy,
    });

deploy

aws dynamodb get-resource-policy --resource-arn arn:aws:dynamodb:eu-west-1:000000000000:table/SandboxStack-TableTestV215EEA02B7-38MHFK131KH9
{
    "Policy": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"arn:aws:iam::000000000000:root\"},\"Action\":\"dynamodb:GetItem\",\"Resource\":\"*\"}]}",
    "RevisionId": "1720511820026"
}

4. add second statement via addToResourcePolicy method

    const statement_one = new iam.PolicyStatement({
      actions: ["dynamodb:GetItem"],
      principals: [new iam.AccountRootPrincipal()],
      resources: ["*"],
    });

    // resource policy document
    const policy = new iam.PolicyDocument({
      statements: [statement_one],
    });

    // table with resource policy
    const table = new dynamodb.TableV2(this, "TableTestV2-1", {
      partitionKey: {
        name: "id",
        type: dynamodb.AttributeType.STRING,
      },
      removalPolicy: RemovalPolicy.DESTROY,
      deletionProtection: false,
      resourcePolicy: policy,
    });

    const statement_two = new iam.PolicyStatement({
      actions: ["dynamodb:GetItem", "dynamodb:PutItem"],
      principals: [new iam.AccountRootPrincipal()],
      resources: ["*"],
    });
    table.addToResourcePolicy(statement_two);

diff

cdk diff

Stack SandboxStack
Hold on while we create a read-only change set to get a diff with accurate replacement information (use --no-change-set to use a less accurate but faster template-only diff)
There were no differences

✨  Number of stacks with differences: 0

deploy

aws dynamodb get-resource-policy --resource-arn arn:aws:dynamodb:eu-west-1:000000000000:table/SandboxStack-TableTestV215EEA02B7-38MHFK131KH9
{
    "Policy": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"arn:aws:iam::000000000000:root\"},\"Action\":\"dynamodb:GetItem\",\"Resource\":\"*\"}]}",
    "RevisionId": "1720511820026"
}

second statement NOT included in diff nor in deployment

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.148.0 (build e5740c0)

Framework Version

No response

Node.js Version

v20.12.2

OS

Ubuntu 22.04.4 LTS

Language

TypeScript

Language Version

5.4.5

Other information

No response

@greg5123334 greg5123334 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 9, 2024
@github-actions github-actions bot added the @aws-cdk/aws-dynamodb Related to Amazon DynamoDB label Jul 9, 2024
@khushail khushail added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Jul 9, 2024
@khushail khushail self-assigned this Jul 9, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-reproduction This issue needs reproduction. labels Jul 9, 2024
@khushail
Copy link
Contributor

khushail commented Jul 10, 2024

Hey @greg5123334 , thanks for sharing the detailed repro code. I can confirm that the mentioned method addToResourcePolicy() does not add the required policy.

https://github.com/aws/aws-cdk/blob/7cd0f65a669abf5144a705fec0652f3b550a9340/packages/aws-cdk-lib/aws-dynamodb/lib/table-v2-base.ts#L474C1-L483C2

public addToResourcePolicy(statement: PolicyStatement): AddToResourcePolicyResult {

On checking this method, it looks like the method should return the object updated with policy statements but its returning assertions. Also the inherent object being created is PolicyDocument which adds statement. Here is the PR link that introduced this change. Marking it as P3 since it has a workaround.

@khushail khushail added the p2 label Jul 10, 2024
@khushail khushail removed their assignment Jul 10, 2024
@khushail khushail added p3 effort/small Small work item – less than a day of effort and removed p2 investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Jul 10, 2024
@jaimm
Copy link

jaimm commented Jul 18, 2024

I'm encountering issues with using grantRead against a dynamodb table (v1) construct where no resource policy is being added. Looking at the addToResourcePolicy for table.ts, it seems the logic is identical to the table-v2-base.ts, so I believe this affects both table constructs.

@everilae
Copy link

Looks like the issue is caused by passing props.resourcePolicy when building CfnTable in the constructor, but then populating this.resourcePolicy lazily in addToResourcePolicy and adding the statements to it. Because this.resourcePolicy is not actually bound to the CfnTable instance, it is effectively just a sink. For example KMS Key populates this.policy in the constructor already and passes to CfnKey, so the statements are visible.

Tried to look around if there's an open or to-be-merged PR for this, but could not find one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-dynamodb Related to Amazon DynamoDB bug This issue is a bug. effort/small Small work item – less than a day of effort p3
Projects
None yet
4 participants