diff --git a/.github/workflows/docker-build.yml b/.github/workflows/docker-build.yml index 0460c8bc..125b0e41 100644 --- a/.github/workflows/docker-build.yml +++ b/.github/workflows/docker-build.yml @@ -112,7 +112,8 @@ jobs: platforms: linux/amd64,linux/arm64 cache-from: type=gha cache-to: type=gha,mode=max - provenance: false + provenance: true + sbom: true - name: Update Docker Hub description if: env.PUBLISH_DOCKERHUB == 'true' diff --git a/apps/frontend/electron.main.js b/apps/frontend/electron.main.js index 544a3151..cb84b328 100644 --- a/apps/frontend/electron.main.js +++ b/apps/frontend/electron.main.js @@ -2,10 +2,14 @@ const { app, BrowserWindow, ipcMain, safeStorage, shell, powerMonitor, session } = require("electron") const path = require("path") const { fork } = require("child_process") +const crypto = require("crypto") const { createApplicationMenu } = require("./menu") let serverProcess const ELECTRON = "Electron" +// Per-launch secret shared with the backend so only this app's renderer can open +// the WebSocket. Regenerated every launch; never persisted. +const wsToken = crypto.randomBytes(32).toString("hex") function startServer() { if (app.isPackaged) { const serverPath = path.join(process.resourcesPath, "server-backend.cjs") @@ -14,6 +18,7 @@ function startServer() { env: { ...process.env, DEPLOYMENT_MODE: ELECTRON, + ELECTRON_WS_TOKEN: wsToken, PROCESS_RESOURCES_PATH: process.resourcesPath, DATA_DIR: path.join(app.getPath("userData"), "metrics-data"), }, @@ -39,6 +44,8 @@ function createWindow() { nodeIntegration: false, contextIsolation: true, preload: path.join(__dirname, "preload.js"), + // Hand the per-launch token to the preload (readable via process.argv). + additionalArguments: [`--valkey-admin-ws-token=${wsToken}`], }, }) diff --git a/apps/frontend/preload.js b/apps/frontend/preload.js index c081778d..c640c79c 100644 --- a/apps/frontend/preload.js +++ b/apps/frontend/preload.js @@ -1,6 +1,14 @@ // eslint-disable-next-line @typescript-eslint/no-require-imports const { contextBridge, ipcRenderer } = require("electron") +// The main process passes the per-launch WebSocket token via additionalArguments. +const wsTokenArg = process.argv.find((arg) => arg.startsWith("--valkey-admin-ws-token=")) +const wsToken = wsTokenArg ? wsTokenArg.slice("--valkey-admin-ws-token=".length) : "" + +contextBridge.exposeInMainWorld("valkeyAdminRuntime", { + wsToken, +}) + contextBridge.exposeInMainWorld("secureStorage", { encrypt: (password) => ipcRenderer.invoke("secure-storage:encrypt", password), decrypt: (encrypted) => ipcRenderer.invoke("secure-storage:decrypt", encrypted), diff --git a/apps/frontend/src/state/epics/wsEpics.ts b/apps/frontend/src/state/epics/wsEpics.ts index faef520b..937d4b4b 100644 --- a/apps/frontend/src/state/epics/wsEpics.ts +++ b/apps/frontend/src/state/epics/wsEpics.ts @@ -29,7 +29,8 @@ let socket$: WebSocketSubject | null = null const getWebsocketURL = () => { // If it's an Electron deployment if (window.location.protocol === "file:") { - return "ws://localhost:8080" + const token = window.valkeyAdminRuntime?.wsToken + return token ? `ws://localhost:8080?token=${encodeURIComponent(token)}` : "ws://localhost:8080" } const protocol = window.location.protocol === "https:" ? "wss" : "ws" diff --git a/apps/frontend/src/types/electron.d.ts b/apps/frontend/src/types/electron.d.ts index dbbac7a1..401edc8e 100644 --- a/apps/frontend/src/types/electron.d.ts +++ b/apps/frontend/src/types/electron.d.ts @@ -2,8 +2,13 @@ export interface ElectronNavigation { onNavigate: (callback: (route: string) => void) => void } +export interface ValkeyAdminRuntime { + wsToken: string +} + declare global { interface Window { electronNavigation: ElectronNavigation + valkeyAdminRuntime?: ValkeyAdminRuntime } } diff --git a/apps/server/src/__tests__/metrics-orchestrator.test.ts b/apps/server/src/__tests__/metrics-orchestrator.test.ts index a2b615ad..3cc6e4a0 100644 --- a/apps/server/src/__tests__/metrics-orchestrator.test.ts +++ b/apps/server/src/__tests__/metrics-orchestrator.test.ts @@ -6,6 +6,8 @@ import { metricsServerMap, stopAllMetricsServers, reconcileClusterMetricsServers, + updateClusterNodeRegistry, + resolveClusterRefreshTarget, clients, clusterNodesRegistry, __test__, @@ -229,12 +231,6 @@ describe("metrics-orchestrator", () => { // Mock all side-effectful internal functions mock.method(__test__, "createClient", async () => ({})) - mock.method(__test__, "getClusterTopology", async () => ({ - clusterNodes: { - node1: { host: "127.0.0.1", port: "6379", tls: false, verifyTlsCertificate: false }, - }, - clusterId: "cluster-1", - })) mock.method(__test__, "updateClusterNodeRegistry", async () => mockClusterNodesRegistry) mock.method(__test__, "updateMetricsServers", async () => {}) mock.method(__test__, "findDiff", async () => ({ nodesToAdd: {}, nodesToRemove: [] })) @@ -271,4 +267,103 @@ describe("metrics-orchestrator", () => { assert.strictEqual(updateMetricsServers.mock.callCount(), 0) }) }) + + describe("topology refresh", () => { + beforeEach(() => { + mock.restoreAll() + clients.clear() + metricsServerMap.clear() + clusterNodesRegistry.clear() + }) + afterEach(() => { + mock.restoreAll() + clients.clear() + metricsServerMap.clear() + clusterNodesRegistry.clear() + }) + + it("updateClusterNodeRegistry replaces stale topology with the freshly discovered one", async () => { + // Seed a stale registry: cluster "node-1" knows only one node. + clusterNodesRegistry.set("node-1", { + "192-168-1-1-6379": { host: "192.168.1.1", port: 6379, tls: false, verifyTlsCertificate: false }, + }) + + // A live client for that cluster whose CLUSTER SLOTS now reports three + // primaries. (SLOTS parsing itself is covered in connection.test.ts.) + const client = { + customCommand: async (args: string[]) => + args[0] === "CLUSTER" && args[1] === "SLOTS" + ? [ + [0, 5460, ["192.168.1.1", 6379, "node-1"]], + [5461, 10922, ["192.168.1.3", 6379, "node-2"]], + [10923, 16383, ["192.168.1.4", 6379, "node-3"]], + ] + : [], + } as never + + const sampleNode = Object.values(clusterNodesRegistry.get("node-1") ?? {})[0] + await updateClusterNodeRegistry(client, sampleNode) + + assert.deepStrictEqual( + Object.keys(clusterNodesRegistry.get("node-1") ?? {}).sort(), + ["192-168-1-1-6379", "192-168-1-3-6379", "192-168-1-4-6379"], + "registry should reflect the newly discovered topology, not the stale snapshot", + ) + }) + + it("overwrites the existing entry when a known clusterId is passed, even if the derived id changed", async () => { + // Existing cluster tracked under "orig-id". + clusterNodesRegistry.set("orig-id", { + "192-168-1-1-6379": { host: "192.168.1.1", port: 6379, tls: false, verifyTlsCertificate: false }, + }) + + // After a failover, CLUSTER SLOTS now lists a different first primary, so the + // derived clusterId would be "new-first-primary" — which would orphan "orig-id". + const client = { + customCommand: async (args: string[]) => + args[0] === "CLUSTER" && args[1] === "SLOTS" + ? [[0, 16383, ["192.168.1.9", 6379, "new-first-primary"]]] + : [], + } as never + + const sampleNode = Object.values(clusterNodesRegistry.get("orig-id") ?? {})[0] + await updateClusterNodeRegistry(client, sampleNode, "orig-id") + + assert.deepStrictEqual( + [...clusterNodesRegistry.keys()], + ["orig-id"], + "the known clusterId should be overwritten in place, leaving no orphaned entry", + ) + assert.deepStrictEqual( + Object.keys(clusterNodesRegistry.get("orig-id") ?? {}), + ["192-168-1-9-6379"], + "the entry should hold the freshly discovered node", + ) + }) + + it("resolveClusterRefreshTarget uses the user client and carries node metadata forward", async () => { + const userClient = { id: "user-client" } as never + const clusterNodes = { + node1: { host: "10.0.0.1", port: 6379, tls: true, verifyTlsCertificate: false, username: "admin", authType: "iam" as const }, + } + + const target = await resolveClusterRefreshTarget("cluster-1", clusterNodes, userClient) + + assert.strictEqual(target?.client, userClient, "should refresh with the cluster's own live client") + // nodeInfo must be a node from THIS cluster (preserving its tls/username/auth), + // not initialConnectionDetails. + assert.deepStrictEqual(target?.nodeInfo, clusterNodes.node1, "should carry the cluster's own node metadata forward") + }) + + it("resolveClusterRefreshTarget skips a cluster with no live client when not preconfigured", async () => { + // DEPLOYMENT_MODE is unset in this file, so preConfiguredConnection is falsy + // and there is no initial client to fall back to. + const target = await resolveClusterRefreshTarget( + "cluster-1", + { node1: { host: "10.0.0.1", port: 6379, tls: false, verifyTlsCertificate: false } }, + undefined, + ) + assert.strictEqual(target, undefined, "a cluster with no live client and no preconfigured fallback is skipped") + }) + }) }) diff --git a/apps/server/src/__tests__/orchestrator-k8s-register.test.ts b/apps/server/src/__tests__/orchestrator-k8s-register.test.ts new file mode 100644 index 00000000..3512fabf --- /dev/null +++ b/apps/server/src/__tests__/orchestrator-k8s-register.test.ts @@ -0,0 +1,114 @@ +import { describe, it, afterEach } from "node:test" +import assert from "node:assert" +import { + ORCHESTRATOR_AUTH_DOMAIN, + ORCHESTRATOR_AUTH_HEADER, + createOrchestratorAuthCredential +} from "valkey-common" +import type { Request, Response } from "express" + +// These tests exercise the Kubernetes-only branches of the orchestrator: +// - resolveCollectorKey falls back to the shared ORCHESTRATOR_KEY, and +// - handleRegister admits a sidecar whose nodeId is part of the discovered +// cluster topology even though the orchestrator never spawned it. +// isKubernetes is a module-load constant, so DEPLOYMENT_MODE and the shared key +// must be set BEFORE importing the module. +process.env.DEPLOYMENT_MODE = "K8" +process.env.ORCHESTRATOR_KEY = "k8s-shared-secret" + +const { + metricsServerMap, + clusterNodesRegistry, + resolveCollectorKey, + __test__, +} = await import("../metrics-orchestrator") + +const NODE_ID = "valkey-6-valkey-headless-valkey-svc-cluster-local-6379" +const URI = "http://10.42.0.32:3000" +const SHARED_KEY = "k8s-shared-secret" + +const makeRes = () => { + const captured: { statusCode: number; body?: unknown } = { statusCode: 200 } + const res = { + status(code: number) { captured.statusCode = code; return res }, + send(body?: unknown) { captured.body = body; return res }, + sendStatus(code: number) { captured.statusCode = code; return res }, + } + return { res: res as unknown as Response, captured } +} + +const makeReq = (body: unknown, credential?: string) => ({ + body, + headers: credential === undefined ? {} : { [ORCHESTRATOR_AUTH_HEADER]: credential }, +}) as unknown as Request + +const signRegister = (fields: { nodeId?: string; metricsServerUri?: string; timestamp?: number } = {}, key = SHARED_KEY) => + createOrchestratorAuthCredential(key, ORCHESTRATOR_AUTH_DOMAIN.REGISTER, { + nodeId: NODE_ID, + metricsServerUri: URI, + timestamp: Date.now(), + ...fields, + }) as string + +// Register a node into the discovered topology so it is a known cluster member. +const seedTopology = (nodeId = NODE_ID) => { + clusterNodesRegistry.set("cluster-1", { + [nodeId]: { host: "10.42.0.32", port: 6379, tls: false, verifyTlsCertificate: false }, + }) +} + +describe("K8s shared-key registration", () => { + afterEach(() => { + metricsServerMap.clear() + clusterNodesRegistry.clear() + __test__.collectorKeys.clear() + }) + + describe("resolveCollectorKey", () => { + it("falls back to the shared ORCHESTRATOR_KEY for a node the orchestrator never spawned", () => { + assert.strictEqual(resolveCollectorKey("never-spawned-node"), SHARED_KEY) + }) + + it("still prefers a per-node minted key when one exists", () => { + __test__.collectorKeys.set("spawned-node", "per-node-key") + assert.strictEqual(resolveCollectorKey("spawned-node"), "per-node-key") + }) + }) + + describe("handleRegister topology-membership gate", () => { + it("admits a correctly signed sidecar for a known cluster node with no pre-existing entry", () => { + seedTopology() + assert.strictEqual(metricsServerMap.has(NODE_ID), false, "precondition: no orchestrator-spawned entry") + + const { res, captured } = makeRes() + __test__.handleRegister(makeReq({ nodeId: NODE_ID, metricsServerUri: URI, timestamp: Date.now() }, signRegister()), res) + + assert.strictEqual(captured.statusCode, 200, "a known cluster node should be allowed to register") + assert.strictEqual(metricsServerMap.get(NODE_ID)?.metricsURI, URI, "the sidecar's entry should be created on first register") + }) + + it("rejects a correctly signed sidecar whose nodeId is not in the discovered topology", () => { + // Topology known, but for a different node. + seedTopology("some-other-node-6379") + + const { res, captured } = makeRes() + __test__.handleRegister(makeReq({ nodeId: NODE_ID, metricsServerUri: URI, timestamp: Date.now() }, signRegister()), res) + + assert.strictEqual(captured.statusCode, 401, "an unknown node must be rejected even with a valid signature") + assert.strictEqual(metricsServerMap.has(NODE_ID), false) + }) + + it("rejects a known cluster node when the signature uses the wrong key", () => { + seedTopology() + + const { res, captured } = makeRes() + __test__.handleRegister( + makeReq({ nodeId: NODE_ID, metricsServerUri: URI, timestamp: Date.now() }, signRegister({}, "wrong-key")), + res, + ) + + assert.strictEqual(captured.statusCode, 401, "a bad signature must be rejected before the membership check") + assert.strictEqual(metricsServerMap.has(NODE_ID), false) + }) + }) +}) diff --git a/apps/server/src/__tests__/topology-refresh-preconfigured.test.ts b/apps/server/src/__tests__/topology-refresh-preconfigured.test.ts new file mode 100644 index 00000000..83ee5fa0 --- /dev/null +++ b/apps/server/src/__tests__/topology-refresh-preconfigured.test.ts @@ -0,0 +1,70 @@ +import { describe, it, afterEach, mock } from "node:test" +import assert from "node:assert" + +// preConfiguredConnection is a module-load constant derived from VALKEY_HOST / +// VALKEY_PORT, so it must be set BEFORE importing the module to exercise the +// headless-refresh fallback (no live user client → initial client). +process.env.VALKEY_HOST = "valkey-0.example" +process.env.VALKEY_PORT = "6379" + +const { + resolveClusterRefreshTarget, + setPreconfiguredClusterId, + initialConnectionDetails, + __test__, +} = await import("../metrics-orchestrator") + +const PRECONFIGURED_ID = "preconfigured-cluster" + +describe("resolveClusterRefreshTarget (preconfigured / headless)", () => { + afterEach(() => { + mock.restoreAll() + setPreconfiguredClusterId(undefined) + }) + + it("refreshes the preconfigured cluster via the initial client + initialConnectionDetails", async () => { + setPreconfiguredClusterId(PRECONFIGURED_ID) + const initialClient = { id: "initial-client" } as never + mock.method(__test__, "getInitialClient", async () => initialClient) + + const target = await resolveClusterRefreshTarget( + PRECONFIGURED_ID, + { node1: { host: "10.0.0.1", port: 6379, tls: false, verifyTlsCertificate: false } }, + undefined, + ) + + assert.strictEqual(target?.client, initialClient, "the preconfigured cluster should refresh via the initial client") + assert.strictEqual( + target?.nodeInfo, + initialConnectionDetails, + "the preconfigured cluster decorates with initialConnectionDetails", + ) + }) + + it("does NOT use the initial client for a different client-less cluster", async () => { + // Preconfigured cluster is A; refreshing a stale, client-less cluster B must + // not rediscover A's topology through the initial client. + setPreconfiguredClusterId(PRECONFIGURED_ID) + const getInitial = mock.method(__test__, "getInitialClient", async () => ({ id: "initial-client" } as never)) + + const target = await resolveClusterRefreshTarget( + "some-other-cluster", + { node1: { host: "10.9.9.9", port: 6379, tls: false, verifyTlsCertificate: false } }, + undefined, + ) + + assert.strictEqual(target, undefined, "an unrelated client-less cluster must be left unchanged") + assert.strictEqual(getInitial.mock.callCount(), 0, "the initial client must not be consulted for other clusters") + }) + + it("still prefers a user client over the initial client when one is present", async () => { + const userClient = { id: "user-client" } as never + mock.method(__test__, "getInitialClient", async () => ({ id: "initial-client" } as never)) + + const clusterNodes = { node1: { host: "10.0.0.1", port: 6379, tls: true, verifyTlsCertificate: false } } + const target = await resolveClusterRefreshTarget("any-cluster", clusterNodes, userClient) + + assert.strictEqual(target?.client, userClient) + assert.deepStrictEqual(target?.nodeInfo, clusterNodes.node1) + }) +}) diff --git a/apps/server/src/__tests__/websocket-origin.test.ts b/apps/server/src/__tests__/websocket-origin.test.ts index 5f7d1b9f..be945ec1 100644 --- a/apps/server/src/__tests__/websocket-origin.test.ts +++ b/apps/server/src/__tests__/websocket-origin.test.ts @@ -4,16 +4,18 @@ import { DEPLOYMENT_TYPE } from "valkey-common" import { isAllowedWebSocketOrigin } from "../websocket-origin" import type { IncomingMessage } from "http" -const makeRequest = (headers: Record) => - ({ headers }) as IncomingMessage +const makeRequest = ({ url, ...headers }: Record) => + ({ headers, url }) as IncomingMessage describe("isAllowedWebSocketOrigin", () => { const originalDeploymentMode = process.env.DEPLOYMENT_MODE const originalAllowedOrigins = process.env.VALKEY_ADMIN_ALLOWED_WS_ORIGINS + const originalWsToken = process.env.ELECTRON_WS_TOKEN afterEach(() => { process.env.DEPLOYMENT_MODE = originalDeploymentMode process.env.VALKEY_ADMIN_ALLOWED_WS_ORIGINS = originalAllowedOrigins + process.env.ELECTRON_WS_TOKEN = originalWsToken }) it("rejects requests without an origin header", () => { @@ -22,28 +24,69 @@ describe("isAllowedWebSocketOrigin", () => { assert.strictEqual(isAllowedWebSocketOrigin(makeRequest({ host: "localhost:8080" })), false) }) - it("allows packaged Electron origins", () => { + it("allows packaged Electron origins only with a valid per-launch token", () => { process.env.DEPLOYMENT_MODE = DEPLOYMENT_TYPE.ELECTRON + process.env.ELECTRON_WS_TOKEN = "secret-token" + + for (const origin of ["file://", "null"]) { + assert.strictEqual( + isAllowedWebSocketOrigin(makeRequest({ origin, host: "localhost:8080", url: "/?token=secret-token" })), + true, + `${origin} with valid token should be allowed`, + ) + } + }) + + it("rejects Electron non-web origins when the token is missing or wrong", () => { + process.env.DEPLOYMENT_MODE = DEPLOYMENT_TYPE.ELECTRON + process.env.ELECTRON_WS_TOKEN = "secret-token" + + for (const origin of ["file://", "null"]) { + // No token + assert.strictEqual( + isAllowedWebSocketOrigin(makeRequest({ origin, host: "localhost:8080", url: "/" })), + false, + `${origin} without a token must be rejected`, + ) + // Wrong token + assert.strictEqual( + isAllowedWebSocketOrigin(makeRequest({ origin, host: "localhost:8080", url: "/?token=nope" })), + false, + `${origin} with a wrong token must be rejected`, + ) + } + }) + + it("fails closed when no server-side token is provisioned", () => { + process.env.DEPLOYMENT_MODE = DEPLOYMENT_TYPE.ELECTRON + delete process.env.ELECTRON_WS_TOKEN assert.strictEqual( - isAllowedWebSocketOrigin(makeRequest({ origin: "file://", host: "localhost:8080" })), - true, - ) - assert.strictEqual( - isAllowedWebSocketOrigin(makeRequest({ origin: "null", host: "localhost:8080" })), - true, + isAllowedWebSocketOrigin(makeRequest({ origin: "null", host: "localhost:8080", url: "/?token=anything" })), + false, ) }) - it("allows loopback origins in Electron mode and blocks remote origins", () => { + it("allows loopback origins in Electron mode only with a token and blocks remote origins", () => { process.env.DEPLOYMENT_MODE = DEPLOYMENT_TYPE.ELECTRON + process.env.ELECTRON_WS_TOKEN = "secret-token" assert.strictEqual( - isAllowedWebSocketOrigin(makeRequest({ origin: "http://localhost:5173", host: "localhost:8080" })), + isAllowedWebSocketOrigin( + makeRequest({ origin: "http://localhost:5173", host: "localhost:8080", url: "/?token=secret-token" }), + ), true, ) + // Loopback origin but no token + assert.strictEqual( + isAllowedWebSocketOrigin(makeRequest({ origin: "http://localhost:5173", host: "localhost:8080", url: "/" })), + false, + ) + // Remote origin is rejected regardless of token assert.strictEqual( - isAllowedWebSocketOrigin(makeRequest({ origin: "https://evil.example", host: "localhost:8080" })), + isAllowedWebSocketOrigin( + makeRequest({ origin: "https://evil.example", host: "localhost:8080", url: "/?token=secret-token" }), + ), false, ) }) diff --git a/apps/server/src/connection.ts b/apps/server/src/connection.ts index 191853e9..7587490d 100644 --- a/apps/server/src/connection.ts +++ b/apps/server/src/connection.ts @@ -22,7 +22,8 @@ import { reconcileClusterMetricsServers, isKubernetes, forgetCollectorKey, - ClusterNodeMap } from "./metrics-orchestrator" + ClusterNodeMap, + type NodeInfo } from "./metrics-orchestrator" import { subscribe } from "./node-watchers" import { clearCpuSamples } from "./node-utilization" import { createClusterValkeyClient, createStandaloneValkeyClient } from "./valkey-client" @@ -595,7 +596,7 @@ function sendStandaloneConnectFulfilled(ws: WebSocket, payload: StandaloneConnec export async function discoverCluster( client: GlideClient | GlideClusterClient, - payload: { connectionDetails: ConnectionDetails, connectionId?: string;}, + payload: { connectionDetails: NodeInfo, connectionId?: string;}, ) { try { // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/apps/server/src/index.ts b/apps/server/src/index.ts index b12006ae..8fae6514 100644 --- a/apps/server/src/index.ts +++ b/apps/server/src/index.ts @@ -34,7 +34,6 @@ import { memoryUsageRequested } from "./actions/memoryUsage" import { monitorRequested } from "./actions/monitorAction" import { unsubscribeAll, getWatcherCount } from "./node-watchers" import { teardownConnection } from "./connection" -import { isElectron } from "./metrics-orchestrator" import { Handler, ReduxAction, safeSend, unknownHandler, type WsActionMessage } from "./actions/utils" import { createMetricsOrchestratorRouter, @@ -47,7 +46,9 @@ import { isKubernetes, preConfiguredConnection, getInitialClient, - updateClusterNodeRegistry + updateClusterNodeRegistry, + resolveClusterRefreshTarget, + setPreconfiguredClusterId } from "./metrics-orchestrator" import { isAllowedWebSocketOrigin } from "./websocket-origin" import { ensureSession, hasAuthorizedSession, isConnectionAuthorized, setSessionExpiryListener } from "./session" @@ -143,7 +144,14 @@ const wss = new WebSocketServer({ noServer: true }) const delay = (ms: number) => new Promise((res) => setTimeout(res, ms)) -function refreshAllClusterRegistries() { +// Upper bound on a single cluster's topology re-discovery. Without it, one hung +// CLUSTER SLOTS would leave the awaited refresh unsettled and stall the broadcast +// for every cluster, not just the unresponsive one. +const TOPOLOGY_REDISCOVERY_TIMEOUT_MS = 10_000 + +async function refreshAllClusterRegistries() { + // Group live connections by cluster once; used both to pick a client to + // re-discover each cluster's topology and to target the broadcast below. const connectionIdsByCluster = new Map() for (const [connectionId, entry] of clients) { if (!entry.clusterId) continue @@ -152,6 +160,28 @@ function refreshAllClusterRegistries() { connectionIdsByCluster.set(entry.clusterId, ids) } + // Re-discover each tracked cluster before broadcasting. User-connected clusters + // use their own client + node metadata; a preconfigured cluster (K8s / headless + // Web) has no entry in `clients`, so it falls back to the initial client so + // scaled-in nodes are picked up without a UI session. Time-bounded so one hung + // node can't stall the whole loop. + await Promise.all( + [...clusterNodesRegistry.entries()].map(async ([clusterId, clusterNodes]) => { + const connectionId = connectionIdsByCluster.get(clusterId)?.[0] + const userClient = connectionId ? clients.get(connectionId)?.client : undefined + + const target = await resolveClusterRefreshTarget(clusterId, clusterNodes, userClient) + if (!target) return + + await Promise.race([ + updateClusterNodeRegistry(target.client, target.nodeInfo, clusterId), + delay(TOPOLOGY_REDISCOVERY_TIMEOUT_MS).then(() => + console.warn(`Topology re-discovery for cluster ${clusterId} timed out; broadcasting last known nodes.`), + ), + ]) + }), + ) + for (const [clusterId, clusterNodes] of clusterNodesRegistry) { const connectionIds = connectionIdsByCluster.get(clusterId) if (!connectionIds) continue @@ -172,7 +202,7 @@ function refreshAllClusterRegistries() { async function refreshAllClusterRegistriesLoop() { while (true) { try { - refreshAllClusterRegistries() + await refreshAllClusterRegistries() } catch (err) { console.warn("Unable to refresh cluster topologies. ", err) } finally { @@ -184,13 +214,16 @@ async function refreshAllClusterRegistriesLoop() { async function updateRegistryforK8() { const client = await getInitialClient() - updateClusterNodeRegistry(client, initialConnectionDetails) + const clusterId = await updateClusterNodeRegistry(client, initialConnectionDetails) + setPreconfiguredClusterId(clusterId) } -// Electron: bind to localhost only — Origin headers are forgeable by non-browser clients, -// so network-level isolation is the only reliable gate for a desktop app. -server.listen(port, isElectron ? "127.0.0.1" : undefined, () => { - console.log(`Server running at http://localhost:${port}`) +// Default to loopback so an unauthenticated Web server isn't reachable off-host; +// containers (Docker/K8s) set SERVER_BIND_HOST=0.0.0.0 explicitly. +const bindHost = + process.env.SERVER_BIND_HOST ?? (isKubernetes ? "0.0.0.0" : "127.0.0.1") +server.listen(port, bindHost, () => { + console.log(`Server running at http://${bindHost}:${port}`) if (process.send) { // Check if process.send is available (i.e., if forked) process.send({ type: "websocket-ready" }) // Send a ready message to the parent process } diff --git a/apps/server/src/metrics-orchestrator.ts b/apps/server/src/metrics-orchestrator.ts index 4d88a414..b71792cb 100644 --- a/apps/server/src/metrics-orchestrator.ts +++ b/apps/server/src/metrics-orchestrator.ts @@ -43,7 +43,7 @@ export type MetricsServerMap = Map -type NodeInfo = { +export type NodeInfo = { host: string; port: number | string; username?: string; @@ -64,6 +64,11 @@ export const clusterNodesRegistry: Map = new Map() export const clusterCredentials: Map = new Map() +let preconfiguredClusterId: string | undefined +export function setPreconfiguredClusterId(clusterId: string | undefined) { + preconfiguredClusterId = clusterId +} + export const metricsServerMap: MetricsServerMap = new Map() /** @@ -79,7 +84,14 @@ const collectorKeys: Map = new Map() * replayed against another. */ export function resolveCollectorKey(nodeId: string): string | undefined { - return collectorKeys.get(nodeId) + const spawnedKey = collectorKeys.get(nodeId) + if (spawnedKey) return spawnedKey + // K8s collectors are external sidecars the orchestrator never spawns, so there + // is no per-node minted key. They authenticate with a shared key provisioned to + // both the orchestrator and every sidecar via a Kubernetes Secret. Scoped to K8s + // so the per-node property is preserved for spawned (Web/Electron) collectors. + if (isKubernetes) return process.env[ORCHESTRATOR_AUTH_KEY_ENV] + return undefined } /** @@ -225,6 +237,18 @@ function isPinnedMetricsHost(uri: string): boolean { } } +/** + * True when `nodeId` is part of a discovered cluster topology. In K8s the + * orchestrator only tracks topology (it never spawns collectors), so this is + * how a sidecar's registration is authorized as belonging to the cluster. + */ +function isKnownClusterNode(nodeId: string): boolean { + for (const clusterNodes of clusterNodesRegistry.values()) { + if (flattenClusterNodeMap(clusterNodes)[nodeId]) return true + } + return false +} + /** * `POST /orchestrator/register` — a collector advertising where it can be * reached. @@ -269,14 +293,22 @@ function handleRegister(req: Request, res: Response): void { return } - const entry = metricsServerMap.get(nodeId) - if (!entry) { - // Key material without an entry means the spawn did not complete. + // Allowed if we already have an entry (orchestrator-spawned) or, in K8s, + // the node is part of the discovered cluster (sidecars are external, so + // they create their entry on first register). + const allowed = metricsServerMap.has(nodeId) || (isKubernetes && isKnownClusterNode(nodeId)) + if (!allowed) { console.warn(`Rejected metrics registration for ${nodeId}: no metrics server entry`) res.status(401).send("Unauthorized") return } + let entry = metricsServerMap.get(nodeId) + if (!entry) { + entry = { metricsURI: "", pid: undefined, lastSeen: Date.now() } + metricsServerMap.set(nodeId, entry) + } + entry.metricsURI = metricsServerUri entry.lastSeen = Date.now() console.log(`Metrics server registered for ${nodeId} at ${metricsServerUri}`) @@ -359,18 +391,21 @@ async function createClient(connectionDetails: ConnectionDetails) { return await createOrchestratorValkeyClient({ addresses, credentials, useTLS: tls, verifyTlsCertificate, databaseId: db }) } -async function getClusterTopology(client: GlideClusterClient | GlideClient | null, node: ConnectionDetails) { - if (!client) client = await createClient(node) - - const { discoveredClusterNodes, clusterId } = await discoverCluster(client, { connectionDetails: node }) - - return { discoveredClusterNodes, clusterId } -} - -export async function updateClusterNodeRegistry(client: GlideClusterClient | GlideClient | null, connectionDetails = initialConnectionDetails) { +export async function updateClusterNodeRegistry( + client: GlideClusterClient | GlideClient, + nodeInfo: NodeInfo, + clusterId?: string, +) { try { - const { discoveredClusterNodes, clusterId } = await getClusterTopology(client, connectionDetails) - if (clusterId && discoveredClusterNodes) clusterNodesRegistry.set(clusterId, discoveredClusterNodes) + const { discoveredClusterNodes, clusterId: discoveredClusterId } = await discoverCluster(client, { connectionDetails: nodeInfo }) + // Prefer the caller's known clusterId when refreshing an existing cluster: the + // discovered id is derived from the first primary in CLUSTER SLOTS, which can + // change on failover/resharding and would otherwise orphan the old entry. + const key = clusterId ?? discoveredClusterId + if (key && discoveredClusterNodes) { + clusterNodesRegistry.set(key, discoveredClusterNodes) + return key + } } catch (err) { if (err instanceof ConnectionError) { @@ -378,7 +413,29 @@ export async function updateClusterNodeRegistry(client: GlideClusterClient | Gli } console.error(err) } - return clusterNodesRegistry + return undefined +} + +/** + * Decide how to refresh one tracked cluster. A user-connected cluster is refreshed + * with its own client and metadata carried forward from an existing node (so a + * refresh doesn't revert TLS/auth to server defaults). A preconfigured cluster + * (K8s / headless Web) has no live user client, so it falls back to the initial + * client with initialConnectionDetails. Returns undefined when the cluster can't + * be refreshed (no client available). + */ +export async function resolveClusterRefreshTarget( + clusterId: string, + clusterNodes: ClusterNodeMap, + userClient: GlideClusterClient | GlideClient | undefined, +): Promise<{ client: GlideClusterClient | GlideClient; nodeInfo: NodeInfo } | undefined> { + if (userClient) return { client: userClient, nodeInfo: Object.values(clusterNodes)[0] } + + // Only the preconfigured cluster may be refreshed via the initial client. + if (preConfiguredConnection && clusterId === preconfiguredClusterId) { + return { client: await internals.getInitialClient(), nodeInfo: initialConnectionDetails } + } + return undefined } async function findDiff(metricsServerMap: MetricsServerMap, clusterNodeMap: ClusterNodeMap) { @@ -582,9 +639,10 @@ export async function startPreconfiguredMetricsServers() { const client = await getInitialClient() if (await belongsToCluster(client)) { if (isWebMode) { - const { discoveredClusterNodes, clusterId } = await internals.getClusterTopology(client, initialConnectionDetails) + const { discoveredClusterNodes, clusterId } = await discoverCluster(client, { connectionDetails: initialConnectionDetails }) if (clusterId && discoveredClusterNodes) { clusterNodesRegistry.set(clusterId, discoveredClusterNodes) + setPreconfiguredClusterId(clusterId) if (!clusterCredentials.has(clusterId)) clusterCredentials.set(clusterId, initialConnectionDetails.password) } runReconcileLoop() @@ -616,7 +674,7 @@ export function cleanupOrchestratorResources() { const internals = { startMetricsServers, createClient, - getClusterTopology, + getInitialClient, updateClusterNodeRegistry, findDiff, flattenClusterNodeMap, diff --git a/apps/server/src/websocket-origin.ts b/apps/server/src/websocket-origin.ts index 8a8172e5..d93ededa 100644 --- a/apps/server/src/websocket-origin.ts +++ b/apps/server/src/websocket-origin.ts @@ -1,10 +1,12 @@ import { DEPLOYMENT_TYPE } from "valkey-common" +import { timingSafeEqual } from "crypto" import type { IncomingMessage } from "http" const LOCALHOST_HOSTNAMES = new Set(["localhost", "127.0.0.1", "::1", "[::1]"]) const LOCAL_PROTOCOLS = new Set(["http:", "https:"]) -// Electron file:// renderers may send Origin as "file://" or "null" on the WebSocket handshake. -const ELECTRON_ORIGINS = new Set(["null", "file://"]) +// Non-web origins a file:// renderer may present; accepted only with a valid token. +const ELECTRON_NONWEB_ORIGINS = new Set(["null", "file://"]) +const ELECTRON_WS_TOKEN_ENV = "ELECTRON_WS_TOKEN" const normalizeHost = (hostname: string) => hostname.replace(/^\[|]$/g, "").toLowerCase() @@ -34,6 +36,29 @@ const isSameOrigin = (origin: URL, req: IncomingMessage) => { return normalizeOrigin(origin.origin) === `${origin.protocol}//${hostHeader.toLowerCase()}` } +// Length-independent comparison so the token can't be recovered by timing. +const tokensMatch = (a: string, b: string) => { + const ab = Buffer.from(a) + const bb = Buffer.from(b) + // timingSafeEqual requires equal lengths; the token is fixed-length so this + // guard leaks nothing useful. + return ab.length === bb.length && timingSafeEqual(ab, bb) +} + +// The renderer appends the per-launch token as `?token=...` on the WS URL. +const hasValidElectronToken = (req: IncomingMessage) => { + const expected = process.env[ELECTRON_WS_TOKEN_ENV] + // Fail closed: if no token was provisioned, the token gate cannot be satisfied. + if (!expected) return false + try { + const url = new URL(req.url ?? "", "http://localhost") + const provided = url.searchParams.get("token") + return provided != null && tokensMatch(provided, expected) + } catch { + return false + } +} + export const isAllowedWebSocketOrigin = (req: IncomingMessage) => { // Browsers send Origin on WebSocket handshakes, so we can reject cross-site pages before accepting the upgrade. const originHeader = req.headers.origin @@ -51,11 +76,16 @@ export const isAllowedWebSocketOrigin = (req: IncomingMessage) => { } if (deploymentMode === DEPLOYMENT_TYPE.ELECTRON) { - try { - return ELECTRON_ORIGINS.has(normalizedOrigin) || isLoopbackOrigin(new URL(normalizedOrigin)) - } catch { // new URL can technically throw - return false + // Require a valid per-launch token alongside the local renderer origin. + let originLooksLocal = ELECTRON_NONWEB_ORIGINS.has(normalizedOrigin) + if (!originLooksLocal) { + try { + originLooksLocal = isLoopbackOrigin(new URL(normalizedOrigin)) + } catch { // new URL can technically throw + originLooksLocal = false + } } + return originLooksLocal && hasValidElectronToken(req) } try { // for Web deployment — only same origin is allowed diff --git a/docker/Dockerfile.app b/docker/Dockerfile.app index 101e6959..ffeccc74 100644 --- a/docker/Dockerfile.app +++ b/docker/Dockerfile.app @@ -23,6 +23,10 @@ WORKDIR /app ENV NODE_ENV=production ENV DEPLOYMENT_MODE=Web +# Bind all interfaces inside the container so published ports (-p) are reachable. +# The server itself defaults to loopback off-container; this is the explicit +# container opt-in to external exposure. +ENV SERVER_BIND_HOST=0.0.0.0 # Copy root workspace files COPY package.json package-lock.json ./ diff --git a/docs-site/src/content/docs/configuration/server.md b/docs-site/src/content/docs/configuration/server.md index b03aa830..c91384f2 100644 --- a/docs-site/src/content/docs/configuration/server.md +++ b/docs-site/src/content/docs/configuration/server.md @@ -38,6 +38,18 @@ The TCP port the Express + WebSocket server listens on. The same port serves the PORT=9090 ``` +### `SERVER_BIND_HOST` + +Network interface the server binds to. Defaults to loopback (`127.0.0.1`) so an unauthenticated Web server is not reachable off-host unless you opt in. Web mode has no authentication, so only bind a routable interface when access is otherwise restricted (authenticating proxy, security groups, etc.). + +- **Default:** `127.0.0.1` (loopback); `0.0.0.0` when `DEPLOYMENT_MODE=K8` +- **Read in:** `apps/server/src/index.ts` +- **Note:** The Docker image sets `SERVER_BIND_HOST=0.0.0.0` so published ports (`-p`) are reachable; bind the published port to loopback (`-p 127.0.0.1:8080:8080`) to keep it local. + +```bash +SERVER_BIND_HOST=0.0.0.0 +``` + ## Mode & Orchestrator ### `DEPLOYMENT_MODE` diff --git a/docs-site/src/content/docs/deployment/aws-elasticache.md b/docs-site/src/content/docs/deployment/aws-elasticache.md index 080b9d78..3bc1886c 100644 --- a/docs-site/src/content/docs/deployment/aws-elasticache.md +++ b/docs-site/src/content/docs/deployment/aws-elasticache.md @@ -19,6 +19,15 @@ docker run -d --name valkey-admin \ Open `http://:8080` and add a connection to your ElastiCache endpoint through the UI. +:::caution[This publishes an unauthenticated service on a routable interface] +Web mode has **no authentication**, and `-p 8080:8080` publishes it on all +interfaces — anyone who can reach `:8080` can access your +ElastiCache cluster through it. For anything beyond a throwaway test, use the +**Production Deployment** below (HTTPS + Cognito), keep the port bound to loopback +(`-p 127.0.0.1:8080:8080`), and restrict access with security groups. Treat +external exposure as a deliberate decision. +::: + For IAM authentication, ensure the instance's IAM role has `elasticache:Connect` permission scoped to your replication group and user. --- diff --git a/docs-site/src/content/docs/deployment/docker.md b/docs-site/src/content/docs/deployment/docker.md index 7f80469c..685cdd5a 100644 --- a/docs-site/src/content/docs/deployment/docker.md +++ b/docs-site/src/content/docs/deployment/docker.md @@ -42,6 +42,18 @@ docker compose up -d Open `http://localhost:8080` and add a connection to your Valkey instance through the UI. +:::caution[Publishing `8080` exposes an unauthenticated service] +Valkey Admin Web mode has **no authentication** — anyone who can reach the published +port can use it to access your Valkey instances. `-p 8080:8080` (and the compose +`ports: "8080:8080"`) publishes it on **all** host interfaces. + +- Keep it local: bind the published port to loopback with `-p 127.0.0.1:8080:8080` + (or `ports: "127.0.0.1:8080:8080"`). +- Only publish on a routable interface if you have placed an authenticating proxy + in front of it or otherwise restricted access. Treat external exposure as a + deliberate decision. +::: + ## With Pre-configured Connection To auto-start metrics collection on startup, provide connection details as environment variables. This works for both **cluster** and **standalone** Valkey instances — Valkey Admin detects the topology automatically: diff --git a/docs-site/src/content/docs/deployment/kubernetes.md b/docs-site/src/content/docs/deployment/kubernetes.md index 74444ca5..4e503e90 100644 --- a/docs-site/src/content/docs/deployment/kubernetes.md +++ b/docs-site/src/content/docs/deployment/kubernetes.md @@ -55,6 +55,21 @@ For a non-local deployment, use published images from a container registry and u ### 3. Deploy the app server +The metrics sidecars authenticate to the orchestrator's `/orchestrator/register` +and `/orchestrator/ping` endpoints with a shared key. Because sidecars are external +(the orchestrator does not spawn them in Kubernetes), the same key must be +provisioned to both the app server and every sidecar via a Secret. Create it first +with your own random value: + +```bash +kubectl create secret generic valkey-admin-orchestrator-key -n valkey \ + --from-literal=ORCHESTRATOR_KEY="$(openssl rand -hex 32)" +``` + +`app.yaml` intentionally does **not** define this Secret — it only references it — +so `kubectl apply` can never overwrite your generated key with a committed +placeholder. Create the Secret first (above), then deploy the app server: + ```bash kubectl apply -f examples/k8s/app.yaml kubectl rollout status deployment/valkey-admin-app -n valkey @@ -181,15 +196,30 @@ kubectl logs -n valkey valkey-0 -c metrics You want to see `Register success` in the metrics sidecar log. -:::caution[Sidecar registration is not yet functional] -Sidecar registration does not currently succeed in `DEPLOYMENT_MODE=K8`. The orchestrator only tracks collectors it spawned itself, and it does not spawn sidecars, so a sidecar's registration is rejected and the sidecar exits after 30 attempts: +:::caution[Sidecar registration requires the shared key] +Registration and ping are authenticated. Each sidecar signs its requests with the +shared `ORCHESTRATOR_KEY`, and the orchestrator verifies against the same key, so +the Secret from step 3 must be present on **both** the app Deployment and the +sidecar. If the key is missing or does not match, the sidecar exits after 30 +attempts: ```text Register failed: 401 Unauthorized Failed to register with server after 30 attempts. Shutting down. ``` -Support is in progress. Until then the Kubernetes sidecar topology on this page will deploy but will not populate metrics panels. Use the [Docker deployment](/deployment/docker/) for a working metrics path. +If you see this, confirm the `valkey-admin-orchestrator-key` Secret exists in the +`valkey` namespace and that both pods reference it (`kubectl get pod ... -o yaml | +grep -A3 ORCHESTRATOR_KEY`). + +The key is shared cluster-wide rather than per-node, so it authenticates a sidecar +as *a* member of the cluster, not as one specific node. Treat it as a +cluster-scoped credential: restrict read access on the Secret, and rotate it by +recreating the Secret and restarting the app Deployment and the sidecars. Note that +in `DEPLOYMENT_MODE=K8` the registered metrics host is not pinned to loopback, so +any holder of the key can point a node's metrics URI at an arbitrary host that the +server will then fetch from — keep the key tightly scoped and the namespace's +network egress constrained accordingly. ::: ### Charts Empty in the UI diff --git a/examples/k8s/app.yaml b/examples/k8s/app.yaml index f06dc692..8dabe5d9 100644 --- a/examples/k8s/app.yaml +++ b/examples/k8s/app.yaml @@ -1,3 +1,15 @@ +# NOTE: the metrics control plane (/orchestrator/register, /orchestrator/ping) +# authenticates sidecars with a shared key read from the ORCHESTRATOR_KEY env +# var below, sourced from the `valkey-admin-orchestrator-key` Secret. Create that +# Secret with your OWN random value before applying this manifest — it is +# intentionally NOT defined here so `kubectl apply` can never overwrite a real +# key with a committed placeholder: +# +# kubectl create secret generic valkey-admin-orchestrator-key -n valkey \ +# --from-literal=ORCHESTRATOR_KEY="$(openssl rand -hex 32)" +# +# The same Secret is referenced by every metrics sidecar +# (see valkey-statefulset-sidecar-patch.yaml). apiVersion: apps/v1 kind: Deployment metadata: @@ -24,10 +36,18 @@ spec: value: "8080" - name: DEPLOYMENT_MODE value: "K8" + # Bind all interfaces so the Service/pod network can reach the server. + - name: SERVER_BIND_HOST + value: "0.0.0.0" - name: VALKEY_HOST value: valkey-0.valkey-headless.valkey.svc.cluster.local - name: VALKEY_PORT value: "6379" + - name: ORCHESTRATOR_KEY + valueFrom: + secretKeyRef: + name: valkey-admin-orchestrator-key + key: ORCHESTRATOR_KEY ports: - containerPort: 8080 name: http diff --git a/examples/k8s/valkey-statefulset-sidecar-patch.yaml b/examples/k8s/valkey-statefulset-sidecar-patch.yaml index 6d84ae4b..eb7428d4 100644 --- a/examples/k8s/valkey-statefulset-sidecar-patch.yaml +++ b/examples/k8s/valkey-statefulset-sidecar-patch.yaml @@ -36,6 +36,11 @@ spec: fieldPath: status.podIP - name: METRICS_ADVERTISE_PORT value: "3000" + - name: ORCHESTRATOR_KEY + valueFrom: + secretKeyRef: + name: valkey-admin-orchestrator-key + key: ORCHESTRATOR_KEY ports: - containerPort: 3000 name: metrics diff --git a/examples/k8s/valkey-statefulset.yaml b/examples/k8s/valkey-statefulset.yaml index a5ddc47d..e188bc6d 100644 --- a/examples/k8s/valkey-statefulset.yaml +++ b/examples/k8s/valkey-statefulset.yaml @@ -1,3 +1,18 @@ +# Example Valkey cluster StatefulSet with an inline metrics sidecar. +# +# PREREQUISITES — the metrics sidecars will crash-loop (and, because StatefulSet +# rollout is ordered, block the whole rollout at valkey-0) unless both are met: +# +# 1. Create the shared orchestrator key Secret first (the sidecar exits with +# "Missing ORCHESTRATOR_KEY ... Shutting down" without it): +# kubectl create secret generic valkey-admin-orchestrator-key -n valkey \ +# --from-literal=ORCHESTRATOR_KEY="$(openssl rand -hex 32)" +# +# 2. Form the cluster after the pods are up. These nodes start with +# --cluster-enabled yes but do NOT self-form, so CLUSTER SLOTS is empty and +# the sidecar dies on "Cluster(No topology views found)". Assign slots once: +# valkey-cli --cluster create :6379 --cluster-replicas +# apiVersion: v1 kind: Service metadata: @@ -107,6 +122,11 @@ spec: fieldPath: status.podIP - name: METRICS_ADVERTISE_PORT value: "3000" + - name: ORCHESTRATOR_KEY + valueFrom: + secretKeyRef: + name: valkey-admin-orchestrator-key + key: ORCHESTRATOR_KEY ports: - containerPort: 3000 name: metrics