-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Generate DynamoDB resource policies #33
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
Conversation
…ourcePolicyResult like S3 * Return AddToResourcePolicyResult[] instead of a (parallel, disconnected) PolicyDocument * Expand assertions for typical use of DynamoDB generator
…ttach the resource policy.
…cy and providing on construction This is what actually works. It's not clear to me: 1. why the TableV2 resourcePolicy property is not populated from constructor within a unit test 2. why adding resource policy statements works in unit tests but not at runtime
…nd produce findings.
Better communicates that the function does not have side effects. Aligns to name for side-effect free KMS policy generator.
| "DynamoDB": { | ||
| "administer-resource": [ | ||
| "dynamodb:CreateBackup", | ||
| "dynamodb:CreateGlobalTable", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These permissions for global tables very likely need to stay. The admin user should be allowed to make replicas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These permissions changes were the result of:
- updating to the full current list of DDB permissions
- removing permissions not supported by DDB resource policy
Here are the administer-resource permissions from the full set of DDB perms that the DDB resource policy API rejected:
(administer-resource) One or more parameter values were invalid: Invalid policy document: The following action names are invalid: "dynamodb:UpdateGlobalTable", "dynamodb:UpdateGlobalTableSettings", "dynamodb:CreateTable", "dynamodb:UpdateGlobalTableVersion", "dynamodb:CreateGlobalTable", "dynamodb:RestoreTableFromBackup", "dynamodb:RestoreTableFromAwsBackup", "dynamodb:CreateTableReplica", "dynamodb:PurchaseReservedCapacityOfferings", "dynamodb:ImportTable"
(I have similar output for read-config, read-data, and write-data if you are interested)
I can see why you logically you want an admin of a table to create a replica. However, I think that probably has to be granted from the Identity side. That aligns more with how Create[Resource] is generally not supported in resource policy. And in this specific case, I would think the principal CFn uses to create the table resources would be the one that needs permissions.
wdyt?
| * @param props specifying desired access | ||
| * @return a PolicyDocument that can be attached to DynamoDB resources | ||
| */ | ||
| export function makeResourcePolicy(props: K9DynamoDBResourcePolicyProps): PolicyDocument { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for our needs, but I was wondering if you still wanted to wrap this in a grantAccessViaResourcePolicy method to be consistent with s3.
| } | ||
|
|
||
| if (!canPrincipalsManageResources(accessSpecsByCapability)) { | ||
| throw Error('At least one principal must be able to administer and read-config for DynamoDB resources' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good check to have.
| } | ||
|
|
||
|
|
||
| export function toPascalCase(input: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you hit a different naming restriction here that forced the casing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. DDB doesn't allow spaces nor hyphens.
|
|
||
| }); | ||
|
|
||
| describe('DynamoDBResourcePolicy', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: break the tests into files to match src.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, might do that.
src/k9policy.ts
Outdated
| * @return true when at least one principal that can administer and read configuration exists | ||
| */ | ||
| export function canPrincipalsManageResources(accessSpecsByCapability: Map<AccessCapability, IAccessSpec>) { | ||
| console.log(`canPrincipalsManageResources eval'ing ${accessSpecsByCapability}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console logging in a CDK library is a bit unusual. I'd advise debug here.
I forgot to mention that a while back. I swallow your console.log's from the KMS key policy generation to hide them. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, that's not supposed to be there. will remove.
| && (readConfigSpec?.allowPrincipalArns && readConfigSpec.allowPrincipalArns.length > 0)) { | ||
| const adminPrincipals = new Set<string>(adminSpec.allowPrincipalArns); | ||
| const readConfigPrincipals = new Set<string>(readConfigSpec.allowPrincipalArns); | ||
| const intersection = new Set( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set has a native intersection method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I don't see an intersection method available on TypeScript 5.4.5 & Node 20.14.0 (what I dev on).
Looks like an upgrade might help here. However, it's tough when CDK supports all the way back to Node 14.15.0 and I know we have users on 16.x
| // Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
|
||
| exports[`DynamoDBResourcePolicy Typical usage 1`] = ` | ||
| Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jest 28 or 29 finally improved snapshot syntax and removed the type literals everywhere. Much easier on the eyes.
|
For future reference... The DynamoDB permissions changes were the result of:
Here are from full set of DDB perms that the DDB resource policy API rejected: |
Add support for generating k9-style least privilege resource policies for DynamoDB resources.