From 59bc78a5b27111b06df3e55f26715eb045120c1f Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 20 Jul 2026 10:25:25 -0700 Subject: [PATCH] fix(gateway): auto-approve trusted-proxy same-key device scope upgrades (#111916) --- ...h.trusted-proxy-device-autoapprove.test.ts | 98 +++++++++++++++-- .../ws-connection/connect-device-pairing.ts | 12 +- src/infra/device-pairing.test.ts | 103 +++++++++++++++++- src/infra/device-pairing.ts | 22 +++- 4 files changed, 214 insertions(+), 21 deletions(-) diff --git a/src/gateway/server.auth.trusted-proxy-device-autoapprove.test.ts b/src/gateway/server.auth.trusted-proxy-device-autoapprove.test.ts index 57ae4f551e4..42220a570be 100644 --- a/src/gateway/server.auth.trusted-proxy-device-autoapprove.test.ts +++ b/src/gateway/server.auth.trusted-proxy-device-autoapprove.test.ts @@ -411,7 +411,7 @@ describe("trusted-proxy browser device auto-approval", () => { ]); }); - test("keeps scope upgrades on existing devices pending for manual approval", async () => { + test("auto-approves same-key scope upgrades on existing devices", async () => { await writeGatewayAuthConfig({ mode: "trusted-proxy", deviceAutoApprove: { @@ -431,18 +431,98 @@ describe("trusted-proxy browser device auto-approval", () => { identityPath, scopes: ["operator.read", "operator.write"], }); - expect(upgrade.ok).toBe(false); - expect(upgrade.error?.message ?? "").toContain("pairing required"); + expect(upgrade.ok).toBe(true); }); const pairing = await listDevicePairing(); - const pending = pairing.pending.filter((entry) => entry.deviceId === identity.deviceId); - expect(pending).toHaveLength(1); - expect(pending[0]).toMatchObject({ - isRepair: true, - scopes: ["operator.read", "operator.write"], + expect(pairing.pending.filter((entry) => entry.deviceId === identity.deviceId)).toHaveLength(0); + expect((await getPairedDevice(identity.deviceId))?.approvedScopes).toEqual([ + "operator.read", + "operator.write", + ]); + }); + + test("rejects a foreign-key connect for an already-paired deviceId", async () => { + await writeGatewayAuthConfig({ + mode: "trusted-proxy", + deviceAutoApprove: { + enabled: true, + scopes: ["operator.read", "operator.write"], + }, }); - expect((await getPairedDevice(identity.deviceId))?.approvedScopes).toEqual(["operator.read"]); + const identityPath = deviceIdentityPath("trusted-proxy-key-mismatch-victim"); + const identity = loadOrCreateDeviceIdentity({ path: identityPath }); + const attackerIdentity = loadOrCreateDeviceIdentity({ + path: deviceIdentityPath("trusted-proxy-key-mismatch-attacker"), + }); + + await withGatewayServer(async ({ port }) => { + const initial = await connectBrowser({ port, identityPath, scopes: ["operator.read"] }); + expect(initial.ok).toBe(true); + + // Same deviceId, different key pair: a valid signature over the attacker + // key must not ride the trusted-proxy upgrade lane. + const ws = await openBrowserWs(port, trustedProxyHeaders()); + try { + const nonce = await readConnectChallengeNonce(ws); + if (!nonce) { + throw new Error("missing connect.challenge nonce"); + } + const signedAt = Date.now(); + const scopes = ["operator.read", "operator.write"]; + const payload = buildDeviceAuthPayloadV3({ + deviceId: identity.deviceId, + clientId: CONTROL_UI_CLIENT.id, + clientMode: CONTROL_UI_CLIENT.mode, + role: "operator", + scopes, + signedAtMs: signedAt, + token: null, + nonce, + platform: CONTROL_UI_CLIENT.platform, + }); + const id = randomUUID(); + const response = onceMessage<{ type: "res"; id: string; ok: boolean }>( + ws, + (message) => message.type === "res" && message.id === id, + ); + ws.send( + JSON.stringify({ + type: "req", + id, + method: "connect", + params: { + minProtocol: PROTOCOL_VERSION, + maxProtocol: PROTOCOL_VERSION, + client: CONTROL_UI_CLIENT, + caps: [], + commands: [], + role: "operator", + scopes, + device: { + id: identity.deviceId, + publicKey: publicKeyRawBase64UrlFromPem(attackerIdentity.publicKeyPem), + signature: signDevicePayload(attackerIdentity.privateKeyPem, payload), + signedAt, + nonce, + }, + }, + }), + ); + expect((await response).ok).toBe(false); + } finally { + ws.close(); + } + }); + + // The connect is rejected before any pending request exists: deviceId is + // bound to the key fingerprint, so a foreign key cannot even enqueue a + // repair, let alone ride the same-key upgrade lane. + const pairing = await listDevicePairing(); + expect(pairing.pending.filter((entry) => entry.deviceId === identity.deviceId)).toEqual([]); + const paired = await getPairedDevice(identity.deviceId); + expect(paired?.approvedScopes).toEqual(["operator.read"]); + expect(paired?.publicKey).toBe(publicKeyRawBase64UrlFromPem(identity.publicKeyPem)); }); test("leaves trusted-proxy pairing unchanged when auto-approval is disabled", async () => { diff --git a/src/gateway/server/ws-connection/connect-device-pairing.ts b/src/gateway/server/ws-connection/connect-device-pairing.ts index 22acb89e0db..7eeeffc00f0 100644 --- a/src/gateway/server/ws-connection/connect-device-pairing.ts +++ b/src/gateway/server/ws-connection/connect-device-pairing.ts @@ -202,9 +202,14 @@ export async function authorizeGatewayConnectDevice( const trustedProxyAutoApproveConfig = configSnapshot.gateway?.auth?.trustedProxy?.deviceAutoApprove; const trustedProxyUser = authResult.user?.trim(); + // A scope upgrade from a device whose paired public key matches the one + // this connect just proved by signature is the same physical browser + // behind the SSO proxy — auto-approvable like a first pairing. A key + // mismatch stays a manual owner decision (possible deviceId squat). + const isTrustedProxySameKeyUpgrade = + reason === "scope-upgrade" && existingPairedDevice?.publicKey === devicePublicKey; const allowTrustedProxyDeviceAutoApproval = - reason === "not-paired" && - !existingPairedDevice && + ((reason === "not-paired" && !existingPairedDevice) || isTrustedProxySameKeyUpgrade) && role === "operator" && (isBrowserOperatorUi || isWebchat) && authMethod === "trusted-proxy" && @@ -294,7 +299,8 @@ export async function authorizeGatewayConnectDevice( allowControlUiOperatorBootstrapPairing, }); const trustedProxyAutoApproveScopes = - allowTrustedProxyDeviceAutoApproval && pairing.request.isRepair !== true + allowTrustedProxyDeviceAutoApproval && + (pairing.request.isRepair !== true || isTrustedProxySameKeyUpgrade) ? resolveTrustedProxyControlUiScopes({ requestedScopes: resolveTrustedProxyDeviceAutoApproveScopes({ requestedScopes: scopes, diff --git a/src/infra/device-pairing.test.ts b/src/infra/device-pairing.test.ts index 430fa88a3f0..7d309d6ffdf 100644 --- a/src/infra/device-pairing.test.ts +++ b/src/infra/device-pairing.test.ts @@ -494,7 +494,7 @@ describe("device pairing tokens", () => { ).resolves.toEqual({ ok: true }); }); - test("caps trusted-proxy auto-approval for new devices and refuses repairs", async () => { + test("caps trusted-proxy auto-approval for new devices and upgrades same-key re-requests", async () => { const baseDir = await makeDevicePairingDir(); const initial = await requestDevicePairing( { @@ -523,7 +523,7 @@ describe("device pairing tokens", () => { approvedVia: "trusted-proxy", }); - const repair = await requestDevicePairing( + const upgrade = await requestDevicePairing( { deviceId: "browser-device-1", publicKey: "public-key-browser-1", @@ -532,6 +532,56 @@ describe("device pairing tokens", () => { }, baseDir, ); + const upgraded = await approveDevicePairing( + upgrade.request.requestId, + { + callerScopes: ["operator.read", "operator.write"], + approvedVia: "trusted-proxy", + autoApproveNewDeviceScopes: ["operator.read", "operator.write"], + }, + baseDir, + ); + expectRecordFields(upgraded, "trusted-proxy upgrade result", { + status: "approved", + requestId: upgrade.request.requestId, + }); + expect((await listDevicePairing(baseDir)).pending).toEqual([]); + expect((await getPairedDevice("browser-device-1", baseDir))?.approvedScopes).toEqual([ + "operator.read", + "operator.write", + ]); + }); + + test("refuses trusted-proxy auto-approval when the pending key mismatches the paired device", async () => { + const baseDir = await makeDevicePairingDir(); + const initial = await requestDevicePairing( + { + deviceId: "browser-device-2", + publicKey: "public-key-browser-2", + role: "operator", + scopes: ["operator.read"], + }, + baseDir, + ); + await approveDevicePairing( + initial.request.requestId, + { + callerScopes: ["operator.read"], + approvedVia: "trusted-proxy", + autoApproveNewDeviceScopes: ["operator.read"], + }, + baseDir, + ); + + const repair = await requestDevicePairing( + { + deviceId: "browser-device-2", + publicKey: "public-key-browser-2-rotated", + role: "operator", + scopes: ["operator.read", "operator.write"], + }, + baseDir, + ); await expect( approveDevicePairing( repair.request.requestId, @@ -543,11 +593,56 @@ describe("device pairing tokens", () => { baseDir, ), ).resolves.toBeNull(); - expect((await listDevicePairing(baseDir)).pending).toContainEqual( expect.objectContaining({ requestId: repair.request.requestId, isRepair: true }), ); - expect((await getPairedDevice("browser-device-1", baseDir))?.approvedScopes).toEqual([ + expect((await getPairedDevice("browser-device-2", baseDir))?.approvedScopes).toEqual([ + "operator.read", + ]); + }); + + test("refuses non-trusted-proxy auto-approval for a known device even with a matching key", async () => { + const baseDir = await makeDevicePairingDir(); + const initial = await requestDevicePairing( + { + deviceId: "browser-device-3", + publicKey: "public-key-browser-3", + role: "operator", + scopes: ["operator.read"], + }, + baseDir, + ); + await approveDevicePairing( + initial.request.requestId, + { + callerScopes: ["operator.read"], + approvedVia: "trusted-proxy", + autoApproveNewDeviceScopes: ["operator.read"], + }, + baseDir, + ); + + const upgrade = await requestDevicePairing( + { + deviceId: "browser-device-3", + publicKey: "public-key-browser-3", + role: "operator", + scopes: ["operator.read", "operator.write"], + }, + baseDir, + ); + await expect( + approveDevicePairing( + upgrade.request.requestId, + { + callerScopes: ["operator.read", "operator.write"], + approvedVia: "silent", + autoApproveNewDeviceScopes: ["operator.read", "operator.write"], + }, + baseDir, + ), + ).resolves.toBeNull(); + expect((await getPairedDevice("browser-device-3", baseDir))?.approvedScopes).toEqual([ "operator.read", ]); }); diff --git a/src/infra/device-pairing.ts b/src/infra/device-pairing.ts index 7f394999b6f..50c93d5de6c 100644 --- a/src/infra/device-pairing.ts +++ b/src/infra/device-pairing.ts @@ -792,9 +792,11 @@ export async function approveDevicePairing( "owner" | "silent" | "trusted-cidr" | "trusted-proxy" | "ssh-verified" >; /** - * Replace the pending scopes only if this is still a brand-new operator device. - * The live role set is rechecked under the pairing lock so a merged request - * cannot inherit non-operator access through browser auto-approval. + * Replace the pending scopes only for a brand-new operator device, or — under + * trusted-proxy approval — for a known operator device re-requesting with its + * already-paired public key. The live role set is rechecked under the pairing + * lock so a merged request cannot inherit non-operator access through browser + * auto-approval. */ autoApproveNewDeviceScopes?: readonly string[]; }, @@ -828,10 +830,20 @@ export async function approveDevicePairing( } const autoApproveScopes = options?.autoApproveNewDeviceScopes; const requestedRoles = resolveRequestedRoles(pendingRecord); + const knownDevice = state.pairedByDeviceId[pendingRecord.deviceId]; + // Trusted-proxy connects carry an SSO-authenticated user, and the connect + // handshake has already proven possession of the pending public key. A + // matching key on the paired record is therefore the same physical device + // re-requesting (typically a scope upgrade) and may auto-approve; a key + // mismatch is a real repair — possibly a deviceId squat — and stays a + // manual owner decision. + const trustedProxySameKeyDevice = + options?.approvedVia === "trusted-proxy" && + knownDevice !== undefined && + knownDevice.publicKey === pendingRecord.publicKey; if ( autoApproveScopes && - (pendingRecord.isRepair || - state.pairedByDeviceId[pendingRecord.deviceId] || + (((pendingRecord.isRepair || knownDevice) && !trustedProxySameKeyDevice) || !sameStringSet(requestedRoles, [OPERATOR_ROLE])) ) { return null;