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
30 changes: 30 additions & 0 deletions packages/@aws-cdk/aws-codebuild/test/test.project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,4 +301,34 @@ export = {

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: role.withoutPolicyUpdates(),
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();
},
};
71 changes: 70 additions & 1 deletion packages/@aws-cdk/aws-iam/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,78 @@ The `grant*` methods accept an `IGrantable` object. This interface is implemente

You can find which `grant*` methods exist for a resource in the [AWS CDK API Reference](https://docs.aws.amazon.com/cdk/api/latest/docs/aws-construct-library.html).

### Roles

Many AWS resources require *Roles* to operate. These Roles define the AWS API
calls an instance or other AWS service is allowed to make.

Creating Roles and populating them with the right permissions *Statements* is
a necessary but tedious part of setting up AWS infrastructure. In order to
help you focus on your business logic, CDK will take care of creating
roles and populating them with least-privilege permissions automatically.

All constructs that require Roles will create one for you if don't specify
one at construction time. Permissions will be added to that role
automatically if you associate the construct with other constructs from the
AWS Construct Library (for example, if you tell an *AWS CodePipeline* to trigger
an *AWS Lambda Function*, the Pipeline's Role will automatically get
`lambda:InvokeFunction` permissions on that particular Lambda Function),
or if you explicitly grant permissions using `grant` functions (see the
previous section).

#### Opting out of automatic permissions management

You may prefer to manage a Role's permissions yourself instead of having the
CDK automatically manage them for you. This may happen in one of the
following cases:

* You don't like the permissions that CDK automatically generates and
want to substitute your own set.
* The least-permissions policy that the CDK generates is becoming too
big for IAM to store, and you need to add some wildcards to keep the
policy size down.

To prevent constructs from updating your Role's policy, pass the object
returned by `myRole.withoutPolicyUpdates()` instead of `myRole` itself.

For example, to have an AWS CodePipeline *not* automatically add the required
permissions to trigger the expected targets, do the following:

```ts
const role = new iam.Role(this, 'Role', {
assumedBy: new iam.ServicePrincipal('codepipeline.amazonaws.com'),
});

new codepipeline.Pipeline(this, 'Pipeline', {
// Give the Pipeline an immutable view of the Role
role: role.withoutPolicyUpdates(),
});

// You now have to manage the Role policies yourself
role.addToPolicy(new iam.PolicyStatement({
action: [/* whatever actions you want */],
resource: [/* whatever resources you intend to touch */],
});
```

#### Using existing roles

If there are Roles in your account that have already been created which you
would like to use in your CDK application, you can use `Role.fromRoleArn` to
import them, as follows:

```ts
const role = iam.Role.fromRoleArn(this, 'Role', 'arn:aws:iam::123456789012:role/MyExistingRole', {
// Set 'mutable' to 'false' to use the role as-is and prevent adding new
// policies to it. The default is 'true', which means the role may be
// modified as part of the deployment.
mutable: false,
});
```

### Configuring an ExternalId

If you need to create roles that will be assumed by 3rd parties, it is generally a good idea to [require an `ExternalId`
If you need to create Roles that will be assumed by 4rd parties, it is generally a good idea to [require an `ExternalId`
to assume them](https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_create_for-user_externalid.html). Configuring
an `ExternalId` works like this:

Expand Down
58 changes: 58 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { DependableTrait } from '@aws-cdk/core';
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) {
// implement IDependable privately
DependableTrait.implement(this, {
dependencyRoots: [ role ]
});
}

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);
}
}
73 changes: 35 additions & 38 deletions packages/@aws-cdk/aws-iam/lib/role.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { Policy } from './policy';
import { PolicyDocument } from './policy-document';
import { PolicyStatement } from './policy-statement';
import { ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals';
import { ImmutableRole } from './private/immutable-role';
import { AttachedPolicies } from './util';

export interface RoleProps {
Expand Down Expand Up @@ -162,16 +163,32 @@ export class Role extends Resource implements IRole {
? resourceName.slice('service-role/'.length)
: resourceName;

abstract class Import extends Resource implements IRole {
class Import extends Resource implements IRole {
public readonly grantPrincipal: IPrincipal = this;
public readonly assumeRoleAction: string = 'sts:AssumeRole';
public readonly policyFragment = new ArnPrincipal(roleArn).policyFragment;
public readonly roleArn = roleArn;
public readonly roleName = roleName;
private readonly attachedPolicies = new AttachedPolicies();
private defaultPolicy?: Policy;

public abstract addToPolicy(statement: PolicyStatement): boolean;
public addToPolicy(statement: PolicyStatement): boolean {
if (!this.defaultPolicy) {
this.defaultPolicy = new Policy(this, 'Policy');
this.attachInlinePolicy(this.defaultPolicy);
}
this.defaultPolicy.addStatements(statement);
return true;
}

public attachInlinePolicy(policy: Policy): void {
const policyAccount = Stack.of(policy).account;

public abstract attachInlinePolicy(policy: Policy): void;
if (accountsAreEqualOrOneIsUnresolved(policyAccount, roleAccount)) {
this.attachedPolicies.attach(policy);
policy.attachToRole(this);
}
}

public addManagedPolicy(_policy: IManagedPolicy): void {
// FIXME: Add warning that we're ignoring this
Expand Down Expand Up @@ -199,44 +216,11 @@ export class Role extends Resource implements IRole {

const roleAccount = parsedArn.account;

class MutableImport extends Import {
private readonly attachedPolicies = new AttachedPolicies();
private defaultPolicy?: Policy;

public addToPolicy(statement: PolicyStatement): boolean {
if (!this.defaultPolicy) {
this.defaultPolicy = new Policy(this, 'Policy');
this.attachInlinePolicy(this.defaultPolicy);
}
this.defaultPolicy.addStatements(statement);
return true;
}

public attachInlinePolicy(policy: Policy): void {
const policyAccount = Stack.of(policy).account;

if (accountsAreEqualOrOneIsUnresolved(policyAccount, roleAccount)) {
this.attachedPolicies.attach(policy);
policy.attachToRole(this);
}
}
}

class ImmutableImport extends Import {
public addToPolicy(_statement: PolicyStatement): boolean {
return true;
}

public attachInlinePolicy(_policy: Policy): void {
// do nothing
}
}

const scopeAccount = scopeStack.account;

return options.mutable !== false && accountsAreEqualOrOneIsUnresolved(scopeAccount, roleAccount)
? new MutableImport(scope, id)
: new ImmutableImport(scope, id);
? new Import(scope, id)
: new ImmutableRole(new Import(scope, id));
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be new Import().withoutPolicyUpdates()?


function accountsAreEqualOrOneIsUnresolved(account1: string | undefined,
account2: string | undefined): boolean {
Expand Down Expand Up @@ -385,6 +369,19 @@ export class Role extends Resource implements IRole {
public grantPassRole(identity: IPrincipal) {
return this.grant(identity, 'iam:PassRole');
}

/**
* Return a copy of this Role object whose Policies will not be updated
*
* Use the object returned by this method if you want this Role to be used by
* a construct without it automatically updating the Role's Policies.
*
* If you do, you are responsible for adding the correct statements to the
* Role's policies yourself.
*/
public withoutPolicyUpdates(): IRole {
Copy link
Contributor

Choose a reason for hiding this comment

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

Going back to our conversation, why can't this be in IRole and works both for Role and imported roles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5569 (comment)

  1. The interface is already starting to get onerous to implement and I don't want to make it worse. There are 2 different parts to it: "what makes an IRole a role", and "operations that can be done to any IRole, which are the same for every role anyway". This is part of the 2nd, but I'd argue those shouldn't actually be on IRole at all.

  2. Even if we put it on IRole, my gut says the 99% case will be immutable, hence following up the import by calling this method. We want to optimize for the common case, and adding more verbiage seems the opposite. Either we (ALSO?) add a withPolicyUpdates() to switch it back on, or we just leave it as import properties for the fromRoleArn().

So either we make IRole even heavier or it doesn't need to be there at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. It’s not that critical.

return new ImmutableRole(this);
}
}

/**
Expand Down
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.Role;
let immutableRole: iam.IRole;

beforeEach(() => {
stack = new Stack();
mutableRole = new iam.Role(stack, 'MutableRole', {
assumedBy: new iam.AnyPrincipal(),
});
immutableRole = mutableRole.withoutPolicyUpdates();
});

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",
},
],
},
});
});
});