From faf3741976f94b6ab22b24ef19f3be93c6b89a54 Mon Sep 17 00:00:00 2001 From: edersonbrilhante Date: Fri, 4 Sep 2026 12:13:10 +0200 Subject: [PATCH 1/4] refactor(storage): extract GitHub App credentials store --- .../control-plane/src/github/auth.ts | 131 ++++---------- .../src/github/rate-limit.test.ts | 166 +++--------------- .../control-plane/src/github/rate-limit.ts | 17 +- .../aws/ssm/environment.d.ts | 3 + .../ssm/github-app-credentials-store.test.ts | 68 +++++++ .../aws/ssm/github-app-credentials-store.ts | 66 +++++++ lambdas/libs/storage-providers/core/index.ts | 10 ++ .../github-app-credentials.ts | 6 + lambdas/libs/storage-providers/index.ts | 3 + 9 files changed, 217 insertions(+), 253 deletions(-) create mode 100644 lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts create mode 100644 lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts create mode 100644 lambdas/libs/storage-providers/github-app-credentials.ts diff --git a/lambdas/functions/control-plane/src/github/auth.ts b/lambdas/functions/control-plane/src/github/auth.ts index f64ac00b30..3fee4d7f5b 100644 --- a/lambdas/functions/control-plane/src/github/auth.ts +++ b/lambdas/functions/control-plane/src/github/auth.ts @@ -1,11 +1,17 @@ import { createAppAuth, type AppAuthentication, type InstallationAccessTokenAuthentication } from '@octokit/auth-app'; import type { OctokitOptions, Octokit as CoreOctokit } from '@octokit/core'; import type { RequestInterface } from '@octokit/types'; +import { createSign, randomUUID } from 'node:crypto'; +import { request } from '@octokit/request'; +import { Octokit } from '@octokit/rest'; +import { retry } from '@octokit/plugin-retry'; +import { throttling } from '@octokit/plugin-throttling'; +import { createChildLogger } from '@aws-github-runner/aws-powertools-util'; +import { createGitHubAppCredentialsStore, type GitHubAppCredential } from '@aws-github-runner/storage-providers'; +import { EndpointDefaults } from '@octokit/types'; -// Define types that are not directly exported type AppAuthOptions = { type: 'app' }; type InstallationAuthOptions = { type: 'installation'; installationId?: number }; -// Use a more generalized AuthInterface to match what createAppAuth returns type AuthInterface = { (options: AppAuthOptions): Promise; (options: InstallationAuthOptions): Promise; @@ -16,33 +22,14 @@ type StrategyOptions = { installationId?: number; request?: RequestInterface; }; -import { createSign, randomUUID } from 'node:crypto'; -import { request } from '@octokit/request'; -import { Octokit } from '@octokit/rest'; -import { retry } from '@octokit/plugin-retry'; -import { throttling } from '@octokit/plugin-throttling'; -import { createChildLogger } from '@aws-github-runner/aws-powertools-util'; -import { getParameters } from '@aws-github-runner/aws-ssm-util'; -import { EndpointDefaults } from '@octokit/types'; const logger = createChildLogger('gh-auth'); - -// Retry caps for the throttling plugin. Returning `true` from a limit handler tells -// the plugin to retry after the interval GitHub asked for; returning `false` gives up. -// Primary rate limits reset on a fixed schedule, so a couple of retries is worthwhile. -// Secondary rate limits are abuse-detection signals — retry once, then back off and -// let the message return to the queue rather than pushing harder. const MAX_RATE_LIMIT_RETRIES = 2; const MAX_SECONDARY_RATE_LIMIT_RETRIES = 1; -// Exported for tests: the plugin only surfaces these via the client constructor, -// so there is no other seam to assert the retry cap against. export function onRateLimit( retryAfter: number, options: Required, - // The throttling plugin types this as @octokit/core's Octokit, not the wider - // @octokit/rest one imported above; matching it keeps the handler assignable to - // the plugin's LimitHandler. Unused here regardless. _octokit: CoreOctokit, retryCount: number, ): boolean { @@ -56,9 +43,6 @@ export function onRateLimit( export function onSecondaryRateLimit( retryAfter: number, options: Required, - // The throttling plugin types this as @octokit/core's Octokit, not the wider - // @octokit/rest one imported above; matching it keeps the handler assignable to - // the plugin's LimitHandler. Unused here regardless. _octokit: CoreOctokit, retryCount: number, ): boolean { @@ -69,52 +53,10 @@ export function onSecondaryRateLimit( return retryCount < MAX_SECONDARY_RATE_LIMIT_RETRIES; } -interface GitHubAppCredential { - appId: number; - privateKey: string; - installationId?: number; -} - let appCredentialsPromise: Promise | null = null; async function loadAppCredentials(): Promise { - if (!process.env.PARAMETER_GITHUB_APP_ID_NAME) { - throw new Error('Environment variable PARAMETER_GITHUB_APP_ID_NAME is not set'); - } - if (!process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME) { - throw new Error('Environment variable PARAMETER_GITHUB_APP_KEY_BASE64_NAME is not set'); - } - const idParams = process.env.PARAMETER_GITHUB_APP_ID_NAME.split(':').filter(Boolean); - const keyParams = process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME.split(':').filter(Boolean); - const installationIdParams = (process.env.PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME || '').split(':'); - if (idParams.length !== keyParams.length) { - throw new Error(`GitHub App parameter count mismatch: ${idParams.length} IDs vs ${keyParams.length} keys`); - } - // Batch fetch all SSM parameters in a single call to reduce API calls - const allParamNames = [...idParams, ...keyParams, ...installationIdParams.filter((p) => p.length > 0)]; - const params = await getParameters(allParamNames); - - const credentials: GitHubAppCredential[] = []; - for (let i = 0; i < idParams.length; i++) { - const appIdValue = params.get(idParams[i]); - if (!appIdValue) { - throw new Error(`Parameter ${idParams[i]} not found`); - } - const appId = parseInt(appIdValue, 10); - const privateKeyBase64 = params.get(keyParams[i]); - if (!privateKeyBase64) { - throw new Error(`Parameter ${keyParams[i]} not found`); - } - // replace literal \n characters with new lines to allow the key to be stored as a - // single line variable. This logic should match how the GitHub Terraform provider - // processes private keys to retain compatibility between the projects - const privateKey = Buffer.from(privateKeyBase64, 'base64').toString().replace(/\\n/g, '\n'); - const installationIdParam = installationIdParams[i]; - const installationIdValue = - installationIdParam && installationIdParam.length > 0 ? params.get(installationIdParam) : undefined; - const installationId = installationIdValue ? parseInt(installationIdValue, 10) : undefined; - credentials.push({ appId, privateKey, installationId }); - } + const credentials = await createGitHubAppCredentialsStore().get(); logger.info(`Loaded ${credentials.length} GitHub App credential(s)`); return credentials; } @@ -137,34 +79,37 @@ export async function getStoredInstallationId(appIndex: number): Promise { + const credential = (await getAppCredentials())[appIndex]; + if (!credential) { + throw new Error(`GitHub App credential at index ${appIndex} not found`); + } + return credential.appId.toString(); +} + export async function createOctokitClient(token: string, ghesApiUrl = ''): Promise { const CustomOctokit = Octokit.plugin(retry, throttling); - const ocktokitOptions: OctokitOptions = { - auth: token, - }; + const octokitOptions: OctokitOptions = { auth: token }; if (ghesApiUrl) { - ocktokitOptions.baseUrl = ghesApiUrl; - ocktokitOptions.previews = ['antiope']; + octokitOptions.baseUrl = ghesApiUrl; + octokitOptions.previews = ['antiope']; } return new CustomOctokit({ - ...ocktokitOptions, + ...octokitOptions, userAgent: process.env.USER_AGENT || 'github-aws-runners', retry: { - onRetry: (retryCount: number, error: Error, request: { method: string; url: string }) => { + onRetry: (retryCount: number, error: Error, retryRequest: { method: string; url: string }) => { logger.warn('GitHub API request retry attempt', { retryCount, - method: request.method, - url: request.url, + method: retryRequest.method, + url: retryRequest.url, error: error.message, status: (error as Error & { status?: number }).status, }); }, }, - throttle: { - onRateLimit, - onSecondaryRateLimit, - }, + throttle: { onRateLimit, onSecondaryRateLimit }, }); } @@ -176,8 +121,7 @@ export async function createGithubAppAuth( const credentials = await getAppCredentials(); const idx = appIndex ?? Math.floor(Math.random() * credentials.length); const auth = await createAuth(installationId, ghesApiUrl, idx); - const result = await auth({ type: 'app' }); - return { ...result, appIndex: idx }; + return { ...(await auth({ type: 'app' })), appIndex: idx }; } export async function createGithubInstallationAuth( @@ -199,21 +143,15 @@ function signJwt(payload: Record, privateKey: string): string { return `${message}.${signature}`; } -async function createAuth( - installationId: number | undefined, - ghesApiUrl: string, - appIndex?: number, -): Promise { +async function createAuth(installationId: number | undefined, ghesApiUrl: string, appIndex?: number): Promise { const credentials = await getAppCredentials(); const selected = appIndex !== undefined ? credentials[appIndex] : credentials[Math.floor(Math.random() * credentials.length)]; + if (!selected) { + throw new Error(`GitHub App credential at index ${appIndex ?? 0} not found`); + } logger.debug(`Selected GitHub App ${selected.appId} for authentication`); - - // Use a custom createJwt callback to include a jti (JWT ID) claim in every token. - // Without this, concurrent Lambda invocations generating JWTs within the same second - // produce byte-identical tokens (same iat, exp, iss), which GitHub rejects as duplicates. - // See: https://github.com/github-aws-runners/terraform-aws-github-runner/issues/5025 const createJwt = async (appId: string | number, timeDifference?: number) => { const now = Math.floor(Date.now() / 1000) + (timeDifference ?? 0); const iat = now - 30; @@ -222,14 +160,9 @@ async function createAuth( return { jwt, expiresAt: new Date(exp * 1000).toISOString() }; }; - let authOptions: StrategyOptions = { appId: selected.appId, createJwt }; - if (installationId) authOptions = { ...authOptions, installationId }; - - logger.debug(`GHES API URL: ${ghesApiUrl}`); + const authOptions: StrategyOptions = { appId: selected.appId, createJwt, ...(installationId ? { installationId } : {}) }; if (ghesApiUrl) { - authOptions.request = request.defaults({ - baseUrl: ghesApiUrl, - }); + authOptions.request = request.defaults({ baseUrl: ghesApiUrl }); } return createAppAuth(authOptions); } diff --git a/lambdas/functions/control-plane/src/github/rate-limit.test.ts b/lambdas/functions/control-plane/src/github/rate-limit.test.ts index d9d18c5921..40c83ff6a3 100644 --- a/lambdas/functions/control-plane/src/github/rate-limit.test.ts +++ b/lambdas/functions/control-plane/src/github/rate-limit.test.ts @@ -1,174 +1,64 @@ import { ResponseHeaders } from '@octokit/types'; import { createSingleMetric } from '@aws-github-runner/aws-powertools-util'; import { MetricUnit } from '@aws-lambda-powertools/metrics'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { getAppId } from './auth'; import { metricGitHubAppRateLimit } from './rate-limit'; -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { getParameter } from '@aws-github-runner/aws-ssm-util'; -process.env.PARAMETER_GITHUB_APP_ID_NAME = 'test'; -vi.mock('@aws-github-runner/aws-ssm-util', async () => { - // Return only what we need without spreading actual - return { - getParameter: vi.fn((name: string) => { - if (name === process.env.PARAMETER_GITHUB_APP_ID_NAME) { - return '1234'; - } else { - return ''; - } - }), - }; -}); +vi.mock('./auth', () => ({ + getAppId: vi.fn(), +})); +vi.mock('@aws-github-runner/aws-powertools-util', () => ({ + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + createSingleMetric: vi.fn(() => ({ addMetadata: vi.fn() })), +})); -vi.mock('@aws-github-runner/aws-powertools-util', async () => { - // Provide only what's needed without spreading actual - return { - // Mock the logger - logger: { - debug: vi.fn(), - info: vi.fn(), - warn: vi.fn(), - error: vi.fn(), - }, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - createSingleMetric: vi.fn((name: string, unit: string, value: number, dimensions?: Record) => { - return { - addMetadata: vi.fn(), - }; - }), - }; -}); +const mockedGetAppId = vi.mocked(getAppId); describe('metricGitHubAppRateLimit', () => { beforeEach(() => { vi.clearAllMocks(); + mockedGetAppId.mockResolvedValue('1234'); + process.env.ENABLE_METRIC_GITHUB_APP_RATE_LIMIT = 'true'; }); - it('should update rate limit metric', async () => { - // set process.env.ENABLE_METRIC_GITHUB_APP_RATE_LIMIT to true - process.env.ENABLE_METRIC_GITHUB_APP_RATE_LIMIT = 'true'; + it('updates the rate limit metric using the selected app credential', async () => { const headers: ResponseHeaders = { 'x-ratelimit-remaining': '10', 'x-ratelimit-limit': '60', }; - await metricGitHubAppRateLimit(headers); + await metricGitHubAppRateLimit(headers, 1); + expect(mockedGetAppId).toHaveBeenCalledWith(1); expect(createSingleMetric).toHaveBeenCalledWith('GitHubAppRateLimitRemaining', MetricUnit.Count, 10, { AppId: '1234', }); }); - it('should not update rate limit metric', async () => { - // set process.env.ENABLE_METRIC_GITHUB_APP_RATE_LIMIT to false + it('does not update the metric when disabled', async () => { process.env.ENABLE_METRIC_GITHUB_APP_RATE_LIMIT = 'false'; - const headers: ResponseHeaders = { - 'x-ratelimit-remaining': '10', - 'x-ratelimit-limit': '60', - }; - await metricGitHubAppRateLimit(headers); + await metricGitHubAppRateLimit({ 'x-ratelimit-remaining': '10', 'x-ratelimit-limit': '60' }); expect(createSingleMetric).not.toHaveBeenCalled(); + expect(mockedGetAppId).not.toHaveBeenCalled(); }); - it('should not update rate limit metric if headers are undefined', async () => { - // set process.env.ENABLE_METRIC_GITHUB_APP_RATE_LIMIT to true - process.env.ENABLE_METRIC_GITHUB_APP_RATE_LIMIT = 'true'; - - await metricGitHubAppRateLimit(undefined as unknown as ResponseHeaders); - + it('does not throw when headers are unavailable', async () => { + await expect(metricGitHubAppRateLimit(undefined as unknown as ResponseHeaders)).resolves.not.toThrow(); expect(createSingleMetric).not.toHaveBeenCalled(); }); - it('should cache GitHub App ID and only call getParameter once', async () => { - // Reset modules to clear the appIdPromises Map cache - vi.resetModules(); - const { metricGitHubAppRateLimit: freshMetricFunction } = await import('./rate-limit'); - - process.env.ENABLE_METRIC_GITHUB_APP_RATE_LIMIT = 'true'; - const headers: ResponseHeaders = { - 'x-ratelimit-remaining': '10', - 'x-ratelimit-limit': '60', - }; - - const mockGetParameter = vi.mocked(getParameter); - mockGetParameter.mockClear(); - - await freshMetricFunction(headers); - await freshMetricFunction(headers); - await freshMetricFunction(headers); - - // getParameter should only be called once due to caching (index 0 cached after first call) - expect(mockGetParameter).toHaveBeenCalledTimes(1); - // split(':')[0] of 'test' is still 'test' - expect(mockGetParameter).toHaveBeenCalledWith(process.env.PARAMETER_GITHUB_APP_ID_NAME); - }); -}); - -describe('metricGitHubAppRateLimit multi-app', () => { - let freshMetricFunction: typeof metricGitHubAppRateLimit; - let mockGetParam: ReturnType; - - beforeEach(async () => { - // Reset modules to get a clean appIdPromises Map for each test - vi.resetModules(); - - process.env.PARAMETER_GITHUB_APP_ID_NAME = 'app0:app1'; - process.env.ENABLE_METRIC_GITHUB_APP_RATE_LIMIT = 'true'; - - mockGetParam = vi.fn((name: string) => { - if (name === 'app0') return Promise.resolve('1234'); - if (name === 'app1') return Promise.resolve('5678'); - return Promise.resolve(''); - }); - - vi.doMock('@aws-github-runner/aws-ssm-util', () => ({ getParameter: mockGetParam })); - vi.doMock('@aws-github-runner/aws-powertools-util', () => ({ - logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, - createSingleMetric: vi.fn(() => ({ addMetadata: vi.fn() })), - })); - - const mod = await import('./rate-limit'); - freshMetricFunction = mod.metricGitHubAppRateLimit; - }); - - afterEach(() => { - vi.resetModules(); - process.env.PARAMETER_GITHUB_APP_ID_NAME = 'test'; - }); - - it('should label metric with correct appId for index 0 (primary app)', async () => { - const { createSingleMetric: mockMetric } = await import('@aws-github-runner/aws-powertools-util'); - const headers: ResponseHeaders = { 'x-ratelimit-remaining': '50', 'x-ratelimit-limit': '5000' }; - await freshMetricFunction(headers, 0); - expect(mockMetric).toHaveBeenCalledWith('GitHubAppRateLimitRemaining', MetricUnit.Count, 50, { AppId: '1234' }); - }); - - it('should label metric with correct appId for index 1 (additional app)', async () => { - const { createSingleMetric: mockMetric } = await import('@aws-github-runner/aws-powertools-util'); - const headers: ResponseHeaders = { 'x-ratelimit-remaining': '100', 'x-ratelimit-limit': '5000' }; - await freshMetricFunction(headers, 1); - expect(mockMetric).toHaveBeenCalledWith('GitHubAppRateLimitRemaining', MetricUnit.Count, 100, { AppId: '5678' }); - }); - - it('should default to index 0 when no appIndex is passed', async () => { - const { createSingleMetric: mockMetric } = await import('@aws-github-runner/aws-powertools-util'); - const headers: ResponseHeaders = { 'x-ratelimit-remaining': '75', 'x-ratelimit-limit': '5000' }; - await freshMetricFunction(headers); - expect(mockMetric).toHaveBeenCalledWith('GitHubAppRateLimitRemaining', MetricUnit.Count, 75, { AppId: '1234' }); - }); - - it('should cache per index and call getParameter separately for each index', async () => { - const headers: ResponseHeaders = { 'x-ratelimit-remaining': '10', 'x-ratelimit-limit': '5000' }; + it('passes each app index to the credential seam', async () => { + mockedGetAppId.mockImplementation(async (appIndex = 0) => String(1000 + appIndex)); + const headers: ResponseHeaders = { 'x-ratelimit-remaining': '10', 'x-ratelimit-limit': '60' }; - // Two calls with index 1, then one with index 0 - await freshMetricFunction(headers, 1); - await freshMetricFunction(headers, 1); - await freshMetricFunction(headers, 0); + await metricGitHubAppRateLimit(headers, 0); + await metricGitHubAppRateLimit(headers, 1); - // getParameter should be called exactly once per distinct index - expect(mockGetParam).toHaveBeenCalledTimes(2); - expect(mockGetParam).toHaveBeenCalledWith('app1'); - expect(mockGetParam).toHaveBeenCalledWith('app0'); + expect(mockedGetAppId).toHaveBeenNthCalledWith(1, 0); + expect(mockedGetAppId).toHaveBeenNthCalledWith(2, 1); }); }); diff --git a/lambdas/functions/control-plane/src/github/rate-limit.ts b/lambdas/functions/control-plane/src/github/rate-limit.ts index df2372a255..710d7cf80a 100644 --- a/lambdas/functions/control-plane/src/github/rate-limit.ts +++ b/lambdas/functions/control-plane/src/github/rate-limit.ts @@ -2,22 +2,7 @@ import { ResponseHeaders } from '@octokit/types'; import { createSingleMetric, logger } from '@aws-github-runner/aws-powertools-util'; import { MetricUnit } from '@aws-lambda-powertools/metrics'; import yn from 'yn'; -import { getParameter } from '@aws-github-runner/aws-ssm-util'; - -// Cache the app ID per app index to avoid repeated SSM calls across Lambda invocations. -// In multi-app mode PARAMETER_GITHUB_APP_ID_NAME is a ':'-joined list of SSM param names, -// one per app in app-index order; index 0 is the primary app. -const appIdPromises = new Map>(); - -async function getAppId(appIndex = 0): Promise { - let cached = appIdPromises.get(appIndex); - if (!cached) { - const paramName = process.env.PARAMETER_GITHUB_APP_ID_NAME.split(':')[appIndex]; - cached = getParameter(paramName); - appIdPromises.set(appIndex, cached); - } - return cached; -} +import { getAppId } from './auth'; export async function metricGitHubAppRateLimit(headers: ResponseHeaders, appIndex?: number): Promise { try { diff --git a/lambdas/libs/storage-providers/aws/ssm/environment.d.ts b/lambdas/libs/storage-providers/aws/ssm/environment.d.ts index c6dd725742..dc236ceedc 100644 --- a/lambdas/libs/storage-providers/aws/ssm/environment.d.ts +++ b/lambdas/libs/storage-providers/aws/ssm/environment.d.ts @@ -5,6 +5,9 @@ declare global { interface ProcessEnv { SSM_PARAMETER_STORE_TAGS?: string; SSM_TOKEN_PATH?: string; + PARAMETER_GITHUB_APP_ID_NAME?: string; + PARAMETER_GITHUB_APP_KEY_BASE64_NAME?: string; + PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME?: string; } } } diff --git a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts new file mode 100644 index 0000000000..1ea3a3d95c --- /dev/null +++ b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts @@ -0,0 +1,68 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { getParameters } from '@aws-github-runner/aws-ssm-util'; + +import { createAwsSsmGitHubAppCredentialsStore } from './github-app-credentials-store'; + +vi.mock('@aws-github-runner/aws-ssm-util', () => ({ + getParameters: vi.fn(), +})); + +const getParametersMock = vi.mocked(getParameters); + +describe('aws_ssm GitHub App credentials store', () => { + beforeEach(() => { + vi.clearAllMocks(); + process.env.PARAMETER_GITHUB_APP_ID_NAME = 'app-id'; + process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME = 'app-key'; + delete process.env.PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME; + }); + + it('loads batched credentials and decodes escaped newlines', async () => { + const privateKey = Buffer.from('private-key\\nline-2').toString('base64'); + getParametersMock.mockResolvedValue( + new Map([ + ['app-id', '123'], + ['app-key', privateKey], + ]), + ); + + await expect(createAwsSsmGitHubAppCredentialsStore().get()).resolves.toEqual([ + { appId: 123, privateKey: 'private-key\nline-2', installationId: undefined }, + ]); + expect(getParametersMock).toHaveBeenCalledWith(['app-id', 'app-key']); + }); + + it('loads per-app installation IDs in the same order as app IDs', async () => { + process.env.PARAMETER_GITHUB_APP_ID_NAME = 'id-0:id-1'; + process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME = 'key-0:key-1'; + process.env.PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME = ':installation-1'; + getParametersMock.mockResolvedValue( + new Map([ + ['id-0', '123'], + ['id-1', '456'], + ['key-0', Buffer.from('key-0').toString('base64')], + ['key-1', Buffer.from('key-1').toString('base64')], + ['installation-1', '789'], + ]), + ); + + await expect(createAwsSsmGitHubAppCredentialsStore().get()).resolves.toMatchObject([ + { appId: 123, installationId: undefined }, + { appId: 456, installationId: 789 }, + ]); + }); + + it.each(['PARAMETER_GITHUB_APP_ID_NAME', 'PARAMETER_GITHUB_APP_KEY_BASE64_NAME'])( + 'requires %s', + (name) => { + delete process.env[name]; + expect(() => createAwsSsmGitHubAppCredentialsStore()).toThrow(`Environment variable ${name} is not set`); + }, + ); + + it('rejects mismatched app and key parameter lists', () => { + process.env.PARAMETER_GITHUB_APP_ID_NAME = 'id-0:id-1'; + expect(() => createAwsSsmGitHubAppCredentialsStore()).toThrow('parameter count mismatch'); + }); +}); diff --git a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts new file mode 100644 index 0000000000..7afaf32175 --- /dev/null +++ b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts @@ -0,0 +1,66 @@ +import { getParameters } from '@aws-github-runner/aws-ssm-util'; + +import type { GitHubAppCredential, GitHubAppCredentialsStore } from '../../core'; + +interface AwsSsmGitHubAppCredentialsEnvironment { + PARAMETER_GITHUB_APP_ID_NAME?: string; + PARAMETER_GITHUB_APP_KEY_BASE64_NAME?: string; + PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME?: string; +} + +export function createAwsSsmGitHubAppCredentialsStore( + environment: Readonly = process.env, +): GitHubAppCredentialsStore { + const idParameters = splitParameterNames(environment.PARAMETER_GITHUB_APP_ID_NAME, 'PARAMETER_GITHUB_APP_ID_NAME'); + const keyParameters = splitParameterNames( + environment.PARAMETER_GITHUB_APP_KEY_BASE64_NAME, + 'PARAMETER_GITHUB_APP_KEY_BASE64_NAME', + ); + const installationIdParameters = environment.PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME?.split(':').filter(Boolean) ?? []; + + if (idParameters.length !== keyParameters.length) { + throw new Error(`GitHub App parameter count mismatch: ${idParameters.length} IDs vs ${keyParameters.length} keys`); + } + + return new AwsSsmGitHubAppCredentialsStore(idParameters, keyParameters, installationIdParameters); +} + +class AwsSsmGitHubAppCredentialsStore implements GitHubAppCredentialsStore { + constructor( + private readonly idParameters: string[], + private readonly keyParameters: string[], + private readonly installationIdParameters: string[], + ) {} + + async get(): Promise { + const parameters = await getParameters([ + ...this.idParameters, + ...this.keyParameters, + ...this.installationIdParameters, + ]); + return this.idParameters.map((idParameter, index) => { + const appIdValue = parameters.get(idParameter); + if (!appIdValue) { + throw new Error(`Parameter ${idParameter} not found`); + } + const privateKeyBase64 = parameters.get(this.keyParameters[index]); + if (!privateKeyBase64) { + throw new Error(`Parameter ${this.keyParameters[index]} not found`); + } + const installationIdParameter = this.installationIdParameters[index]; + const installationIdValue = installationIdParameter ? parameters.get(installationIdParameter) : undefined; + return { + appId: Number.parseInt(appIdValue, 10), + privateKey: Buffer.from(privateKeyBase64, 'base64').toString().replace(/\\n/g, '\n'), + installationId: installationIdValue ? Number.parseInt(installationIdValue, 10) : undefined, + }; + }); + } +} + +function splitParameterNames(value: string | undefined, name: string): string[] { + if (!value || value.trim() === '') { + throw new Error(`Environment variable ${name} is not set`); + } + return value.split(':').filter(Boolean); +} diff --git a/lambdas/libs/storage-providers/core/index.ts b/lambdas/libs/storage-providers/core/index.ts index a408097ed7..542b779cfa 100644 --- a/lambdas/libs/storage-providers/core/index.ts +++ b/lambdas/libs/storage-providers/core/index.ts @@ -17,6 +17,16 @@ export interface RunnerConfigHousekeeper { houseKeeper(): Promise; } +export interface GitHubAppCredential { + appId: number; + privateKey: string; + installationId?: number; +} + +export interface GitHubAppCredentialsStore { + get(): Promise; +} + export interface RunnerGroupCacheRecord { runnerGroupName: string; runnerGroupId: number; diff --git a/lambdas/libs/storage-providers/github-app-credentials.ts b/lambdas/libs/storage-providers/github-app-credentials.ts new file mode 100644 index 0000000000..fe89840455 --- /dev/null +++ b/lambdas/libs/storage-providers/github-app-credentials.ts @@ -0,0 +1,6 @@ +import { createAwsSsmGitHubAppCredentialsStore } from './aws/ssm/github-app-credentials-store'; +import type { GitHubAppCredentialsStore } from './core'; + +export function createGitHubAppCredentialsStore(): GitHubAppCredentialsStore { + return createAwsSsmGitHubAppCredentialsStore(); +} diff --git a/lambdas/libs/storage-providers/index.ts b/lambdas/libs/storage-providers/index.ts index 0e26830879..2045ddbbde 100644 --- a/lambdas/libs/storage-providers/index.ts +++ b/lambdas/libs/storage-providers/index.ts @@ -5,7 +5,10 @@ export type { RunnerConfigStore, RunnerGroupCacheRecord, RunnerGroupCacheStore, + GitHubAppCredential, + GitHubAppCredentialsStore, } from './core'; export { createRunnerConfigStore } from './runner-config'; export { createRunnerConfigHousekeeper } from './runner-config-housekeeper'; export { createRunnerGroupCacheStore } from './runner-group-cache'; +export { createGitHubAppCredentialsStore } from './github-app-credentials'; From 9e51faf0175d87f1dc2d94d947dde12de86afdce Mon Sep 17 00:00:00 2001 From: edersonbrilhante Date: Fri, 4 Sep 2026 14:14:28 +0200 Subject: [PATCH 2/4] fix(auth): update credentials store test expectations --- .../control-plane/src/pool/pool.test.ts | 2 +- .../src/scale-runners/scale-up.test.ts | 30 +++++++++++++------ 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/lambdas/functions/control-plane/src/pool/pool.test.ts b/lambdas/functions/control-plane/src/pool/pool.test.ts index 8372ab6403..a619a5fddc 100644 --- a/lambdas/functions/control-plane/src/pool/pool.test.ts +++ b/lambdas/functions/control-plane/src/pool/pool.test.ts @@ -325,7 +325,7 @@ describe('pool adjustment', () => { await adjust({ poolSize: 3 }); - expect(mockedInstallationAuth).toHaveBeenCalledWith(expect.any(Number), expect.any(String), 1); + expect(mockedInstallationAuth).toHaveBeenCalledWith(expect.any(Number), expect.any(String), 1, expect.anything()); }); it('looks up installationId using the selected app JWT', async () => { 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 1ca7536b2a..d406c7c5f5 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 @@ -1033,8 +1033,18 @@ describe('scaleUp with GHES', () => { await scaleUpModule.scaleUp(messages); expect(mockCreateClient).toHaveBeenCalledTimes(3); // 1 app client, 2 repo installation clients - expect(mockedInstallationAuth).toHaveBeenCalledWith(100, 'https://github.enterprise.something/api/v3', 0); - expect(mockedInstallationAuth).toHaveBeenCalledWith(200, 'https://github.enterprise.something/api/v3', 0); + expect(mockedInstallationAuth).toHaveBeenCalledWith( + 100, + 'https://github.enterprise.something/api/v3', + 0, + expect.anything(), + ); + expect(mockedInstallationAuth).toHaveBeenCalledWith( + 200, + 'https://github.enterprise.something/api/v3', + 0, + expect.anything(), + ); }); it('Should reuse GitHub clients for same installation', async () => { @@ -1467,8 +1477,8 @@ describe('scaleUp with public GH', () => { await scaleUpModule.scaleUp(messages); expect(mockCreateClient).toHaveBeenCalledTimes(3); // 1 app client, 2 repo installation clients - expect(mockedInstallationAuth).toHaveBeenCalledWith(100, '', 0); - expect(mockedInstallationAuth).toHaveBeenCalledWith(200, '', 0); + expect(mockedInstallationAuth).toHaveBeenCalledWith(100, '', 0, expect.anything()); + expect(mockedInstallationAuth).toHaveBeenCalledWith(200, '', 0, expect.anything()); }); it('Should reuse GitHub clients for same installation', async () => { @@ -1945,8 +1955,8 @@ describe('scaleUp with Github Data Residency', () => { await scaleUpModule.scaleUp(messages); expect(mockCreateClient).toHaveBeenCalledTimes(3); // 1 app client, 2 repo installation clients - expect(mockedInstallationAuth).toHaveBeenCalledWith(100, '', 0); - expect(mockedInstallationAuth).toHaveBeenCalledWith(200, '', 0); + expect(mockedInstallationAuth).toHaveBeenCalledWith(100, '', 0, expect.anything()); + expect(mockedInstallationAuth).toHaveBeenCalledWith(200, '', 0, expect.anything()); }); it('Should reuse GitHub clients for same installation', async () => { @@ -2209,6 +2219,7 @@ describe('Multi-app round-robin', () => { expect.any(Number), expect.any(String), 1, // appIndex must match the one from createGithubAppAuth + expect.anything(), ); }); @@ -2234,6 +2245,7 @@ describe('Multi-app round-robin', () => { TEST_DATA_SINGLE.installationId, // from mockOctokit.apps.getOrgInstallation mock expect.any(String), 1, + expect.anything(), ); }); @@ -2251,7 +2263,7 @@ describe('Multi-app round-robin', () => { // Should use 999 from webhook directly — no API lookup expect(mockOctokit.apps.getOrgInstallation).not.toHaveBeenCalled(); - expect(mockedInstallationAuth).toHaveBeenCalledWith(999, expect.any(String), 0); + expect(mockedInstallationAuth).toHaveBeenCalledWith(999, expect.any(String), 0, expect.anything()); }); it('primary app (appIndex 0) reuses webhook installationId even in multi-app deployment', async () => { @@ -2270,7 +2282,7 @@ describe('Multi-app round-robin', () => { // Primary app must NOT do an API lookup — reuses webhook installationId expect(mockOctokit.apps.getOrgInstallation).not.toHaveBeenCalled(); - expect(mockedInstallationAuth).toHaveBeenCalledWith(999, expect.any(String), 0); + expect(mockedInstallationAuth).toHaveBeenCalledWith(999, expect.any(String), 0, expect.anything()); }); it('stored installationId takes precedence over webhook payload for additional app', async () => { @@ -2289,7 +2301,7 @@ describe('Multi-app round-robin', () => { // Stored id (77) wins — no API lookup needed expect(mockOctokit.apps.getOrgInstallation).not.toHaveBeenCalled(); - expect(mockedInstallationAuth).toHaveBeenCalledWith(77, expect.any(String), 1); + expect(mockedInstallationAuth).toHaveBeenCalledWith(77, expect.any(String), 1, expect.anything()); }); }); From a1c144fd3030416c1e39a57da29840af7c9659dd Mon Sep 17 00:00:00 2001 From: edersonbrilhante Date: Fri, 4 Sep 2026 14:25:44 +0200 Subject: [PATCH 3/4] fix(format): format GitHub App storage changes --- lambdas/functions/control-plane/src/github/auth.ts | 12 ++++++++++-- .../aws/ssm/github-app-credentials-store.test.ts | 11 ++++------- .../aws/ssm/github-app-credentials-store.ts | 3 ++- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/lambdas/functions/control-plane/src/github/auth.ts b/lambdas/functions/control-plane/src/github/auth.ts index 3fee4d7f5b..a70d9402cf 100644 --- a/lambdas/functions/control-plane/src/github/auth.ts +++ b/lambdas/functions/control-plane/src/github/auth.ts @@ -143,7 +143,11 @@ function signJwt(payload: Record, privateKey: string): string { return `${message}.${signature}`; } -async function createAuth(installationId: number | undefined, ghesApiUrl: string, appIndex?: number): Promise { +async function createAuth( + installationId: number | undefined, + ghesApiUrl: string, + appIndex?: number, +): Promise { const credentials = await getAppCredentials(); const selected = appIndex !== undefined ? credentials[appIndex] : credentials[Math.floor(Math.random() * credentials.length)]; @@ -160,7 +164,11 @@ async function createAuth(installationId: number | undefined, ghesApiUrl: string return { jwt, expiresAt: new Date(exp * 1000).toISOString() }; }; - const authOptions: StrategyOptions = { appId: selected.appId, createJwt, ...(installationId ? { installationId } : {}) }; + const authOptions: StrategyOptions = { + appId: selected.appId, + createJwt, + ...(installationId ? { installationId } : {}), + }; if (ghesApiUrl) { authOptions.request = request.defaults({ baseUrl: ghesApiUrl }); } diff --git a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts index 1ea3a3d95c..b028a4c098 100644 --- a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts +++ b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.test.ts @@ -53,13 +53,10 @@ describe('aws_ssm GitHub App credentials store', () => { ]); }); - it.each(['PARAMETER_GITHUB_APP_ID_NAME', 'PARAMETER_GITHUB_APP_KEY_BASE64_NAME'])( - 'requires %s', - (name) => { - delete process.env[name]; - expect(() => createAwsSsmGitHubAppCredentialsStore()).toThrow(`Environment variable ${name} is not set`); - }, - ); + it.each(['PARAMETER_GITHUB_APP_ID_NAME', 'PARAMETER_GITHUB_APP_KEY_BASE64_NAME'])('requires %s', (name) => { + delete process.env[name]; + expect(() => createAwsSsmGitHubAppCredentialsStore()).toThrow(`Environment variable ${name} is not set`); + }); it('rejects mismatched app and key parameter lists', () => { process.env.PARAMETER_GITHUB_APP_ID_NAME = 'id-0:id-1'; diff --git a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts index 7afaf32175..1a3dcff45b 100644 --- a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts +++ b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts @@ -16,7 +16,8 @@ export function createAwsSsmGitHubAppCredentialsStore( environment.PARAMETER_GITHUB_APP_KEY_BASE64_NAME, 'PARAMETER_GITHUB_APP_KEY_BASE64_NAME', ); - const installationIdParameters = environment.PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME?.split(':').filter(Boolean) ?? []; + const installationIdParameters = + environment.PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME?.split(':').filter(Boolean) ?? []; if (idParameters.length !== keyParameters.length) { throw new Error(`GitHub App parameter count mismatch: ${idParameters.length} IDs vs ${keyParameters.length} keys`); From 4ecd082496c196a7d20827214dee19d34146232e Mon Sep 17 00:00:00 2001 From: edersonbrilhante Date: Fri, 4 Sep 2026 14:30:39 +0200 Subject: [PATCH 4/4] fix(storage): preserve app installation ID slots --- .../control-plane/src/pool/pool.test.ts | 2 +- .../src/scale-runners/scale-up.test.ts | 30 ++++++------------- .../aws/ssm/github-app-credentials-store.ts | 5 ++-- 3 files changed, 12 insertions(+), 25 deletions(-) diff --git a/lambdas/functions/control-plane/src/pool/pool.test.ts b/lambdas/functions/control-plane/src/pool/pool.test.ts index a619a5fddc..8372ab6403 100644 --- a/lambdas/functions/control-plane/src/pool/pool.test.ts +++ b/lambdas/functions/control-plane/src/pool/pool.test.ts @@ -325,7 +325,7 @@ describe('pool adjustment', () => { await adjust({ poolSize: 3 }); - expect(mockedInstallationAuth).toHaveBeenCalledWith(expect.any(Number), expect.any(String), 1, expect.anything()); + expect(mockedInstallationAuth).toHaveBeenCalledWith(expect.any(Number), expect.any(String), 1); }); it('looks up installationId using the selected app JWT', async () => { 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 d406c7c5f5..1ca7536b2a 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 @@ -1033,18 +1033,8 @@ describe('scaleUp with GHES', () => { await scaleUpModule.scaleUp(messages); expect(mockCreateClient).toHaveBeenCalledTimes(3); // 1 app client, 2 repo installation clients - expect(mockedInstallationAuth).toHaveBeenCalledWith( - 100, - 'https://github.enterprise.something/api/v3', - 0, - expect.anything(), - ); - expect(mockedInstallationAuth).toHaveBeenCalledWith( - 200, - 'https://github.enterprise.something/api/v3', - 0, - expect.anything(), - ); + expect(mockedInstallationAuth).toHaveBeenCalledWith(100, 'https://github.enterprise.something/api/v3', 0); + expect(mockedInstallationAuth).toHaveBeenCalledWith(200, 'https://github.enterprise.something/api/v3', 0); }); it('Should reuse GitHub clients for same installation', async () => { @@ -1477,8 +1467,8 @@ describe('scaleUp with public GH', () => { await scaleUpModule.scaleUp(messages); expect(mockCreateClient).toHaveBeenCalledTimes(3); // 1 app client, 2 repo installation clients - expect(mockedInstallationAuth).toHaveBeenCalledWith(100, '', 0, expect.anything()); - expect(mockedInstallationAuth).toHaveBeenCalledWith(200, '', 0, expect.anything()); + expect(mockedInstallationAuth).toHaveBeenCalledWith(100, '', 0); + expect(mockedInstallationAuth).toHaveBeenCalledWith(200, '', 0); }); it('Should reuse GitHub clients for same installation', async () => { @@ -1955,8 +1945,8 @@ describe('scaleUp with Github Data Residency', () => { await scaleUpModule.scaleUp(messages); expect(mockCreateClient).toHaveBeenCalledTimes(3); // 1 app client, 2 repo installation clients - expect(mockedInstallationAuth).toHaveBeenCalledWith(100, '', 0, expect.anything()); - expect(mockedInstallationAuth).toHaveBeenCalledWith(200, '', 0, expect.anything()); + expect(mockedInstallationAuth).toHaveBeenCalledWith(100, '', 0); + expect(mockedInstallationAuth).toHaveBeenCalledWith(200, '', 0); }); it('Should reuse GitHub clients for same installation', async () => { @@ -2219,7 +2209,6 @@ describe('Multi-app round-robin', () => { expect.any(Number), expect.any(String), 1, // appIndex must match the one from createGithubAppAuth - expect.anything(), ); }); @@ -2245,7 +2234,6 @@ describe('Multi-app round-robin', () => { TEST_DATA_SINGLE.installationId, // from mockOctokit.apps.getOrgInstallation mock expect.any(String), 1, - expect.anything(), ); }); @@ -2263,7 +2251,7 @@ describe('Multi-app round-robin', () => { // Should use 999 from webhook directly — no API lookup expect(mockOctokit.apps.getOrgInstallation).not.toHaveBeenCalled(); - expect(mockedInstallationAuth).toHaveBeenCalledWith(999, expect.any(String), 0, expect.anything()); + expect(mockedInstallationAuth).toHaveBeenCalledWith(999, expect.any(String), 0); }); it('primary app (appIndex 0) reuses webhook installationId even in multi-app deployment', async () => { @@ -2282,7 +2270,7 @@ describe('Multi-app round-robin', () => { // Primary app must NOT do an API lookup — reuses webhook installationId expect(mockOctokit.apps.getOrgInstallation).not.toHaveBeenCalled(); - expect(mockedInstallationAuth).toHaveBeenCalledWith(999, expect.any(String), 0, expect.anything()); + expect(mockedInstallationAuth).toHaveBeenCalledWith(999, expect.any(String), 0); }); it('stored installationId takes precedence over webhook payload for additional app', async () => { @@ -2301,7 +2289,7 @@ describe('Multi-app round-robin', () => { // Stored id (77) wins — no API lookup needed expect(mockOctokit.apps.getOrgInstallation).not.toHaveBeenCalled(); - expect(mockedInstallationAuth).toHaveBeenCalledWith(77, expect.any(String), 1, expect.anything()); + expect(mockedInstallationAuth).toHaveBeenCalledWith(77, expect.any(String), 1); }); }); diff --git a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts index 1a3dcff45b..0b9ce7bc5a 100644 --- a/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts +++ b/lambdas/libs/storage-providers/aws/ssm/github-app-credentials-store.ts @@ -16,8 +16,7 @@ export function createAwsSsmGitHubAppCredentialsStore( environment.PARAMETER_GITHUB_APP_KEY_BASE64_NAME, 'PARAMETER_GITHUB_APP_KEY_BASE64_NAME', ); - const installationIdParameters = - environment.PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME?.split(':').filter(Boolean) ?? []; + const installationIdParameters = environment.PARAMETER_GITHUB_APP_INSTALLATION_ID_NAME?.split(':') ?? []; if (idParameters.length !== keyParameters.length) { throw new Error(`GitHub App parameter count mismatch: ${idParameters.length} IDs vs ${keyParameters.length} keys`); @@ -37,7 +36,7 @@ class AwsSsmGitHubAppCredentialsStore implements GitHubAppCredentialsStore { const parameters = await getParameters([ ...this.idParameters, ...this.keyParameters, - ...this.installationIdParameters, + ...this.installationIdParameters.filter(Boolean), ]); return this.idParameters.map((idParameter, index) => { const appIdValue = parameters.get(idParameter);