diff --git a/CHANGELOG.md b/CHANGELOG.md index 596e1265..53340922 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ product overview. ## Unreleased +- Fix open SSH sessions retaining stale channel secrets after additions, rotations or removals. + Refreshes now recheck the developer’s access and close sessions when access is revoked or + their mounted workspace/runtime changes. + - Show the active model in Slack's native Stop control using a per-session display name. Existing apps need `chat:write.customize` and reinstallation; missing customization permission preserves the native Stop button with its usual name. diff --git a/FEATURES.md b/FEATURES.md index cda079e4..589bbd03 100644 --- a/FEATURES.md +++ b/FEATURES.md @@ -3326,7 +3326,10 @@ are retired, bullet by bullet; everything else stands. their last session ends, refreshed every 20 minutes AND within `CONFIG_REFRESH_DEBOUNCE_MS` of a configuration write that concerns the session (`src/config/change-events.js`: every channel-meta, user and organization-secret write emits after it commits; the broker re-prepares the sessions - of that channel, that developer, or all of them, coalesced per session). The login is an + of that channel, that developer, or all of them, coalesced per session). Each refresh reauthorizes + the key and developer against current channel metadata and serializes preparation, so secret + additions, rotations and removals reach an already open session. Revoked access or a changed + mounted workspace/runtime closes the session; reconnecting uses the new configuration. The login is an ACCESS-ONLY `.credentials.json` in the channel's config dir plus the operator's account record — never a refresh token — written through the relay and removed with the channel's last session, so an interactive `claude` shows "Claude Max account", diff --git a/TEST-PLAN.md b/TEST-PLAN.md index dadadc88..d0812948 100644 --- a/TEST-PLAN.md +++ b/TEST-PLAN.md @@ -8445,6 +8445,10 @@ are the v0.8 production deployment gate and are executed in the QA loop that fol change that concerns it — its channel, its developer, or the organization's secrets — one refresh per burst, never for another channel's or developer's change, never for an ended session (automated: `test/change-events.test.js`, `test/ssh-broker.test.js`). +- [x] Unit: an attached session reads fresh metadata after actual secret add, rotate and remove + writes, and current developer authority after a user write. Revoking the SSH grant or + approved status closes the session and releases its lease; a changed mounted work folder + closes it before preparing credentials for the new target (automated: `test/ssh-broker.test.js`). - [x] Unit: one tool per verb for secrets — `list_secrets` lists the three scopes in one call (names, masked tails, authors; never a value; the organization's tails only for admins; a person only their own personal scope; an unverified principal no personal scope; `scope` diff --git a/src/gateway/ssh-broker.js b/src/gateway/ssh-broker.js index dd507114..4139dd3a 100644 --- a/src/gateway/ssh-broker.js +++ b/src/gateway/ssh-broker.js @@ -137,6 +137,17 @@ function writeLine(socket, payload) { } } +// A refresh may change credentials and tool policy, but cannot move an attached shell to a +// different container or mount set. Compare the unresolved plans on both sides: ensureUp fills +// in image/auth details on the original target after this snapshot is taken. +function sessionTargetKey(target) { + return JSON.stringify([ + target?.backend, target?.runtimeScope, target?.workDir, target?.cleanWorkDir, target?.artifactDir, + target?.container?.name, target?.container?.network, + target?.container?.mounts, target?.container?.credentialMode, + ]); +} + // One brokered session, from an already-parsed header to the last byte. async function runSession(socket, header, leftover, state) { const { deps, log } = state; @@ -167,6 +178,7 @@ async function runSession(socket, header, leftover, state) { socket.end(); return; } + const targetKey = sessionTargetKey(target); // The lease comes FIRST — before ensureUp, like run.js — so the reaper cannot stop the container // between "it is up" and "sshd is inside". const lease = target.runtime.acquireLease(target, { kind: "ssh", id }); @@ -219,16 +231,46 @@ async function runSession(socket, header, leftover, state) { let prepared = { mcpServers: [], secrets: [], rejectedMcps: [], problems: [], toolset: "" }; count(1); counted = true; - const refresh = async () => { - try { - prepared = await deps.prepareSession({ target, entry, meta, user, cliBin, log }); - claude = prepared.claude || { relayed: true, source: "", reason: "" }; - } catch (error) { - claude = { relayed: false, reason: String(error?.message || error) }; - log.warn?.(`[ssh] Claude relay for ${entry.slug}: ${claude.reason}`); - } + let refreshQueue = Promise.resolve(); + const refresh = () => { + // Re-read at execution time, including after a queued refresh. A configuration event is + // only a wake-up; neither the attach-time metadata nor the original user rank is authority. + const next = refreshQueue.then(async () => { + if (done) return false; + let current, currentTarget; + try { + current = await deps.authorize({ ...header, channel: entry.channelId }); + if (!current.ok || current.key.id !== key.id || current.user.id !== user.id + || current.entry.slug !== entry.slug || current.entry.channelId !== entry.channelId) { + await finish("SSH access is no longer authorized"); + return false; + } + currentTarget = deps.resolveTarget(current.entry.slug, current.meta); + if (sessionTargetKey(currentTarget) !== targetKey) { + await finish("the channel runtime changed — reconnect to use its current configuration"); + return false; + } + } catch { + await finish("SSH authorization could not be refreshed"); + return false; + } + if (done) return false; + try { + prepared = await deps.prepareSession({ + target: { ...target, meta: current.meta, settings: currentTarget.settings }, + entry: current.entry, meta: current.meta, user: current.user, cliBin, log, + }); + claude = prepared.claude || { relayed: true, source: "", reason: "" }; + } catch (error) { + claude = { relayed: false, reason: String(error?.message || error) }; + log.warn?.(`[ssh] Claude relay for ${entry.slug}: ${claude.reason}`); + } + return !done; + }); + refreshQueue = next.catch(() => {}); + return next; }; - await refresh(); + if (!(await refresh())) return; session.refresh = refresh; child = deps.spawnExec(target, cliBin); touchSshKey(key.id); diff --git a/test/ssh-broker.test.js b/test/ssh-broker.test.js index d54b6737..eb8750d5 100644 --- a/test/ssh-broker.test.js +++ b/test/ssh-broker.test.js @@ -273,6 +273,81 @@ test("a configuration write that concerns a live session re-prepares it at once; assert.equal(prepared.length, base + 3, "an ended session is never re-prepared"); }); +test("an open session receives current channel secrets and user authority after actual writes", async () => { + const state = await broker.startSshBroker({ dir: SSH_DIR, log: silent, retryMs: 0, deps, authorizeOptions }); + const prepareSession = state.deps.prepareSession; + const original = await getChannelMeta(entry.slug); + const observed = []; + state.deps.prepareSession = async (session) => { + observed.push({ value: session.meta.env?.QA_REFRESH?.value, admin: session.user.admin }); + return prepareSession(session); + }; + const waitFor = async (predicate) => { + const deadline = Date.now() + 2000; + while (!predicate()) { + assert.ok(Date.now() < deadline, "session did not observe the current configuration"); + await settle(); + } + }; + let socket; + try { + ({ socket } = await attach({ v: 1, key: ED25519, channel: entry.slug, client: "secret-refresh" })); + assert.equal(observed.at(-1).value, undefined); + for (const value of ["added", "rotated", undefined]) { + const before = observed.length; + await saveChannelMeta(entry.slug, { ...original, env: value ? { QA_REFRESH: { provider: "local", value, exposure: "readable" } } : {} }); + await waitFor(() => observed.length > before); + assert.equal(observed.at(-1).value, value, "add, rotation and removal reach the same attached session"); + assert.equal(broker.liveSshSessions({ slug: entry.slug }).length, 1); + } + const before = observed.length; + await setUser(DEV, { name: "Apps", approved: true, isAdmin: true }); + await waitFor(() => observed.length > before); + assert.equal(observed.at(-1).admin, true, "the refreshed user rank is current too"); + await saveChannelMeta(entry.slug, { ...original, sshUsers: [] }); + await waitFor(() => broker.liveSshSessions({ slug: entry.slug }).length === 0); + assert.equal(children.at(-1).signalCode, "SIGTERM", "revoked access closes the existing session"); + } finally { + socket?.destroy(); + await settle(); + state.deps.prepareSession = prepareSession; + await saveChannelMeta(entry.slug, original); + await setUser(DEV, { name: "Apps", approved: true, isAdmin: false }); + } +}); + +test("refresh refuses revoked user authority or a changed mounted work folder", async () => { + const state = await broker.startSshBroker({ dir: SSH_DIR, log: silent, retryMs: 0, deps, authorizeOptions }); + const original = await getChannelMeta(entry.slug); + const resolve = state.deps.resolveTarget; + state.deps.resolveTarget = (slug, meta) => ({ ...resolve(slug, meta), workDir: meta.workDir || `/work/${slug}` }); + try { + for (const change of ["user", "workDir"]) { + const { socket, line } = await attach({ v: 1, key: ED25519, channel: entry.slug, client: change }); + assert.equal(line.ok, true); + const child = children.at(-1), before = prepared.length; + if (change === "user") await setUser(DEV, { name: "Apps", approved: false, isAdmin: false }); + else await saveChannelMeta(entry.slug, { ...original, workDir: "/work/another-project" }); + const deadline = Date.now() + 2000; + while (broker.liveSshSessions({ slug: entry.slug }).length) { + assert.ok(Date.now() < deadline, "the old session remained authorized"); + await settle(); + } + await settle(); + assert.equal(child.signalCode, "SIGTERM"); + assert.equal(prepared.length, before, "no refreshed credentials are staged into the old runtime"); + assert.equal(reaper.leaseCount(`cg-${entry.slug}`), 0); + socket.destroy(); + await setUser(DEV, { name: "Apps", approved: true, isAdmin: false }); + await saveChannelMeta(entry.slug, original); + } + } finally { + state.deps.resolveTarget = resolve; + await setUser(DEV, { name: "Apps", approved: true, isAdmin: false }); + await saveChannelMeta(entry.slug, original); + } +}); + test("refusals name the remedy and leave no lease, no session and no exec behind", async () => { const before = children.length; const cases = [