Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/aws-cdk-lib/aws-dynamodb/lib/private/principal-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import type { IPrincipal } from '../../../aws-iam';

/**
* Returns true if the principal resolves to a Service principal in the policy document.
* Checks the policyFragment output to handle wrapped principals
* (e.g. PrincipalWithConditions, SessionTagsPrincipal).
*/
export function isServicePrincipal(principal: IPrincipal): boolean {
const principalJson = principal.policyFragment.principalJson;
return Object.keys(principalJson).length === 1 && 'Service' in principalJson;
}
13 changes: 12 additions & 1 deletion packages/aws-cdk-lib/aws-dynamodb/lib/table-grants.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import type { ITableRef } from './dynamodb.generated';
import * as perms from './perms';
import * as iam from '../../aws-iam';
import { ArnFormat, Lazy, Stack, ValidationError } from '../../core';
import { Annotations, ArnFormat, Lazy, Stack, ValidationError } from '../../core';
import { isServicePrincipal } from './private/principal-utils';

/**
* Construction properties for TableGrants
Expand Down Expand Up @@ -108,6 +109,16 @@ export class TableGrants {
* @param actions The set of actions to allow (i.e. "dynamodb:PutItem", "dynamodb:GetItem", ...)
*/
public actions(grantee: iam.IGrantable, ...actions: string[]): iam.Grant {
if (isServicePrincipal(grantee.grantPrincipal)) {
Annotations.of(this.table).addWarningV2(
'@aws-cdk/aws-dynamodb:servicePrincipalGrantDropped',
'DynamoDB grant* methods do not support ServicePrincipal grantees. ' +
'Use table.addToResourcePolicy() for an explicit service-specific table policy ' +
'with required service principal, actions, and conditions',
);
return iam.Grant.drop(grantee, 'DynamoDB grant* does not support ServicePrincipal grantees');
}

return this.policyResource ? iam.Grant.addToPrincipalOrResource({
grantee,
actions,
Expand Down
20 changes: 19 additions & 1 deletion packages/aws-cdk-lib/aws-dynamodb/lib/table-v2-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import { MathExpression, Metric } from '../../aws-cloudwatch';
import type { AddToResourcePolicyResult, GrantOnKeyResult, IGrantable, IResourceWithPolicy, PolicyDocument, PolicyStatement } from '../../aws-iam';
import { Grant } from '../../aws-iam';
import type { IKey } from '../../aws-kms';
import { Resource, ValidationError } from '../../core';
import { Annotations, Resource, ValidationError } from '../../core';
import { isServicePrincipal } from './private/principal-utils';
import type { TableReference } from '../../interfaces/generated/aws-dynamodb-interfaces.generated';

/**
Expand Down Expand Up @@ -101,6 +102,9 @@ export abstract class TableBaseV2 extends Resource implements ITableV2, IResourc
* @param actions the set of actions to allow (i.e., 'dynamodb:PutItem', 'dynamodb:GetItem', etc.)
*/
public grant(grantee: IGrantable, ...actions: string[]): Grant {
if (isServicePrincipal(grantee.grantPrincipal)) {
return this.dropServicePrincipalGrant(grantee);
}
const resourceArns = [this.tableArn];
this.hasIndex && resourceArns.push(`${this.tableArn}/index/*`);
return Grant.addToPrincipalOrResource({
Expand Down Expand Up @@ -490,6 +494,10 @@ export abstract class TableBaseV2 extends Resource implements ITableV2, IResourc
tablePrinicipalExclusiveActions?: string[];
streamActions?: string[];
}) {
if (isServicePrincipal(grantee.grantPrincipal)) {
return this.dropServicePrincipalGrant(grantee);
}

if (options.keyActions && this.encryptionKey) {
this.encryptionKey.grant(grantee, ...options.keyActions);
}
Expand Down Expand Up @@ -541,6 +549,16 @@ export abstract class TableBaseV2 extends Resource implements ITableV2, IResourc
});
}

private dropServicePrincipalGrant(grantee: IGrantable): Grant {
Annotations.of(this).addWarningV2(
'@aws-cdk/aws-dynamodb:servicePrincipalGrantDropped',
'DynamoDB grant* methods do not support ServicePrincipal grantees. ' +
'Use table.addToResourcePolicy() for an explicit service-specific table policy ' +
'with required service principal, actions, and conditions',
);
return Grant.drop(grantee, 'DynamoDB grant* does not support ServicePrincipal grantees');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda sounds like that SPs are never allowed here, but elsewhere you have stated at a small limited list of SPs is actually allowed. I don't think we should drop all SPs, but have a allow list.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there are few SPs documented for DynamoDB resource policies, but they are integration-specific and require their own actions/conditions (e.g. replication.dynamodb.amazonaws.com). The generic grant* methods add standard data actions (dynamodb:GetItem, dynamodb:PutItem, etc.), which are not the right action set for those service-principal resource policies.
So even if we had an allowlist of permitted SPs, the actions being granted by grant* methods could produce incorrect resource policy which might not fail deployment, but it could still be functionally incorrect.

Because of that, I think the right path for SPs is the explicit addToResourcePolicy API, where the caller can provide the correct integration-specific principal, actions, and conditions.

}

/**
* Adds a statement to the resource policy associated with this table.
* A resource policy will be automatically created upon the first call to `addToResourcePolicy`.
Expand Down
56 changes: 43 additions & 13 deletions packages/aws-cdk-lib/aws-dynamodb/test/dynamodb.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2464,6 +2464,45 @@ describe('grants', () => {
testGrant(['*'], (p, t) => t.grantFullAccess(p));
});

test('grant* with ServicePrincipal drops grant', () => {
// GIVEN
const stack = new Stack();
const table = new Table(stack, 'Table', {
partitionKey: { name: 'id', type: AttributeType.STRING },
});

// WHEN
const grant = table.grantReadWriteData(new iam.ServicePrincipal('bedrock.amazonaws.com'));

// THEN
expect(grant.success).toBe(false);
const template = Template.fromStack(stack);
template.hasResourceProperties('AWS::DynamoDB::Table', {
ResourcePolicy: Match.absent(),
});
});

test('grant* with wrapped ServicePrincipal (withConditions) drops grant', () => {
// GIVEN
const stack = new Stack();
const table = new Table(stack, 'Table', {
partitionKey: { name: 'id', type: AttributeType.STRING },
});

// WHEN
const principal = new iam.ServicePrincipal('bedrock.amazonaws.com').withConditions({
StringEquals: { 'aws:SourceAccount': '123456789012' },
});
const grant = table.grantReadData(principal);

// THEN
expect(grant.success).toBe(false);
const template = Template.fromStack(stack);
template.hasResourceProperties('AWS::DynamoDB::Table', {
ResourcePolicy: Match.absent(),
});
});

testDeprecated('"Table.grantListStreams" allows principal to list all streams', () => {
// GIVEN
const stack = new Stack();
Expand Down Expand Up @@ -5296,27 +5335,18 @@ test('Throws when more than four multi-attribute sort keys are specified', () =>
});

describe('L1 table grants', () => {
test('grant read permission to service principal (L1)', () => {
test('grant read permission to service principal (L1) drops grant', () => {
const stack = new Stack();
const table = new CfnTable(stack, 'Table', {
keySchema: [{ attributeName: 'id', keyType: 'HASH' }],
attributeDefinitions: [{ attributeName: 'id', attributeType: 'S' }],
});
const principal = new iam.ServicePrincipal('lambda.amazonaws.com');

TableGrants.fromTable(table).readData(principal);

const grant = TableGrants.fromTable(table).readData(principal);
expect(grant.success).toBe(false);
Template.fromStack(stack).hasResourceProperties('AWS::DynamoDB::Table', {
ResourcePolicy: {
PolicyDocument: {
Statement: Match.arrayWith([{
Action: ['dynamodb:BatchGetItem', 'dynamodb:Query', 'dynamodb:GetItem', 'dynamodb:Scan', 'dynamodb:ConditionCheckItem', 'dynamodb:DescribeTable'],
Effect: 'Allow',
Principal: { Service: 'lambda.amazonaws.com' },
Resource: '*',
}]),
},
},
ResourcePolicy: Match.absent(),
});
});
});
Expand Down
61 changes: 61 additions & 0 deletions packages/aws-cdk-lib/aws-dynamodb/test/table-v2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1310,6 +1310,67 @@ describe('grants', () => {
]),
});
});

test('grant* with ServicePrincipal drops grant', () => {
// GIVEN
const stack = new Stack();
const table = new TableV2(stack, 'Table', {
partitionKey: { name: 'id', type: AttributeType.STRING },
});

// WHEN
const grant = table.grantReadWriteData(new iam.ServicePrincipal('bedrock.amazonaws.com'));

// THEN
expect(grant.success).toBe(false);

Template.fromStack(stack).hasResourceProperties('AWS::DynamoDB::GlobalTable', {
Replicas: Match.arrayWith([
Match.objectLike({
ResourcePolicy: Match.absent(),
}),
]),
});
});

test('grant with ServicePrincipal drops grant', () => {
// GIVEN
const stack = new Stack();
const table = new TableV2(stack, 'Table', {
partitionKey: { name: 'id', type: AttributeType.STRING },
});

// WHEN
const grant = table.grant(new iam.ServicePrincipal('bedrock.amazonaws.com'), 'dynamodb:GetItem');

// THEN
expect(grant.success).toBe(false);

Template.fromStack(stack).hasResourceProperties('AWS::DynamoDB::GlobalTable', {
Replicas: Match.arrayWith([
Match.objectLike({
ResourcePolicy: Match.absent(),
}),
]),
});
});

test('grant* with wrapped ServicePrincipal (withConditions) drops grant', () => {
// GIVEN
const stack = new Stack();
const table = new TableV2(stack, 'Table', {
partitionKey: { name: 'id', type: AttributeType.STRING },
});

// WHEN
const principal = new iam.ServicePrincipal('bedrock.amazonaws.com').withConditions({
StringEquals: { 'aws:SourceAccount': '123456789012' },
});
const grant = table.grantReadData(principal);

// THEN
expect(grant.success).toBe(false);
});
});

describe('replica tables', () => {
Expand Down
Loading