Cherry-pick security fixes into release/1.2.0 (#512, #521, #516) - #523
Merged
Merged
Conversation
* Fix K8s sidecar registration and stale cluster topology broadcast Two coupled defects broke the Kubernetes metrics path and left the Cluster Topology view stale: 1. Sidecar registration (regressed in #379): the register handler required a pre-existing metricsServerMap entry, but in K8s the orchestrator never spawns collectors, so externally-managed sidecars were always rejected (401/404). Registration is now allowed when the nodeId belongs to the discovered cluster topology, and the sidecar's entry is created on first register. Scoped to K8s so the strict no-entry gate is preserved for spawned (Web/Electron) collectors. Sidecars authenticate with a shared ORCHESTRATOR_KEY provisioned to both the orchestrator and every sidecar via a Kubernetes Secret; resolveCollectorKey falls back to that key only in K8s mode. 2. Stale topology broadcast (regressed in #279): refreshAllClusterRegistries broadcast the registry every 30s but no longer re-discovered topology, so clients kept the connect-time snapshot. It now re-discovers each tracked cluster through a live client belonging to that cluster before broadcasting. Adds tests covering registry refresh on topology change and that reconcile acts on the refreshed topology. Updates the K8s example manifests (Secret + env wiring) and deployment docs. Signed-off-by: ravjotb <ravjot.brar@improving.com> * Reuse the per-cluster connection grouping in refreshAllClusterRegistries Build connectionIdsByCluster once and use it both to pick a client for topology re-discovery and to target the broadcast, instead of making a second pass over `clients` for a separate clientByCluster map. The client is reached via clients.get(connectionId).client, keyed by the same connectionIds already grouped for the broadcast. No behavior change. Signed-off-by: ravjotb <ravjot.brar@improving.com> * Don't ship a placeholder orchestrator Secret in app.yaml app.yaml defined a Secret named valkey-admin-orchestrator-key with a committed placeholder value. Applying it after creating a real random Secret of the same name would overwrite the real key with the public placeholder, leaving the orchestrator and every sidecar signing with a known key (CWE-798). An attacker able to reach the orchestrator could then register a known cluster node with an attacker-controlled metrics URI. Remove the Secret definition from app.yaml — the manifest now only references the Secret by name. The key is created out-of-band with a user-supplied random value (kubectl create secret ...), so apply can never clobber it. Docs updated to state the Secret is created first and is intentionally not defined in the manifest. Signed-off-by: ravjotb <ravjot.brar@improving.com> * Address review feedback on topology refresh and K8s registration - Carry per-cluster connection metadata forward on refresh instead of reverting to initialConnectionDetails: updateClusterNodeRegistry now takes a required NodeInfo, and the refresh loop sources it from an existing node of that cluster. Narrow discoverCluster to NodeInfo and drop the redundant getClusterTopology wrapper. - Re-discover preconfigured clusters (K8s sidecars, headless Web) through the initial client each cycle, since they have no entry in `clients`; otherwise a node scaled in after boot never registers until a UI session opens. Await the boot-time K8s discovery. - Bound each cluster's re-discovery with a timeout so one hung CLUSTER SLOTS can't stall the whole broadcast loop. - examples/k8s/valkey-statefulset.yaml: give the inline metrics sidecar the ORCHESTRATOR_KEY it needs, and document that the Secret must exist and the cluster must be formed before the sidecars start. - Document that the shared orchestrator key is a cluster-scoped credential and that the K8s metrics host is not pinned to loopback. - Rework the topology-refresh test to exercise real discovery via a fake CLUSTER SLOTS client; drop a tautological reconcile test. Signed-off-by: ravjotb <ravjot.brar@improving.com> * Overwrite the known cluster entry on refresh instead of orphaning it updateClusterNodeRegistry keyed the registry on the clusterId derived from CLUSTER SLOTS (the first slot range's primary). That id changes on failover/resharding, so re-discovering an existing cluster could write a new entry under a different id and leave the old one orphaned in the map. Add an optional clusterId override: the refresh loop passes the cluster's existing id so the entry is overwritten in place, while boot-time first discovery still falls back to the derived id. Adds a test covering the changed-derived-id case. Signed-off-by: ravjotb <ravjot.brar@improving.com> * Add coverage for the K8s register and topology-refresh fixes The branch's new behavior was largely untested, and the K8s-gated paths are unreachable from the default test suite because isKubernetes is a module-load constant. - orchestrator-k8s-register.test.ts sets DEPLOYMENT_MODE=K8 before import to cover the shared-key path: resolveCollectorKey falls back to ORCHESTRATOR_KEY (and still prefers a minted per-node key), and handleRegister admits a signed sidecar for a known cluster node with no pre-existing entry while rejecting unknown nodes and wrong-key signatures. - Extract resolveClusterRefreshTarget (the refresh client/nodeInfo choice) so it is unit-testable: a user-connected cluster carries its own node metadata forward; a preconfigured/headless cluster falls back to the initial client + initialConnectionDetails. Covered in metrics-orchestrator (user path / skip) and topology-refresh-preconfigured (fallback path). getInitialClient is routed via internals so the fallback is mockable; no behavior change. Signed-off-by: ravjotb <ravjot.brar@improving.com> * Use the initial client only for the preconfigured cluster on refresh resolveClusterRefreshTarget fell back to the initial client for any client-less cluster when preConfiguredConnection was set. In a mixed deployment, an inactive cluster could then be rediscovered through a different preconfigured cluster's client, storing that topology under the inactive cluster's id. Track the preconfigured clusterId (recorded at boot from the Web and K8s discovery paths; updateClusterNodeRegistry now returns the written id) and only use the initial client when the cluster being refreshed matches it. Other client-less clusters are left unchanged. A user-connected cluster still refreshes through its own client, so the preconfigured cluster is covered by that path when a session is open. Signed-off-by: ravjotb <ravjot.brar@improving.com> --------- Signed-off-by: ravjotb <ravjot.brar@improving.com> (cherry picked from commit 9836fb4)
Web mode has no authentication, yet the server bound to all interfaces (0.0.0.0) by default, silently exposing an unauthenticated admin tool to the network on any host/VM deployment. Introduce SERVER_BIND_HOST (mirroring the existing METRICS_BIND_HOST convention). It defaults to 127.0.0.1, so external exposure is now an explicit operator decision: - index.ts binds SERVER_BIND_HOST ?? (K8 ? 0.0.0.0 : 127.0.0.1) - Dockerfile.app sets SERVER_BIND_HOST=0.0.0.0 so published ports work; docs show binding the published port to loopback to keep it local - examples/k8s/app.yaml sets it explicitly - Docker and AWS ElastiCache quick-starts warn that publishing 8080 exposes an unauthenticated service - documented SERVER_BIND_HOST in the server configuration reference Also flips the Docker image build to attach provenance + SBOM attestations (provenance: true, sbom: true) in docker-build.yml. Signed-off-by: ravjotb <ravjot.brar@improving.com> (cherry picked from commit 80c442e)
In Electron mode the WebSocket handshake was accepted for any request presenting an Origin of "null" or "file://". That opaque origin is not unique to the desktop renderer — a sandboxed iframe (or data:/blob: document) in an ordinary browser produces the same "null" origin, and the backend listens on loopback, so any visited web page could open a socket to it and drive the user's Valkey connections with no credentials. Gate the non-web / loopback renderer origins behind a per-launch token: - electron.main.js mints a random 256-bit token each launch, passes it to the backend (fork env ELECTRON_WS_TOKEN) and to the renderer (webPreferences.additionalArguments). - preload.js exposes it to the page via contextBridge. - wsEpics.ts appends it to the Electron WS URL (?token=...). - websocket-origin.ts requires a matching token (crypto.timingSafeEqual) alongside the local origin, and fails closed if no token is provisioned. Web mode is unchanged (strict same-origin + configured allowlist). Tests updated to require the token and cover missing/wrong/absent-token cases. Signed-off-by: ravjotb <ravjot.brar@improving.com> (cherry picked from commit bfaa579)
github-actions
Bot
added
area/frontend
Collaborator
Author
|
Cherry-picked directly onto release/1.2.0 per the usual process (no cherry branch/PR needed). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-picks the merged security fixes from main into
release/1.2.0, in main's merge order (all picked clean with-x, no conflicts):9836fb49)80c442e2)bfaa5796)Not included: #522 (safeStorage fail-closed) — still open on main; will follow once merged.
Verification on this branch
npx eslint .: clean