Merge pull request #4007 from github/mbg/use-registry-proxy-for-repo-auth

Use private registry proxy for API requests when available

src/api-client.test.ts

@@ -6,8 +6,8 @@ import * as sinon from "sinon";
 import * as actionsUtil from "./actions-util";
 import * as api from "./api-client";
 import { DO_NOT_RETRY_STATUSES } from "./api-client";
-import { ActionsEnvVars } from "./environment";
-import { getTestEnv, setupTests } from "./testing-utils";
+import { ActionsEnvVars, RegistryProxyVars } from "./environment";
+import { callee, getTestEnv, setupTests } from "./testing-utils";
 import * as util from "./util";
 
 setupTests(test);
@@ -27,14 +27,17 @@ test.serial("getApiClient", async (t) => {
 
   sinon.stub(actionsUtil, "getRequiredInput").withArgs("token").returns("xyz");
 
-  api.getApiClient(env);
+  const apiClient = api.getApiClient(env);
+  t.truthy(apiClient);
 
+  t.true(githubStub.calledOnce);
   t.assert(
     githubStub.calledOnceWithExactly({
       auth: "token xyz",
       baseUrl: "http://api.github.localhost",
       log: sinon.match.any,
       userAgent: `CodeQL-Action/${actionsUtil.getActionVersion()}`,
+      request: sinon.match.any,
       retry: {
         doNotRetry: DO_NOT_RETRY_STATUSES,
       },
@@ -204,3 +207,47 @@ test.serial(
     }
   },
 );
+
+test("getRegistryProxy - returns undefined if the proxy is not configured", async (t) => {
+  const target = callee(api.getRegistryProxy).withArgs();
+
+  // Empty environment.
+  await target.passes(t.is, undefined);
+  // Only the host.
+  await target
+    .withEnv(getTestEnv({ [RegistryProxyVars.PROXY_HOST]: "localhost" }))
+    .passes(t.is, undefined);
+  // Only the port.
+  await target
+    .withEnv(getTestEnv({ [RegistryProxyVars.PROXY_PORT]: "1234" }))
+    .passes(t.is, undefined);
+});
+
+test("getRegistryProxy - returns value when both vars are set", async (t) => {
+  await callee(api.getRegistryProxy)
+    .withArgs()
+    .withEnv(
+      getTestEnv({
+        [RegistryProxyVars.PROXY_HOST]: "localhost",
+        [RegistryProxyVars.PROXY_PORT]: "1234",
+      }),
+    )
+    .passes(t.truthy);
+});
+
+test("getRegistryProxyConfig - gets the configuration from the env vars", async (t) => {
+  const host = "localhost";
+  const port = "1234";
+  const ca = "cert";
+
+  await callee(api.getRegistryProxyConfig)
+    .withArgs()
+    .withEnv(
+      getTestEnv({
+        [RegistryProxyVars.PROXY_HOST]: host,
+        [RegistryProxyVars.PROXY_PORT]: port,
+        [RegistryProxyVars.PROXY_CA_CERTIFICATE]: ca,
+      }),
+    )
+    .passes(t.like, { host, port, ca });
+});

src/api-client.ts

@@ -4,9 +4,23 @@ import { type Octokit } from "@octokit/core";
 import { type PaginateInterface } from "@octokit/plugin-paginate-rest";
 import { type Api } from "@octokit/plugin-rest-endpoint-methods";
 import * as retry from "@octokit/plugin-retry";
+import { RequestRequestOptions } from "@octokit/types";
+import {
+  ProxyAgent,
+  RequestInfo,
+  RequestInit,
+  fetch as undiciFetch,
+} from "undici";
 
+import type { ActionState } from "./action-common";
 import { getActionVersion, getRequiredInput } from "./actions-util";
-import { EnvVar, ReadOnlyEnv, ActionsEnvVars, getEnv } from "./environment";
+import {
+  ActionsEnvVars,
+  EnvVar,
+  ReadOnlyEnv,
+  RegistryProxyVars,
+  getEnv,
+} from "./environment";
 import { Logger } from "./logging";
 import { getRepositoryNwo, RepositoryNwo } from "./repository";
 import {
@@ -46,16 +60,84 @@ export interface GitHubApiExternalRepoDetails {
   apiURL: string | undefined;
 }
 
+/**
+ * Gets the configuration for the private registry authentication proxy,
+ * if it is available in the environment.
+ *
+ * @param action The required Action state.
+ * @returns The hostname, port, and CA retrieved from the corresponding environment variables.
+ */
+export function getRegistryProxyConfig(action: ActionState<["ReadOnlyEnv"]>) {
+  return {
+    host: action.env.getOptional(RegistryProxyVars.PROXY_HOST),
+    port: action.env.getOptional(RegistryProxyVars.PROXY_PORT),
+    ca: action.env.getOptional(RegistryProxyVars.PROXY_CA_CERTIFICATE),
+  };
+}
+
+/**
+ * Gets the configuration for the private registry authentication proxy,
+ * and uses it to initialise a corresponding `ProxyAgent`.
+ *
+ * @param action The required Action state.
+ * @returns A `ProxyAgent` corresponding to the private registry proxy,
+ *          or `undefined` if we couldn't retrieve the host and port.
+ */
+export function getRegistryProxy(
+  action: ActionState<["Logger", "ReadOnlyEnv"]>,
+): ProxyAgent | undefined {
+  const { host, port, ca } = getRegistryProxyConfig(action);
+
+  if (host && port) {
+    const uri = `http://${host}:${port}`;
+    action.logger.debug(
+      `Using private registry proxy at '${uri}' for API client.`,
+    );
+    return new ProxyAgent({
+      uri,
+      keepAliveTimeout: 10,
+      keepAliveMaxTimeout: 10,
+      requestTls: ca ? { ca } : undefined,
+    });
+  }
+
+  return undefined;
+}
+
+/**
+ * Constructs a `RequestRequestOptions` with a custom `fetch` implementation
+ * that uses `dispatcher` as a proxy for requests.
+ *
+ * @param dispatcher The proxy to use.
+ */
+export function makeProxyRequestOptions(
+  dispatcher: ProxyAgent,
+): RequestRequestOptions {
+  return {
+    fetch: (req: RequestInfo, init?: RequestInit) => {
+      return undiciFetch(req, { ...init, dispatcher });
+    },
+  };
+}
+
 /** The type of GitHub API client we use. */
 export type ApiClient = Octokit & Api & { paginate: PaginateInterface };
 
+/** Options for `createApiClientWithDetails`. */
+interface CreateApiClientOptions {
+  allowExternal?: boolean;
+  proxy?: ProxyAgent;
+}
+
 function createApiClientWithDetails(
   apiDetails: GitHubApiCombinedDetails,
-  { allowExternal = false } = {},
+  { allowExternal = false, proxy = undefined }: CreateApiClientOptions = {},
 ): ApiClient {
   const auth =
     (allowExternal && apiDetails.externalRepoAuth) || apiDetails.auth;
   const retryingOctokit = githubUtils.GitHub.plugin(retry.retry);
+  const requestOptions =
+    proxy === undefined ? undefined : makeProxyRequestOptions(proxy);
   return new retryingOctokit(
     githubUtils.getOctokitOptions(auth, {
       baseUrl: apiDetails.apiURL,
@@ -66,6 +148,7 @@ function createApiClientWithDetails(
         warn: core.warning,
         error: core.error,
       },
+      request: requestOptions,
       retry: {
         doNotRetry: DO_NOT_RETRY_STATUSES,
       },
@@ -87,8 +170,9 @@ export function getApiClient(env: ReadOnlyEnv = getEnv()) {
 
 export function getApiClientWithExternalAuth(
   apiDetails: GitHubApiCombinedDetails,
+  proxy?: ProxyAgent,
 ) {
-  return createApiClientWithDetails(apiDetails, { allowExternal: true });
+  return createApiClientWithDetails(apiDetails, { allowExternal: true, proxy });
 }
 
 /**

src/config/file.test.ts

@@ -1,11 +1,18 @@
+import * as github from "@actions/github";
 import test from "ava";
 import sinon from "sinon";
 
+import * as api from "../api-client";
+import { RegistryProxyVars } from "../environment";
 import { Feature } from "../feature-flags";
 import { RepositoryPropertyName } from "../feature-flags/properties";
-import { callee, setupTests } from "../testing-utils";
+import {
+  callee,
+  SAMPLE_DOTCOM_API_DETAILS,
+  setupTests,
+} from "../testing-utils";
 
-import { getConfigFileInput } from "./file";
+import { getConfigFileInput, getRemoteConfig } from "./file";
 
 setupTests(test);
 
@@ -67,3 +74,61 @@ test("getConfigFileInput ignores repository property value when FF is off", asyn
     )
     .passes(t.is, undefined);
 });
+
+test.serial("getRemoteConfig uses proxy when it is supposed to", async (t) => {
+  const client = github.getOctokit("123");
+  const response = {
+    data: {
+      content: Buffer.from("disable-default-queries: false").toString("base64"),
+    },
+  };
+  sinon
+    .stub(client.rest.repos, "getContent")
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-argument
+    .resolves(response as any);
+
+  // We stub `getApiClientWithExternalAuth` so that it throws if no
+  // proxy is provided and returns the client otherwise. This allows us
+  // to verify the result in the following test cases.
+  const errorMessage = "No `proxy` was provided by the caller.";
+  sinon
+    .stub(api, "getApiClientWithExternalAuth")
+    .callsFake((_details, proxy) => {
+      // Throw if proxy isn't defined.
+      if (proxy === undefined) {
+        throw new Error(errorMessage);
+      }
+      // Otherwise return the client object.
+      return client;
+    });
+
+  const target = callee(getRemoteConfig)
+    .withDefaultActionsEnv()
+    .withArgs("file.yml", SAMPLE_DOTCOM_API_DETAILS);
+
+  // Should use it when the FF is enabled and the environment variables are set.
+  await target
+    .withFeatures([Feature.ProxyApiRequests, Feature.NewRemoteFileAddresses])
+    .withEnv((env) => {
+      env.set(RegistryProxyVars.PROXY_HOST, "localhost");
+      env.set(RegistryProxyVars.PROXY_PORT, "1234");
+    })
+    .logs(t, "Using private registry proxy at 'http://localhost:1234'")
+    .passes(t.truthy);
+
+  // But not when the FF is not enabled.
+  await target
+    .withFeatures([Feature.NewRemoteFileAddresses])
+    .withEnv((env) => {
+      env.set(RegistryProxyVars.PROXY_HOST, "localhost");
+      env.set(RegistryProxyVars.PROXY_PORT, "1234");
+    })
+    .notLogs(t, "Using private registry proxy at 'http://localhost:1234'")
+    .throws(t, { message: errorMessage });
+
+  // And not when the environment variables aren't set.
+  await target
+    .withFeatures([Feature.ProxyApiRequests, Feature.NewRemoteFileAddresses])
+    .notLogs(t, "Using private registry proxy at 'http://localhost:1234'")
+    .throws(t, { message: errorMessage });
+});

src/config/file.ts

@@ -82,8 +82,15 @@ export async function getRemoteConfig(
 ): Promise<UserConfig> {
   const address = await parseRemoteFileAddress(actionState, configFile);
 
+  const shouldProxyRequest = await actionState.features.getValue(
+    Feature.ProxyApiRequests,
+  );
+  const proxy = shouldProxyRequest
+    ? api.getRegistryProxy(actionState)
+    : undefined;
+
   const response = await api
-    .getApiClientWithExternalAuth(apiDetails)
+    .getApiClientWithExternalAuth(apiDetails, proxy)
     .rest.repos.getContent({
       owner: address.owner,
       repo: address.repo,

src/environment.ts

@@ -1,3 +1,13 @@
+/**
+ * Environment variables used by Default Setup to communicate the private registry proxy configuration.
+ */
+export enum RegistryProxyVars {
+  PROXY_HOST = "CODEQL_PROXY_HOST",
+  PROXY_PORT = "CODEQL_PROXY_PORT",
+  PROXY_CA_CERTIFICATE = "CODEQL_PROXY_CA_CERTIFICATE",
+  PROXY_URLS = "CODEQL_PROXY_URLS",
+}
+
 /**
  * Environment variables used by the CodeQL Action.
  *
@@ -202,7 +212,7 @@ export enum ActionsEnvVars {
 }
 
 /** A type representing all known environment variables. */
-export type KnownEnvVar = EnvVar | ActionsEnvVars;
+export type KnownEnvVar = EnvVar | ActionsEnvVars | RegistryProxyVars;
 
 /**
  * Gets an environment variable, but throws an error if it is not set.
@@ -255,6 +265,11 @@ export function getOptionalEnvVar(paramName: string): string | undefined {
 export class ReadOnlyEnv<T extends string | undefined = string | undefined> {
   constructor(protected readonly vars: Record<string, T>) {}
 
+  /** Clones the object while detaching the underlying environment from the original. */
+  public clone(): this {
+    return Object.create(this, { vars: { value: { ...this.vars } } }) as this;
+  }
+
   /** Tries to get the value for `name` and throws if there isn't one. */
   public getRequired(name: string): string {
     return getRequiredEnvVar(this.vars, name);

src/feature-flags.ts

@@ -138,6 +138,8 @@ export enum Feature {
   /** Controls whether overlay build failures on the default branch are stored in the Actions cache. */
   OverlayAnalysisStatusSave = "overlay_analysis_status_save",
   QaTelemetryEnabled = "qa_telemetry_enabled",
+  /** Routes (some) API requests through the registry proxy. */
+  ProxyApiRequests = "proxy_api_requests",
   /** Note that this currently only disables baseline file coverage information. */
   SkipFileCoverageOnPrs = "skip_file_coverage_on_prs",
   StartProxyUseFeaturesRelease = "start_proxy_use_features_release",
@@ -389,6 +391,11 @@ export const featureConfig = {
     legacyApi: true,
     minimumVersion: undefined,
   },
+  [Feature.ProxyApiRequests]: {
+    defaultValue: false,
+    envVar: "CODEQL_ACTION_PROXY_API_REQUESTS",
+    minimumVersion: undefined,
+  },
   [Feature.SkipFileCoverageOnPrs]: {
     defaultValue: false,
     envVar: "CODEQL_ACTION_SKIP_FILE_COVERAGE_ON_PRS",

src/testing-utils.ts

@@ -178,8 +178,7 @@ export function makeMacro<Args extends unknown[]>(
   return wrapper;
 }
 
-export function getTestEnv(): Env {
-  const testEnv: NodeJS.ProcessEnv = {};
+export function getTestEnv(testEnv: NodeJS.ProcessEnv = {}): Env {
   return getEnv(testEnv);
 }
 
@@ -250,7 +249,7 @@ abstract class BaseEnvBuilder<
       cloneFrom !== undefined
         ? ({
             ...cloneFrom.state,
-            env: Object.create(cloneFrom.state.env),
+            env: cloneFrom.state.env.clone(),
             actions: Object.create(cloneFrom.state.actions),
             logger: this.logger,
           } satisfies ActionState<AllState>)