Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(iam): Role.withoutPolicyUpdates() #5569

Merged
merged 12 commits into from
Jan 15, 2020
9 changes: 4 additions & 5 deletions packages/@aws-cdk/aws-codebuild/lib/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import * as s3 from '@aws-cdk/aws-s3';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import { Aws, CfnResource, Construct, Duration, IResource, Lazy, PhysicalName, Resource, Stack } from '@aws-cdk/core';
import { Aws, Construct, Duration, IResource, Lazy, PhysicalName, Resource, Stack } from '@aws-cdk/core';
import { IArtifacts } from './artifacts';
import { BuildSpec } from './build-spec';
import { Cache } from './cache';
Expand Down Expand Up @@ -973,10 +973,9 @@ 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);
// otherwise, creating the Project fails, as it requires these permissions
// to be already attached to the Project's Role
project.node.addDependency(policy);
}

private validateCodePipelineSettings(artifacts: IArtifacts) {
Expand Down
64 changes: 62 additions & 2 deletions packages/@aws-cdk/aws-codebuild/test/test.project.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, haveResource, haveResourceLike, not } from '@aws-cdk/assert';
import { countResources, expect, haveResource, haveResourceLike, not, ResourcePart } from '@aws-cdk/assert';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import * as s3 from '@aws-cdk/aws-s3';
Expand Down Expand Up @@ -271,4 +271,64 @@ 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));

// Check that the CodeBuild project does not have a DependsOn
expect(stack).to(haveResource('AWS::CodeBuild::Project', (res: any) => {
if (res.DependsOn && res.DependsOn.length > 0) {
throw new Error(`CodeBuild project should have no DependsOn, but got: ${JSON.stringify(res, undefined, 2)}`);
}
return true;
}, ResourcePart.CompleteDefinition));

test.done();
},

'can use an ImmutableRole for a Project within a VPC'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'can use an ImmutableRole for a Project within a VPC'(test: Test) {
'can use role.withoutPolicyUpdates() for a Project within a VPC'(test: Test) {

const stack = new cdk.Stack();

const role = new iam.Role(stack, 'Role', {
assumedBy: new iam.ServicePrincipal('codebuild.amazonaws.com')
});

const vpc = new ec2.Vpc(stack, 'Vpc');

new codebuild.Project(stack, 'Project', {
source: codebuild.Source.gitHubEnterprise({
httpsCloneUrl: 'https://mygithub-enterprise.com/myuser/myrepo',
}),
role: new iam.ImmutableRole(role),
vpc,
});

expect(stack).to(countResources('AWS::IAM::Policy', 0));

// Check that the CodeBuild project does not have a DependsOn
expect(stack).to(haveResource('AWS::CodeBuild::Project', (res: any) => {
if (res.DependsOn && res.DependsOn.length > 0) {
throw new Error(`CodeBuild project should have no DependsOn, but got: ${JSON.stringify(res, undefined, 2)}`);
}
return true;
}, ResourcePart.CompleteDefinition));

test.done();
},
};
53 changes: 53 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/immutable-role.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { Grant } from './grant';
import { IManagedPolicy } from './managed-policy';
import { Policy } from './policy';
import { PolicyStatement } from './policy-statement';
import { IPrincipal } from './principals';
import { IRole } from './role';

/**
* An immutable wrapper around an IRole
*
* This wrapper 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 attachInlinePolicy(_policy: Policy): void {
// do nothing
}

public addManagedPolicy(_policy: IManagedPolicy): void {
// do nothing
}

public addToPolicy(_statement: PolicyStatement): boolean {
// Not really added, but for the purposes of consumer code pretend that it was.
return true;
}

public grant(grantee: IPrincipal, ...actions: string[]): Grant {
return this.role.grant(grantee, ...actions);
}

public grantPassRole(grantee: IPrincipal): Grant {
return this.role.grantPassRole(grantee);
}
}
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-iam/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export * from './principals';
export * from './identity-base';
export * from './grant';
export * from './unknown-principal';
export * from './immutable-role';

// AWS::IAM CloudFormation Resources:
export * from './iam.generated';
76 changes: 65 additions & 11 deletions packages/@aws-cdk/aws-iam/lib/policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,22 @@ export interface PolicyProps {
* @default - No statements.
*/
readonly statements?: PolicyStatement[];

/**
* Whether an `AWS::IAM::Policy` must be created
*
* Unless set to `true`, this `Policy` construct will not materialize to an
* `AWS::IAM::Policy` CloudFormation resource in case it would have no effect
* (for example, if it remains unattached to an IAM identity or if it has no
* statements). This is generally desired behavior, since it prevents
* creating invalid--and hence undeployable--CloudFormation templates.
*
* In cases where you know the policy must be created and it is actually
* an error if no statements have been added to it, you can se this to `true`.
*
* @default false
*/
readonly mustCreate?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about this name... How about lazy?: boolean?

}

/**
Expand All @@ -79,16 +95,12 @@ export class Policy extends Resource implements IPolicy {
*/
public readonly document = new PolicyDocument();

/**
* The name of this policy.
*
* @attribute
*/
public readonly policyName: string;

private readonly _policyName: string;
private readonly roles = new Array<IRole>();
private readonly users = new Array<IUser>();
private readonly groups = new Array<IGroup>();
private readonly mustCreate: boolean;
private referenceTaken = false;

constructor(scope: Construct, id: string, props: PolicyProps = {}) {
super(scope, id, {
Expand All @@ -107,7 +119,8 @@ export class Policy extends Resource implements IPolicy {
groups: undefinedIfEmpty(() => this.groups.map(g => g.groupName)),
});

this.policyName = this.physicalName!;
this._policyName = this.physicalName!;
this.mustCreate = props.mustCreate !== undefined ? props.mustCreate : false;

if (props.users) {
props.users.forEach(u => this.attachToUser(u));
Expand Down Expand Up @@ -160,19 +173,60 @@ export class Policy extends Resource implements IPolicy {
group.attachInlinePolicy(this);
}

/**
* The name of this policy.
*
* @attribute
*/
public get policyName(): string {
this.referenceTaken = true;
return this._policyName;
}

protected validate(): string[] {
const result = new Array<string>();

// validate that the policy document is not empty
if (this.document.isEmpty) {
result.push('Policy is empty. You must add statements to the policy');
if (this.mustCreate) {
result.push('Policy created with mustCreate=true is empty. You must add statements to the policy');
}
if (!this.mustCreate && this.referenceTaken) {
result.push('Policy name has been read of empty policy. You must add statements to the policy so it can exist.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can word this a little more clearly... how about: This Policy has been referenced by another resource, and so it must contain at least one statement.

}
}

// validate that the policy is attached to at least one principal (role, user or group).
if (this.groups.length + this.users.length + this.roles.length === 0) {
result.push(`Policy must be attached to at least one principal: user, group or role`);
if (!this.isAttached) {
if (this.mustCreate) {
result.push(`Policy created with mustCreate=true must be attached to at least one principal: user, group or role`);
}
if (!this.mustCreate && this.referenceTaken) {
result.push('Policy name has been read of unattached policy. Attach to at least one principal: user, group or role.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: This Policy has been referenced by another resource, and so it must be attached to at least one principal: user, group or role.

}
}

return result;
}

protected prepare() {
// Remove the resource if it shouldn't exist. This will prevent it from being rendered to the template.
if (!this.shouldExist) {
this.node.removeChild('Resource');
}
}

/**
* Whether the policy resource has been attached to any identity
*/
private get isAttached() {
return this.groups.length + this.users.length + this.roles.length > 0;
}

/**
* Whether the policy resource should be created
*/
private get shouldExist() {
return this.mustCreate || this.referenceTaken || (!this.document.isEmpty && this.isAttached);
}
}
87 changes: 87 additions & 0 deletions packages/@aws-cdk/aws-iam/test/immutable-role.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import '@aws-cdk/assert/jest';
import { Stack } from '@aws-cdk/core';
import * as iam from '../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 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",
},
],
},
});
});
});
Loading