From 65ee534b33d38cea312561d230f3038b1cd3a053 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 29 Jan 2021 16:51:03 +0100 Subject: [PATCH 1/4] feat(iam): Permissions Boundaries Allow configuring Permissions Boundaries for an entire subtree using Aspects, add a sample policy which can be used to reduce future misconfiguration risk for untrusted CodeBuild projects as an example. Fixes #3242. --- .../assert/lib/assertions/have-resource.ts | 2 +- packages/@aws-cdk/aws-codebuild/lib/index.ts | 1 + .../lib/untrusted-code-boundary-policy.ts | 86 +++++++++++++++ .../aws-codebuild/test/test.project.ts | 2 + .../test/test.untrusted-code-boundary.ts | 56 ++++++++++ packages/@aws-cdk/aws-iam/README.md | 33 ++++++ packages/@aws-cdk/aws-iam/lib/index.ts | 1 + .../aws-iam/lib/permissions-boundary.ts | 53 +++++++++ .../aws-iam/test/permissions-boundary.test.ts | 101 ++++++++++++++++++ 9 files changed, 334 insertions(+), 1 deletion(-) create mode 100644 packages/@aws-cdk/aws-codebuild/lib/untrusted-code-boundary-policy.ts create mode 100644 packages/@aws-cdk/aws-codebuild/test/test.untrusted-code-boundary.ts create mode 100644 packages/@aws-cdk/aws-iam/lib/permissions-boundary.ts create mode 100644 packages/@aws-cdk/aws-iam/test/permissions-boundary.test.ts diff --git a/packages/@aws-cdk/assert/lib/assertions/have-resource.ts b/packages/@aws-cdk/assert/lib/assertions/have-resource.ts index 2f3352bee16ef..3e44013188b2c 100644 --- a/packages/@aws-cdk/assert/lib/assertions/have-resource.ts +++ b/packages/@aws-cdk/assert/lib/assertions/have-resource.ts @@ -66,7 +66,7 @@ export class HaveResourceAssertion extends JestFriendlyAssertion for (const logicalId of Object.keys(inspector.value.Resources || {})) { const resource = inspector.value.Resources[logicalId]; if (resource.Type === this.resourceType) { - const propsToCheck = this.part === ResourcePart.Properties ? resource.Properties : resource; + const propsToCheck = this.part === ResourcePart.Properties ? (resource.Properties ?? {}) : resource; // Pass inspection object as 2nd argument, initialize failure with default string, // to maintain backwards compatibility with old predicate API. diff --git a/packages/@aws-cdk/aws-codebuild/lib/index.ts b/packages/@aws-cdk/aws-codebuild/lib/index.ts index 96731b2130043..5c2de5f3119c2 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/index.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/index.ts @@ -10,6 +10,7 @@ export * from './cache'; export * from './build-spec'; export * from './file-location'; export * from './linux-gpu-build-image'; +export * from './untrusted-code-boundary-policy'; // AWS::CodeBuild CloudFormation Resources: export * from './codebuild.generated'; diff --git a/packages/@aws-cdk/aws-codebuild/lib/untrusted-code-boundary-policy.ts b/packages/@aws-cdk/aws-codebuild/lib/untrusted-code-boundary-policy.ts new file mode 100644 index 0000000000000..9594ec953c82b --- /dev/null +++ b/packages/@aws-cdk/aws-codebuild/lib/untrusted-code-boundary-policy.ts @@ -0,0 +1,86 @@ +import * as iam from '@aws-cdk/aws-iam'; +import { Construct } from 'constructs'; + +/** + * Construction properties for UntrustedCodeBoundaryPolicy + */ +export interface UntrustedCodeBoundaryPolicyProps { + /** + * Additional statements to add to the default set of statements + * + * @default - No additional statements + */ + readonly additionalStatements?: iam.PolicyStatement[]; +} + +/** + * Permissions Boundary for a CodeBuild Project running untrusted code + * + * This class is a Policy, intended to be used as a Permissions Boundary + * for a CodeBuild project. It allows most of the actions necessary to run + * the CodeBuild project, but disallows reading from Parameter Store + * and Secrets Manager. + * + * Use this when your CodeBuild project is running untrusted code (for + * example, if you are using one to automatically build Pull Requests + * that anyone can submit), and you want to prevent your future self + * from accidentally exposing Secrets to this build. + * + * (The reason you might want to do this is because otherwise anyone + * who can submit a Pull Request to your project can write a script + * to email those secrets to themselves). + * + * @example + * + * iam.PermissionsBoundary.of(project).apply(new UntrustedCodeBoundaryPolicy(this, 'Boundary')); + */ +export class UntrustedCodeBoundaryPolicy extends iam.ManagedPolicy { + constructor(scope: Construct, id: string, props: UntrustedCodeBoundaryPolicyProps = {}) { + super(scope, id, { + description: 'Permissions Boundary Policy for CodeBuild Projects running untrusted code', + statements: [ + new iam.PolicyStatement({ + actions: [ + // For logging + 'logs:CreateLogGroup', + 'logs:CreateLogStream', + 'logs:PutLogEvents', + + // For test reports + 'codebuild:CreateReportGroup', + 'codebuild:CreateReport', + 'codebuild:UpdateReport', + 'codebuild:BatchPutTestCases', + 'codebuild:BatchPutCodeCoverages', + + // For batch builds + 'codebuild:StartBuild', + 'codebuild:StopBuild', + 'codebuild:RetryBuild', + + // For pulling ECR images + 'ecr:GetDownloadUrlForLayer', + 'ecr:BatchGetImage', + 'ecr:BatchCheckLayerAvailability', + + // For running in a VPC + 'ec2:CreateNetworkInterfacePermission', + 'ec2:CreateNetworkInterface', + 'ec2:DescribeNetworkInterfaces', + 'ec2:DeleteNetworkInterface', + 'ec2:DescribeSubnets', + 'ec2:DescribeSecurityGroups', + 'ec2:DescribeDhcpOptions', + 'ec2:DescribeVpcs', + + // NOTABLY MISSING: + // - Reading secrets + // - Reading parameterstore + ], + resources: ['*'], + }), + ...props.additionalStatements ?? [], + ], + }); + } +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-codebuild/test/test.project.ts b/packages/@aws-cdk/aws-codebuild/test/test.project.ts index 2912d832bd2c0..f4659e27d081e 100644 --- a/packages/@aws-cdk/aws-codebuild/test/test.project.ts +++ b/packages/@aws-cdk/aws-codebuild/test/test.project.ts @@ -482,6 +482,8 @@ export = { test.done(); }, + + 'metric method generates a valid CloudWatch metric'(test: Test) { const stack = new cdk.Stack(); diff --git a/packages/@aws-cdk/aws-codebuild/test/test.untrusted-code-boundary.ts b/packages/@aws-cdk/aws-codebuild/test/test.untrusted-code-boundary.ts new file mode 100644 index 0000000000000..04196e631f5c8 --- /dev/null +++ b/packages/@aws-cdk/aws-codebuild/test/test.untrusted-code-boundary.ts @@ -0,0 +1,56 @@ +import { expect, haveResourceLike, arrayWith } from '@aws-cdk/assert'; +import * as iam from '@aws-cdk/aws-iam'; +import * as cdk from '@aws-cdk/core'; +import { Test } from 'nodeunit'; +import * as codebuild from '../lib'; + +export = { + 'can attach permissions boundary to Project'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + const project = new codebuild.Project(stack, 'Project', { + source: codebuild.Source.gitHub({ owner: 'a', repo: 'b' }), + }); + iam.PermissionsBoundary.of(project).apply(new codebuild.UntrustedCodeBoundaryPolicy(stack, 'Boundary')); + + // THEN + expect(stack).to(haveResourceLike('AWS::IAM::Role', { + PermissionsBoundary: { Ref: 'BoundaryEA298153' }, + })); + + test.done(); + }, + + 'can add additional statements Boundary'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + const project = new codebuild.Project(stack, 'Project', { + source: codebuild.Source.gitHub({ owner: 'a', repo: 'b' }), + }); + iam.PermissionsBoundary.of(project).apply(new codebuild.UntrustedCodeBoundaryPolicy(stack, 'Boundary', { + additionalStatements: [ + new iam.PolicyStatement({ + actions: ['a:a'], + resources: ['b'], + }), + ], + })); + + // THEN + expect(stack).to(haveResourceLike('AWS::IAM::ManagedPolicy', { + PolicyDocument: { + Statement: arrayWith({ + Effect: 'Allow', + Action: 'a:a', + Resource: 'b', + }), + }, + })); + + test.done(); + }, +}; \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/README.md b/packages/@aws-cdk/aws-iam/README.md index d9488e7d081c8..c53bc4fee462f 100644 --- a/packages/@aws-cdk/aws-iam/README.md +++ b/packages/@aws-cdk/aws-iam/README.md @@ -264,6 +264,39 @@ const newPolicy = new Policy(stack, 'MyNewPolicy', { }); ``` +## Permissions Boundaries + +[Permissions +Boundaries](https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies_boundaries.html) +can be used as a mechanism to prevent privilege esclation by creating new +`Role`s. Permissions Boundaries are a Managed Policy, attached to Roles or +Users, that represent the *maximum* set of permissions they can have. The +effective set of permissions of a Role (or User) will be the intersection of +the Identity Policy and the Permissions Boundary attached to the Role (or +User). Permissions Boundaries are typically created by account +Administrators, and their use on newly created `Role`s will be enforced by +IAM policies. + +It is possible to attach Permissions Boundaries to all Roles created in a construct +tree all at once: + +```ts +// This imports an existing policy. You can also create a new one. +const boundary = iam.ManagedPolicy.fromManagedPolicyArn(this, 'Boundary', 'arn:aws:iam::123456789012:policy/boundary'); + +// Directly apply the boundary to a Role you create +iam.PermissionsBoundary.of(role).apply(boundary); + +// Apply the boundary to an Role that was implicitly created for you +iam.PermissionsBoundary.of(lambdaFunction).apply(boundary); + +// Apply the boundary to all Roles in a stack +iam.PermissionsBoundary.of(stack).apply(boundary); + +// Remove a Permissions Boundary that is inherited, for example from the Stack level +iam.PermissionsBoundary.of(customResource).clear(); +``` + ## OpenID Connect Providers OIDC identity providers are entities in IAM that describe an external identity diff --git a/packages/@aws-cdk/aws-iam/lib/index.ts b/packages/@aws-cdk/aws-iam/lib/index.ts index ba9250ca1e08e..19b8a156ba598 100644 --- a/packages/@aws-cdk/aws-iam/lib/index.ts +++ b/packages/@aws-cdk/aws-iam/lib/index.ts @@ -11,6 +11,7 @@ export * from './identity-base'; export * from './grant'; export * from './unknown-principal'; export * from './oidc-provider'; +export * from './permissions-boundary'; // AWS::IAM CloudFormation Resources: export * from './iam.generated'; diff --git a/packages/@aws-cdk/aws-iam/lib/permissions-boundary.ts b/packages/@aws-cdk/aws-iam/lib/permissions-boundary.ts new file mode 100644 index 0000000000000..7b4320462a1ac --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/permissions-boundary.ts @@ -0,0 +1,53 @@ +import { Node, IConstruct } from 'constructs'; +import { CfnRole, CfnUser } from './iam.generated'; +import { IManagedPolicy } from './managed-policy'; + +/** + * Modify the Permissions Boundaries of Users and Roles in a construct tree + * + * @example + * + * const policy = ManagedPolicy.fromAwsManagedPolicyName('ReadOnlyAccess'); + * PermissionsBoundary.of(stack).apply(policy); + */ +export class PermissionsBoundary { + /** + * Access the Permissions Boundaries of a construct tree + */ + public static of(scope: IConstruct): PermissionsBoundary { + return new PermissionsBoundary(scope); + } + + private constructor(private readonly scope: IConstruct) { + } + + /** + * Apply the given policy as Permissions Boundary to all Roles in the scope + * + * Will override any Permissions Boundaries configured previously; in case + * a Permission Boundary is applied in multiple scopes, the Boundary applied + * closest to the Role wins. + */ + public apply(boundaryPolicy: IManagedPolicy) { + Node.of(this.scope).applyAspect({ + visit(node: IConstruct) { + if (node instanceof CfnRole || node instanceof CfnUser) { + node.permissionsBoundary = boundaryPolicy.managedPolicyArn; + } + }, + }); + } + + /** + * Remove previously applied Permissions Boundaries + */ + public clear() { + Node.of(this.scope).applyAspect({ + visit(node: IConstruct) { + if (node instanceof CfnRole || node instanceof CfnUser) { + node.permissionsBoundary = undefined; + } + }, + }); + } +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/test/permissions-boundary.test.ts b/packages/@aws-cdk/aws-iam/test/permissions-boundary.test.ts new file mode 100644 index 0000000000000..99a14de55f2e1 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/test/permissions-boundary.test.ts @@ -0,0 +1,101 @@ +import { ABSENT } from '@aws-cdk/assert'; +import '@aws-cdk/assert/jest'; +import { App, Stack } from '@aws-cdk/core'; +import * as iam from '../lib'; + +let app: App; +let stack: Stack; +beforeEach(() => { + app = new App(); + stack = new Stack(app, 'Stack'); +}); + +test('apply imported boundary to a role', () => { + // GIVEN + const role = new iam.Role(stack, 'Role', { + assumedBy: new iam.ServicePrincipal('service.amazonaws.com'), + }); + + // WHEN + iam.PermissionsBoundary.of(role).apply(iam.ManagedPolicy.fromAwsManagedPolicyName('ReadOnlyAccess')); + + // THEN + expect(stack).toHaveResource('AWS::IAM::Role', { + PermissionsBoundary: { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + ':iam::aws:policy/ReadOnlyAccess', + ]], + }, + }); +}); + +test('apply imported boundary to a user', () => { + // GIVEN + const user = new iam.User(stack, 'User'); + + // WHEN + iam.PermissionsBoundary.of(user).apply(iam.ManagedPolicy.fromAwsManagedPolicyName('ReadOnlyAccess')); + + // THEN + expect(stack).toHaveResource('AWS::IAM::User', { + PermissionsBoundary: { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + ':iam::aws:policy/ReadOnlyAccess', + ]], + }, + }); +}); + +test('apply newly created boundary to a role', () => { + // GIVEN + const role = new iam.Role(stack, 'Role', { + assumedBy: new iam.ServicePrincipal('service.amazonaws.com'), + }); + + // 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'); + + // WHEN + iam.PermissionsBoundary.of(stack).apply(iam.ManagedPolicy.fromAwsManagedPolicyName('ReadOnlyAccess')); + iam.PermissionsBoundary.of(user).clear(); + + // THEN + expect(stack).toHaveResource('AWS::IAM::User', { + PermissionsBoundary: ABSENT, + }); +}); + +test('unapply inherited boundary from a user: order 2', () => { + // GIVEN + const user = new iam.User(stack, 'User'); + + // WHEN + iam.PermissionsBoundary.of(user).clear(); + iam.PermissionsBoundary.of(stack).apply(iam.ManagedPolicy.fromAwsManagedPolicyName('ReadOnlyAccess')); + + // THEN + expect(stack).toHaveResource('AWS::IAM::User', { + PermissionsBoundary: ABSENT, + }); +}); \ No newline at end of file From 9bdddef83b2c7956b9ef85f5391183b042cd2340 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 29 Jan 2021 18:29:54 +0100 Subject: [PATCH 2/4] Thanks eslint --- packages/@aws-cdk/aws-codebuild/test/test.project.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/@aws-cdk/aws-codebuild/test/test.project.ts b/packages/@aws-cdk/aws-codebuild/test/test.project.ts index f4659e27d081e..2912d832bd2c0 100644 --- a/packages/@aws-cdk/aws-codebuild/test/test.project.ts +++ b/packages/@aws-cdk/aws-codebuild/test/test.project.ts @@ -482,8 +482,6 @@ export = { test.done(); }, - - 'metric method generates a valid CloudWatch metric'(test: Test) { const stack = new cdk.Stack(); From bd628178d86f1723c601cd19885312e4df24251f Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 1 Feb 2021 09:26:33 +0100 Subject: [PATCH 3/4] Add more examples, fix build --- .../lib/untrusted-code-boundary-policy.ts | 8 ++++++++ packages/@aws-cdk/aws-iam/README.md | 13 ++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-codebuild/lib/untrusted-code-boundary-policy.ts b/packages/@aws-cdk/aws-codebuild/lib/untrusted-code-boundary-policy.ts index 9594ec953c82b..229cb547e7c1f 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/untrusted-code-boundary-policy.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/untrusted-code-boundary-policy.ts @@ -5,6 +5,13 @@ import { Construct } from 'constructs'; * Construction properties for UntrustedCodeBoundaryPolicy */ export interface UntrustedCodeBoundaryPolicyProps { + /** + * The name of the managed policy. + * + * @default - A name is automatically generated. + */ + readonly managedPolicyName?: string; + /** * Additional statements to add to the default set of statements * @@ -37,6 +44,7 @@ export interface UntrustedCodeBoundaryPolicyProps { export class UntrustedCodeBoundaryPolicy extends iam.ManagedPolicy { constructor(scope: Construct, id: string, props: UntrustedCodeBoundaryPolicyProps = {}) { super(scope, id, { + managedPolicyName: props.managedPolicyName, description: 'Permissions Boundary Policy for CodeBuild Projects running untrusted code', statements: [ new iam.PolicyStatement({ diff --git a/packages/@aws-cdk/aws-iam/README.md b/packages/@aws-cdk/aws-iam/README.md index c53bc4fee462f..f2b86ccff2f59 100644 --- a/packages/@aws-cdk/aws-iam/README.md +++ b/packages/@aws-cdk/aws-iam/README.md @@ -281,9 +281,20 @@ It is possible to attach Permissions Boundaries to all Roles created in a constr tree all at once: ```ts -// This imports an existing policy. You can also create a new one. +// This imports an existing policy. const boundary = iam.ManagedPolicy.fromManagedPolicyArn(this, 'Boundary', 'arn:aws:iam::123456789012:policy/boundary'); +// This creates a new boundary +const boundary2 = new iam.ManagedPolicy(this, 'Boundary2', { + statements: [ + new iam.PolicyStatement({ + effect: iam.Effect.DENY, + actions: ['iam:*'], + resources: ['*'], + }), + ], +}); + // Directly apply the boundary to a Role you create iam.PermissionsBoundary.of(role).apply(boundary); From 488c4e5d0fc582d080c6c4c5e99d595adbf508e3 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 1 Feb 2021 12:59:39 +0100 Subject: [PATCH 4/4] Fix tests --- .../@aws-cdk/aws-ecs/test/ec2/test.ec2-task-definition.ts | 2 +- packages/@aws-cdk/aws-ecs/test/test.aws-log-driver.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-task-definition.ts b/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-task-definition.ts index 8581738c6da51..9264c47e4550c 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-task-definition.ts +++ b/packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-task-definition.ts @@ -543,7 +543,7 @@ export = { }); // THEN - expect(stack).notTo(haveResource('AWS::ECR::Repository', {})); + expect(stack).to(haveResource('AWS::ECR::Repository', {})); test.done(); }, diff --git a/packages/@aws-cdk/aws-ecs/test/test.aws-log-driver.ts b/packages/@aws-cdk/aws-ecs/test/test.aws-log-driver.ts index 1ea5942386ad0..a4d5c9af9c53d 100644 --- a/packages/@aws-cdk/aws-ecs/test/test.aws-log-driver.ts +++ b/packages/@aws-cdk/aws-ecs/test/test.aws-log-driver.ts @@ -126,7 +126,7 @@ export = { test.done(); }, - 'without a defined log group'(test: Test) { + 'without a defined log group: creates one anyway'(test: Test) { // GIVEN td.addContainer('Container', { image, @@ -136,7 +136,7 @@ export = { }); // THEN - expect(stack).notTo(haveResource('AWS::Logs::LogGroup', {})); + expect(stack).to(haveResource('AWS::Logs::LogGroup', {})); test.done(); },