From a8ea8bb8e4b0a81cd81efc40ef4b448e9ad02bdd Mon Sep 17 00:00:00 2001 From: Calvin Combs <66279577+comcalvi@users.noreply.github.com> Date: Wed, 15 Jun 2022 06:51:40 -0600 Subject: [PATCH] fix(iam): add `defaultPolicyName` to prevent policies overwriting each other in multi-stack deployments (#20705) This adds a prop, `defaultPolicyName`, that can be specified for imported roles. If the same role is imported in at least two stacks, and both of them create grant permissions to that role, the permissions granted in whichever stack was deployed last will overwrite the permissions granted by all others. Specifying this option allows users to specify different policy names across different stacks, which will prevent this overwrite issue. Closes #16074. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/aws-iam/lib/role.ts | 31 +++++++++- packages/@aws-cdk/aws-iam/package.json | 1 + .../aws-iam/test/role.from-role-arn.test.ts | 60 ++++++++++++++++++- 3 files changed, 86 insertions(+), 6 deletions(-) diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index 7451f68f31736..edf527647e8e9 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -162,8 +162,25 @@ export interface FromRoleArnOptions { * @default false */ readonly addGrantsToResources?: boolean; + + /** + * Any policies created by this role will use this value as their ID, if specified. + * Specify this if importing the same role in multiple stacks, and granting it + * different permissions in at least two stacks. If this is not specified + * (or if the same name is specified in more than one stack), + * a CloudFormation issue will result in the policy created in whichever stack + * is deployed last overwriting the policies created by the others. + * + * @default 'Policy' + */ + readonly defaultPolicyName?: string; } +/** + * Options allowing customizing the behavior of {@link Role.fromRoleName}. + */ +export interface FromRoleNameOptions extends FromRoleArnOptions { } + /** * IAM Role * @@ -206,12 +223,15 @@ export class Role extends Resource implements IRole { public readonly roleArn = roleArn; public readonly roleName = roleName; private readonly attachedPolicies = new AttachedPolicies(); + private readonly defaultPolicyName?: string; private defaultPolicy?: Policy; constructor(_scope: Construct, _id: string) { super(_scope, _id, { account: roleAccount, }); + + this.defaultPolicyName = options.defaultPolicyName; } public addToPolicy(statement: PolicyStatement): boolean { @@ -220,7 +240,7 @@ export class Role extends Resource implements IRole { public addToPrincipalPolicy(statement: PolicyStatement): AddToPrincipalPolicyResult { if (!this.defaultPolicy) { - this.defaultPolicy = new Policy(this, 'Policy'); + this.defaultPolicy = new Policy(this, this.defaultPolicyName ?? 'Policy'); this.attachInlinePolicy(this.defaultPolicy); } this.defaultPolicy.addStatements(statement); @@ -298,14 +318,19 @@ export class Role extends Resource implements IRole { * * The imported role is assumed to exist in the same account as the account * the scope's containing Stack is being deployed to. + + * @param scope construct scope + * @param id construct id + * @param roleName the name of the role to import + * @param options allow customizing the behavior of the returned role */ - public static fromRoleName(scope: Construct, id: string, roleName: string) { + public static fromRoleName(scope: Construct, id: string, roleName: string, options: FromRoleNameOptions = {}) { return Role.fromRoleArn(scope, id, Stack.of(scope).formatArn({ region: '', service: 'iam', resource: 'role', resourceName: roleName, - })); + }), options); } public readonly grantPrincipal: IPrincipal = this; diff --git a/packages/@aws-cdk/aws-iam/package.json b/packages/@aws-cdk/aws-iam/package.json index 3fd900bcba8e8..4e1b23769d375 100644 --- a/packages/@aws-cdk/aws-iam/package.json +++ b/packages/@aws-cdk/aws-iam/package.json @@ -109,6 +109,7 @@ "awslint": { "exclude": [ "from-signature:@aws-cdk/aws-iam.Role.fromRoleArn", + "from-signature:@aws-cdk/aws-iam.Role.fromRoleName", "from-method:@aws-cdk/aws-iam.AccessKey", "construct-interface-extends-iconstruct:@aws-cdk/aws-iam.IManagedPolicy", "props-physical-name:@aws-cdk/aws-iam.OpenIdConnectProviderProps", diff --git a/packages/@aws-cdk/aws-iam/test/role.from-role-arn.test.ts b/packages/@aws-cdk/aws-iam/test/role.from-role-arn.test.ts index 05a0fdaac4d58..5ccc7a6828d39 100644 --- a/packages/@aws-cdk/aws-iam/test/role.from-role-arn.test.ts +++ b/packages/@aws-cdk/aws-iam/test/role.from-role-arn.test.ts @@ -1,6 +1,6 @@ import { Template } from '@aws-cdk/assertions'; -import { App, Aws, CfnElement, Lazy, Stack } from '@aws-cdk/core'; -import { AnyPrincipal, ArnPrincipal, IRole, Policy, PolicyStatement, Role } from '../lib'; +import { App, Aws, CfnElement, CfnResource, Lazy, Stack } from '@aws-cdk/core'; +import { AnyPrincipal, ArnPrincipal, Grant, IRole, Policy, PolicyStatement, Role } from '../lib'; /* eslint-disable quote-props */ @@ -326,6 +326,60 @@ describe('IAM Role.fromRoleArn', () => { }); }); }); + + describe('imported with a user specified default policy name', () => { + test('user specified default policy is used when fromRoleArn() creates a default policy', () => { + roleStack = new Stack(app, 'RoleStack'); + new CfnResource(roleStack, 'SomeResource', { + type: 'CDK::Test::SomeResource', + }); + importedRole = Role.fromRoleArn(roleStack, 'ImportedRole', + `arn:aws:iam::${roleAccount}:role/${roleName}`, { defaultPolicyName: 'UserSpecifiedDefaultPolicy' }); + + Grant.addToPrincipal({ + actions: ['service:DoAThing'], + grantee: importedRole, + resourceArns: ['*'], + }); + + Template.fromStack(roleStack).templateMatches({ + Resources: { + ImportedRoleUserSpecifiedDefaultPolicy7CBF6E85: { + Type: 'AWS::IAM::Policy', + Properties: { + PolicyName: 'ImportedRoleUserSpecifiedDefaultPolicy7CBF6E85', + }, + }, + }, + }); + }); + }); + + test('`fromRoleName()` with options matches behavior of `fromRoleArn()`', () => { + roleStack = new Stack(app, 'RoleStack'); + new CfnResource(roleStack, 'SomeResource', { + type: 'CDK::Test::SomeResource', + }); + importedRole = Role.fromRoleName(roleStack, 'ImportedRole', + `${roleName}`, { defaultPolicyName: 'UserSpecifiedDefaultPolicy' }); + + Grant.addToPrincipal({ + actions: ['service:DoAThing'], + grantee: importedRole, + resourceArns: ['*'], + }); + + Template.fromStack(roleStack).templateMatches({ + Resources: { + ImportedRoleUserSpecifiedDefaultPolicy7CBF6E85: { + Type: 'AWS::IAM::Policy', + Properties: { + PolicyName: 'ImportedRoleUserSpecifiedDefaultPolicy7CBF6E85', + }, + }, + }, + }); + }); }); describe('imported with a dynamic ARN', () => { @@ -560,7 +614,7 @@ describe('IAM Role.fromRoleArn', () => { }); }); -test('Role.fromRoleName', () => { +test('Role.fromRoleName with no options ', () => { const app = new App(); const stack = new Stack(app, 'Stack', { env: { region: 'asdf', account: '1234' } }); const role = Role.fromRoleName(stack, 'MyRole', 'MyRole');