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] Creating a new Table using fromTableName returns a Table object that doesn't include secondary indexes #11445

Closed
alonjupiter opened this issue Nov 12, 2020 · 12 comments
Labels
@aws-cdk/aws-dynamodb Related to Amazon DynamoDB closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@alonjupiter
Copy link

alonjupiter commented Nov 12, 2020

Hello!

I’ve noticed some discrepancies between the behavior of a dynamodb.Table created as new Table() and ones that are created by using Table.fromTableName()

Using new Table(...).grantReadWriteData(myLambdaFunction) delegates permissions to the primary table index, alongside all available GSIs.

However, using new Table.fromTableName(…).grantReadWriteData(myLambdaFunction) seems to delegate permissions to the primary table index only.

Reproduction Steps

const table = new Table.fromTableName(this, TABLE_NAME, TABLE_NAME).grantReadWriteData(
  myLambdaFunction
);

table.grantReadWrite(lambdaFn);

What did you expect to happen?

Lambda function should be granted permissions to ReadWrite to all of the table's indexes, both primary and LSIs/GSIs.

What actually happened?

Lambda function was granted permissions to ReadWrite to the primary index only.

Environment

  • CDK CLI Version : 1.69.0
  • Framework Version: 1.69.0
  • Node.js Version: 12.9.0
  • OS : Linux
  • Language (Version): TypeScript 3.6.4

Other

To workaround this issue one could explicitly define all partitions and then grant the necessary permissions to each, as such:

const primaryIndex = Table.fromTableName(this, "MyTable", "MyTable");

const secondaryIndex = Table.fromTableName(
  this,
  `MyTableGsi`,
  `MyTable/index/Gsi-index`
);

primaryIndex.grantReadWriteData(lambdaFn);
secondaryIndex.grantReadWriteData(lambdaFn);

This is 🐛 Bug Report

@alonjupiter alonjupiter added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 12, 2020
@github-actions github-actions bot added the @aws-cdk/aws-dynamodb Related to Amazon DynamoDB label Nov 12, 2020
@skinny85
Copy link
Contributor

skinny85 commented Nov 12, 2020

Hello @alonjupiter ,

thanks for opening the issue. Table.fromTableName() does not, in fact, create a new Table, but allows you to refer to an already existing one. Because of that, you have to specify all details of the Table yourself. If the Table has indexes, you need to use Table.fromTableAttributes method, and provide the indexes yourself.

Thanks,
Adam

@skinny85 skinny85 added guidance Question that needs advice or information. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 12, 2020
@alonjupiter
Copy link
Author

alonjupiter commented Nov 14, 2020

Hi @skinny85,

Thanks for the quick response. Now you mention it does make sense that CDK will not be able to import all of its attributes just be referring to it by name.

That said, I ran a quick test today to see if this can be worked around by explicitly defining the table's GSIs using TablefromTableAttributes as you suggested, however I could still see somewhat of an unexpected behavior.

This the code I used:

Table.fromTableAttributes(this, "MyTable", {
  tableName: "MyTable",
  globalIndexes: ["foo", "bar"],
}).grantReadData(lambdaFn);

When I tried to deploy my stack the IAM statement warned me of the following changes:

IAM Statement Changes
┌───┬───────────────────────────────────────────┬────────┬───────────────────────────────────────────┬───────────────────────────────────────────┬───────────┐
│   │ Resource                                  │ Effect │ Action                                    │ Principal                                 │ Condition │
├───┼───────────────────────────────────────────┼────────┼───────────────────────────────────────────┼───────────────────────────────────────────┼───────────┤
│ + │ arn:${AWS::Partition}:dynamodb:us-east-1: │ Allow  │ dynamodb:BatchGetItem                     │ AWS:${MyStack/MyLambdaRole.               │           │
│   │ 123456789012:table/MyTable                │        │ dynamodb:GetItem                          │ e}                                        │           │
│   │ arn:${AWS::Partition}:dynamodb:us-east-1: │        │ dynamodb:GetRecords                       │                                           │           │
│   │ 123456789012:table/MyTable/index/*        │        │ dynamodb:GetShardIterator                 │                                           │           │
│   │                                           │        │ dynamodb:Query                            │                                           │           │
│   │                                           │        │ dynamodb:Scan                             │                                           │           │
└───┴───────────────────────────────────────────┴────────┴───────────────────────────────────────────┴───────────────────────────────────────────┴───────────┘

It looks as if the IAM role generated for the Lambda function is too permissive as it will grant permissions to all GSIs, while I explicitly stated only 2 out of 4 GSIs.

Is this too the expected behaviour?

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

Hi @alonjupiter ,

yes, this is the expected behavior. I believe it's done that way, because you don't know the names of indexes before they are deployed in the case of a new Table.

In general, we don't assign per-index permissions - seems a little overkill I think...? Let me know if you disagree.

@skinny85 skinny85 added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 16, 2020
@alonjupiter
Copy link
Author

Hi @skinny85!

I'd say I disagree.

Generally speaking the security posture approach advocated by AWS is is to always go on the path of least privilege and only give access to the services/features/indices that are needed for that specific function or job.

Granting too permissive permissions to a resource goes against the above, and therefore this behavior should be amended to grant resources the least permissive permissions by default.

Happy to further discuss if needed!

@skinny85
Copy link
Contributor

OK. Let's change this to a feature request then 🙂.

@skinny85 skinny85 added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 and removed guidance Question that needs advice or information. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Nov 17, 2020
@alonjupiter
Copy link
Author

Great stuff, thank you @skinny85!

@skinny85
Copy link
Contributor

No problem @alonjupiter . BTW, Pull Requests are very welcome 😉. https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md

@ranjith-jagadeesh
Copy link
Contributor

ranjith-jagadeesh commented Jan 11, 2022

@skinny85 Is this feature request has been moved?, I'm planning to start work on it

@skinny85
Copy link
Contributor

If by "moved", you mean "has someone started working on it", then the answer is "No". It's all yours 🙂.

@ghost
Copy link

ghost commented Jan 12, 2022

@skinny85 Yes, Thanks for the update.

@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 12, 2023
@github-actions github-actions bot added closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jan 17, 2023
@ChelseaBradbury
Copy link

FYI looks like specifying grantIndexPermissions when importing the table should help with this:

/**
* If set to true, grant methods always grant permissions for all indexes.
* If false is provided, grant methods grant the permissions
* only when `globalIndexes` or `localIndexes` is specified.
*
* @default - false
*/
readonly grantIndexPermissions?: boolean;

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 closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1
Projects
None yet
4 participants