From f40edeb4693dbb01f95645174697300c03dbdb98 Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Mon, 14 Oct 2019 15:36:59 -0700 Subject: [PATCH] feat(iam): immutable IAM roles This adds an IRole implementation that ignores all mutating operations. To accommodate this new behavior, add a new method to IIdentity: createAndAttachPolicy, which is meant to replace attachInlinePolicy, which can leave Policy resources unattached, which is illegal in CloudFormation. Fixes #2985 Fixes #4465 --- .../@aws-cdk/aws-codebuild/lib/project.ts | 17 ++-- .../test/integ.project-vpc.expected.json | 6 +- .../aws-codebuild/test/test.project.ts | 24 ++++- packages/@aws-cdk/aws-iam/lib/group.ts | 8 +- .../@aws-cdk/aws-iam/lib/identity-base.ts | 24 ++++- .../@aws-cdk/aws-iam/lib/immutable-role.ts | 52 +++++++++++ packages/@aws-cdk/aws-iam/lib/index.ts | 1 + packages/@aws-cdk/aws-iam/lib/lazy-role.ts | 8 +- packages/@aws-cdk/aws-iam/lib/role.ts | 20 +++- packages/@aws-cdk/aws-iam/lib/user.ts | 12 ++- .../aws-iam/test/immutable-role.test.ts | 93 +++++++++++++++++++ 11 files changed, 245 insertions(+), 20 deletions(-) create mode 100644 packages/@aws-cdk/aws-iam/lib/immutable-role.ts create mode 100644 packages/@aws-cdk/aws-iam/test/immutable-role.test.ts diff --git a/packages/@aws-cdk/aws-codebuild/lib/project.ts b/packages/@aws-cdk/aws-codebuild/lib/project.ts index d42c0bd6055df..b599493561924 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/project.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/project.ts @@ -945,8 +945,7 @@ export class Project extends ProjectBase { }, })); - const policy = new iam.Policy(this, 'PolicyDocument', { - policyName: 'CodeBuildEC2Policy', + const policy = this.role.addPolicy('CodeBuildVpcPolicy', { statements: [ new iam.PolicyStatement({ resources: ['*'], @@ -962,13 +961,13 @@ export class Project extends ProjectBase { }), ], }); - this.role.attachInlinePolicy(policy); - - // add an explicit dependency between the EC2 Policy and this Project - - // otherwise, creating the Project fails, - // as it requires these permissions to be already attached to the Project's Role - const cfnPolicy = policy.node.findChild('Resource') as CfnResource; - project.addDependsOn(cfnPolicy); + if (policy) { + // add an explicit dependency between the VPC Policy and this Project - + // otherwise, creating the Project fails, + // as it requires these permissions to be already attached to the Project's Role + const cfnPolicy = policy.node.defaultChild as CfnResource; + project.addDependsOn(cfnPolicy); + } } private validateCodePipelineSettings(artifacts: IArtifacts) { diff --git a/packages/@aws-cdk/aws-codebuild/test/integ.project-vpc.expected.json b/packages/@aws-cdk/aws-codebuild/test/integ.project-vpc.expected.json index 43b22ac3a1d55..51ce356745600 100644 --- a/packages/@aws-cdk/aws-codebuild/test/integ.project-vpc.expected.json +++ b/packages/@aws-cdk/aws-codebuild/test/integ.project-vpc.expected.json @@ -343,7 +343,7 @@ ] } }, - "MyProjectPolicyDocument646EE0F2": { + "MyProjectRoleCodeBuildVpcPolicyE806ED54": { "Type": "AWS::IAM::Policy", "Properties": { "PolicyDocument": { @@ -364,7 +364,7 @@ ], "Version": "2012-10-17" }, - "PolicyName": "CodeBuildEC2Policy", + "PolicyName": "MyProjectRoleCodeBuildVpcPolicyE806ED54", "Roles": [ { "Ref": "MyProjectRole9BBE5233" @@ -414,7 +414,7 @@ } }, "DependsOn": [ - "MyProjectPolicyDocument646EE0F2" + "MyProjectRoleCodeBuildVpcPolicyE806ED54" ] } } diff --git a/packages/@aws-cdk/aws-codebuild/test/test.project.ts b/packages/@aws-cdk/aws-codebuild/test/test.project.ts index d7a0907a37873..8aea9e8960bb4 100644 --- a/packages/@aws-cdk/aws-codebuild/test/test.project.ts +++ b/packages/@aws-cdk/aws-codebuild/test/test.project.ts @@ -1,4 +1,4 @@ -import { expect, haveResource, haveResourceLike, not } from '@aws-cdk/assert'; +import { countResources, expect, haveResource, haveResourceLike, not } from '@aws-cdk/assert'; import ec2 = require('@aws-cdk/aws-ec2'); import iam = require('@aws-cdk/aws-iam'); import s3 = require('@aws-cdk/aws-s3'); @@ -274,4 +274,26 @@ export = { test.done(); }, + + 'can use an imported Role with mutable = false for a Project within a VPC'(test: Test) { + const stack = new cdk.Stack(); + + const importedRole = iam.Role.fromRoleArn(stack, 'Role', + 'arn:aws:iam::1234567890:role/service-role/codebuild-bruiser-service-role', { + mutable: false, + }); + const vpc = new ec2.Vpc(stack, 'Vpc'); + + new codebuild.Project(stack, 'Project', { + source: codebuild.Source.gitHubEnterprise({ + httpsCloneUrl: 'https://mygithub-enterprise.com/myuser/myrepo', + }), + role: importedRole, + vpc, + }); + + expect(stack).to(countResources('AWS::IAM::Policy', 0)); + + test.done(); + }, }; diff --git a/packages/@aws-cdk/aws-iam/lib/group.ts b/packages/@aws-cdk/aws-iam/lib/group.ts index 7a755b85cf26f..4f6bb3ddcb0a9 100644 --- a/packages/@aws-cdk/aws-iam/lib/group.ts +++ b/packages/@aws-cdk/aws-iam/lib/group.ts @@ -2,7 +2,7 @@ import { Construct, Lazy, Resource, Stack } from '@aws-cdk/core'; import { CfnGroup } from './iam.generated'; import { IIdentity } from './identity-base'; import { IManagedPolicy } from './managed-policy'; -import { Policy } from './policy'; +import { Policy, PolicyProps } from './policy'; import { PolicyStatement } from './policy-statement'; import { ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; import { IUser } from './user'; @@ -73,6 +73,12 @@ abstract class GroupBase extends Resource implements IGroup { return new ArnPrincipal(this.groupArn).policyFragment; } + public addPolicy(id: string, props?: PolicyProps): Policy | undefined { + const policy = new Policy(this, id, props); + this.attachInlinePolicy(policy); + return policy; + } + /** * Attaches a policy to this group. * @param policy The policy to attach. diff --git a/packages/@aws-cdk/aws-iam/lib/identity-base.ts b/packages/@aws-cdk/aws-iam/lib/identity-base.ts index f4cc2e08c5d14..4d9bafad85f52 100644 --- a/packages/@aws-cdk/aws-iam/lib/identity-base.ts +++ b/packages/@aws-cdk/aws-iam/lib/identity-base.ts @@ -1,6 +1,6 @@ import { IResource } from '@aws-cdk/core'; import { IManagedPolicy } from './managed-policy'; -import { Policy } from "./policy"; +import { Policy, PolicyProps } from "./policy"; import { IPrincipal } from "./principals"; /** @@ -8,9 +8,27 @@ import { IPrincipal } from "./principals"; */ export interface IIdentity extends IPrincipal, IResource { /** - * Attaches an inline policy to this principal. - * This is the same as calling `policy.addToXxx(principal)`. + * Creates and attaches a Policy to this principal, if it is mutable. + * If this principal is immutable, does nothing, and returns undefined. + * + * @param id the logical identifier to use for the new policy resource. + * It will be created in the scope of this principal (if it's mutable) + * @param props the construction properties of the new policy + * @returns the newly created Policy resource if this principal is mutable, + * or undefined if this principal is immutable + */ + addPolicy(id: string, props?: PolicyProps): Policy | undefined; + + /** + * Attaches an inline policy to this principal, if this principal is mutable. + * For a mutable principal, it is the same as calling `policy.addToXxx(principal)`. + * For an immutable principal, this method does nothing. + * * @param policy The policy resource to attach to this principal [disable-awslint:ref-via-interface] + * + * @deprecated for immutable principals, this will leave the policy passed as the first parameter + * possibly not attached to any principal, which is invalid in CloudFormation. + * Use the {@link addPolicy} method instead */ attachInlinePolicy(policy: Policy): void; diff --git a/packages/@aws-cdk/aws-iam/lib/immutable-role.ts b/packages/@aws-cdk/aws-iam/lib/immutable-role.ts new file mode 100644 index 0000000000000..f056ce266c0d2 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/immutable-role.ts @@ -0,0 +1,52 @@ +import { Grant } from './grant'; +import { IManagedPolicy } from './managed-policy'; +import { Policy, PolicyProps } from './policy'; +import { PolicyStatement } from './policy-statement'; +import { IPrincipal } from './principals'; +import { IRole } from './role'; + +/** + * An immutable wrapper around an IRole that ignores all mutating operations, + * like attaching policies or adding policy statements. + * Useful in cases where you want to turn off CDK's automatic permissions management, + * and instead have full control over all permissions. + * Note: if you want to ignore all mutations for an externally defined role + * which was imported into the CDK with {@link Role.fromRoleArn}, you don't have to use this class - + * simply pass the property mutable = false when calling {@link Role.fromRoleArn}. + */ +export class ImmutableRole implements IRole { + public readonly assumeRoleAction = this.role.assumeRoleAction; + public readonly policyFragment = this.role.policyFragment; + public readonly grantPrincipal = this.role.grantPrincipal; + public readonly roleArn = this.role.roleArn; + public readonly roleName = this.role.roleName; + public readonly node = this.role.node; + public readonly stack = this.role.stack; + + constructor(private readonly role: IRole) { + } + + public addPolicy(_id: string, _props?: PolicyProps): Policy | undefined { + return undefined; + } + + public attachInlinePolicy(_policy: Policy): void { + // do nothing + } + + public addManagedPolicy(_policy: IManagedPolicy): void { + // do nothing + } + + public addToPolicy(_statement: PolicyStatement): boolean { + return false; + } + + public grant(grantee: IPrincipal, ...actions: string[]): Grant { + return this.role.grant(grantee, ...actions); + } + + public grantPassRole(grantee: IPrincipal): Grant { + return this.role.grantPassRole(grantee); + } +} diff --git a/packages/@aws-cdk/aws-iam/lib/index.ts b/packages/@aws-cdk/aws-iam/lib/index.ts index 0d0804ed8faca..377ba2685660f 100644 --- a/packages/@aws-cdk/aws-iam/lib/index.ts +++ b/packages/@aws-cdk/aws-iam/lib/index.ts @@ -2,6 +2,7 @@ export * from './policy-document'; export * from './policy-statement'; export * from './managed-policy'; export * from './role'; +export * from './immutable-role'; export * from './policy'; export * from './user'; export * from './group'; diff --git a/packages/@aws-cdk/aws-iam/lib/lazy-role.ts b/packages/@aws-cdk/aws-iam/lib/lazy-role.ts index e3385d511cbfd..2cfd80cac6178 100644 --- a/packages/@aws-cdk/aws-iam/lib/lazy-role.ts +++ b/packages/@aws-cdk/aws-iam/lib/lazy-role.ts @@ -1,7 +1,7 @@ import cdk = require('@aws-cdk/core'); import { Grant } from './grant'; import { IManagedPolicy } from './managed-policy'; -import { Policy } from './policy'; +import { Policy, PolicyProps } from './policy'; import { PolicyStatement } from './policy-statement'; import { IPrincipal, PrincipalPolicyFragment } from './principals'; import { IRole, Role, RoleProps } from './role'; @@ -49,6 +49,12 @@ export class LazyRole extends cdk.Resource implements IRole { } } + public addPolicy(id: string, props?: PolicyProps): Policy | undefined { + const policy = new Policy(this, id, props); + this.attachInlinePolicy(policy); + return policy; + } + /** * Attaches a policy to this role. * @param policy The policy to attach diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index 7fb0ceb982765..c47c701eb8773 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -3,7 +3,7 @@ import { Grant } from './grant'; import { CfnRole } from './iam.generated'; import { IIdentity } from './identity-base'; import { IManagedPolicy } from './managed-policy'; -import { Policy } from './policy'; +import { Policy, PolicyProps } from './policy'; import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; import { ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; @@ -168,6 +168,8 @@ export class Role extends Resource implements IRole { public abstract attachInlinePolicy(policy: Policy): void; + public abstract addPolicy(id: string, props?: PolicyProps): Policy | undefined; + public addManagedPolicy(_policy: IManagedPolicy): void { // FIXME: Add warning that we're ignoring this } @@ -207,6 +209,12 @@ export class Role extends Resource implements IRole { return true; } + public addPolicy(identifier: string, props?: PolicyProps): Policy | undefined { + const policy = new Policy(this, identifier, props); + this.attachInlinePolicy(policy); + return policy; + } + public attachInlinePolicy(policy: Policy): void { const policyAccount = Stack.of(policy).account; @@ -222,6 +230,10 @@ export class Role extends Resource implements IRole { return false; } + public addPolicy(_id: string, _props?: PolicyProps): Policy | undefined { + return undefined; + } + public attachInlinePolicy(_policy: Policy): void { // do nothing } @@ -353,6 +365,12 @@ export class Role extends Resource implements IRole { this.managedPolicies.push(policy); } + public addPolicy(id: string, props?: PolicyProps): Policy | undefined { + const policy = new Policy(this, id, props); + this.attachInlinePolicy(policy); + return policy; + } + /** * Attaches a policy to this role. * @param policy The policy to attach diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts index 0d5bcfa7e481a..1b647043c0ce5 100644 --- a/packages/@aws-cdk/aws-iam/lib/user.ts +++ b/packages/@aws-cdk/aws-iam/lib/user.ts @@ -3,7 +3,7 @@ import { IGroup } from './group'; import { CfnUser } from './iam.generated'; import { IIdentity } from './identity-base'; import { IManagedPolicy } from './managed-policy'; -import { Policy } from './policy'; +import { Policy, PolicyProps } from './policy'; import { PolicyStatement } from './policy-statement'; import { ArnPrincipal, PrincipalPolicyFragment } from './principals'; import { IPrincipal } from './principals'; @@ -147,6 +147,10 @@ export class User extends Resource implements IIdentity, IUser { throw new Error('Cannot add imported User to Group'); } + public addPolicy(_id: string, _props?: PolicyProps): Policy | undefined { + return undefined; + } + public attachInlinePolicy(_policy: Policy): void { throw new Error('Cannot add inline policy to imported User'); } @@ -234,6 +238,12 @@ export class User extends Resource implements IIdentity, IUser { this.managedPolicies.push(policy); } + public addPolicy(id: string, props?: PolicyProps): Policy | undefined { + const policy = new Policy(this, id, props); + this.attachInlinePolicy(policy); + return policy; + } + /** * Attaches a policy to this user. */ diff --git a/packages/@aws-cdk/aws-iam/test/immutable-role.test.ts b/packages/@aws-cdk/aws-iam/test/immutable-role.test.ts new file mode 100644 index 0000000000000..df169dff35275 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/test/immutable-role.test.ts @@ -0,0 +1,93 @@ +import '@aws-cdk/assert/jest'; +import { Stack } from '@aws-cdk/core'; +import iam = require('../lib'); + +// tslint:disable:object-literal-key-quotes + +describe('ImmutableRole', () => { + let stack: Stack; + let mutableRole: iam.IRole; + let immutableRole: iam.IRole; + + beforeEach(() => { + stack = new Stack(); + mutableRole = new iam.Role(stack, 'MutableRole', { + assumedBy: new iam.AnyPrincipal(), + }); + immutableRole = new iam.ImmutableRole(mutableRole); + }); + + test('ignores calls to addPolicy', () => { + immutableRole.addPolicy('Policy'); + + expect(stack).not.toHaveResourceLike('AWS::IAM::Policy'); + }); + + test('ignores calls to attachInlinePolicy', () => { + const user = new iam.User(stack, 'User'); + const policy = new iam.Policy(stack, 'Policy', { + statements: [new iam.PolicyStatement({ + resources: ['*'], + actions: ['s3:*'], + })], + users: [user], + }); + + immutableRole.attachInlinePolicy(policy); + + expect(stack).toHaveResource('AWS::IAM::Policy', { + "PolicyDocument": { + "Statement": [ + { + "Action": "s3:*", + "Resource": "*", + "Effect": "Allow", + }, + ], + "Version": "2012-10-17", + }, + "PolicyName": "Policy23B91518", + "Users": [ + { + "Ref": "User00B015A1", + }, + ], + }); + }); + + test('ignores calls to addManagedPolicy', () => { + mutableRole.addManagedPolicy({ managedPolicyArn: 'Arn1' }); + + immutableRole.addManagedPolicy({ managedPolicyArn: 'Arn2' }); + + expect(stack).toHaveResourceLike('AWS::IAM::Role', { + "ManagedPolicyArns": [ + 'Arn1', + ], + }); + }); + + test('ignores calls to addToPolicy', () => { + mutableRole.addToPolicy(new iam.PolicyStatement({ + resources: ['*'], + actions: ['s3:*'], + })); + + immutableRole.addToPolicy(new iam.PolicyStatement({ + resources: ['*'], + actions: ['iam:*'], + })); + + expect(stack).toHaveResourceLike('AWS::IAM::Policy', { + "PolicyDocument": { + "Statement": [ + { + "Resource": "*", + "Action": "s3:*", + "Effect": "Allow", + }, + ], + }, + }); + }); +});