Skip to content

Commit

Permalink
fix(iam): permissions boundaries not added to custom resource roles (a…
Browse files Browse the repository at this point in the history
…ws#14754)

The role created by `CustomResourceProvider` is a `CfnResource` with a manual type, not a `CfnRole` to avoid a cyclical dependency. But since `PermissionBoundary` assumes all role/user resources in scope are instances of `CfnRole` or `CfnUser`, a permission boundary is not correctly applied to the custom resource's role (or any other role or user created directly through `CfnResource`).

This PR solves the above problem by adding extra conditionals for the `CfnResource` case and adds permission boundaries through the `addPropertyOverride` escape hatch.

fixes aws#13310

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
BenChaimberg authored and hollanddd committed Aug 26, 2021
1 parent 07dcb27 commit 2413596
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 4 deletions.
16 changes: 14 additions & 2 deletions packages/@aws-cdk/aws-iam/lib/permissions-boundary.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { CfnResource } from '@aws-cdk/core';
import { Node, IConstruct } from 'constructs';
import { CfnRole, CfnUser } from './iam.generated';
import { IManagedPolicy } from './managed-policy';
Expand All @@ -22,7 +23,8 @@ export class PermissionsBoundary {
}

/**
* Apply the given policy as Permissions Boundary to all Roles in the scope
* Apply the given policy as Permissions Boundary to all Roles and Users in
* the scope.
*
* Will override any Permissions Boundaries configured previously; in case
* a Permission Boundary is applied in multiple scopes, the Boundary applied
Expand All @@ -33,6 +35,11 @@ export class PermissionsBoundary {
visit(node: IConstruct) {
if (node instanceof CfnRole || node instanceof CfnUser) {
node.permissionsBoundary = boundaryPolicy.managedPolicyArn;
} else if (
node instanceof CfnResource &&
(node.cfnResourceType == CfnRole.CFN_RESOURCE_TYPE_NAME || node.cfnResourceType == CfnUser.CFN_RESOURCE_TYPE_NAME)
) {
node.addPropertyOverride('PermissionsBoundary', boundaryPolicy.managedPolicyArn);
}
},
});
Expand All @@ -46,8 +53,13 @@ export class PermissionsBoundary {
visit(node: IConstruct) {
if (node instanceof CfnRole || node instanceof CfnUser) {
node.permissionsBoundary = undefined;
} else if (
node instanceof CfnResource &&
(node.cfnResourceType == CfnRole.CFN_RESOURCE_TYPE_NAME || node.cfnResourceType == CfnUser.CFN_RESOURCE_TYPE_NAME)
) {
node.addPropertyDeletionOverride('PermissionsBoundary');
}
},
});
}
}
}
Empty file.
72 changes: 70 additions & 2 deletions packages/@aws-cdk/aws-iam/test/permissions-boundary.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as path from 'path';
import { ABSENT } from '@aws-cdk/assert-internal';
import '@aws-cdk/assert-internal/jest';
import { App, Stack } from '@aws-cdk/core';
import { App, CfnResource, CustomResourceProvider, CustomResourceProviderRuntime, Stack } from '@aws-cdk/core';
import * as iam from '../lib';

let app: App;
Expand Down Expand Up @@ -72,6 +73,73 @@ test('apply newly created boundary to a role', () => {
});
});

test('apply boundary to role created by a custom resource', () => {
// GIVEN
const provider = CustomResourceProvider.getOrCreateProvider(stack, 'Empty', {
codeDirectory: path.join(__dirname, 'custom-resource'),
runtime: CustomResourceProviderRuntime.NODEJS_12_X,
});

// WHEN
iam.PermissionsBoundary.of(provider).apply(new iam.ManagedPolicy(stack, 'Policy', {
statements: [
new iam.PolicyStatement({
actions: ['*'],
resources: ['*'],
}),
],
}));

// THEN
expect(stack).toHaveResource('AWS::IAM::Role', {
PermissionsBoundary: { Ref: 'Policy23B91518' },
});
});

test('apply boundary to users created via CfnResource', () => {
// GIVEN
const user = new CfnResource(stack, 'User', {
type: 'AWS::IAM::User',
});

// WHEN
iam.PermissionsBoundary.of(user).apply(new iam.ManagedPolicy(stack, 'Policy', {
statements: [
new iam.PolicyStatement({
actions: ['*'],
resources: ['*'],
}),
],
}));

// THEN
expect(stack).toHaveResource('AWS::IAM::User', {
PermissionsBoundary: { Ref: 'Policy23B91518' },
});
});

test('apply boundary to roles created via CfnResource', () => {
// GIVEN
const role = new CfnResource(stack, 'Role', {
type: 'AWS::IAM::Role',
});

// WHEN
iam.PermissionsBoundary.of(role).apply(new iam.ManagedPolicy(stack, 'Policy', {
statements: [
new iam.PolicyStatement({
actions: ['*'],
resources: ['*'],
}),
],
}));

// THEN
expect(stack).toHaveResource('AWS::IAM::Role', {
PermissionsBoundary: { Ref: 'Policy23B91518' },
});
});

test('unapply inherited boundary from a user: order 1', () => {
// GIVEN
const user = new iam.User(stack, 'User');
Expand All @@ -98,4 +166,4 @@ test('unapply inherited boundary from a user: order 2', () => {
expect(stack).toHaveResource('AWS::IAM::User', {
PermissionsBoundary: ABSENT,
});
});
});

0 comments on commit 2413596

Please sign in to comment.