From b03000e3963174fbfe9af039696416123d76ba34 Mon Sep 17 00:00:00 2001 From: LukeGus Date: Sat, 3 Oct 2026 15:41:44 -0500 Subject: [PATCH] fix: op:// secret references rejected as ssh keys on credentials (#1394) --- .../routes/credential-deploy-routes.ts | 18 +- .../database/routes/credential-key-routes.ts | 22 +- src/backend/database/routes/credentials.ts | 34 ++- src/backend/database/routes/host.ts | 6 +- src/backend/hosts/stored-ssh-key.ts | 43 ++++ src/backend/plugins/user-data.ts | 4 +- .../credentials-secret-reference.test.ts | 193 ++++++++++++++++++ .../tests/hosts/stored-ssh-key.test.ts | 43 ++++ 8 files changed, 342 insertions(+), 21 deletions(-) create mode 100644 src/backend/hosts/stored-ssh-key.ts create mode 100644 src/backend/tests/database/routes/credentials-secret-reference.test.ts create mode 100644 src/backend/tests/hosts/stored-ssh-key.test.ts diff --git a/src/backend/database/routes/credential-deploy-routes.ts b/src/backend/database/routes/credential-deploy-routes.ts index aef470fe7..1af7c45fd 100644 --- a/src/backend/database/routes/credential-deploy-routes.ts +++ b/src/backend/database/routes/credential-deploy-routes.ts @@ -9,6 +9,9 @@ import { createCurrentHostResolutionRepository } from "../repositories/factory.j import { connectHost } from "../../hosts/connect/connect-host.js"; import type { SshConnectHost } from "../../hosts/connect/types.js"; import { resolveHostById } from "../../hosts/host-resolver.js"; +import { isSecretReference } from "../../hosts/external-secrets.js"; +import { resolveKeyReferences } from "../../hosts/stored-ssh-key.js"; +import { parseSSHKey } from "../../utils/ssh-key-utils.js"; function describeDeployError(err: unknown): string { const message = getErrorMessage(err, "Connection failed"); @@ -363,7 +366,20 @@ export function registerCredentialDeployRoutes( }); } - const publicKey = credData.publicKey; + let publicKey = credData.publicKey; + const storedKey = credData.privateKey || credData.key; + if (!publicKey && isSecretReference(storedKey)) { + // A key kept in a secret source has no stored public key; derive + // it from the resolved key. + const resolved = await resolveKeyReferences( + userId, + storedKey, + (credData as { keyPassword?: string | null }).keyPassword, + ); + publicKey = + parseSSHKey(resolved.key, resolved.passphrase).publicKey || + undefined; + } if (!publicKey) { return res.status(400).json({ success: false, diff --git a/src/backend/database/routes/credential-key-routes.ts b/src/backend/database/routes/credential-key-routes.ts index 5cc72b238..746095d56 100644 --- a/src/backend/database/routes/credential-key-routes.ts +++ b/src/backend/database/routes/credential-key-routes.ts @@ -8,6 +8,8 @@ import { parseSSHKey, validateKeyPair, } from "../../utils/ssh-key-utils.js"; +import { resolveKeyReferences } from "../../hosts/stored-ssh-key.js"; +import type { AuthenticatedRequest } from "../../../types/index.js"; const { utils: ssh2Utils } = ssh2Pkg; @@ -99,13 +101,17 @@ export function registerCredentialKeyRoutes( requireCredentialViewPermission, requireDataAccess, async (req: Request, res: Response) => { - const { privateKey, keyPassword } = req.body; - - if (!privateKey || typeof privateKey !== "string") { + if (!req.body?.privateKey || typeof req.body.privateKey !== "string") { return res.status(400).json({ error: "Private key is required" }); } try { + const { key: privateKey, passphrase: keyPassword } = + await resolveKeyReferences( + (req as AuthenticatedRequest).userId, + req.body.privateKey, + req.body.keyPassword, + ); const keyInfo = parseSSHKey(privateKey, keyPassword); const response = { @@ -352,13 +358,17 @@ export function registerCredentialKeyRoutes( requireCredentialViewPermission, requireDataAccess, async (req: Request, res: Response) => { - const { privateKey, keyPassword } = req.body; - - if (!privateKey || typeof privateKey !== "string") { + if (!req.body?.privateKey || typeof req.body.privateKey !== "string") { return res.status(400).json({ error: "Private key is required" }); } try { + const { key: privateKey, passphrase: keyPassword } = + await resolveKeyReferences( + (req as AuthenticatedRequest).userId, + req.body.privateKey, + req.body.keyPassword, + ); let privateKeyObj; const parseAttempts = []; diff --git a/src/backend/database/routes/credentials.ts b/src/backend/database/routes/credentials.ts index 1b5cab025..ae1d3777e 100644 --- a/src/backend/database/routes/credentials.ts +++ b/src/backend/database/routes/credentials.ts @@ -7,7 +7,7 @@ import express, { type Request, type Response } from "express"; import { authLogger } from "../../utils/logger.js"; import { PermissionManager } from "../../utils/permission-manager.js"; import { AuthManager } from "../../utils/auth-manager.js"; -import { parseSSHKey } from "../../utils/ssh-key-utils.js"; +import { parseKeyForStorage } from "../../hosts/stored-ssh-key.js"; import { registerCredentialKeyRoutes } from "./credential-key-routes.js"; import { registerCredentialDeployRoutes } from "./credential-deploy-routes.js"; import { registerCredentialBulkRoutes } from "./credential-bulk-routes.js"; @@ -165,7 +165,7 @@ router.post( let keyInfo = null; if (authType === "key" && plainKey) { - keyInfo = parseSSHKey(plainKey, plainKeyPassword); + keyInfo = parseKeyForStorage(plainKey, plainKeyPassword); if (!keyInfo.success) { authLogger.warn("SSH key parsing failed", { operation: "credential_create", @@ -192,7 +192,11 @@ router.post( password: plainPassword, key: plainKey, privateKey: keyInfo?.privateKey || plainKey, - publicKey: keyInfo?.publicKey || null, + publicKey: + keyInfo?.publicKey || + (keyInfo?.reference && typeof req.body?.publicKey === "string" + ? req.body.publicKey.trim() || null + : null), keyPassword: plainKeyPassword, keyType: keyType || null, detectedKeyType: keyInfo?.keyType || null, @@ -502,7 +506,7 @@ router.post( let keyInfo = null; if (authType === "key" && plainKey) { - keyInfo = parseSSHKey(plainKey, plainKeyPassword); + keyInfo = parseKeyForStorage(plainKey, plainKeyPassword); if (!keyInfo.success) { return res.status(400).json({ error: keyInfo.error @@ -524,7 +528,11 @@ router.post( password: authType === "password" ? plainPassword : null, key: authType === "key" ? plainKey : null, privateKey: authType === "key" ? keyInfo?.privateKey || plainKey : null, - publicKey: authType === "key" ? keyInfo?.publicKey || null : null, + publicKey: + authType === "key" + ? keyInfo?.publicKey || + (keyInfo?.reference ? source.publicKey || null : null) + : null, keyPassword: authType === "key" ? plainKeyPassword : null, keyType: source.keyType, detectedKeyType: authType === "key" ? keyInfo?.keyType || null : null, @@ -688,8 +696,14 @@ router.put( if (updateData.key !== undefined) { updateFields.key = updateData.key || null; - if (updateData.key && existingCredential.authType === "key") { - const keyInfo = parseSSHKey(updateData.key, updateData.keyPassword); + if ( + updateData.key && + (updateData.authType ?? existingCredential.authType) === "key" + ) { + const keyInfo = parseKeyForStorage( + updateData.key, + updateData.keyPassword, + ); if (!keyInfo.success) { authLogger.warn("SSH key parsing failed during update", { operation: "credential_update", @@ -704,8 +718,10 @@ router.put( }); } updateFields.privateKey = keyInfo.privateKey; - updateFields.publicKey = keyInfo.publicKey; - updateFields.detectedKeyType = keyInfo.keyType; + updateFields.publicKey = keyInfo.reference + ? updateData.publicKey?.trim() || null + : keyInfo.publicKey; + updateFields.detectedKeyType = keyInfo.keyType || null; } } if (updateData.keyPassword !== undefined) { diff --git a/src/backend/database/routes/host.ts b/src/backend/database/routes/host.ts index 73a3942f4..31e75a726 100644 --- a/src/backend/database/routes/host.ts +++ b/src/backend/database/routes/host.ts @@ -11,7 +11,7 @@ import { pluginEvents, TOPICS } from "../../plugins/events.js"; import { AuthManager } from "../../utils/auth-manager.js"; import { PermissionManager } from "../../utils/permission-manager.js"; import { DataCrypto } from "../../utils/data-crypto.js"; -import { parseSSHKey } from "../../utils/ssh-key-utils.js"; +import { parseKeyForStorage } from "../../hosts/stored-ssh-key.js"; import { pickResolvedPassword, pickResolvedUsername, @@ -358,7 +358,7 @@ router.post( sshDataObj.keyType = null; } else if (effectiveAuthType === "key") { if (key && typeof key === "string") { - const keyValidation = parseSSHKey( + const keyValidation = parseKeyForStorage( key, typeof keyPassword === "string" ? keyPassword : undefined, ); @@ -940,7 +940,7 @@ router.put( sshDataObj.keyType = null; } else if (effectiveAuthType === "key") { if (key && typeof key === "string") { - const keyValidation = parseSSHKey( + const keyValidation = parseKeyForStorage( key, typeof keyPassword === "string" ? keyPassword : undefined, ); diff --git a/src/backend/hosts/stored-ssh-key.ts b/src/backend/hosts/stored-ssh-key.ts new file mode 100644 index 000000000..e50709846 --- /dev/null +++ b/src/backend/hosts/stored-ssh-key.ts @@ -0,0 +1,43 @@ +import { parseSSHKey, type KeyInfo } from "../utils/ssh-key-utils.js"; +import { + isSecretReference, + resolveSecretReference, +} from "./external-secrets.js"; + +/** + * Checks a private key before it is saved. A secret reference (op://...) is + * resolved when connecting, so it is stored as given: there is nothing to + * parse yet, and parsing the reference itself always failed (#1394). + */ +export function parseKeyForStorage( + key: string, + passphrase?: string | null, +): KeyInfo & { reference: boolean } { + if (isSecretReference(key) || isSecretReference(passphrase)) { + return { + success: true, + privateKey: key.trim(), + publicKey: "", + keyType: "", + reference: true, + }; + } + return { + ...parseSSHKey(key, passphrase ?? undefined), + reference: false, + }; +} + +/** The real key and passphrase behind any references, for key helpers. */ +export async function resolveKeyReferences( + userId: string, + key: string, + passphrase?: string | null, +): Promise<{ key: string; passphrase?: string }> { + const resolve = async (value: string) => + isSecretReference(value) ? resolveSecretReference(userId, value) : value; + return { + key: await resolve(key), + passphrase: passphrase ? await resolve(passphrase) : undefined, + }; +} diff --git a/src/backend/plugins/user-data.ts b/src/backend/plugins/user-data.ts index cdb9e6e0d..d6a333758 100644 --- a/src/backend/plugins/user-data.ts +++ b/src/backend/plugins/user-data.ts @@ -107,7 +107,7 @@ interface SqliteLike { run(...params: unknown[]): unknown; all(...params: unknown[]): unknown[]; }; - transaction? unknown>(fn: T): T; + transaction?(fn: (rows: Row[]) => void): (rows: Row[]) => unknown; } const INSERT_CHUNK = 2000; @@ -148,7 +148,7 @@ export async function writeUserPluginTables( : writeChunk; for (let start = 0; start < rows.length; start += INSERT_CHUNK) { const chunk = rows.slice(start, start + INSERT_CHUNK); - runChunk(chunk as never); + runChunk(chunk); written += chunk.length; await yieldToEventLoop(); } diff --git a/src/backend/tests/database/routes/credentials-secret-reference.test.ts b/src/backend/tests/database/routes/credentials-secret-reference.test.ts new file mode 100644 index 000000000..4f27a6943 --- /dev/null +++ b/src/backend/tests/database/routes/credentials-secret-reference.test.ts @@ -0,0 +1,193 @@ +/** + * A key credential whose private key is a secret reference (op://...) is + * saved as given and resolved when connecting (#1394). Parsing the reference + * as a key used to reject it, and an update kept the old private key. + */ + +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import express from "express"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const h = vi.hoisted(() => ({ + created: [] as Array>, + updated: [] as Array>, + existing: null as Record | null, +})); + +const pass = (_req: unknown, _res: unknown, next: () => void) => next(); + +vi.mock("../../../utils/auth-manager.js", () => ({ + AuthManager: { + getInstance: () => ({ + createAuthMiddleware: + () => (req: { userId?: string }, _res: unknown, next: () => void) => { + req.userId = "u1"; + next(); + }, + createDataAccessMiddleware: () => pass, + }), + }, +})); +vi.mock("../../../utils/permission-manager.js", () => ({ + PermissionManager: { + getInstance: () => ({ requirePermission: () => pass }), + }, +})); +vi.mock("../../../sync/shared-copy-guard.js", () => ({ + rejectSharedCopyWrites: () => pass, +})); +vi.mock("../../../utils/audit-logger.js", () => ({ + logAudit: async () => {}, + getAuditUsername: async () => "u1", + getRequestMeta: () => ({ ipAddress: null, userAgent: null }), +})); +vi.mock("../../../utils/logger.js", () => { + const log = { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + debug: vi.fn(), + }; + return { authLogger: log, sshLogger: log, databaseLogger: log }; +}); +vi.mock("../../../database/routes/credential-key-routes.js", () => ({ + registerCredentialKeyRoutes: () => {}, +})); +vi.mock("../../../database/routes/credential-deploy-routes.js", () => ({ + registerCredentialDeployRoutes: () => {}, +})); +vi.mock("../../../database/routes/credential-bulk-routes.js", () => ({ + registerCredentialBulkRoutes: () => {}, +})); +vi.mock("../../../hosts/delete-credential.js", () => ({ + deleteOwnedCredential: async () => {}, +})); +vi.mock("../../../hosts/connect/core-providers.js", () => ({ + ensureCoreSshAuthProviders: () => {}, +})); +vi.mock("../../../hosts/connect/auth-provider-registry.js", () => ({ + listCredentialTypes: () => ["password", "key"], +})); +vi.mock("../../../utils/shared-host-secrets-manager.js", () => ({ + SharedHostSecretsManager: { + getInstance: () => ({ resyncHostsForCredential: async () => {} }), + }, +})); +vi.mock("../../../utils/shared-credential-secrets-manager.js", () => ({ + SharedCredentialSecretsManager: { + getInstance: () => ({ resyncCredential: async () => {} }), + }, +})); +vi.mock("../../../database/repositories/factory.js", () => ({ + createCurrentCredentialRepository: () => ({ + createEncryptedForUser: async ( + userId: string, + data: Record, + ) => { + h.created.push(data); + return { id: 1, userId, ...data }; + }, + findById: async () => (h.existing ? { id: 1, userId: "u1" } : null), + findDecryptedByIdForUser: async () => h.existing, + updateEncryptedForUser: async ( + _userId: string, + _id: number, + fields: Record, + ) => { + h.updated.push(fields); + return { ...h.existing, ...fields }; + }, + }), + createCurrentUserRepository: () => ({}), + createCurrentCredentialAccessRepository: () => ({}), + createCurrentRoleRepository: () => ({}), + createCurrentHostResolutionRepository: () => ({}), + createCurrentHostRepository: () => ({}), +})); + +const { default: router } = + await import("../../../database/routes/credentials.js"); + +// Never parsed here: only the update replacing it matters. +const REAL_KEY = "stored-key-material"; + +let server: http.Server; +let base: string; + +beforeEach(async () => { + h.created = []; + h.updated = []; + h.existing = null; + const app = express(); + app.use(express.json()); + app.use("/credentials", router); + server = http.createServer(app); + await new Promise((resolve) => server.listen(0, resolve)); + base = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +}); + +afterEach(async () => { + await new Promise((resolve) => server.close(() => resolve())); +}); + +function send(method: string, path: string, body: unknown) { + return fetch(`${base}/credentials${path}`, { + method, + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +describe("credentials with a secret reference key", () => { + it("saves an op:// private key as given", async () => { + const response = await send("POST", "", { + name: "vault key", + authType: "key", + username: "root", + key: "op://vault/item/private_key", + }); + + expect(response.status).toBe(201); + expect(h.created[0]).toMatchObject({ + key: "op://vault/item/private_key", + privateKey: "op://vault/item/private_key", + publicKey: null, + detectedKeyType: null, + }); + }); + + it("still rejects a key that is neither a key nor a reference", async () => { + const response = await send("POST", "", { + name: "bad", + authType: "key", + key: "not a key", + }); + expect(response.status).toBe(400); + }); + + it("replaces the stored private key when updating to a reference", async () => { + h.existing = { + id: 1, + userId: "u1", + name: "k", + authType: "key", + key: REAL_KEY, + privateKey: REAL_KEY, + publicKey: "ssh-ed25519 AAAA old", + }; + + const response = await send("PUT", "/1", { + key: "op://vault/item/private_key", + }); + + expect(response.status).toBe(200); + expect(h.updated[0]).toMatchObject({ + key: "op://vault/item/private_key", + privateKey: "op://vault/item/private_key", + publicKey: null, + detectedKeyType: null, + }); + }); +}); diff --git a/src/backend/tests/hosts/stored-ssh-key.test.ts b/src/backend/tests/hosts/stored-ssh-key.test.ts new file mode 100644 index 000000000..fb50a5f71 --- /dev/null +++ b/src/backend/tests/hosts/stored-ssh-key.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it, vi } from "vitest"; + +const resolve = vi.fn(async (_userId: string, ref: string) => `real:${ref}`); +vi.mock("../../hosts/external-secrets.js", async (importOriginal) => ({ + ...(await importOriginal()), + resolveSecretReference: (userId: string, ref: string) => resolve(userId, ref), +})); + +const { parseKeyForStorage, resolveKeyReferences } = + await import("../../hosts/stored-ssh-key.js"); + +describe("parseKeyForStorage", () => { + it("accepts a secret reference without parsing it", () => { + expect(parseKeyForStorage(" op://v/i/private_key ")).toEqual({ + success: true, + privateKey: "op://v/i/private_key", + publicKey: "", + keyType: "", + reference: true, + }); + }); + + it("does not try to open a real key with a referenced passphrase", () => { + const result = parseKeyForStorage("-----BEGIN...", "op://v/i/pass"); + expect(result).toMatchObject({ success: true, reference: true }); + }); + + it("parses anything else as a key", () => { + const result = parseKeyForStorage("not a key"); + expect(result.success).toBe(false); + expect(result.reference).toBe(false); + }); +}); + +describe("resolveKeyReferences", () => { + it("resolves only the values that are references", async () => { + expect(await resolveKeyReferences("u1", "op://v/i/key", "plain")).toEqual({ + key: "real:op://v/i/key", + passphrase: "plain", + }); + expect(resolve).toHaveBeenCalledTimes(1); + }); +});