Merge pull request #3473 from github/mbg/start-proxy/cert-gen

Improve proxy certificate generation

src/feature-flags.ts

@@ -47,6 +47,7 @@ export enum Feature {
   DisableKotlinAnalysisEnabled = "disable_kotlin_analysis_enabled",
   ExportDiagnosticsEnabled = "export_diagnostics_enabled",
   IgnoreGeneratedFiles = "ignore_generated_files",
+  ImprovedProxyCertificates = "improved_proxy_certificates",
   OverlayAnalysis = "overlay_analysis",
   OverlayAnalysisActions = "overlay_analysis_actions",
   OverlayAnalysisCodeScanningActions = "overlay_analysis_code_scanning_actions",
@@ -168,6 +169,11 @@ export const featureConfig = {
     envVar: "CODEQL_ACTION_IGNORE_GENERATED_FILES",
     minimumVersion: undefined,
   },
+  [Feature.ImprovedProxyCertificates]: {
+    defaultValue: false,
+    envVar: "CODEQL_ACTION_IMPROVED_PROXY_CERTIFICATES",
+    minimumVersion: undefined,
+  },
   [Feature.OverlayAnalysis]: {
     defaultValue: false,
     envVar: "CODEQL_ACTION_OVERLAY_ANALYSIS",

src/start-proxy-action.ts

@@ -2,7 +2,6 @@ import { ChildProcess, spawn } from "child_process";
 import * as path from "path";
 
 import * as core from "@actions/core";
-import { pki } from "node-forge";
 
 import * as actionsUtil from "./actions-util";
 import { getGitHubVersion } from "./api-client";
@@ -19,81 +18,14 @@ import {
   ProxyInfo,
   sendFailedStatusReport,
   sendSuccessStatusReport,
-  Credential,
   Registry,
+  ProxyConfig,
 } from "./start-proxy";
+import { generateCertificateAuthority } from "./start-proxy/ca";
 import { checkConnections } from "./start-proxy/reachability";
 import { ActionName, sendUnhandledErrorStatusReport } from "./status-report";
 import * as util from "./util";
 
-const KEY_SIZE = 2048;
-const KEY_EXPIRY_YEARS = 2;
-
-type CertificateAuthority = {
-  cert: string;
-  key: string;
-};
-
-type BasicAuthCredentials = {
-  username: string;
-  password: string;
-};
-
-type ProxyConfig = {
-  /** The validated configurations for the proxy. */
-  all_credentials: Credential[];
-  ca: CertificateAuthority;
-  proxy_auth?: BasicAuthCredentials;
-};
-
-const CERT_SUBJECT = [
-  {
-    name: "commonName",
-    value: "Dependabot Internal CA",
-  },
-  {
-    name: "organizationName",
-    value: "GitHub inc.",
-  },
-  {
-    shortName: "OU",
-    value: "Dependabot",
-  },
-  {
-    name: "countryName",
-    value: "US",
-  },
-  {
-    shortName: "ST",
-    value: "California",
-  },
-  {
-    name: "localityName",
-    value: "San Francisco",
-  },
-];
-
-function generateCertificateAuthority(): CertificateAuthority {
-  const keys = pki.rsa.generateKeyPair(KEY_SIZE);
-  const cert = pki.createCertificate();
-  cert.publicKey = keys.publicKey;
-  cert.serialNumber = "01";
-  cert.validity.notBefore = new Date();
-  cert.validity.notAfter = new Date();
-  cert.validity.notAfter.setFullYear(
-    cert.validity.notBefore.getFullYear() + KEY_EXPIRY_YEARS,
-  );
-
-  cert.setSubject(CERT_SUBJECT);
-  cert.setIssuer(CERT_SUBJECT);
-  cert.setExtensions([{ name: "basicConstraints", cA: true }]);
-  cert.sign(keys.privateKey);
-
-  const pem = pki.certificateToPem(cert);
-  const key = pki.privateKeyToPem(keys.privateKey);
-  return { cert: pem, key };
-}
-
 async function run(startedAt: Date) {
   // To capture errors appropriately, keep as much code within the try-catch as
   // possible, and only use safe functions outside.
@@ -144,7 +76,9 @@ async function run(startedAt: Date) {
         .join("\n")}`,
     );
 
-    const ca = generateCertificateAuthority();
+    const ca = generateCertificateAuthority(
+      await features.getValue(Feature.ImprovedProxyCertificates),
+    );
 
     const proxyConfig: ProxyConfig = {
       all_credentials: credentials,

src/start-proxy/ca.test.ts

@@ -0,0 +1,93 @@
+import test, { ExecutionContext } from "ava";
+import { pki } from "node-forge";
+
+import { setupTests } from "../testing-utils";
+
+import * as ca from "./ca";
+
+setupTests(test);
+
+const toMap = <T>(array: T[], func: (e: T) => string) =>
+  new Map<string, T>(array.map((val) => [func(val), val]));
+
+function checkCertAttributes(
+  t: ExecutionContext<unknown>,
+  cert: pki.Certificate,
+) {
+  const subjectMap = toMap(
+    cert.subject.attributes,
+    (attr) => attr.name as string,
+  );
+  const issuerMap = toMap(
+    cert.issuer.attributes,
+    (attr) => attr.name as string,
+  );
+
+  t.is(subjectMap.get("commonName")?.value, "Dependabot Internal CA");
+  t.is(issuerMap.get("commonName")?.value, "Dependabot Internal CA");
+
+  for (const attrName of subjectMap.keys()) {
+    t.deepEqual(subjectMap.get(attrName), issuerMap.get(attrName));
+  }
+}
+
+test("generateCertificateAuthority - generates certificates", (t) => {
+  const result = ca.generateCertificateAuthority(false);
+  const cert = pki.certificateFromPem(result.cert);
+  const key = pki.privateKeyFromPem(result.key);
+
+  t.truthy(cert);
+  t.truthy(key);
+
+  checkCertAttributes(t, cert);
+
+  // Check the validity.
+  t.true(
+    cert.validity.notBefore <= new Date(),
+    "notBefore date is in the future",
+  );
+  t.true(cert.validity.notAfter > new Date(), "notAfter date is in the past");
+
+  // Check that the extensions are set as we'd expect.
+  const exts = cert.extensions as ca.Extension[];
+  t.is(exts.length, 1);
+  t.is(exts[0].name, "basicConstraints");
+  t.is(exts[0].cA, true);
+
+  t.truthy(cert.siginfo);
+});
+
+test("generateCertificateAuthority - generates certificates with FF", (t) => {
+  const result = ca.generateCertificateAuthority(true);
+  const cert = pki.certificateFromPem(result.cert);
+  const key = pki.privateKeyFromPem(result.key);
+
+  t.truthy(cert);
+  t.truthy(key);
+
+  checkCertAttributes(t, cert);
+
+  // Check the validity.
+  t.true(
+    cert.validity.notBefore <= new Date(),
+    "notBefore date is in the future",
+  );
+  t.true(cert.validity.notAfter > new Date(), "notAfter date is in the past");
+
+  // Check that the extensions are set as we'd expect.
+  const exts = toMap(cert.extensions as ca.Extension[], (ext) => ext.name);
+  t.is(exts.size, 4);
+  t.true(exts.get("basicConstraints")?.cA);
+  t.truthy(exts.get("subjectKeyIdentifier"));
+  t.truthy(exts.get("authorityKeyIdentifier"));
+
+  const keyUsage = exts.get("keyUsage");
+  if (t.truthy(keyUsage)) {
+    t.true(keyUsage.critical);
+    t.true(keyUsage.keyCertSign);
+    t.true(keyUsage.cRLSign);
+    t.true(keyUsage.digitalSignature);
+  }
+
+  t.truthy(cert.siginfo);
+});

src/start-proxy/ca.ts

@@ -0,0 +1,93 @@
+import { md, pki } from "node-forge";
+
+import { CertificateAuthority } from "./types";
+
+const KEY_SIZE = 2048;
+const KEY_EXPIRY_YEARS = 2;
+
+const CERT_SUBJECT = [
+  {
+    name: "commonName",
+    value: "Dependabot Internal CA",
+  },
+  {
+    name: "organizationName",
+    value: "GitHub inc.",
+  },
+  {
+    shortName: "OU",
+    value: "Dependabot",
+  },
+  {
+    name: "countryName",
+    value: "US",
+  },
+  {
+    shortName: "ST",
+    value: "California",
+  },
+  {
+    name: "localityName",
+    value: "San Francisco",
+  },
+];
+
+export type Extension = {
+  name: string;
+  [key: string]: unknown;
+};
+
+const extraExtensions: Extension[] = [
+  {
+    name: "keyUsage",
+    critical: true,
+    keyCertSign: true,
+    cRLSign: true,
+    digitalSignature: true,
+  },
+  { name: "subjectKeyIdentifier" },
+  { name: "authorityKeyIdentifier", keyIdentifier: true },
+];
+
+/**
+ * Generates a CA certificate for the proxy.
+ *
+ * @param newCertGenFF Whether to use the updated certificate generation.
+ * @returns The private and public keys.
+ */
+export function generateCertificateAuthority(
+  newCertGenFF: boolean,
+): CertificateAuthority {
+  const keys = pki.rsa.generateKeyPair(KEY_SIZE);
+  const cert = pki.createCertificate();
+  cert.publicKey = keys.publicKey;
+  cert.serialNumber = "01";
+  cert.validity.notBefore = new Date();
+  cert.validity.notAfter = new Date();
+  cert.validity.notAfter.setFullYear(
+    cert.validity.notBefore.getFullYear() + KEY_EXPIRY_YEARS,
+  );
+
+  cert.setSubject(CERT_SUBJECT);
+  cert.setIssuer(CERT_SUBJECT);
+
+  const extensions: Extension[] = [{ name: "basicConstraints", cA: true }];
+
+  // Add the extra CA extensions if the FF is enabled.
+  if (newCertGenFF) {
+    extensions.push(...extraExtensions);
+  }
+
+  cert.setExtensions(extensions);
+
+  // Specifically use SHA256 when the FF is enabled.
+  if (newCertGenFF) {
+    cert.sign(keys.privateKey, md.sha256.create());
+  } else {
+    cert.sign(keys.privateKey);
+  }
+
+  const pem = pki.certificateToPem(cert);
+  const key = pki.privateKeyToPem(keys.privateKey);
+  return { cert: pem, key };
+}

src/start-proxy/types.ts

@@ -59,3 +59,23 @@ export interface ProxyInfo {
   cert: string;
   registries: Registry[];
 }
+
+export type CertificateAuthority = {
+  cert: string;
+  key: string;
+};
+
+export type BasicAuthCredentials = {
+  username: string;
+  password: string;
+};
+
+/**
+ * Represents configurations for the authentication proxy.
+ */
+export type ProxyConfig = {
+  /** The validated configurations for the proxy. */
+  all_credentials: Credential[];
+  ca: CertificateAuthority;
+  proxy_auth?: BasicAuthCredentials;
+};