Remove node forge dependency (coder#648)

Replaced `node-forge` with `@peculiar/x509` (more modern, lightweight, and 
widely adopted). Node.js's built-in `crypto` module was attempted first, but 
`keyUsage` returns `undefined`.

**Testing in Electron**

All vitest tests now run in Electron to mirror VS Code's environment. This adds 
a few seconds overhead vs Node.js. `electron` was added as dev dependency 
for BoringSSL compatibility (Node.js uses OpenSSL).

Author: EhabY

.vscode/settings.json

@@ -10,5 +10,9 @@
 	},
 	"[jsonc]": {
 		"editor.defaultFormatter": "esbenp.prettier-vscode"
-	}
+	},
+	"vitest.nodeEnv": {
+		"ELECTRON_RUN_AS_NODE": "1"
+	},
+	"vitest.nodeExecutable": "node_modules/.bin/electron"
 }

package.json

@@ -25,7 +25,7 @@
 		"package": "webpack --mode production --devtool hidden-source-map",
 		"package:prerelease": "npx vsce package --pre-release",
 		"pretest": "tsc -p . --outDir out && tsc -p test --outDir out && yarn run build && yarn run lint",
-		"test": "vitest",
+		"test": "ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs",
 		"test:ci": "CI=true yarn test",
 		"test:integration": "vscode-test",
 		"vscode:prepublish": "yarn package",
@@ -343,12 +343,12 @@
 		"word-wrap": "1.2.5"
 	},
 	"dependencies": {
+		"@peculiar/x509": "^1.14.0",
 		"axios": "1.12.2",
 		"date-fns": "^3.6.0",
 		"eventsource": "^3.0.6",
 		"find-process": "https://github.com/coder/find-process#fix/sequoia-compat",
 		"jsonc-parser": "^3.3.1",
-		"node-forge": "^1.3.1",
 		"openpgp": "^6.2.2",
 		"pretty-bytes": "^7.1.0",
 		"proxy-agent": "^6.5.0",
@@ -361,7 +361,6 @@
 		"@types/eventsource": "^3.0.0",
 		"@types/glob": "^7.1.3",
 		"@types/node": "^22.14.1",
-		"@types/node-forge": "^1.3.14",
 		"@types/semver": "^7.7.1",
 		"@types/ua-parser-js": "0.7.36",
 		"@types/vscode": "^1.73.0",
@@ -375,6 +374,7 @@
 		"bufferutil": "^4.0.9",
 		"coder": "https://github.com/coder/coder#main",
 		"dayjs": "^1.11.13",
+		"electron": "^39.1.2",
 		"eslint": "^8.57.1",
 		"eslint-config-prettier": "^10.1.8",
 		"eslint-import-resolver-typescript": "^4.4.4",

src/error.ts

@@ -1,7 +1,11 @@
+import {
+	X509Certificate,
+	KeyUsagesExtension,
+	KeyUsageFlags,
+} from "@peculiar/x509";
 import { isAxiosError } from "axios";
 import { isApiError, isApiErrorResponse } from "coder/site/src/api/errors";
-import * as forge from "node-forge";
-import * as tls from "tls";
+import * as tls from "node:tls";
 import * as vscode from "vscode";
 
 import { type Logger } from "./logging/logger";
@@ -23,10 +27,6 @@ export enum X509_ERR {
 	UNTRUSTED_CHAIN = "Your Coder deployment's certificate chain does not appear to be trusted by this system. The root of the certificate chain must be added to this system's trust store. ",
 }
 
-interface KeyUsage {
-	keyCertSign: boolean;
-}
-
 export class CertificateError extends Error {
 	public static ActionAllowInsecure = "Allow Insecure";
 	public static ActionOK = "OK";
@@ -80,7 +80,7 @@ export class CertificateError extends Error {
 				const url = new URL(address);
 				const socket = tls.connect(
 					{
-						port: parseInt(url.port, 10) || 443,
+						port: Number.parseInt(url.port, 10) || 443,
 						host: url.hostname,
 						rejectUnauthorized: false,
 					},
@@ -91,29 +91,27 @@ export class CertificateError extends Error {
 							throw new Error("no peer certificate");
 						}
 
-						// We use node-forge for two reasons:
-						// 1. Node/Electron only provide extended key usage.
-						// 2. Electron's checkIssued() will fail because it suffers from same
-						//    the key usage bug that we are trying to work around here in the
-						//    first place.
-						const cert = forge.pki.certificateFromPem(x509.toString());
-						if (!cert.issued(cert)) {
+						// We use "@peculiar/x509" because Node's x509 returns an undefined `keyUsage`.
+						const cert = new X509Certificate(x509.toString());
+						const isSelfIssued = cert.subject === cert.issuer;
+						if (!isSelfIssued) {
 							return resolve(X509_ERR.PARTIAL_CHAIN);
 						}
 
 						// The key usage needs to exist but not have cert signing to fail.
-						const keyUsage = cert.getExtension({ name: "keyUsage" }) as
-							| KeyUsage
-							| undefined;
-						if (keyUsage && !keyUsage.keyCertSign) {
-							return resolve(X509_ERR.NON_SIGNING);
-						} else {
-							// This branch is currently untested; it does not appear possible to
-							// get the error "unable to verify" with a self-signed certificate
-							// unless the key usage was the issue since it would have errored
-							// with "self-signed certificate" instead.
-							return resolve(X509_ERR.UNTRUSTED_LEAF);
+						const extension = cert.getExtension(KeyUsagesExtension);
+						if (extension) {
+							const hasKeyCertSign =
+								extension.usages & KeyUsageFlags.keyCertSign;
+							if (!hasKeyCertSign) {
+								return resolve(X509_ERR.NON_SIGNING);
+							}
 						}
+						// This branch is currently untested; it does not appear possible to
+						// get the error "unable to verify" with a self-signed certificate
+						// unless the key usage was the issue since it would have errored
+						// with "self-signed certificate" instead.
+						return resolve(X509_ERR.UNTRUSTED_LEAF);
 					},
 				);
 				socket.on("error", reject);

src/remote/remote.ts

@@ -344,6 +344,7 @@ export class Remote {
 								} catch (error) {
 									subscription.dispose();
 									reject(error);
+									return;
 								} finally {
 									inProgress = false;
 								}

test/unit/error.test.ts

@@ -1,6 +1,11 @@
+import {
+	KeyUsagesExtension,
+	X509Certificate as X509CertificatePeculiar,
+} from "@peculiar/x509";
 import axios from "axios";
-import * as fs from "fs/promises";
-import https from "https";
+import { X509Certificate as X509CertificateNode } from "node:crypto";
+import * as fs from "node:fs/promises";
+import https from "node:https";
 import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";
 
 import { CertificateError, X509_ERR, X509_ERR_CODE } from "@/error";
@@ -12,14 +17,11 @@ describe("Certificate errors", () => {
 	// Before each test we make a request to sanity check that we really get the
 	// error we are expecting, then we run it through CertificateError.
 
-	// TODO: These sanity checks need to be ran in an Electron environment to
-	// reflect real usage in VS Code.  We should either revert back to the standard
-	// extension testing framework which I believe runs in a headless VS Code
-	// instead of using vitest or at least run the tests through Electron running as
-	// Node (for now I do this manually by shimming Node).
-	const isElectron =
-		(process.versions.electron || process.env.ELECTRON_RUN_AS_NODE) &&
-		!process.env.VSCODE_PID; // Running from the test explorer in VS Code
+	// These tests run in Electron (BoringSSL) for accurate certificate validation testing.
+
+	it("should run in Electron environment", () => {
+		expect(process.versions.electron).toBeTruthy();
+	});
 
 	beforeAll(() => {
 		vi.mock("vscode", () => {
@@ -114,8 +116,7 @@ describe("Certificate errors", () => {
 	});
 
 	// In Electron a self-issued certificate without the signing capability fails
-	// (again with the same "unable to verify" error) but in Node self-issued
-	// certificates are not required to have the signing capability.
+	// (again with the same "unable to verify" error)
 	it("detects self-signed certificates without signing capability", async () => {
 		const address = await startServer("no-signing");
 		const request = axios.get(address, {
@@ -124,26 +125,16 @@ describe("Certificate errors", () => {
 				servername: "localhost",
 			}),
 		});
-		if (isElectron) {
-			await expect(request).rejects.toHaveProperty(
-				"code",
-				X509_ERR_CODE.UNABLE_TO_VERIFY_LEAF_SIGNATURE,
-			);
-			try {
-				await request;
-			} catch (error) {
-				const wrapped = await CertificateError.maybeWrap(
-					error,
-					address,
-					logger,
-				);
-				expect(wrapped instanceof CertificateError).toBeTruthy();
-				expect((wrapped as CertificateError).x509Err).toBe(
-					X509_ERR.NON_SIGNING,
-				);
-			}
-		} else {
-			await expect(request).resolves.toHaveProperty("data", "foobar");
+		await expect(request).rejects.toHaveProperty(
+			"code",
+			X509_ERR_CODE.UNABLE_TO_VERIFY_LEAF_SIGNATURE,
+		);
+		try {
+			await request;
+		} catch (error) {
+			const wrapped = await CertificateError.maybeWrap(error, address, logger);
+			expect(wrapped instanceof CertificateError).toBeTruthy();
+			expect((wrapped as CertificateError).x509Err).toBe(X509_ERR.NON_SIGNING);
 		}
 	});
 
@@ -157,6 +148,24 @@ describe("Certificate errors", () => {
 		await expect(request).resolves.toHaveProperty("data", "foobar");
 	});
 
+	// Node's X509Certificate.keyUsage is unreliable, so use a third-party parser
+	it("parses no-signing cert keyUsage with third-party library", async () => {
+		const certPem = await fs.readFile(
+			getFixturePath("tls", "no-signing.crt"),
+			"utf-8",
+		);
+
+		// Node's implementation seems to always return `undefined`
+		const nodeCert = new X509CertificateNode(certPem);
+		expect(nodeCert.keyUsage).toBeUndefined();
+
+		// Here we can correctly get the KeyUsages
+		const peculiarCert = new X509CertificatePeculiar(certPem);
+		const extension = peculiarCert.getExtension(KeyUsagesExtension);
+		expect(extension).toBeDefined();
+		expect(extension?.usages).toBeTruthy();
+	});
+
 	// Both environments give the same error code when a self-issued certificate is
 	// untrusted.
 	it("detects self-signed certificates", async () => {