Skip to content

Commit

Permalink
fix(iam): add defaultPolicyName to prevent policies overwriting eac…
Browse files Browse the repository at this point in the history
…h 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*
  • Loading branch information
comcalvi authored Jun 15, 2022
1 parent c262034 commit 703e62e
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 6 deletions.
31 changes: 28 additions & 3 deletions packages/@aws-cdk/aws-iam/lib/role.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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 {
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-iam/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
60 changes: 57 additions & 3 deletions packages/@aws-cdk/aws-iam/test/role.from-role-arn.test.ts
Original file line number Diff line number Diff line change
@@ -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 */

Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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');
Expand Down

0 comments on commit 703e62e

Please sign in to comment.