From be0ec5d397442702548d0bbecca22462ce7e0a2e Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 28 Jan 2019 13:03:06 +0100 Subject: [PATCH] feat(aws-iam): grants support non-identity principals Add support for non-identity Principals in grants (for example, principals that represent accounts or organization IDs). For resources that support them, the required IAM statements will be added to the resource policy. For resources that don't support them (because they don't have resource policies) an error will be thrown. Add a new `OrganizationPrincipal` principal which represents all identities in the given AWS Organization. Fixes #236. --- .../test/test.pipeline-actions.ts | 3 +- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 14 ++--- packages/@aws-cdk/aws-dynamodb/lib/table.ts | 37 ++++++------ .../@aws-cdk/aws-ecr/lib/repository-ref.ts | 31 +++++----- packages/@aws-cdk/aws-iam/lib/group.ts | 3 +- .../@aws-cdk/aws-iam/lib/identity-base.ts | 14 +---- packages/@aws-cdk/aws-iam/lib/index.ts | 1 + packages/@aws-cdk/aws-iam/lib/lazy-role.ts | 16 ++--- packages/@aws-cdk/aws-iam/lib/principals.ts | 29 +++++++++ packages/@aws-cdk/aws-iam/lib/role.ts | 8 ++- packages/@aws-cdk/aws-iam/lib/user.ts | 3 +- .../aws-iam/test/test.policy-document.ts | 1 + packages/@aws-cdk/aws-kinesis/lib/stream.ts | 6 +- packages/@aws-cdk/aws-kms/lib/key.ts | 60 +++++++++++++++++++ .../@aws-cdk/aws-lambda/lib/lambda-ref.ts | 25 +++++--- .../@aws-cdk/aws-lambda/lib/permission.ts | 4 +- packages/@aws-cdk/aws-logs/lib/log-group.ts | 12 ++-- packages/@aws-cdk/aws-s3/lib/bucket.ts | 27 +++------ packages/@aws-cdk/aws-s3/test/test.bucket.ts | 59 ++++++++++++++++++ packages/@aws-cdk/aws-sns/lib/topic-ref.ts | 15 +++-- packages/@aws-cdk/aws-sqs/lib/queue-ref.ts | 19 +++--- 21 files changed, 266 insertions(+), 121 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudformation/test/test.pipeline-actions.ts b/packages/@aws-cdk/aws-cloudformation/test/test.pipeline-actions.ts index 65820d877ff1b..841260c6358c4 100644 --- a/packages/@aws-cdk/aws-cloudformation/test/test.pipeline-actions.ts +++ b/packages/@aws-cdk/aws-cloudformation/test/test.pipeline-actions.ts @@ -344,9 +344,10 @@ class RoleDouble extends iam.Role { super(scope, id, props); } - public addToPolicy(statement: iam.PolicyStatement) { + public addToPolicy(statement: iam.PolicyStatement): boolean { super.addToPolicy(statement); this.statements.push(statement); + return true; } } diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index d3a5bc9b5d4e7..c2dacf3ac2ac2 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -85,14 +85,14 @@ export class Metric { /** * Grant permissions to the given identity to write metrics. * - * @param identity The IAM identity to give permissions to. + * @param principal The IAM identity to give permissions to. */ - public static grantPutMetricData(identity?: iam.IPrincipal) { - if (!identity) { return; } - - identity.addToPolicy(new iam.PolicyStatement() - .addAllResources() - .addAction("cloudwatch:PutMetricData")); + public static grantPutMetricData(principal?: iam.IPrincipal) { + iam.grant({ + principal, + actions: ['cloudwatch:PutMetricData'], + resourceArns: ['*'], + }); } public readonly dimensions?: DimensionHash; diff --git a/packages/@aws-cdk/aws-dynamodb/lib/table.ts b/packages/@aws-cdk/aws-dynamodb/lib/table.ts index 9fb9d7f887c5b..b540b1df7ec8b 100644 --- a/packages/@aws-cdk/aws-dynamodb/lib/table.ts +++ b/packages/@aws-cdk/aws-dynamodb/lib/table.ts @@ -185,12 +185,12 @@ export class Table extends Construct { * @param principal The principal (no-op if undefined) */ public static grantListStreams(principal?: iam.IPrincipal): void { - if (principal) { - principal.addToPolicy(new iam.PolicyStatement() - .addAction('dynamodb:ListStreams') - .addResource("*")); - } - } + iam.grant({ + principal, + actions: ['dynamodb:ListStreams'], + resourceArns: ['*'], + }); + } public readonly tableArn: string; public readonly tableName: string; @@ -443,12 +443,14 @@ export class Table extends Construct { * @param actions The set of actions to allow (i.e. "dynamodb:PutItem", "dynamodb:GetItem", ...) */ public grant(principal?: iam.IPrincipal, ...actions: string[]) { - if (!principal) { - return; - } - principal.addToPolicy(new iam.PolicyStatement() - .addResources(this.tableArn, new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString()) - .addActions(...actions)); + iam.grant({ + principal, + actions, + resourceArns: [ + this.tableArn, + new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString() + ] + }); } /** @@ -458,12 +460,11 @@ export class Table extends Construct { * @param actions The set of actions to allow (i.e. "dynamodb:DescribeStream", "dynamodb:GetRecords", ...) */ public grantStream(principal?: iam.IPrincipal, ...actions: string[]) { - if (!principal) { - return; - } - principal.addToPolicy(new iam.PolicyStatement() - .addResource(this.tableStreamArn) - .addActions(...actions)); + iam.grant({ + principal, + actions, + resourceArns: [this.tableStreamArn] + }); } /** diff --git a/packages/@aws-cdk/aws-ecr/lib/repository-ref.ts b/packages/@aws-cdk/aws-ecr/lib/repository-ref.ts index ad7e8870f7ac1..d65e58aa724af 100644 --- a/packages/@aws-cdk/aws-ecr/lib/repository-ref.ts +++ b/packages/@aws-cdk/aws-ecr/lib/repository-ref.ts @@ -212,26 +212,27 @@ export abstract class RepositoryBase extends cdk.Construct implements IRepositor /** * Grant the given principal identity permissions to perform the actions on this repository */ - public grant(identity?: iam.IPrincipal, ...actions: string[]) { - if (!identity) { - return; - } - identity.addToPolicy(new iam.PolicyStatement() - .addResource(this.repositoryArn) - .addActions(...actions)); + public grant(principal?: iam.IPrincipal, ...actions: string[]) { + iam.grant({ + principal, + actions, + resourceArns: [this.repositoryArn], + addToResourcePolicy: this.addToResourcePolicy.bind(this), + }); } /** * Grant the given identity permissions to use the images in this repository */ - public grantPull(identity?: iam.IPrincipal) { - this.grant(identity, "ecr:BatchCheckLayerAvailability", "ecr:GetDownloadUrlForLayer", "ecr:BatchGetImage"); - - if (identity) { - identity.addToPolicy(new iam.PolicyStatement() - .addActions("ecr:GetAuthorizationToken", "logs:CreateLogStream", "logs:PutLogEvents") - .addAllResources()); - } + public grantPull(principal?: iam.IPrincipal) { + this.grant(principal, "ecr:BatchCheckLayerAvailability", "ecr:GetDownloadUrlForLayer", "ecr:BatchGetImage"); + + iam.grant({ + principal, + actions: ["ecr:GetAuthorizationToken", "logs:CreateLogStream", "logs:PutLogEvents"], + resourceArns: ['*'], + skipResourcePolicy: true, + }); } /** diff --git a/packages/@aws-cdk/aws-iam/lib/group.ts b/packages/@aws-cdk/aws-iam/lib/group.ts index 421eae23db319..82ce3de305f1e 100644 --- a/packages/@aws-cdk/aws-iam/lib/group.ts +++ b/packages/@aws-cdk/aws-iam/lib/group.ts @@ -97,12 +97,13 @@ export class Group extends Construct implements IIdentity { /** * Adds an IAM statement to the default policy. */ - public addToPolicy(statement: PolicyStatement) { + public addToPolicy(statement: PolicyStatement): boolean { if (!this.defaultPolicy) { this.defaultPolicy = new Policy(this, 'DefaultPolicy'); this.defaultPolicy.attachToGroup(this); } this.defaultPolicy.addStatement(statement); + return true; } } diff --git a/packages/@aws-cdk/aws-iam/lib/identity-base.ts b/packages/@aws-cdk/aws-iam/lib/identity-base.ts index 83f899a0e83ae..a3e9c9b7eaace 100644 --- a/packages/@aws-cdk/aws-iam/lib/identity-base.ts +++ b/packages/@aws-cdk/aws-iam/lib/identity-base.ts @@ -1,7 +1,6 @@ -import cdk = require('@aws-cdk/cdk'); import { Policy } from "./policy"; import { PolicyStatement } from "./policy-document"; -import { IPrincipal, PrincipalPolicyFragment } from "./principals"; +import { IPrincipal } from "./principals"; /** * A construct that represents an IAM principal, such as a user, group or role. @@ -11,7 +10,7 @@ export interface IIdentity extends IPrincipal { * Adds an IAM statement to the default inline policy associated with this * principal. If a policy doesn't exist, it is created. */ - addToPolicy(statement: PolicyStatement): void; + addToPolicy(statement: PolicyStatement): boolean; /** * Attaches an inline policy to this principal. @@ -25,13 +24,4 @@ export interface IIdentity extends IPrincipal { * @param arn The ARN of the managed policy */ attachManagedPolicy(arn: string): void; -} - -export abstract class IdentityBase extends cdk.Construct implements IIdentity { - public readonly assumeRoleAction: string = 'sts:AssumeRole'; - - public abstract policyFragment: PrincipalPolicyFragment; - public abstract addToPolicy(statement: PolicyStatement): void; - public abstract attachInlinePolicy(policy: Policy): void; - public abstract attachManagedPolicy(arn: string): void; } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/lib/index.ts b/packages/@aws-cdk/aws-iam/lib/index.ts index ce29004f943f3..11526872d93e7 100644 --- a/packages/@aws-cdk/aws-iam/lib/index.ts +++ b/packages/@aws-cdk/aws-iam/lib/index.ts @@ -7,6 +7,7 @@ export * from './group'; export * from './lazy-role'; export * from './principals'; export * from './identity-base'; +export * from './grant'; // AWS::IAM CloudFormation Resources: export * from './iam.generated'; diff --git a/packages/@aws-cdk/aws-iam/lib/lazy-role.ts b/packages/@aws-cdk/aws-iam/lib/lazy-role.ts index fcf8307fadb39..1e0ff661c7fd9 100644 --- a/packages/@aws-cdk/aws-iam/lib/lazy-role.ts +++ b/packages/@aws-cdk/aws-iam/lib/lazy-role.ts @@ -1,6 +1,7 @@ import cdk = require('@aws-cdk/cdk'); import { Policy } from './policy'; -import { PolicyPrincipal, PolicyStatement } from './policy-document'; +import { PolicyStatement } from './policy-document'; +import { PrincipalPolicyFragment } from './principals'; import { IRole, Role, RoleImportProps, RoleProps } from './role'; /** @@ -13,6 +14,7 @@ import { IRole, Role, RoleImportProps, RoleProps } from './role'; * not be synthesized or deployed. */ export class LazyRole extends cdk.Construct implements IRole { + public readonly assumeRoleAction: string = 'sts:AssumeRole'; private role?: Role; private readonly statements = new Array(); private readonly policies = new Array(); @@ -31,11 +33,12 @@ export class LazyRole extends cdk.Construct implements IRole { * If there is no default policy attached to this role, it will be created. * @param permission The permission statement to add to the policy document */ - public addToPolicy(statement: PolicyStatement): void { + public addToPolicy(statement: PolicyStatement): boolean { if (this.role) { - this.role.addToPolicy(statement); + return this.role.addToPolicy(statement); } else { this.statements.push(statement); + return true; } } @@ -85,11 +88,8 @@ export class LazyRole extends cdk.Construct implements IRole { return this.instantiate().roleName; } - /** - * Returns a Principal object representing the ARN of this role. - */ - public get principal(): PolicyPrincipal { - return this.instantiate().principal; + public get policyFragment(): PrincipalPolicyFragment { + return this.instantiate().policyFragment; } private instantiate(): Role { diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts index 762a3041038cb..de943d9f0c8a0 100644 --- a/packages/@aws-cdk/aws-iam/lib/principals.ts +++ b/packages/@aws-cdk/aws-iam/lib/principals.ts @@ -1,4 +1,5 @@ import cdk = require('@aws-cdk/cdk'); +import { PolicyStatement } from './policy-document'; import { mergePrincipal } from './util'; /** @@ -16,6 +17,13 @@ export interface IPrincipal { * Return the policy fragment that identifies this principal in a Policy. */ readonly policyFragment: PrincipalPolicyFragment; + + /** + * Add to the policy of this principal. + * + * @returns true if the policy was added, false if the policy could not be added + */ + addToPolicy(statement: PolicyStatement): boolean; } /** @@ -31,6 +39,11 @@ export abstract class PrincipalBase implements IPrincipal { * Return the policy fragment that identifies this principal in a Policy. */ public abstract policyFragment: PrincipalPolicyFragment; + + public addToPolicy(_statement: PolicyStatement): boolean { + // None of these have a policy to add to + return false; + } } /** @@ -75,6 +88,22 @@ export class ServicePrincipal extends PrincipalBase { } } +/** + * A principal that represents an AWS Organization + */ +export class OrganizationPrincipal extends PrincipalBase { + constructor(public readonly organizationId: string) { + super(); + } + + public get policyFragment(): PrincipalPolicyFragment { + return new PrincipalPolicyFragment( + { AWS: ['*'] }, + { StringEquals: { 'aws:PrincipalOrgID': this.organizationId } } + ); + } +} + /** * A policy prinicipal for canonicalUserIds - useful for S3 bucket policies that use * Origin Access identities. diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index c5c7e6e050ea5..ab9a62bd24ffd 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -182,13 +182,14 @@ export class Role extends Construct implements IRole { * If there is no default policy attached to this role, it will be created. * @param permission The permission statement to add to the policy document */ - public addToPolicy(statement: PolicyStatement) { + public addToPolicy(statement: PolicyStatement): boolean { if (!this.defaultPolicy) { this.defaultPolicy = new Policy(this, 'DefaultPolicy'); this.attachInlinePolicy(this.defaultPolicy); this.dependencyElements.push(this.defaultPolicy); } this.defaultPolicy.addStatement(statement); + return true; } /** @@ -310,8 +311,9 @@ class ImportedRole extends Construct implements IRole { return this.props; } - public addToPolicy(_statement: PolicyStatement): void { - // FIXME: Add warning that we're ignoring this + public addToPolicy(_statement: PolicyStatement): boolean { + // Statement will be added to resource instead + return false; } public attachInlinePolicy(_policy: Policy): void { diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts index b0dc4374f2724..17a4e956ea445 100644 --- a/packages/@aws-cdk/aws-iam/lib/user.ts +++ b/packages/@aws-cdk/aws-iam/lib/user.ts @@ -130,13 +130,14 @@ export class User extends Construct implements IIdentity { /** * Adds an IAM statement to the default policy. */ - public addToPolicy(statement: PolicyStatement) { + public addToPolicy(statement: PolicyStatement): boolean { if (!this.defaultPolicy) { this.defaultPolicy = new Policy(this, 'DefaultPolicy'); this.defaultPolicy.attachToUser(this); } this.defaultPolicy.addStatement(statement); + return true; } private parseLoginProfile(props: UserProps): CfnUser.LoginProfileProperty | undefined { diff --git a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts index 5d3b15b162116..b2ccb35e24dba 100644 --- a/packages/@aws-cdk/aws-iam/test/test.policy-document.ts +++ b/packages/@aws-cdk/aws-iam/test/test.policy-document.ts @@ -281,6 +281,7 @@ export = { const arrayPrincipal: PrincipalBase = { assumeRoleAction: 'sts:AssumeRole', policyFragment: new PrincipalPolicyFragment({ AWS: ['foo', 'bar'] }), + addToPolicy() { return false; } }; const s = new PolicyStatement().addAccountRootPrincipal() .addPrincipal(arrayPrincipal); diff --git a/packages/@aws-cdk/aws-kinesis/lib/stream.ts b/packages/@aws-cdk/aws-kinesis/lib/stream.ts index 0b569ccf07227..8b57408893120 100644 --- a/packages/@aws-cdk/aws-kinesis/lib/stream.ts +++ b/packages/@aws-cdk/aws-kinesis/lib/stream.ts @@ -255,14 +255,14 @@ export abstract class StreamBase extends cdk.Construct implements IStream { return dest.logSubscriptionDestination(sourceLogGroup); } - private grant(identity: iam.IPrincipal, actions: { streamActions: string[], keyActions: string[] }) { - identity.addToPolicy(new iam.PolicyStatement() + private grant(principal: iam.IPrincipal, actions: { streamActions: string[], keyActions: string[] }) { + principal.addToPolicy(new iam.PolicyStatement() .addResource(this.streamArn) .addActions(...actions.streamActions)); // grant key permissions if there's an associated key. if (this.encryptionKey) { - identity.addToPolicy(new iam.PolicyStatement() + principal.addToPolicy(new iam.PolicyStatement() .addResource(this.encryptionKey.keyArn) .addActions(...actions.keyActions)); } diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index 7b259bfb8fc25..d162a165fe188 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -1,3 +1,4 @@ +import iam = require('@aws-cdk/aws-iam'); import { PolicyDocument, PolicyStatement } from '@aws-cdk/aws-iam'; import { Construct, DeletionPolicy, IConstruct, Output, TagManager, Tags } from '@aws-cdk/cdk'; import { EncryptionKeyAlias } from './alias'; @@ -28,6 +29,21 @@ export interface IEncryptionKey extends IConstruct { * @returns a key ref which can be used in a call to `EncryptionKey.import(ref)`. */ export(): EncryptionKeyImportProps; + + /** + * Grant the indicated permissions on this key to the given principal + */ + grant(principal: iam.IPrincipal | undefined, actions: string[]): void; + + /** + * Grant decryption permisisons using this key to the given principal + */ + grantDecrypt(principal: iam.IPrincipal | undefined): void; + + /** + * Grant encryption permisisons using this key to the given principal + */ + grantEncrypt(principal: iam.IPrincipal | undefined): void; } export interface EncryptionKeyImportProps { @@ -75,6 +91,50 @@ export abstract class EncryptionKeyBase extends Construct { } public abstract export(): EncryptionKeyImportProps; + + /** + * Grant the indicated permissions on this key to the given principal + * + * This modifies both the principal's policy as well as the resource policy, + * since the default CloudFormation setup for KMS keys is that the policy + * must not be empty and so default grants won't work. + */ + public grant(principal: iam.IPrincipal | undefined, actions: string[]): void { + iam.grant({ + principal, + actions, + resourceArns: [this.keyArn], + skipResourcePolicy: true, + }); + + if (principal) { + this.addToResourcePolicy(new iam.PolicyStatement() + .addPrincipal(principal) + .addActions(...actions) + .addAllResources()); + } + } + + /** + * Grant decryption permisisons using this key to the given principal + */ + public grantDecrypt(principal: iam.IPrincipal | undefined): void { + return this.grant(principal, [ + 'kms:Decrypt', + 'kms:DescribeKey', + ]); + } + + /** + * Grant encryption permisisons using this key to the given principal + */ + public grantEncrypt(principal: iam.IPrincipal | undefined): void { + return this.grant(principal, [ + 'kms:Encrypt', + 'kms:ReEncrypt*', + 'kms:GenerateDataKey*' + ]); + } } /** diff --git a/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts b/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts index 8c241d5428af6..e7c5fb11953ee 100644 --- a/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts +++ b/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts @@ -267,11 +267,23 @@ export abstract class FunctionBase extends cdk.Construct implements IFunction { /** * Grant the given identity permissions to invoke this Lambda */ - public grantInvoke(identity?: iam.IPrincipal) { - if (identity) { - identity.addToPolicy(new iam.PolicyStatement() - .addAction('lambda:InvokeFunction') - .addResource(this.functionArn)); + public grantInvoke(principal?: iam.IPrincipal) { + const added = iam.grant({ + principal, + actions: ['lambda:InvokeFunction'], + resourceArns: [this.functionArn], + // Can't use a resource policy because adding resource permissions + // looks different on a Lambda Function. + skipResourcePolicy: true + }); + + if (!added && principal) { + // Couldn't add permissions to the principal, so add them locally. + const identifier = 'Invoke' + JSON.stringify(principal!.policyFragment.principalJson); + this.addPermission(identifier, { + principal, + action: 'lambda:InvokeFunction', + }); } } @@ -401,11 +413,10 @@ export abstract class FunctionBase extends cdk.Construct implements IFunction { source.bind(this); } - private parsePermissionPrincipal(principal?: iam.PolicyPrincipal) { + private parsePermissionPrincipal(principal?: iam.IPrincipal) { if (!principal) { return undefined; } - // use duck-typing, not instance of if ('accountId' in principal) { diff --git a/packages/@aws-cdk/aws-lambda/lib/permission.ts b/packages/@aws-cdk/aws-lambda/lib/permission.ts index de4ab91c6794d..91fb834c9045d 100644 --- a/packages/@aws-cdk/aws-lambda/lib/permission.ts +++ b/packages/@aws-cdk/aws-lambda/lib/permission.ts @@ -1,4 +1,4 @@ -import { PolicyPrincipal } from '@aws-cdk/aws-iam'; +import iam = require('@aws-cdk/aws-iam'); /** * Represents a permission statement that can be added to a Lambda's resource policy @@ -34,7 +34,7 @@ export interface Permission { * * The principal can be either an AccountPrincipal or a ServicePrincipal. */ - principal: PolicyPrincipal; + principal: iam.IPrincipal; /** * The AWS account ID (without hyphens) of the source owner. For example, if diff --git a/packages/@aws-cdk/aws-logs/lib/log-group.ts b/packages/@aws-cdk/aws-logs/lib/log-group.ts index 4f548fd295bdc..42c5775264c29 100644 --- a/packages/@aws-cdk/aws-logs/lib/log-group.ts +++ b/packages/@aws-cdk/aws-logs/lib/log-group.ts @@ -179,13 +179,13 @@ export abstract class LogGroupBase extends cdk.Construct implements ILogGroup { * Give the indicated permissions on this log group and all streams */ public grant(principal?: iam.IPrincipal, ...actions: string[]) { - if (!principal) { return; } - - principal.addToPolicy(new iam.PolicyStatement() - .addActions(...actions) - // This ARN includes a ':*' at the end to include the log streams. + iam.grant({ + principal, + actions, + // A LogGroup ARN out of CloudFormation already includes a ':*' at the end to include the log streams under the group. // See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-loggroup.html#w2ab1c21c10c63c43c11 - .addResource(`${this.logGroupArn}`)); + resourceArns: [this.logGroupArn] + }); } } diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index b33445332bbab..3dd7d06e0f93f 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -503,32 +503,21 @@ export abstract class BucketBase extends cdk.Construct implements IBucket { return statement; } - private grant(identity: iam.IPrincipal | undefined, + private grant(principal: iam.IPrincipal | undefined, bucketActions: string[], keyActions: string[], resourceArn: string, ...otherResourceArns: string[]) { - - if (!identity) { - return; - } - const resources = [ resourceArn, ...otherResourceArns ]; - identity.addToPolicy(new iam.PolicyStatement() - .addResources(...resources) - .addActions(...bucketActions)); + iam.grant({ + principal, + actions: bucketActions, + resourceArns: resources, + addToResourcePolicy: this.addToResourcePolicy.bind(this), + }); - // grant key permissions if there's an associated key. if (this.encryptionKey) { - // KMS permissions need to be granted both directions - identity.addToPolicy(new iam.PolicyStatement() - .addResource(this.encryptionKey.keyArn) - .addActions(...keyActions)); - - this.encryptionKey.addToResourcePolicy(new iam.PolicyStatement() - .addAllResources() - .addPrincipal(identity.principal) - .addActions(...keyActions)); + this.encryptionKey.grant(principal, keyActions); } } } diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 19c10b0840951..b1cca0910d834 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -673,6 +673,65 @@ export = { test.done(); }, + 'grant permissions to non-identity principal'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + const bucket = new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.Kms }); + + // WHEN + bucket.grantRead(new iam.OrganizationPrincipal('o-1234')); + + // THEN + expect(stack).to(haveResource('AWS::S3::BucketPolicy', { + PolicyDocument: { + "Version": "2012-10-17", + "Statement": [ + { + "Action": [ "s3:GetObject*", "s3:GetBucket*", "s3:List*" ], + "Condition": { "StringEquals": { "aws:PrincipalOrgID": "o-1234" } }, + "Effect": "Allow", + "Principal": "*", + "Resource": [ + { "Fn::GetAtt": [ "MyBucketF68F3FF0", "Arn" ] }, + { "Fn::Join": [ "", [ { "Fn::GetAtt": [ "MyBucketF68F3FF0", "Arn" ] }, "/*" ] ] } + ] + } + ] + } + })); + + expect(stack).to(haveResource('AWS::KMS::Key', { + "KeyPolicy": { + "Statement": [ + { + "Action": [ "kms:Create*", "kms:Describe*", "kms:Enable*", "kms:List*", "kms:Put*", "kms:Update*", + "kms:Revoke*", "kms:Disable*", "kms:Get*", "kms:Delete*", "kms:ScheduleKeyDeletion", "kms:CancelKeyDeletion" ], + "Effect": "Allow", + "Principal": { + "AWS": { + "Fn::Join": [ "", [ + "arn:", { "Ref": "AWS::Partition" }, ":iam::", { "Ref": "AWS::AccountId" }, ":root" + ]] + } + }, + "Resource": "*" + }, + { + "Action": [ "kms:Decrypt", "kms:DescribeKey" ], + "Effect": "Allow", + "Resource": "*", + "Principal": "*", + "Condition": { "StringEquals": { "aws:PrincipalOrgID": "o-1234" } }, + } + ], + "Version": "2012-10-17" + }, + + })); + + test.done(); + }, + 'if an encryption key is included, encrypt/decrypt permissions are also added both ways'(test: Test) { const stack = new cdk.Stack(); const bucket = new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.Kms }); diff --git a/packages/@aws-cdk/aws-sns/lib/topic-ref.ts b/packages/@aws-cdk/aws-sns/lib/topic-ref.ts index 4a562651af442..d6a749b1f88e7 100644 --- a/packages/@aws-cdk/aws-sns/lib/topic-ref.ts +++ b/packages/@aws-cdk/aws-sns/lib/topic-ref.ts @@ -285,14 +285,13 @@ export abstract class TopicBase extends cdk.Construct implements ITopic { /** * Grant topic publishing permissions to the given identity */ - public grantPublish(identity?: iam.IPrincipal) { - if (!identity) { - return; - } - - identity.addToPolicy(new iam.PolicyStatement() - .addResource(this.topicArn) - .addActions('sns:Publish')); + public grantPublish(principal?: iam.IPrincipal) { + iam.grant({ + principal, + actions: ['sns:Publish'], + resourceArns: [this.topicArn], + addToResourcePolicy: this.addToResourcePolicy.bind(this), + }); } /** diff --git a/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts b/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts index 69391f1257850..c452e6cb0f810 100644 --- a/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts +++ b/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts @@ -260,17 +260,16 @@ export abstract class QueueBase extends cdk.Construct implements IQueue { * Grant the actions defined in queueActions to the identity Principal given * on this SQS queue resource. * - * @param identity Principal to grant right to - * @param queueActions The actions to grant + * @param principal Principal to grant right to + * @param actions The actions to grant */ - public grant(identity?: iam.IPrincipal, ...queueActions: string[]) { - if (!identity) { - return; - } - - identity.addToPolicy(new iam.PolicyStatement() - .addResource(this.queueArn) - .addActions(...queueActions)); + public grant(principal?: iam.IPrincipal, ...actions: string[]) { + iam.grant({ + principal, + actions, + resourceArns: [this.queueArn], + addToResourcePolicy: this.addToResourcePolicy.bind(this), + }); } }