Merge pull request #1626 from pacocartones/fix/api-key-timing-safe-comparison

fix(gateway): compare API keys in constant time

packages/core/src/gateway/auth/api-key-authorizer.ts

@@ -1,3 +1,4 @@
+import { timingSafeEqual } from "node:crypto";
 import type { IncomingMessage, ServerResponse } from "node:http";
 import type { ApiKeyConfig, AppConfig } from "@ccr/core/contracts/app";
 import { loadPersistedApiKeys } from "@ccr/core/config/config-repository";
@@ -24,10 +25,10 @@ export async function authorize(
   }
 
   const token = readAuthToken(request.headers) || readRemoteControlQueryAuthToken(request);
-  let apiKey = token ? apiKeys.find((item) => item.key === token) : undefined;
+  let apiKey = token ? findApiKeyByToken(apiKeys, token) : undefined;
   if (!apiKey && token) {
     apiKeys = await configuredApiKeys(config, { refresh: true });
-    apiKey = apiKeys.find((item) => item.key === token);
+    apiKey = findApiKeyByToken(apiKeys, token);
   }
   if (apiKey) {
     if (isApiKeyExpired(apiKey)) {
@@ -121,6 +122,16 @@ async function loadPersistedApiKeysCached(options: { refresh?: boolean } = {}):
   }
 }
 
+function findApiKeyByToken(apiKeys: ApiKeyConfig[], token: string): ApiKeyConfig | undefined {
+  return apiKeys.find((item) => constantTimeEqual(item.key, token));
+}
+
+function constantTimeEqual(left: string, right: string): boolean {
+  const leftBuffer = Buffer.from(left);
+  const rightBuffer = Buffer.from(right);
+  return leftBuffer.length === rightBuffer.length && timingSafeEqual(leftBuffer, rightBuffer);
+}
+
 function isApiKeyExpired(apiKey: ApiKeyConfig): boolean {
   if (!apiKey.expiresAt) return false;
   const expiresAt = Date.parse(apiKey.expiresAt);

packages/core/test/unit/gateway/api-key-authorizer.test.mjs

@@ -0,0 +1,119 @@
+import assert from "node:assert/strict";
+import { readFileSync } from "node:fs";
+import path from "node:path";
+import test from "node:test";
+import { createDefaultAppConfig } from "@ccr/core/config/default-config.ts";
+import { authorize } from "@ccr/core/gateway/auth/api-key-authorizer.ts";
+
+const authorizerSourceFile = path.join(
+  process.cwd(),
+  "packages",
+  "core",
+  "src",
+  "gateway",
+  "auth",
+  "api-key-authorizer.ts"
+);
+const gatewayApiKey = "ccr-3f8a1c7d9e2b4056a1c7d9e2b4056f8a";
+
+function configWithApiKeys(apiKeys) {
+  const config = createDefaultAppConfig();
+  config.APIKEYS = apiKeys;
+  return config;
+}
+
+function createResponse() {
+  const response = {
+    payload: undefined,
+    statusCode: undefined,
+    end(chunk) {
+      response.payload = JSON.parse(String(chunk));
+    },
+    writeHead(statusCode) {
+      response.statusCode = statusCode;
+    }
+  };
+  return response;
+}
+
+async function authorizeRequest(config, request) {
+  const response = createResponse();
+  const result = await authorize({ headers: {}, url: "/v1/messages", ...request }, response, config);
+  return { response, result };
+}
+
+test("gateway authorization accepts the configured API key on every supported carrier", async () => {
+  const config = configWithApiKeys([
+    { createdAt: new Date(0).toISOString(), id: "primary", key: gatewayApiKey }
+  ]);
+
+  for (const request of [
+    { headers: { authorization: `Bearer ${gatewayApiKey}` } },
+    { headers: { "x-api-key": gatewayApiKey } },
+    { url: `/__ccr/remote/status?api_key=${gatewayApiKey}` }
+  ]) {
+    const { response, result } = await authorizeRequest(config, request);
+
+    assert.equal(result.ok, true);
+    assert.equal(result.apiKey.id, "primary");
+    assert.equal(response.statusCode, undefined);
+  }
+});
+
+test("gateway authorization rejects near-miss tokens of any length without throwing", async () => {
+  const config = configWithApiKeys([
+    { createdAt: new Date(0).toISOString(), id: "primary", key: gatewayApiKey }
+  ]);
+
+  for (const token of [
+    `X${gatewayApiKey.slice(1)}`,
+    `${gatewayApiKey.slice(0, -1)}b`,
+    gatewayApiKey.toUpperCase(),
+    gatewayApiKey.slice(0, -1),
+    `${gatewayApiKey}-extra`
+  ]) {
+    const { response, result } = await authorizeRequest(config, {
+      headers: { authorization: `Bearer ${token}` }
+    });
+
+    assert.equal(result.ok, false);
+    assert.equal(response.statusCode, 401);
+    assert.equal(response.payload.error.message, "Invalid API key.");
+  }
+});
+
+test("gateway authorization separates a missing token from an expired key", async () => {
+  const config = configWithApiKeys([
+    {
+      createdAt: new Date(0).toISOString(),
+      expiresAt: new Date(Date.now() - 60_000).toISOString(),
+      id: "expired",
+      key: gatewayApiKey
+    }
+  ]);
+
+  const missing = await authorizeRequest(config, {});
+  assert.equal(missing.result.ok, false);
+  assert.equal(missing.response.statusCode, 401);
+  assert.equal(missing.response.payload.error.message, "API key is missing.");
+
+  const expired = await authorizeRequest(config, {
+    headers: { authorization: `Bearer ${gatewayApiKey}` }
+  });
+  assert.equal(expired.result.ok, false);
+  assert.equal(expired.response.statusCode, 401);
+  assert.equal(expired.response.payload.error.message, "API key is expired.");
+});
+
+// A constant-time comparison is behaviour-preserving by construction, so the
+// tests above pass on both sides of the change and only prove there is no
+// regression. The invariant itself is asserted on the module source, the same
+// way test/architecture/gateway-service-architecture.test.mjs asserts that the
+// config compiler never reaches for node:fs.
+test("gateway API key matching never uses a short-circuiting equality check", () => {
+  const source = readFileSync(authorizerSourceFile, "utf8");
+
+  assert.match(source, /from "node:crypto"/);
+  assert.match(source, /timingSafeEqual\(/);
+  assert.doesNotMatch(source, /\.key\s*===/);
+});