From 4548884cf4f76c08145512179fa302f47462cc89 Mon Sep 17 00:00:00 2001 From: akash1810 Date: Thu, 8 Feb 2024 09:38:06 +0000 Subject: [PATCH] fix(ec2)!: Tighten default EC2 access to SSM Parameters The SSM+SSH capability requires an IAM Policy. This change swaps the AmazonSSMManagedInstanceCore AWS Managed Policy with one of our own to more closely follow the principle of least privilege. In addition to enabling SSM+SSH, AmazonSSMManagedInstanceCore adds permissions to read _all_ SSM Parameters. We use SSM Parameters for configuration. As AmazonSSMManagedInstanceCore allows reading of all SSM Parameters, every EC2 instance can read the configuration of all apps. This is not least privilege. https://github.com/guardian/cdk/pull/1694 comments that using AmazonSSMManagedInstanceCore enabled usage of AWS Inspector v2. However, as we haven't enabled AWS Inspector across the estate yet, we can add the permissions in a later PR. See https://docs.aws.amazon.com/aws-managed-policy/latest/reference/AmazonSSMManagedInstanceCore.html. --- .changeset/moody-needles-end.md | 82 +++++++ src/constructs/iam/policies/ssm-ssh.ts | 51 ++++ .../__snapshots__/instance-role.test.ts.snap | 221 ++++++++++++------ .../iam/roles/instance-role.test.ts | 8 +- src/constructs/iam/roles/instance-role.ts | 7 +- .../ec2-app/__snapshots__/base.test.ts.snap | 102 +++++--- 6 files changed, 365 insertions(+), 106 deletions(-) create mode 100644 .changeset/moody-needles-end.md create mode 100644 src/constructs/iam/policies/ssm-ssh.ts diff --git a/.changeset/moody-needles-end.md b/.changeset/moody-needles-end.md new file mode 100644 index 0000000000..dc4666b856 --- /dev/null +++ b/.changeset/moody-needles-end.md @@ -0,0 +1,82 @@ +--- +"@guardian/cdk": major +--- + +This change includes some potentially breaking changes for consumers of: +- [`GuEc2App`](https://guardian.github.io/cdk/classes/patterns.GuEc2App.html) +- [`GuPlayApp`](https://guardian.github.io/cdk/classes/patterns.GuPlayApp.html) (a subclass of `GuEc2App`) +- [`GuPlayWorkerApp`](https://guardian.github.io/cdk/classes/patterns.GuPlayWorkerApp.html) (a subclass of `GuEc2App`) +- [`GuNodeApp`](https://guardian.github.io/cdk/classes/patterns.GuNodeApp.html) (a subclass of `GuEc2App`) + +Since [v49.0.2](https://github.com/guardian/cdk/releases/tag/v49.0.2), +the EC2 instance profile created in `GuEc2App`, and it's subclasses, +used the [`AmazonSSMManagedInstanceCore`](https://docs.aws.amazon.com/aws-managed-policy/latest/reference/AmazonSSMManagedInstanceCore.html) AWS Managed Policy +to enable the [SSM+SSH capability](https://github.com/guardian/ssm-scala?tab=readme-ov-file#in-aws). + +In addition to enabling SSM+SSH, this Managed Policy also provided read access to all SSM Parameters. +This is not least privilege. + +In this version, usage of the `AmazonSSMManagedInstanceCore` Managed Policy is swapped for a custom, +more minimal, policy. + +> [!IMPORTANT] +> Before upgrading to this version, +> ensure your application is not relying on the IAM Policy behaviour provided by `AmazonSSMManagedInstanceCore`. + +If your application is reading SSM Parameters outside the `/STAGE/STACK/APP/*` namespace, +you will need to add an explicit policy. + +An IAM Policy to read SSM Parameters in the `/STAGE/STACK/APP/*` namespace is already provided by the `GuEc2App` construct, +via [`GuParameterStoreReadPolicy`](https://guardian.github.io/cdk/classes/constructs_iam.GuParameterStoreReadPolicy.html) + +To understand if your application is impacted, +consult [this Service Catalogue query](https://metrics.gutools.co.uk/goto/KZhWJVoIg?orgId=1) +showing CloudFormation stacks using the above _and_ using GuCDK v49.0.2 or above. + +
Query ran in Service Catalogue +

+ +```sql +with data as ( + select cfn.account_id + , acc.name as account_name + , tml.stack_id + , cfn.last_updated_time + , cfn.region + , cfn.stack_name + , tml.metadata ->> 'gu:cdk:version' as gucdk_version + , cfn.tags ->> 'gu:repo' as repository + , cfn.tags ->> 'Stack' as stack + , cfn.tags ->> 'Stage' as stage + , cfn.tags ->> 'App' as app + from aws_cloudformation_template_summaries tml + join aws_accounts acc on tml.account_id = acc.id + join aws_cloudformation_stacks cfn on tml.stack_arn = cfn.arn + where tml.metadata is not null + and ( + (metadata -> 'gu:cdk:constructs')::jsonb ? 'GuEc2App' + OR (metadata -> 'gu:cdk:constructs')::jsonb ? 'GuPlayApp' + OR (metadata -> 'gu:cdk:constructs')::jsonb ? 'GuPlayWorkerApp' + OR (metadata -> 'gu:cdk:constructs')::jsonb ? 'GuNodeApp' + ) +), +ownership as ( + select distinct full_name + , galaxies_team + , team_contact_email + from view_repo_ownership + where galaxies_team is not null + and team_contact_email is not null +) + +select data.* + , ownership.galaxies_team + , ownership.team_contact_email +from data + left join ownership on data.repository = ownership.full_name +where gucdk_version like '49%' -- affected version is 49.0.2 onwards, so this will catch some extra stacks, but hopefully not too many! + OR gucdk_version like '5%'; +``` + +

+
diff --git a/src/constructs/iam/policies/ssm-ssh.ts b/src/constructs/iam/policies/ssm-ssh.ts new file mode 100644 index 0000000000..0d87980daf --- /dev/null +++ b/src/constructs/iam/policies/ssm-ssh.ts @@ -0,0 +1,51 @@ +import { isSingletonPresentInStack } from "../../../utils/singleton"; +import type { GuStack } from "../../core"; +import { GuAllowPolicy } from "./base-policy"; + +/** + * A minimal policy enabling SSM+SSH access to EC2 instances. + * + * This is favoured over the AWS Managed Policy `AmazonSSMManagedInstanceCore`, + * `AmazonSSMManagedInstanceCore` provides read access to all SSM Parameters. + * This is not required for SSM+SSH, and given our usage of SSM Parameters for configuration, is not following least privilege. + * Access to SSM Parameters should be scoped to the Stack, Stage, and Application. + * + * @see https://docs.aws.amazon.com/aws-managed-policy/latest/reference/AmazonSSMManagedInstanceCore.html + * @see https://github.com/guardian/ssm-scala + */ +export class GuSsmSshPolicy extends GuAllowPolicy { + private static instance: GuSsmSshPolicy | undefined; + + private constructor(scope: GuStack) { + super(scope, "SsmSshPolicy", { + policyName: "ssm-ssh-policy", + actions: [ + "ec2messages:AcknowledgeMessage", + "ec2messages:DeleteMessage", + "ec2messages:FailMessage", + "ec2messages:GetEndpoint", + "ec2messages:GetMessages", + "ec2messages:SendReply", + "ssm:UpdateInstanceInformation", + "ssm:ListInstanceAssociations", + "ssm:DescribeInstanceProperties", + "ssm:DescribeDocumentParameters", + "ssmmessages:CreateControlChannel", + "ssmmessages:CreateDataChannel", + "ssmmessages:OpenControlChannel", + "ssmmessages:OpenDataChannel", + ], + + // TODO can this be scoped to the stack, stage, and application? If so, it won't make sense to be a singleton. + resources: ["*"], + }); + } + + public static getInstance(stack: GuStack): GuSsmSshPolicy { + if (!this.instance || !isSingletonPresentInStack(stack, this.instance)) { + this.instance = new GuSsmSshPolicy(stack); + } + + return this.instance; + } +} diff --git a/src/constructs/iam/roles/__snapshots__/instance-role.test.ts.snap b/src/constructs/iam/roles/__snapshots__/instance-role.test.ts.snap index 595b4e1448..84a9604483 100644 --- a/src/constructs/iam/roles/__snapshots__/instance-role.test.ts.snap +++ b/src/constructs/iam/roles/__snapshots__/instance-role.test.ts.snap @@ -7,6 +7,7 @@ exports[`The GuInstanceRole construct should allow additional policies to be spe "GuStack", "GuGetS3ObjectsPolicy", "GuInstanceRole", + "GuSsmSshPolicy", "GuDescribeEC2Policy", "GuDistributionBucketParameter", "GuGetDistributablePolicy", @@ -115,20 +116,6 @@ exports[`The GuInstanceRole construct should allow additional policies to be spe ], "Version": "2012-10-17", }, - "ManagedPolicyArns": [ - { - "Fn::Join": [ - "", - [ - "arn:", - { - "Ref": "AWS::Partition", - }, - ":iam::aws:policy/AmazonSSMManagedInstanceCore", - ], - ], - }, - ], "Path": "/", "Tags": [ { @@ -214,6 +201,42 @@ exports[`The GuInstanceRole construct should allow additional policies to be spe }, "Type": "AWS::IAM::Policy", }, + "SsmSshPolicy4CFC977E": { + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": [ + "ec2messages:AcknowledgeMessage", + "ec2messages:DeleteMessage", + "ec2messages:FailMessage", + "ec2messages:GetEndpoint", + "ec2messages:GetMessages", + "ec2messages:SendReply", + "ssm:UpdateInstanceInformation", + "ssm:ListInstanceAssociations", + "ssm:DescribeInstanceProperties", + "ssm:DescribeDocumentParameters", + "ssmmessages:CreateControlChannel", + "ssmmessages:CreateDataChannel", + "ssmmessages:OpenControlChannel", + "ssmmessages:OpenDataChannel", + ], + "Effect": "Allow", + "Resource": "*", + }, + ], + "Version": "2012-10-17", + }, + "PolicyName": "ssm-ssh-policy", + "Roles": [ + { + "Ref": "InstanceRoleTestingCB7BD146", + }, + ], + }, + "Type": "AWS::IAM::Policy", + }, }, } `; @@ -224,6 +247,7 @@ exports[`The GuInstanceRole construct should be possible to create multiple inst "gu:cdk:constructs": [ "GuStack", "GuInstanceRole", + "GuSsmSshPolicy", "GuDescribeEC2Policy", "GuLoggingStreamNameParameter", "GuLogShippingPolicy", @@ -401,20 +425,6 @@ exports[`The GuInstanceRole construct should be possible to create multiple inst ], "Version": "2012-10-17", }, - "ManagedPolicyArns": [ - { - "Fn::Join": [ - "", - [ - "arn:", - { - "Ref": "AWS::Partition", - }, - ":iam::aws:policy/AmazonSSMManagedInstanceCore", - ], - ], - }, - ], "Path": "/", "Tags": [ { @@ -455,20 +465,6 @@ exports[`The GuInstanceRole construct should be possible to create multiple inst ], "Version": "2012-10-17", }, - "ManagedPolicyArns": [ - { - "Fn::Join": [ - "", - [ - "arn:", - { - "Ref": "AWS::Partition", - }, - ":iam::aws:policy/AmazonSSMManagedInstanceCore", - ], - ], - }, - ], "Path": "/", "Tags": [ { @@ -613,6 +609,45 @@ exports[`The GuInstanceRole construct should be possible to create multiple inst }, "Type": "AWS::IAM::Policy", }, + "SsmSshPolicy4CFC977E": { + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": [ + "ec2messages:AcknowledgeMessage", + "ec2messages:DeleteMessage", + "ec2messages:FailMessage", + "ec2messages:GetEndpoint", + "ec2messages:GetMessages", + "ec2messages:SendReply", + "ssm:UpdateInstanceInformation", + "ssm:ListInstanceAssociations", + "ssm:DescribeInstanceProperties", + "ssm:DescribeDocumentParameters", + "ssmmessages:CreateControlChannel", + "ssmmessages:CreateDataChannel", + "ssmmessages:OpenControlChannel", + "ssmmessages:OpenDataChannel", + ], + "Effect": "Allow", + "Resource": "*", + }, + ], + "Version": "2012-10-17", + }, + "PolicyName": "ssm-ssh-policy", + "Roles": [ + { + "Ref": "InstanceRoleMyfirstapp5C11A22B", + }, + { + "Ref": "InstanceRoleMysecondapp48DD15D7", + }, + ], + }, + "Type": "AWS::IAM::Policy", + }, }, } `; @@ -623,6 +658,7 @@ exports[`The GuInstanceRole construct should create an additional logging policy "gu:cdk:constructs": [ "GuStack", "GuInstanceRole", + "GuSsmSshPolicy", "GuDescribeEC2Policy", "GuLoggingStreamNameParameter", "GuLogShippingPolicy", @@ -759,20 +795,6 @@ exports[`The GuInstanceRole construct should create an additional logging policy ], "Version": "2012-10-17", }, - "ManagedPolicyArns": [ - { - "Fn::Join": [ - "", - [ - "arn:", - { - "Ref": "AWS::Partition", - }, - ":iam::aws:policy/AmazonSSMManagedInstanceCore", - ], - ], - }, - ], "Path": "/", "Tags": [ { @@ -858,6 +880,42 @@ exports[`The GuInstanceRole construct should create an additional logging policy }, "Type": "AWS::IAM::Policy", }, + "SsmSshPolicy4CFC977E": { + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": [ + "ec2messages:AcknowledgeMessage", + "ec2messages:DeleteMessage", + "ec2messages:FailMessage", + "ec2messages:GetEndpoint", + "ec2messages:GetMessages", + "ec2messages:SendReply", + "ssm:UpdateInstanceInformation", + "ssm:ListInstanceAssociations", + "ssm:DescribeInstanceProperties", + "ssm:DescribeDocumentParameters", + "ssmmessages:CreateControlChannel", + "ssmmessages:CreateDataChannel", + "ssmmessages:OpenControlChannel", + "ssmmessages:OpenDataChannel", + ], + "Effect": "Allow", + "Resource": "*", + }, + ], + "Version": "2012-10-17", + }, + "PolicyName": "ssm-ssh-policy", + "Roles": [ + { + "Ref": "InstanceRoleTestingCB7BD146", + }, + ], + }, + "Type": "AWS::IAM::Policy", + }, }, } `; @@ -868,6 +926,7 @@ exports[`The GuInstanceRole construct should create the correct resources with m "gu:cdk:constructs": [ "GuStack", "GuInstanceRole", + "GuSsmSshPolicy", "GuDescribeEC2Policy", "GuDistributionBucketParameter", "GuGetDistributablePolicy", @@ -955,20 +1014,6 @@ exports[`The GuInstanceRole construct should create the correct resources with m ], "Version": "2012-10-17", }, - "ManagedPolicyArns": [ - { - "Fn::Join": [ - "", - [ - "arn:", - { - "Ref": "AWS::Partition", - }, - ":iam::aws:policy/AmazonSSMManagedInstanceCore", - ], - ], - }, - ], "Path": "/", "Tags": [ { @@ -1054,6 +1099,42 @@ exports[`The GuInstanceRole construct should create the correct resources with m }, "Type": "AWS::IAM::Policy", }, + "SsmSshPolicy4CFC977E": { + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": [ + "ec2messages:AcknowledgeMessage", + "ec2messages:DeleteMessage", + "ec2messages:FailMessage", + "ec2messages:GetEndpoint", + "ec2messages:GetMessages", + "ec2messages:SendReply", + "ssm:UpdateInstanceInformation", + "ssm:ListInstanceAssociations", + "ssm:DescribeInstanceProperties", + "ssm:DescribeDocumentParameters", + "ssmmessages:CreateControlChannel", + "ssmmessages:CreateDataChannel", + "ssmmessages:OpenControlChannel", + "ssmmessages:OpenDataChannel", + ], + "Effect": "Allow", + "Resource": "*", + }, + ], + "Version": "2012-10-17", + }, + "PolicyName": "ssm-ssh-policy", + "Roles": [ + { + "Ref": "InstanceRoleTestingCB7BD146", + }, + ], + }, + "Type": "AWS::IAM::Policy", + }, }, } `; diff --git a/src/constructs/iam/roles/instance-role.test.ts b/src/constructs/iam/roles/instance-role.test.ts index 907f4334e7..78578f3af0 100644 --- a/src/constructs/iam/roles/instance-role.test.ts +++ b/src/constructs/iam/roles/instance-role.test.ts @@ -12,7 +12,7 @@ describe("The GuInstanceRole construct", () => { expect(template.toJSON()).toMatchSnapshot(); template.resourceCountIs("AWS::IAM::Role", 1); - template.resourceCountIs("AWS::IAM::Policy", 3); + template.resourceCountIs("AWS::IAM::Policy", 4); }); it("should create an additional logging policy if logging stream is specified", () => { @@ -23,7 +23,7 @@ describe("The GuInstanceRole construct", () => { expect(template.toJSON()).toMatchSnapshot(); template.resourceCountIs("AWS::IAM::Role", 1); - template.resourceCountIs("AWS::IAM::Policy", 4); + template.resourceCountIs("AWS::IAM::Policy", 5); }); it("should allow additional policies to be specified", () => { @@ -39,7 +39,7 @@ describe("The GuInstanceRole construct", () => { expect(template.toJSON()).toMatchSnapshot(); template.resourceCountIs("AWS::IAM::Role", 1); - template.resourceCountIs("AWS::IAM::Policy", 4); + template.resourceCountIs("AWS::IAM::Policy", 5); }); it("should be possible to create multiple instance roles in a single stack", () => { @@ -57,6 +57,6 @@ describe("The GuInstanceRole construct", () => { expect(template.toJSON()).toMatchSnapshot(); template.resourceCountIs("AWS::IAM::Role", 2); - template.resourceCountIs("AWS::IAM::Policy", 6); // 2 shared policies + 2 policies per role (2 + (2*2)) + template.resourceCountIs("AWS::IAM::Policy", 7); // 3 shared policies + 2 policies per role (3 + (2*2)) }); }); diff --git a/src/constructs/iam/roles/instance-role.ts b/src/constructs/iam/roles/instance-role.ts index 547966d320..a6bc4a60b9 100644 --- a/src/constructs/iam/roles/instance-role.ts +++ b/src/constructs/iam/roles/instance-role.ts @@ -1,4 +1,4 @@ -import { ManagedPolicy, ServicePrincipal } from "aws-cdk-lib/aws-iam"; +import { ServicePrincipal } from "aws-cdk-lib/aws-iam"; import { GuAppAwareConstruct } from "../../../utils/mixin/app-aware-construct"; import type { AppIdentity, GuStack } from "../../core"; import { @@ -8,6 +8,7 @@ import { GuParameterStoreReadPolicy, } from "../policies"; import type { GuPolicy } from "../policies"; +import { GuSsmSshPolicy } from "../policies/ssm-ssh"; import { GuRole } from "./roles"; export interface GuInstanceRoleProps { @@ -48,6 +49,7 @@ export class GuInstanceRole extends GuAppAwareConstruct(GuRole) { }); const sharedPolicies = [ + GuSsmSshPolicy.getInstance(scope), GuDescribeEC2Policy.getInstance(scope), ...(props.withoutLogShipping ? [] : [GuLogShippingPolicy.getInstance(scope)]), ]; @@ -59,9 +61,6 @@ export class GuInstanceRole extends GuAppAwareConstruct(GuRole) { ...(props.additionalPolicies ? props.additionalPolicies : []), ]; - const managedPolicies = [ManagedPolicy.fromAwsManagedPolicyName("AmazonSSMManagedInstanceCore")]; - policies.forEach((p) => p.attachToRole(this)); - managedPolicies.forEach((p) => this.addManagedPolicy(p)); } } diff --git a/src/patterns/ec2-app/__snapshots__/base.test.ts.snap b/src/patterns/ec2-app/__snapshots__/base.test.ts.snap index 1866c44070..e427113710 100644 --- a/src/patterns/ec2-app/__snapshots__/base.test.ts.snap +++ b/src/patterns/ec2-app/__snapshots__/base.test.ts.snap @@ -11,6 +11,7 @@ exports[`the GuEC2App pattern can produce a restricted EC2 app locked to specifi "GuEc2App", "GuCertificate", "GuInstanceRole", + "GuSsmSshPolicy", "GuDescribeEC2Policy", "GuLoggingStreamNameParameter", "GuLogShippingPolicy", @@ -364,20 +365,6 @@ exports[`the GuEC2App pattern can produce a restricted EC2 app locked to specifi ], "Version": "2012-10-17", }, - "ManagedPolicyArns": [ - { - "Fn::Join": [ - "", - [ - "arn:", - { - "Ref": "AWS::Partition", - }, - ":iam::aws:policy/AmazonSSMManagedInstanceCore", - ], - ], - }, - ], "Path": "/", "Tags": [ { @@ -695,6 +682,42 @@ exports[`the GuEC2App pattern can produce a restricted EC2 app locked to specifi }, "Type": "AWS::EC2::SecurityGroupEgress", }, + "SsmSshPolicy4CFC977E": { + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": [ + "ec2messages:AcknowledgeMessage", + "ec2messages:DeleteMessage", + "ec2messages:FailMessage", + "ec2messages:GetEndpoint", + "ec2messages:GetMessages", + "ec2messages:SendReply", + "ssm:UpdateInstanceInformation", + "ssm:ListInstanceAssociations", + "ssm:DescribeInstanceProperties", + "ssm:DescribeDocumentParameters", + "ssmmessages:CreateControlChannel", + "ssmmessages:CreateDataChannel", + "ssmmessages:OpenControlChannel", + "ssmmessages:OpenDataChannel", + ], + "Effect": "Allow", + "Resource": "*", + }, + ], + "Version": "2012-10-17", + }, + "PolicyName": "ssm-ssh-policy", + "Roles": [ + { + "Ref": "InstanceRoleTestguec2appC325BE42", + }, + ], + }, + "Type": "AWS::IAM::Policy", + }, "TargetGroupTestguec2app9F67D503": { "Properties": { "HealthCheckIntervalSeconds": 10, @@ -975,6 +998,7 @@ exports[`the GuEC2App pattern should produce a functional EC2 app with minimal a "GuEc2App", "GuCertificate", "GuInstanceRole", + "GuSsmSshPolicy", "GuDescribeEC2Policy", "GuLoggingStreamNameParameter", "GuLogShippingPolicy", @@ -1306,20 +1330,6 @@ exports[`the GuEC2App pattern should produce a functional EC2 app with minimal a ], "Version": "2012-10-17", }, - "ManagedPolicyArns": [ - { - "Fn::Join": [ - "", - [ - "arn:", - { - "Ref": "AWS::Partition", - }, - ":iam::aws:policy/AmazonSSMManagedInstanceCore", - ], - ], - }, - ], "Path": "/", "Tags": [ { @@ -1558,6 +1568,42 @@ exports[`the GuEC2App pattern should produce a functional EC2 app with minimal a }, "Type": "AWS::IAM::Policy", }, + "SsmSshPolicy4CFC977E": { + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": [ + "ec2messages:AcknowledgeMessage", + "ec2messages:DeleteMessage", + "ec2messages:FailMessage", + "ec2messages:GetEndpoint", + "ec2messages:GetMessages", + "ec2messages:SendReply", + "ssm:UpdateInstanceInformation", + "ssm:ListInstanceAssociations", + "ssm:DescribeInstanceProperties", + "ssm:DescribeDocumentParameters", + "ssmmessages:CreateControlChannel", + "ssmmessages:CreateDataChannel", + "ssmmessages:OpenControlChannel", + "ssmmessages:OpenDataChannel", + ], + "Effect": "Allow", + "Resource": "*", + }, + ], + "Version": "2012-10-17", + }, + "PolicyName": "ssm-ssh-policy", + "Roles": [ + { + "Ref": "InstanceRoleTestguec2appC325BE42", + }, + ], + }, + "Type": "AWS::IAM::Policy", + }, "TargetGroupTestguec2app9F67D503": { "Properties": { "HealthCheckIntervalSeconds": 10,