fix(dynamodb): grant also gives access to indexes #1564

rix0rrr · 2019-01-17T09:52:45Z

If a table has an index, the grant() methods will give permissions to
the index as well.

Updates the security diff engine to support AWS::NoValue.

Pull Request Checklist

Testing
- Unit test added
- CLI change?: manually run integration tests and paste output as a PR comment
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

If a table has an index, the `grant()` methods will give permissions to the index as well. Updates the security diff engine to support `AWS::NoValue`. Fixes #1540.

eladb · 2019-01-17T09:58:35Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

@@ -429,7 +429,7 @@ export class Table extends Construct {
      return;
    }
    principal.addToPolicy(new iam.PolicyStatement()
-      .addResource(this.tableArn)
+      .addResources(this.tableArn, new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString())


Why not just always add the index?

Because this won't throw up IAM diffs to everyone who's already deployed an indexless table.

ha! not a bad reason. thanks.

also, wondering perhaps addResource can just ignore undefined, so you don't need to specify noValue. A bit more "cdk-native", no?

Agreed, although according to the typing addResource() takes a string and not a string | undefined. It would be annoying if a stringified Token would change its type halfway through processing...

(Nothing preventing you from doing that today, but I'm not sure our code would be robust against it).

Got it. No worries.

sam-goodwin · 2019-01-17T20:41:32Z

Is this the desired behavior? What if I want to give access to the table but not the index?

rix0rrr · 2019-01-18T09:20:41Z

That's a question in line with "what if I only want to give dynamodb:GetItem and not dynamodb:Query".

It's a valid question, but I would say those cases are rare and we're optimizing for the common case where this "just work". My response to this would be that if you really care, you can throw an IAM policy together yourself?

If a table has an index, the `grant()` methods will give permissions to the index as well. Updates the security diff engine to support `AWS::NoValue`. Fixes #1540.

cursive-ide · 2020-01-09T04:36:32Z

I'm seeing a problem similar to the original one from #1540, and this fix is not working for me using 1.20.0. There are a couple of difference in my env from the original issue:

Because I'm migrating existing infrastructure to CDK, I'm creating my Table referencing an existing table rather than creating the definition in CDK, like this:

const licenceOrder = dynamodb.Table.fromTableName(this, 'LicenceOrderTable', 'licence-order')
licenceOrder.grantReadData(buyPreauthFn)
licenceOrder.grantReadData(renewPreauthFn)
licenceOrder.grantReadWriteData(webhookFn)

My index is a global index, not a local one.

I'm still getting the same error: Error calling DynamoDB: AccessDeniedException: User: arn:aws:sts::REDACTED is not authorized to perform: dynamodb:Query on resource: arn:aws:dynamodb:us-east-1:REDACTED:table/licence-order/index/by-name. In this case, it's renewPreauthFn having the problem, which has been granted read privileges in the code above.

Is the expectation that this fix should apply to my case?

skinny85 · 2020-01-09T17:34:56Z

@cursive-ide the issue is that we do not (currently) support adding permissions on the index for imported Tables. If you look in the code, you'll see that the index resource is only added if the hasIndex check is true, but the imported Table always sets it to false.

What we would need to do is add extra attributes to the fromTableAttributes method, so you would be able to pass your indexes when importing it:

const licenceOrder = dynamodb.Table.fromTableAttributes(this, 'LicenceOrderTable', {
  tableName: 'licence-order',
  globalIndexes: ['my-global-index-name'],
  localIndexes: ['my-local-index-name'],
});

This would also allow us to implement methods like autoScaleGlobalSecondaryIndexReadCapacity for imported Tables as well, not only for new ones.

Does this solution make sense @cursive-ide ?

cursive-ide · 2020-01-09T22:04:04Z

@skinny85 yes, I think so.

Is there a good mechanism for importing existing resources to CDK for cases like this? My tables have production data so I would rather not delete & recreate them, but it seems unfortunate to have to repeat similar table schema information in a different format. Is it feasible to allow tables to be created using existing CDK syntax but to allow a useExistingTableFromArn config item or something similar?

skinny85 · 2020-01-09T22:26:02Z

Is it feasible to allow tables to be created using existing CDK syntax but to allow a useExistingTableFromArn config item or something similar?

Not really, no, and I'm not sure how that solves the repetition problem you mentioned.

cursive-ide · 2020-01-09T23:18:45Z

Ok, I suspected that might be hard. Never mind, I think your solution will work fine for my case.

cursive-ide · 2020-01-09T23:20:07Z

BTW is there a good workaround for this in the meantime?

skinny85 · 2020-01-10T00:46:16Z

Probably adding the permissions "manually" (basically, doing what the grant method does) to the role is your best bet. Something like:

role.addToPolicy(new iam.PolicyStatement({
  actions: ['dynamodb:*'], // probably want to scope it down to something more manageable
  resources: [`${this.tableArn}/index/*`],
}));

ptitjes · 2020-02-19T13:13:55Z

@skinny85, I just hit the same problem as @cursive-ide. I applied your workaround.

However, could we add an additional parameter to the Table.grant*Data(...) methods just like there is an optional parameter to the Bucket.grantReadWrite(identity: iam.IGrantable, objectsKeyPattern: any = '*') method ?

Something along the line:

  grantReadData(grantee: iam.IGrantable, grantIndices = true): iam.Grant;
  grantWriteData(grantee: iam.IGrantable, grantIndices = true): iam.Grant;
  grantReadWriteData(grantee: iam.IGrantable, grantIndices = true): iam.Grant;

skinny85 · 2020-02-21T00:30:04Z

However, could we add an additional parameter to the Table.grant*Data(...) methods just like there is an optional parameter to the Bucket.grantReadWrite(identity: iam.IGrantable, objectsKeyPattern: any = '*') method ?

Something along the line:
  grantReadData(grantee: iam.IGrantable, grantIndices = true): iam.Grant;
  grantWriteData(grantee: iam.IGrantable, grantIndices = true): iam.Grant;
  grantReadWriteData(grantee: iam.IGrantable, grantIndices = true): iam.Grant;

I don't think this is the correct solution. Instead, we should do what I mentioned in this comment. I've opened a feature request to do that: #6392

leantorres73 · 2020-05-05T15:40:23Z

I'm having the same issue when importing a dynamo table,

w- · 2020-05-23T08:11:32Z

This totally just bit me. Really needs to be clearer in docs grantRead will only give permissions on the table and not the index.

fix(dynamodb): grant also gives access to indexes

e8577c9

If a table has an index, the `grant()` methods will give permissions to the index as well. Updates the security diff engine to support `AWS::NoValue`. Fixes #1540.

rix0rrr requested a review from a team as a code owner January 17, 2019 09:52

eladb approved these changes Jan 17, 2019

View reviewed changes

Fix line length linter error

bf1c24d

eladb approved these changes Jan 17, 2019

View reviewed changes

rix0rrr merged commit 33c2a6d into master Jan 17, 2019

rix0rrr deleted the huijbers/dynamodb-index-perms branch January 17, 2019 10:43

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

skinny85 mentioned this pull request Feb 21, 2020

Add indexes to dynamodb.Table.fromTableAttributes #6392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dynamodb): grant also gives access to indexes #1564

fix(dynamodb): grant also gives access to indexes #1564

rix0rrr commented Jan 17, 2019

eladb Jan 17, 2019

rix0rrr Jan 17, 2019

eladb Jan 17, 2019

rix0rrr Jan 17, 2019

eladb Jan 17, 2019

sam-goodwin commented Jan 17, 2019

rix0rrr commented Jan 18, 2019

cursive-ide commented Jan 9, 2020 •

edited

Loading

skinny85 commented Jan 9, 2020 •

edited

Loading

cursive-ide commented Jan 9, 2020

skinny85 commented Jan 9, 2020

cursive-ide commented Jan 9, 2020

cursive-ide commented Jan 9, 2020

skinny85 commented Jan 10, 2020 •

edited

Loading

ptitjes commented Feb 19, 2020

skinny85 commented Feb 21, 2020

leantorres73 commented May 5, 2020

w- commented May 23, 2020

fix(dynamodb): grant also gives access to indexes #1564

fix(dynamodb): grant also gives access to indexes #1564

Conversation

rix0rrr commented Jan 17, 2019

Pull Request Checklist

eladb Jan 17, 2019

Choose a reason for hiding this comment

rix0rrr Jan 17, 2019

Choose a reason for hiding this comment

eladb Jan 17, 2019

Choose a reason for hiding this comment

rix0rrr Jan 17, 2019

Choose a reason for hiding this comment

eladb Jan 17, 2019

Choose a reason for hiding this comment

sam-goodwin commented Jan 17, 2019

rix0rrr commented Jan 18, 2019

cursive-ide commented Jan 9, 2020 • edited Loading

skinny85 commented Jan 9, 2020 • edited Loading

cursive-ide commented Jan 9, 2020

skinny85 commented Jan 9, 2020

cursive-ide commented Jan 9, 2020

cursive-ide commented Jan 9, 2020

skinny85 commented Jan 10, 2020 • edited Loading

ptitjes commented Feb 19, 2020

skinny85 commented Feb 21, 2020

leantorres73 commented May 5, 2020

w- commented May 23, 2020

cursive-ide commented Jan 9, 2020 •

edited

Loading

skinny85 commented Jan 9, 2020 •

edited

Loading

skinny85 commented Jan 10, 2020 •

edited

Loading