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: Table grant inconsistencies #20105

Closed
vito-laurenza-zocdoc opened this issue Apr 27, 2022 · 8 comments
Closed

aws_dynamodb: Table grant inconsistencies #20105

vito-laurenza-zocdoc opened this issue Apr 27, 2022 · 8 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management 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 guidance Question that needs advice or information. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@vito-laurenza-zocdoc
Copy link

Describe the bug

When attempting to grant table permissions to an IPrincipal, we expect those permissions to be added but they are silently ignored.

Expected Behavior

Table permissions should be granted to any IGrantable.

Current Behavior

Table permissions are not granted to IPrincipals.

Reproduction Steps

Example code:

const table = new aws_dynamodb.Table(this, 'my-table', {
    encryption: aws_dynamodb.TableEncryption.CUSTOMER_MANAGED,
    partitionKey: {
        name: 'id',
        type: aws_dynamodb.AttributeType.STRING,
    },
    tableName: 'my-table-name',
});

const role = aws_iam.Role.fromRoleArn(this, 'Role1', `arn:aws:iam::${accountId}:role/Role1`);
const principal = new aws_iam.ArnPrincipal(`arn:aws:iam::${accountId}:role/Role2`);

table.grantReadWriteData(role);
table.grantReadWriteData(principal);

Example synth'd template snippet:

Resources:
  mytableKeyF511837D:
    Type: AWS::KMS::Key
    Properties:
      KeyPolicy:
        Statement:
          - Action: kms:*
            Effect: Allow
            Principal:
              AWS:
                Fn::Join:
                  - ""
                  - - "arn:"
                    - Ref: AWS::Partition
                    - :iam::000000000000:root
            Resource: "*"
          - Action:
              - kms:Decrypt
              - kms:DescribeKey
              - kms:Encrypt
              - kms:ReEncrypt*
              - kms:GenerateDataKey*
            Effect: Allow
            Principal:
              AWS: arn:aws:iam::000000000000:role/Role2
            Resource: "*"
        Version: "2012-10-17"
      Description: Customer-managed key auto-created for encrypting DynamoDB table at my-stack/my-table
      EnableKeyRotation: true
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
  mytable0324D45C:
    Type: AWS::DynamoDB::Table
    Properties:
      KeySchema:
        - AttributeName: id
          KeyType: HASH
      AttributeDefinitions:
        - AttributeName: id
          AttributeType: S
      ProvisionedThroughput:
        ReadCapacityUnits: 5
        WriteCapacityUnits: 5
      SSESpecification:
        KMSMasterKeyId:
          Fn::GetAtt:
            - mytableKeyF511837D
            - Arn
        SSEEnabled: true
        SSEType: KMS
      TableName: my-table-name
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
  Role1Policy1275B564:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action:
              - dynamodb:BatchGetItem
              - dynamodb:GetRecords
              - dynamodb:GetShardIterator
              - dynamodb:Query
              - dynamodb:GetItem
              - dynamodb:Scan
              - dynamodb:ConditionCheckItem
              - dynamodb:BatchWriteItem
              - dynamodb:PutItem
              - dynamodb:UpdateItem
              - dynamodb:DeleteItem
              - dynamodb:DescribeTable
            Effect: Allow
            Resource:
              - Fn::GetAtt:
                  - mytable0324D45C
                  - Arn
              - Ref: AWS::NoValue
          - Action:
              - kms:Decrypt
              - kms:DescribeKey
              - kms:Encrypt
              - kms:ReEncrypt*
              - kms:GenerateDataKey*
            Effect: Allow
            Resource:
              Fn::GetAtt:
                - mytableKeyF511837D
                - Arn
        Version: "2012-10-17"
      PolicyName: Role1Policy1275B564
      Roles:
        - Role1

You can see here that the ArnPrincipal is only granted KMS-related usage. It is ignored for the table permissions policy.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.21.1 (build a6ee543)

Framework Version

No response

Node.js Version

v14.19.1

OS

Darwin Kernel Version 20.6.0: Tue Feb 22 21:10:41 PST 2022; root:xnu-7195.141.26~1/RELEASE_X86_64

Language

Typescript

Language Version

TypeScript (4.6.3)

Other information

No response

@vito-laurenza-zocdoc vito-laurenza-zocdoc added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 27, 2022
@github-actions github-actions bot added the @aws-cdk/aws-dynamodb Related to Amazon DynamoDB label Apr 27, 2022
@ryparker ryparker added p1 effort/small Small work item – less than a day of effort labels Apr 27, 2022
@peterwoodworth peterwoodworth removed the needs-triage This issue or PR still needs to be triaged. label Apr 27, 2022
@skinny85
Copy link
Contributor

skinny85 commented May 3, 2022

Hi @vito-laurenza-zocdoc,

thanks for opening the issue. The reason the ArnPrincipal is ignored by the Table is that an ArnPrincipal does not have a Policy document where the permission to access the DynamoDB Table might be registered.

Note that the KMS Key has a resource policy, which can be made to trust that Principal, but DynamoDB Tables do not have a resource policy.

Did you perhaps want to use the Role.fromRoleArn() method instead?

Thanks,
Adam

@skinny85 skinny85 added guidance Question that needs advice or information. @aws-cdk/aws-iam Related to AWS Identity and Access Management response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed bug This issue is a bug. p1 @aws-cdk/aws-dynamodb Related to Amazon DynamoDB labels May 3, 2022
@skinny85 skinny85 removed their assignment May 3, 2022
@vito-laurenza-zocdoc
Copy link
Author

vito-laurenza-zocdoc commented May 5, 2022

Hi @vito-laurenza-zocdoc,

thanks for opening the issue. The reason the ArnPrincipal is ignored by the Table is that an ArnPrincipal does not have a Policy document where the permission to access the DynamoDB Table might be registered.

Note that the KMS Key has a resource policy, which can be made to trust that Principal, but DynamoDB Tables do not have a resource policy.

Did you perhaps want to use the Role.fromRoleArn() method instead?

Thanks, Adam

Hi Adam,

Thanks for looking at this. In my exact use case I was attempting to grant Table permissions to a CompositePrincipal. I just simplified things for this GH issue report to more cleanly illustrate the issue as I see it.

I have a list of roles that I want to grant permissions to. If I loop over them we end up with lots of duplication in the policy that seems unnecessary at best, and has the potential to cause the policy to bump against the hard PolicySize quota at worst.

All that said, it seems wrong that the types at play here imply I would be able to use an IPrincipal as a grantee, only to find out that the policy was not created silently.

Thanks again for looking at this,
Vito

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

skinny85 commented May 5, 2022

Unfortunately, things are a little complex, as you've seen, so failing is probably not a good idea 🙂.

Can you make sure you're on a version no earlier than2.18.0 (or 1.150.0 if you're using V1), when this change was released, enable the @aws-cdk/aws-iam:minimizePolicies feature flag in your cdk.context.json file, and see if that removes the duplication in the policies generated by looping through the principals?

Thanks,
Adam

@skinny85 skinny85 added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 5, 2022
@github-actions
Copy link

github-actions bot commented May 7, 2022

This issue has not received a response in a while. 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 closing-soon This issue will automatically close in 4 days unless further comments are made. 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 May 7, 2022
@vito-laurenza-zocdoc
Copy link
Author

@skinny85 -- sorry for the late reply. That feature flag doesn't work for my use case because the merge does not happen on the Roles parameter.

e.g. this code:

const table = new aws_dynamodb.Table(this, 'my-table', {
    encryption: aws_dynamodb.TableEncryption.CUSTOMER_MANAGED,
    partitionKey: {
        name: 'id',
        type: aws_dynamodb.AttributeType.STRING,
    },
    tableName: 'my-table-name',
});

const roleOne = aws_iam.Role.fromRoleArn(this, 'Role1', `arn:aws:iam::${accountId}:role/Role1`);
const roleTwo = aws_iam.Role.fromRoleArn(this, 'Role2', `arn:aws:iam::${accountId}:role/Role2`);

table.grantReadWriteData(roleOne);
table.grantReadWriteData(roleTwo);

results in this synth'd template snippet:

  Role1Policy1275B564:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action:
              - dynamodb:BatchGetItem
              - dynamodb:BatchWriteItem
              - dynamodb:ConditionCheckItem
              - dynamodb:DeleteItem
              - dynamodb:DescribeTable
              - dynamodb:GetItem
              - dynamodb:GetRecords
              - dynamodb:GetShardIterator
              - dynamodb:PutItem
              - dynamodb:Query
              - dynamodb:Scan
              - dynamodb:UpdateItem
            Effect: Allow
            Resource:
              - Fn::GetAtt:
                  - mytable0324D45C
                  - Arn
              - Ref: AWS::NoValue
          - Action:
              - kms:Decrypt
              - kms:DescribeKey
              - kms:Encrypt
              - kms:GenerateDataKey*
              - kms:ReEncrypt*
            Effect: Allow
            Resource:
              Fn::GetAtt:
                - mytableKeyF511837D
                - Arn
        Version: "2012-10-17"
      PolicyName: Role1Policy1275B564
      Roles:
        - Role1
  Role2PolicyFB7A062B:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action:
              - dynamodb:BatchGetItem
              - dynamodb:BatchWriteItem
              - dynamodb:ConditionCheckItem
              - dynamodb:DeleteItem
              - dynamodb:DescribeTable
              - dynamodb:GetItem
              - dynamodb:GetRecords
              - dynamodb:GetShardIterator
              - dynamodb:PutItem
              - dynamodb:Query
              - dynamodb:Scan
              - dynamodb:UpdateItem
            Effect: Allow
            Resource:
              - Fn::GetAtt:
                  - mytable0324D45C
                  - Arn
              - Ref: AWS::NoValue
          - Action:
              - kms:Decrypt
              - kms:DescribeKey
              - kms:Encrypt
              - kms:GenerateDataKey*
              - kms:ReEncrypt*
            Effect: Allow
            Resource:
              Fn::GetAtt:
                - mytableKeyF511837D
                - Arn
        Version: "2012-10-17"
      PolicyName: Role2PolicyFB7A062B
      Roles:
        - Role2

@skinny85
Copy link
Contributor

So, what's the problem here? Those are separate Policies, so which IAM quota are bumping against?

@vito-laurenza-zocdoc
Copy link
Author

vito-laurenza-zocdoc commented May 18, 2022

So, what's the problem here? Those are separate Policies, so which IAM quota are bumping against?

It's certainly not optimal to have essentially identical Policies which only vary by the entity that they are being attached to. The specific concern is that we could potentially bump against the maximum policy size hard limit and also size limits for the stack itself.

From the documentation:

You can add as many inline policies as you want to an IAM user, role, or group. But the total aggregate policy size (the
sum size of all inline policies) per entity cannot exceed the following quotas:
User policy size cannot exceed 2,048 characters.
Role policy size cannot exceed 10,240 characters.
Group policy size cannot exceed 5,120 characters.
Note
IAM does not count white space when calculating the size of a policy against these quotas.

@skinny85
Copy link
Contributor

I wouldn't worry about it - the template size & resource count limit (that's what I assume you mean by "Stack size limits") are pretty high, and it's unlikely these Policy resources would push the Stack above them, and merging the two Policies into one, referencing both Roles, has no effect on the total size of the Policies allowed for a single Role.

Note also that you can always import the Role as immutable (https://docs.aws.amazon.com/cdk/api/v1/docs/aws-iam-readme.html#using-existing-roles), and the Policies would not be generated at all. But of course, that means the Roles need the correct dynamodb: permissions to be granted outside CDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management 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 guidance Question that needs advice or information. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

4 participants