Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lambdas/functions/control-plane/src/github/auth.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { createAppAuth } from '@octokit/auth-app';
import { StrategyOptions } from '@octokit/auth-app/dist-types/types';
import { createAppAuth, type StrategyOptions } from '@octokit/auth-app';
import { request } from '@octokit/request';
import { RequestInterface, RequestParameters } from '@octokit/types';
import { getParameter } from '@aws-github-runner/aws-ssm-util';
import * as nock from 'nock';

import { reset } from './cache';
import { createGithubAppAuth, createOctokitClient } from './auth';
import { describe, it, expect, beforeEach, vi } from 'vitest';

Expand All @@ -29,6 +28,7 @@ const PARAMETER_GITHUB_APP_KEY_BASE64_NAME = `/actions-runner/${ENVIRONMENT}/git
const mockedGet = vi.mocked(getParameter);

beforeEach(() => {
reset(); // clear all caches before each test
vi.resetModules();
vi.clearAllMocks();
process.env = { ...cleanEnv };
Expand Down
35 changes: 26 additions & 9 deletions lambdas/functions/control-plane/src/github/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import { throttling } from '@octokit/plugin-throttling';
import { createChildLogger } from '@aws-github-runner/aws-powertools-util';
import { getParameter } from '@aws-github-runner/aws-ssm-util';
import { EndpointDefaults } from '@octokit/types';
import { getInstallationAuthObject, getAuthConfig, createAuthCacheKey, createAuthConfigCacheKey } from './cache';
import type { GithubAppConfig } from './types';

const logger = createChildLogger('gh-auth');

Expand Down Expand Up @@ -64,32 +66,47 @@ export async function createGithubInstallationAuth(
installationId: number | undefined,
ghesApiUrl = '',
): Promise<InstallationAccessTokenAuthentication> {
const auth = await createAuth(installationId, ghesApiUrl);
const installationAuthOptions: InstallationAuthOptions = { type: 'installation', installationId };
return auth(installationAuthOptions);
const cacheKey = createAuthCacheKey('installation', installationId, ghesApiUrl);

return getInstallationAuthObject(cacheKey, async () => {
const auth = await createAuth(installationId, ghesApiUrl);
const installationAuthOptions: InstallationAuthOptions = { type: 'installation', installationId };
return auth(installationAuthOptions);
});
}

async function createAuth(installationId: number | undefined, ghesApiUrl: string): Promise<AuthInterface> {
const appId = parseInt(await getParameter(process.env.PARAMETER_GITHUB_APP_ID_NAME));
let authOptions: StrategyOptions = {
appId,
privateKey: Buffer.from(
const configCacheKey = createAuthConfigCacheKey(ghesApiUrl);

const config = await getAuthConfig(configCacheKey, async (): Promise<GithubAppConfig> => {
const appId = parseInt(await getParameter(process.env.PARAMETER_GITHUB_APP_ID_NAME));
const privateKey = Buffer.from(
await getParameter(process.env.PARAMETER_GITHUB_APP_KEY_BASE64_NAME),
'base64',
// 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
)
.toString()
.replace('/[\\n]/g', String.fromCharCode(10)),
.replace('/[\\n]/g', String.fromCharCode(10));
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

String argument /[\\n]/g looks like a regular expression, but it will be interpreted as a string.

Suggested change
.replace('/[\\n]/g', String.fromCharCode(10));
.replace(/\\n/g, String.fromCharCode(10));

Copilot uses AI. Check for mistakes.

return {
appId,
privateKey,
};
});

let authOptions: StrategyOptions = {
appId: config.appId,
privateKey: config.privateKey,
};
if (installationId) authOptions = { ...authOptions, installationId };

logger.debug(`GHES API URL: ${ghesApiUrl}`);
if (ghesApiUrl) {
authOptions.request = request.defaults({
baseUrl: ghesApiUrl,
});
}) as RequestInterface;
}
return createAppAuth(authOptions);
}
42 changes: 42 additions & 0 deletions lambdas/functions/control-plane/src/github/cache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import type { InstallationAccessTokenAuthentication } from '@octokit/auth-app';
import type { GithubAppConfig } from './types';

const installationAuthObjects = new Map<string, InstallationAccessTokenAuthentication>();
const authConfigs = new Map<string, GithubAppConfig>();

export function createAuthCacheKey(type: 'app' | 'installation', installationId?: number, ghesApiUrl: string = '') {
const id = installationId || 'none';
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The cache key generation uses installationId || 'none' which treats falsy values (0, null, undefined) the same way. While 0 is likely not a valid GitHub installation ID, this could lead to cache key collisions for different falsy values.

Consider using nullish coalescing instead for more precise handling:

const id = installationId ?? 'none';

This will only treat null and undefined as 'none', while preserving 0 if it were ever a valid value.

Suggested change
const id = installationId || 'none';
const id = installationId ?? 'none';

Copilot uses AI. Check for mistakes.
return `${type}-auth-${id}-${ghesApiUrl}`;
}

export function createAuthConfigCacheKey(ghesApiUrl: string = '') {
return `auth-config-${ghesApiUrl}`;
}

export async function getInstallationAuthObject(
key: string,
creator: () => Promise<InstallationAccessTokenAuthentication>,
): Promise<InstallationAccessTokenAuthentication> {
if (installationAuthObjects.has(key)) {
return installationAuthObjects.get(key)!;
}

const authObj = await creator();
installationAuthObjects.set(key, authObj);
return authObj;
}
Comment on lines +16 to +27
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

Installation access tokens have expiration times (typically 1 hour) and should not be cached indefinitely. The current implementation caches InstallationAccessTokenAuthentication objects without considering token expiration, which could lead to using expired tokens.

Consider either:

  1. Not caching installation auth objects at all, or
  2. Implementing TTL-based caching that respects the token's expiresAt field from the authentication object

Copilot uses AI. Check for mistakes.
Copy link
Member

@npalm npalm Nov 21, 2025

Choose a reason for hiding this comment

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

@copilot Lambda max TTL is 15 minutes


export async function getAuthConfig(key: string, creator: () => Promise<GithubAppConfig>) {
if (authConfigs.has(key)) {
return authConfigs.get(key)!;
}

const config = await creator();
authConfigs.set(key, config);
return config;
}

export function reset() {
installationAuthObjects.clear();
authConfigs.clear();
}
2 changes: 2 additions & 0 deletions lambdas/functions/control-plane/src/github/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './cache';
export * from './types';
5 changes: 5 additions & 0 deletions lambdas/functions/control-plane/src/github/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export interface GithubAppConfig {
appId: number;
privateKey: string;
installationId?: number;
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The installationId field is defined in the GithubAppConfig interface but is never populated or used. It should either be:

  1. Removed from the interface if not needed, or
  2. Populated in the createAuth function (line 109-112) if it's intended to be part of the config

Currently, the installationId is handled separately in createAuth (line 119) rather than being part of the cached config.

Suggested change
installationId?: number;

Copilot uses AI. Check for mistakes.
}
1 change: 0 additions & 1 deletion lambdas/functions/control-plane/src/local.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ const sqsEvent = {
{
messageId: 'e8d74d08-644e-42ca-bf82-a67daa6c4dad',
receiptHandle:
// eslint-disable-next-line max-len
'AQEBCpLYzDEKq4aKSJyFQCkJduSKZef8SJVOperbYyNhXqqnpFG5k74WygVAJ4O0+9nybRyeOFThvITOaS21/jeHiI5fgaM9YKuI0oGYeWCIzPQsluW5CMDmtvqv1aA8sXQ5n2x0L9MJkzgdIHTC3YWBFLQ2AxSveOyIHwW+cHLIFCAcZlOaaf0YtaLfGHGkAC4IfycmaijV8NSlzYgDuxrC9sIsWJ0bSvk5iT4ru/R4+0cjm7qZtGlc04k9xk5Fu6A+wRxMaIyiFRY+Ya19ykcevQldidmEjEWvN6CRToLgclk=',
body: {
repositoryName: 'self-hosted',
Expand Down