From 9399cf29bec963dfa305f367f37c098a76130371 Mon Sep 17 00:00:00 2001 From: Niek Palm Date: Wed, 22 May 2024 14:05:55 +0200 Subject: [PATCH] feat: Restrict instance SSM permissions (#3918) ## Restrict instance SSM permissions Previously, EC2 instances could read other instances' tokens (via path .../tokens/...) from SSM parameters. This PR restricts access to only read / delete tokens owned by the instances Co-authored-by: Blake Burkhart --- .../src/scale-runners/scale-up.test.ts | 30 +++++++++++++++++++ .../src/scale-runners/scale-up.ts | 8 +++-- lambdas/libs/aws-ssm-util/src/index.ts | 10 +++++-- .../instance-ssm-parameters-policy.json | 7 ++++- modules/runners/policies/lambda-scale-up.json | 3 +- .../runners/pool/policies/lambda-pool.json | 1 + 6 files changed, 53 insertions(+), 6 deletions(-) diff --git a/lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts b/lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts index f53a998f01..f14319a3b6 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts @@ -338,6 +338,12 @@ describe('scaleUp with GHES', () => { Name: '/github-action-runners/default/runners/config/i-12345', Value: 'TEST_JIT_CONFIG_ORG', Type: 'SecureString', + Tags: [ + { + Key: 'InstanceId', + Value: 'i-12345', + }, + ], }); }); @@ -353,6 +359,12 @@ describe('scaleUp with GHES', () => { '--url https://github.enterprise.something/Codertocat --token 1234abcd ' + '--labels label1,label2 --runnergroup Default', Type: 'SecureString', + Tags: [ + { + Key: 'InstanceId', + Value: 'i-12345', + }, + ], }); }); it.each(RUNNER_TYPES)( @@ -708,6 +720,12 @@ describe('scaleUp with public GH', () => { Name: '/github-action-runners/default/runners/config/i-12345', Value: 'TEST_JIT_CONFIG_REPO', Type: 'SecureString', + Tags: [ + { + Key: 'InstanceId', + Value: 'i-12345', + }, + ], }); }); @@ -724,6 +742,12 @@ describe('scaleUp with public GH', () => { Name: '/github-action-runners/default/runners/config/i-12345', Value: '--url https://github.com/Codertocat/hello-world --token 1234abcd --ephemeral', Type: 'SecureString', + Tags: [ + { + Key: 'InstanceId', + Value: 'i-12345', + }, + ], }); }); @@ -741,6 +765,12 @@ describe('scaleUp with public GH', () => { Name: '/github-action-runners/default/runners/config/i-12345', Value: '--url https://github.com/Codertocat/hello-world --token 1234abcd --labels jit', Type: 'SecureString', + Tags: [ + { + Key: 'InstanceId', + Value: 'i-12345', + }, + ], }); }); diff --git a/lambdas/functions/control-plane/src/scale-runners/scale-up.ts b/lambdas/functions/control-plane/src/scale-runners/scale-up.ts index 0c184ff673..8f537224d2 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-up.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-up.ts @@ -362,7 +362,9 @@ async function createRegistrationTokenConfig( }); for (const instance of instances) { - await putParameter(`${githubRunnerConfig.ssmTokenPath}/${instance}`, runnerServiceConfig.join(' '), true); + await putParameter(`${githubRunnerConfig.ssmTokenPath}/${instance}`, runnerServiceConfig.join(' '), true, { + tags: [{ Key: 'InstanceId', Value: instance }], + }); if (isDelay) { // Delay to prevent AWS ssm rate limits by being within the max throughput limit await delay(25); @@ -405,7 +407,9 @@ async function createJitConfig(githubRunnerConfig: CreateGitHubRunnerConfig, ins logger.debug('Runner JIT config for ephemeral runner generated.', { instance: instance, }); - await putParameter(`${githubRunnerConfig.ssmTokenPath}/${instance}`, runnerConfig.data.encoded_jit_config, true); + await putParameter(`${githubRunnerConfig.ssmTokenPath}/${instance}`, runnerConfig.data.encoded_jit_config, true, { + tags: [{ Key: 'InstanceId', Value: instance }], + }); if (isDelay) { // Delay to prevent AWS ssm rate limits by being within the max throughput limit await delay(25); diff --git a/lambdas/libs/aws-ssm-util/src/index.ts b/lambdas/libs/aws-ssm-util/src/index.ts index 36d5a6d8e8..3824926d09 100644 --- a/lambdas/libs/aws-ssm-util/src/index.ts +++ b/lambdas/libs/aws-ssm-util/src/index.ts @@ -1,4 +1,4 @@ -import { GetParameterCommand, PutParameterCommand, SSMClient } from '@aws-sdk/client-ssm'; +import { GetParameterCommand, PutParameterCommand, SSMClient, Tag } from '@aws-sdk/client-ssm'; import { getTracedAWSV3Client } from '@terraform-aws-github-runner/aws-powertools-util'; export async function getParameter(parameter_name: string): Promise { @@ -7,13 +7,19 @@ export async function getParameter(parameter_name: string): Promise { ?.Value as string; } -export async function putParameter(parameter_name: string, parameter_value: string, secure: boolean): Promise { +export async function putParameter( + parameter_name: string, + parameter_value: string, + secure: boolean, + options: { tags?: Tag[] } = {}, +): Promise { const client = getTracedAWSV3Client(new SSMClient({ region: process.env.AWS_REGION })); await client.send( new PutParameterCommand({ Name: parameter_name, Value: parameter_value, Type: secure ? 'SecureString' : 'String', + Tags: options.tags, }), ); } diff --git a/modules/runners/policies/instance-ssm-parameters-policy.json b/modules/runners/policies/instance-ssm-parameters-policy.json index 72f8193cc6..fcd82304e1 100644 --- a/modules/runners/policies/instance-ssm-parameters-policy.json +++ b/modules/runners/policies/instance-ssm-parameters-policy.json @@ -8,7 +8,12 @@ "ssm:GetParameters", "ssm:GetParameter" ], - "Resource": "${arn_ssm_parameters_path_tokens}*" + "Resource": "${arn_ssm_parameters_path_tokens}/*", + "Condition": { + "StringLike": { + "ec2:SourceInstanceARN": "*/$${aws:ResourceTag/InstanceId}" + } + } }, { "Effect": "Allow", diff --git a/modules/runners/policies/lambda-scale-up.json b/modules/runners/policies/lambda-scale-up.json index 5892de68c3..1c5e942b2f 100644 --- a/modules/runners/policies/lambda-scale-up.json +++ b/modules/runners/policies/lambda-scale-up.json @@ -22,7 +22,8 @@ { "Effect": "Allow", "Action": [ - "ssm:PutParameter" + "ssm:PutParameter", + "ssm:AddTagsToResource" ], "Resource": "*" }, diff --git a/modules/runners/pool/policies/lambda-pool.json b/modules/runners/pool/policies/lambda-pool.json index 5bd7bb82f7..f8e3f39a23 100644 --- a/modules/runners/pool/policies/lambda-pool.json +++ b/modules/runners/pool/policies/lambda-pool.json @@ -22,6 +22,7 @@ { "Effect": "Allow", "Action": [ + "ssm:AddTagsToResource", "ssm:PutParameter" ], "Resource": "*"