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.

Sign up for GitHub

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

fix(dynamodb): addtoresourcepolicy fix for table (v1) construct #31516

Closed
wants to merge 6 commits into from
Closed

fix(dynamodb): addtoresourcepolicy fix for table (v1) construct #31516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
49bfdb1
fix(dynamodb): addtoresourcepolicy fix for table (v1)
Sep 21, 2024
57dab53
Merge branch 'main' into addtorbp-fix-v1
LeeroyHannigan Sep 21, 2024
057ab1e
Fixed addToResourcePolicy, grants need work
Sep 24, 2024
4668ce7
Merge branch 'main' into addtorbp-fix-v1
LeeroyHannigan Oct 12, 2024
3fb88ac
Fix failing unit tests with statement length check
Oct 12, 2024
99284b8
Fixing addToResourcePolicy
Oct 12, 2024
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
2 changes: 1 addition & 1 deletion ...-testing/framework-integ/test/aws-dynamodb/test/integ.dynamodb.policy.js.snapshot/cdk.out
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 1 addition & 11 deletions ...sting/framework-integ/test/aws-dynamodb/test/integ.dynamodb.policy.js.snapshot/integ.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 5 additions & 6 deletions ...ng/framework-integ/test/aws-dynamodb/test/integ.dynamodb.policy.js.snapshot/manifest.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions ...est/aws-dynamodb/test/integ.dynamodb.policy.js.snapshot/resource-policy-stack.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 31 additions & 1 deletion ...t/aws-dynamodb/test/integ.dynamodb.policy.js.snapshot/resource-policy-stack.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"PolicyDocument": {
"Statement": [
{
"Action": "dynamodb:*",
"Action": "dynamodb:GetItem",
"Effect": "Allow",
"Principal": {
"AWS": {
Expand Down Expand Up @@ -71,6 +71,36 @@
"ProvisionedThroughput": {
"ReadCapacityUnits": 5,
"WriteCapacityUnits": 5
},
"ResourcePolicy": {
"PolicyDocument": {
"Statement": [
{
"Action": "dynamodb:PutItem",
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":iam::",
{
"Ref": "AWS::AccountId"
},
":root"
]
]
}
},
"Resource": "*"
}
],
"Version": "2012-10-17"
}
}
},
"UpdateReplacePolicy": "Delete",
Expand Down
2 changes: 1 addition & 1 deletion ...odb.policy.js.snapshot/resourcepolicyintegtestDefaultTestDeployAssert9778837B.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 31 additions & 1 deletion ...esting/framework-integ/test/aws-dynamodb/test/integ.dynamodb.policy.js.snapshot/tree.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 23 additions & 11 deletions packages/@aws-cdk-testing/framework-integ/test/aws-dynamodb/test/integ.dynamodb.policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,31 @@ export class TestStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
super(scope, id, props);

const doc = new iam.PolicyDocument({
statements: [
new iam.PolicyStatement({
actions: ['dynamodb:*'],
principals: [new iam.AccountRootPrincipal()],
resources: ['*'],
}),
],
});
// const doc = new iam.PolicyDocument({
// statements: [
// new iam.PolicyStatement({
// actions: ['dynamodb:*'],
// principals: [new iam.AccountRootPrincipal()],
// resources: ['*'],
// }),
// ],
// });

this.table = new dynamodb.Table(this, 'TableTest1', {
partitionKey: {
name: 'id',
type: dynamodb.AttributeType.STRING,
},
removalPolicy: RemovalPolicy.DESTROY,
resourcePolicy: doc,
// resourcePolicy: doc,
});

this.table.addToResourcePolicy(new iam.PolicyStatement({
actions: ['dynamodb:GetItem'],
principals: [new iam.AccountRootPrincipal()],
resources: ['*'],
}));

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the change in this PR is already covered in the unit test. Integ test does not seem necessary to me because we are not using new L1 props. Is there anything the integ test would catch while the unit test cannot?

this.tableTwo = new dynamodb.Table(this, 'TableTest2', {
partitionKey: {
name: 'PK',
Expand All @@ -39,7 +45,13 @@ export class TestStack extends Stack {
removalPolicy: RemovalPolicy.DESTROY,
});

this.tableTwo.grantReadData(new iam.AccountPrincipal('123456789012'));
this.tableTwo.addToResourcePolicy( new iam.PolicyStatement({
actions: ['dynamodb:PutItem'],
principals: [new iam.AccountRootPrincipal()],
resources: ['*'],
}));

this.tableTwo.grantReadData(new iam.AccountPrincipal('012345678910'));
}
}

Expand Down
57 changes: 31 additions & 26 deletions packages/aws-cdk-lib/aws-dynamodb/lib/table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ export interface TableAttributes {
}

export abstract class TableBase extends Resource implements ITable, iam.IResourceWithPolicy {

/**
* @attribute
*/
Expand Down Expand Up @@ -553,6 +554,22 @@ export abstract class TableBase extends Resource implements ITable, iam.IResourc

protected readonly regionalArns = new Array<string>();

/**
* Adds a statement to the resource policy associated with this table.
* A resource policy will be automatically created upon the first call to `addToResourcePolicy`.
*
* Note that this does not work with imported tables
*
* @param statement The policy statement to add
*/
public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult {

this.resourcePolicy = this.resourcePolicy ?? new iam.PolicyDocument({ statements: [] });
this.resourcePolicy?.addStatements(statement);

return { statementAdded: true, policyDependable: this.resourcePolicy };
}

/**
* Adds an IAM policy statement associated with this table to an IAM
* principal's policy.
Expand All @@ -564,7 +581,7 @@ export abstract class TableBase extends Resource implements ITable, iam.IResourc
* @param actions The set of actions to allow (i.e. "dynamodb:PutItem", "dynamodb:GetItem", ...)
*/
public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant {
return iam.Grant.addToPrincipalOrResource({
return iam.Grant.addToPrincipal({
Copy link

Choose a reason for hiding this comment

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

Why does this need to become addToPrincipal? We are for example already using the resource policy to grant cross-account access to a table directly. We build the policy in CDK from external account ArnPrincipals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because addToPrincipalOrResource never adds to the resource, as IAM is missing methods. So I'm putting it back to how it was before I changed it until I have time to implement the missing methods in IAM

Copy link

Choose a reason for hiding this comment

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

I don't fully understand what methods are missing. Looking at Grant, it seems to be handling this already, if given the resource in the call: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-iam/lib/grant.ts#L145-L157. It's just that Table.addToResourcePolicy is not functional, but instead adds statements to a policy instance not bound with the internal CfnTable.

Copy link
Contributor

Choose a reason for hiding this comment

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

@LeeroyHannigan I want to check if I follow your line of thoughts here.

I see the iam.Grant.addToPrincipalOrResource is implemented here. It basically does 2 things, call addToPrincipal and a bit further down on line 151, it calls options.resource.addToResourcePolicy, which means the Table.addToResourcePolicy. Since the method is reportedly a no-op (from #30793), you are saying your change here from addToPrincipalOrResource to addToPrincipal will not change any actual behaviour.

I agree with that. But if I understand @everilae correctly, even though Table.addToResourcePolicy was a no-op, there may be CDK users who are currently relying on what iam.Grant.addToPrincipalOrResource returns (it returns the resource policy statement whereas iam.Grant.addToPrincipal returns principal policy statement.). This allows CDK users to use this method to build the resource policy instead of creating their own.

Copy link

Choose a reason for hiding this comment

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

Hi,

We rely on the "...OrResource" part, or would like to, once all this is fixed. If the call is replaced with just "addToPrincipal", then attempts at granting to external accounts would not work.

grantee,
actions,
resourceArns: [
Expand All @@ -575,7 +592,7 @@ export abstract class TableBase extends Resource implements ITable, iam.IResourc
produce: () => this.hasIndex ? `${arn}/index/*` : Aws.NO_VALUE,
})),
],
resource: this,
scope: this,
});
}
/**
Expand Down Expand Up @@ -691,23 +708,6 @@ export abstract class TableBase extends Resource implements ITable, iam.IResourc
return this.combinedGrant(grantee, { keyActions, tableActions: ['dynamodb:*'] });
}

/**
* Adds a statement to the resource policy associated with this file system.
* A resource policy will be automatically created upon the first call to `addToResourcePolicy`.
*
* Note that this does not work with imported file systems.
*
* @param statement The policy statement to add
*/
public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult {
this.resourcePolicy = this.resourcePolicy ?? new iam.PolicyDocument({ statements: [] });
this.resourcePolicy.addStatements(statement);
return {
statementAdded: true,
policyDependable: this,
};
}

/**
* Return the given named metric for this Table
*
Expand Down Expand Up @@ -958,11 +958,11 @@ export abstract class TableBase extends Resource implements ITable, iam.IResourc
produce: () => this.hasIndex ? `${arn}/index/*` : Aws.NO_VALUE,
})),
];
const ret = iam.Grant.addToPrincipalOrResource({
const ret = iam.Grant.addToPrincipal({
Copy link

Choose a reason for hiding this comment

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

Same as above, this would break Table v1 for us.

grantee,
actions: opts.tableActions,
resourceArns: resources,
resource: this,
scope: this,
});
return ret;
}
Expand All @@ -971,11 +971,11 @@ export abstract class TableBase extends Resource implements ITable, iam.IResourc
throw new Error(`DynamoDB Streams must be enabled on the table ${this.node.path}`);
}
const resources = [this.tableStreamArn];
const ret = iam.Grant.addToPrincipalOrResource({
const ret = iam.Grant.addToPrincipal({
grantee,
actions: opts.streamActions,
resourceArns: resources,
resource: this,
scope: this,
});
return ret;
}
Expand Down Expand Up @@ -1150,6 +1150,8 @@ export class Table extends TableBase {
}
this.validateProvisioning(props);

this.resourcePolicy = props.resourcePolicy ?? new iam.PolicyDocument();

this.table = new CfnTable(this, 'Resource', {
tableName: this.physicalName,
keySchema: this.keySchema,
Expand Down Expand Up @@ -1177,9 +1179,11 @@ export class Table extends TableBase {
kinesisStreamSpecification: props.kinesisStream ? { streamArn: props.kinesisStream.streamArn } : undefined,
deletionProtectionEnabled: props.deletionProtection,
importSourceSpecification: this.renderImportSourceSpecification(props.importSource),
resourcePolicy: props.resourcePolicy
? { policyDocument: props.resourcePolicy }
: undefined,
resourcePolicy: Lazy.any({
produce: () => (this.resourcePolicy && this.resourcePolicy.statementCount > 0
? { policyDocument: this.resourcePolicy.toJSON() }
: undefined),
}),
});
this.table.applyRemovalPolicy(props.removalPolicy);

Expand Down Expand Up @@ -1732,6 +1736,7 @@ export class Table extends TableBase {
},
};
}

}

/**
Expand Down
Loading
Loading