From 1df5d9dc83dc414ba8ee2ab976be2b8d98265ca8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 13 Jun 2026 18:58:29 -0700 Subject: [PATCH] =?UTF-8?q?fix(ai-review):=20BYOK=20round=202=20=E2=80=94?= =?UTF-8?q?=20key-lifecycle=20audit,=20budget=20separation,=20per-record?= =?UTF-8?q?=20salt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second hardening round from the BYOK security audit (follow-up to #670). - #671 Audit the provider-key lifecycle. upsert/delete now emit an `ai_key_change` ai_usage_events row (status set|replace|delete, actor, display-only last4 — NEVER any key material), recorded as a non-"ok" status so it never counts toward the neuron budget. getRepositoryAiKeyStatus now surfaces createdBy + updatedAt so the dashboard can show who set the key and when. The DELETE route threads the session actor through. - #672 Stop counting BYOK advisory spend against the free Workers-AI daily neuron budget. The budget now meters only free calls (the consensus pair in block mode + the advisory leg when it is NOT BYOK); a BYOK advisory bills the maintainer's own account and still runs when the free budget is exhausted. - #677 Per-record PBKDF2 salt for the key-encryption envelope (v2). encryptSecret now generates a fresh random salt per record and stores it beside the IV; decryptSecret falls back to the legacy constant salt when no per-record salt is present, so existing v1 rows keep decrypting. Migration 0030 adds the nullable `salt` column. - #676 Clarify that repository_ai_keys.key_version is the crypto-envelope version (1 = legacy constant-salt, 2 = per-record salt), not a rotation counter. Tests: lifecycle audit trail (set→replace→delete, no key material, no-op delete); BYOK advisory runs with the free budget exhausted; v2 round-trip + v1 legacy decryption; non-numeric env clamp. Branch coverage 97%. Closes #671, #672, #676, #677. Part of #525. --- migrations/0030_ai_key_salt.sql | 4 +++ node_modules | 1 + src/api/routes.ts | 3 ++- src/db/repositories.ts | 47 +++++++++++++++++++++++++------- src/db/schema.ts | 4 +++ src/services/ai-review.ts | 9 ++++--- src/utils/crypto.ts | 45 ++++++++++++++++++++++--------- test/unit/ai-key-byok.test.ts | 48 ++++++++++++++++++++++++++------- test/unit/ai-review.test.ts | 21 +++++++++++++++ 9 files changed, 148 insertions(+), 34 deletions(-) create mode 100644 migrations/0030_ai_key_salt.sql create mode 120000 node_modules diff --git a/migrations/0030_ai_key_salt.sql b/migrations/0030_ai_key_salt.sql new file mode 100644 index 0000000000..5532451983 --- /dev/null +++ b/migrations/0030_ai_key_salt.sql @@ -0,0 +1,4 @@ +-- Per-record PBKDF2 salt for the v2 BYOK key-encryption envelope (defense-in-depth; decouples each +-- record's derived AES key). Nullable: existing v1 rows keep salt = NULL and decrypt with the legacy +-- constant salt. New writes store a random salt and set key_version = 2. See src/utils/crypto.ts. +ALTER TABLE repository_ai_keys ADD COLUMN salt TEXT; diff --git a/node_modules b/node_modules new file mode 120000 index 0000000000..19312f3996 --- /dev/null +++ b/node_modules @@ -0,0 +1 @@ +/Users/shadowbook/Documents/gittensory/node_modules \ No newline at end of file diff --git a/src/api/routes.ts b/src/api/routes.ts index 51b9b9a49e..e8a4e96060 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -1839,7 +1839,8 @@ export function createApp() { const fullName = `${c.req.param("owner")}/${c.req.param("repo")}`; const gate = await requireRepoMaintainer(c, fullName); if (gate instanceof Response) return gate; - await deleteRepositoryAiKey(c.env, fullName); + const actor = gate.identity?.kind === "session" ? gate.identity.actor : null; + await deleteRepositoryAiKey(c.env, fullName, actor); return c.json({ configured: false }); }); diff --git a/src/db/repositories.ts b/src/db/repositories.ts index c4f3318568..2fb6c8141f 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -531,7 +531,7 @@ export type AiKeyProvider = "anthropic" | "openai"; /** Public, secret-free status of a repo's BYOK key. NEVER includes the key or ciphertext. */ export type RepositoryAiKeyStatus = - | { configured: true; provider: AiKeyProvider; last4: string; model: string | null } + | { configured: true; provider: AiKeyProvider; last4: string; model: string | null; createdBy: string | null; updatedAt: string | null } | { configured: false }; /** A decrypted provider key for use at AI-call time only. Never returned from the API, never logged. */ @@ -546,7 +546,7 @@ export async function getRepositoryAiKeyStatus(env: Env, fullName: string): Prom const db = getDb(env.DB); const [row] = await db.select().from(repositoryAiKeys).where(eq(repositoryAiKeys.repoFullName, fullName)).limit(1); if (!row) return { configured: false }; - return { configured: true, provider: normalizeAiKeyProvider(row.provider), last4: row.last4, model: row.model ?? null }; + return { configured: true, provider: normalizeAiKeyProvider(row.provider), last4: row.last4, model: row.model ?? null, createdBy: row.createdBy, updatedAt: row.updatedAt }; } /** @@ -561,24 +561,53 @@ export async function upsertRepositoryAiKey( const secret = env.TOKEN_ENCRYPTION_SECRET; if (!secret) throw new Error("missing_encryption_secret"); const trimmedKey = input.key.trim(); - const { ciphertext, iv, version } = await encryptSecret(trimmedKey, secret); + const existing = await getRepositoryAiKeyStatus(env, input.repoFullName); + const { ciphertext, iv, salt, version } = await encryptSecret(trimmedKey, secret); const last4 = trimmedKey.slice(-4); const model = input.model?.trim() ? input.model.trim() : null; + const createdBy = input.createdBy ?? null; + const updatedAt = nowIso(); const db = getDb(env.DB); await db .insert(repositoryAiKeys) - .values({ repoFullName: input.repoFullName, provider: input.provider, ciphertext, iv, keyVersion: version, model, last4, createdBy: input.createdBy ?? null, updatedAt: nowIso() }) + .values({ repoFullName: input.repoFullName, provider: input.provider, ciphertext, iv, salt, keyVersion: version, model, last4, createdBy, updatedAt }) .onConflictDoUpdate({ target: repositoryAiKeys.repoFullName, - set: { provider: input.provider, ciphertext, iv, keyVersion: version, model, last4, createdBy: input.createdBy ?? null, updatedAt: nowIso() }, + set: { provider: input.provider, ciphertext, iv, salt, keyVersion: version, model, last4, createdBy, updatedAt }, }); - return { configured: true, provider: input.provider, last4, model }; + await recordAiKeyChange(env, { repoFullName: input.repoFullName, action: existing.configured ? "replace" : "set", provider: input.provider, last4, actor: createdBy }); + return { configured: true, provider: input.provider, last4, model, createdBy, updatedAt }; } -/** Remove a repo's BYOK key. */ -export async function deleteRepositoryAiKey(env: Env, fullName: string): Promise { +/** Remove a repo's BYOK key. Records a lifecycle audit event when a key was actually present. */ +export async function deleteRepositoryAiKey(env: Env, fullName: string, actor?: string | null): Promise { + const existing = await getRepositoryAiKeyStatus(env, fullName); const db = getDb(env.DB); await db.delete(repositoryAiKeys).where(eq(repositoryAiKeys.repoFullName, fullName)); + if (existing.configured) { + await recordAiKeyChange(env, { repoFullName: fullName, action: "delete", provider: existing.provider, last4: existing.last4, actor: actor ?? null }); + } +} + +/** + * Audit a BYOK key lifecycle change (set/replace/delete). Stored in ai_usage_events as a non-"ok" + * status so it never counts toward the daily neuron budget. NEVER includes any key material — only the + * display-only last4 and the actor who made the change. + */ +async function recordAiKeyChange( + env: Env, + input: { repoFullName: string; action: "set" | "replace" | "delete"; provider: AiKeyProvider; last4: string; actor: string | null }, +): Promise { + await recordAiUsageEvent(env, { + feature: "ai_key_change", + actor: input.actor, + route: "maintainer.ai_key", + model: `byok:${input.provider}`, + status: input.action, + estimatedNeurons: 0, + detail: `provider key ${input.action}`, + metadata: { repoFullName: input.repoFullName, action: input.action, provider: input.provider, last4: input.last4 }, + }); } /** @@ -593,7 +622,7 @@ export async function getDecryptedRepositoryAiKey(env: Env, fullName: string): P const [row] = await db.select().from(repositoryAiKeys).where(eq(repositoryAiKeys.repoFullName, fullName)).limit(1); if (!row) return null; try { - const key = await decryptSecret(row.ciphertext, row.iv, secret); + const key = await decryptSecret(row.ciphertext, row.iv, secret, row.salt); return { provider: normalizeAiKeyProvider(row.provider), key, model: row.model ?? null }; } catch { return null; diff --git a/src/db/schema.ts b/src/db/schema.ts index 5264248575..4e7aa5c4d7 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -75,6 +75,10 @@ export const repositoryAiKeys = sqliteTable("repository_ai_keys", { provider: text("provider").notNull(), ciphertext: text("ciphertext").notNull(), iv: text("iv").notNull(), + // Per-record PBKDF2 salt (base64) for the v2 crypto envelope; null for legacy v1 rows (constant salt). + salt: text("salt"), + // Crypto-envelope version (NOT a key-rotation counter): 1 = legacy constant-salt, 2 = per-record salt. + // upsert overwrites in place; there is no rotation history. See src/utils/crypto.ts. keyVersion: integer("key_version").notNull().default(1), model: text("model"), last4: text("last4").notNull(), diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index adf366eb0c..c6eba0de28 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -304,9 +304,12 @@ export async function runGittensoryAiReview(env: Env, input: GittensoryAiReviewI const maxTokens = clampNumber(Number(env.AI_MAX_OUTPUT_TOKENS || 256), 256, 1024); const user = buildUserPrompt(input); - // block mode = advisory pass + consensus pass (2 models each, minus 1 when BYOK writes the advisory). - const calls = input.mode === "block" ? 3 : input.providerKey ? 1 : 2; - const estimatedNeurons = estimateNeurons(REVIEW_SYSTEM_PROMPT.length + user.length, maxTokens, calls); + // The daily neuron budget governs FREE Workers-AI spend only. BYOK advisory calls bill the maintainer's + // own provider account, so they are not counted here (and a BYOK advisory still runs when the free + // budget is exhausted). Free calls = the consensus pair in block mode (always Workers AI), plus the + // advisory leg only when it is NOT BYOK. + const freeAiCalls = (input.mode === "block" ? 2 : 0) + (input.providerKey ? 0 : 1); + const estimatedNeurons = freeAiCalls === 0 ? 0 : estimateNeurons(REVIEW_SYSTEM_PROMPT.length + user.length, maxTokens, freeAiCalls); const budget = clampNumber(Number(env.AI_DAILY_NEURON_BUDGET || 10000), 0, 1_000_000); const used = await sumAiEstimatedNeuronsSince(env, utcDayStartIso()); const remainingBudget = Math.max(0, budget - used); diff --git a/src/utils/crypto.ts b/src/utils/crypto.ts index b5bc418b52..13e2e66372 100644 --- a/src/utils/crypto.ts +++ b/src/utils/crypto.ts @@ -46,13 +46,20 @@ function hexToBytes(hex: string): Uint8Array { // AI-call time. The AES key is derived from the worker secret TOKEN_ENCRYPTION_SECRET via PBKDF2; a // fresh random 12-byte IV is used per encryption so ciphertexts are unique and the GCM tag authenticates // them. The plaintext key is never persisted, never logged, and never returned from the API. -const SECRET_KDF_SALT = new TextEncoder().encode("gittensory-secret-encryption-v1"); -const SECRET_KEY_VERSION = 1; - -async function deriveSecretAesKey(keyMaterial: string): Promise { +// +// Envelope versions (stored as key_version alongside the row): +// 1 = legacy: a single constant KDF salt for every record (SECRET_KDF_SALT_V1). +// 2 = current: a fresh random per-record salt, stored beside the IV, so each record's AES key is +// independently derived (defense-in-depth; decouples derived keys, eases future KDF rotation). +// Decryption keys off whether a per-record salt is present, so existing v1 rows (salt = null) keep +// decrypting with the constant salt. +const SECRET_KDF_SALT_V1 = new TextEncoder().encode("gittensory-secret-encryption-v1"); +const SECRET_KEY_VERSION_CURRENT = 2; + +async function deriveSecretAesKey(keyMaterial: string, salt: Uint8Array): Promise { const baseKey = await crypto.subtle.importKey("raw", new TextEncoder().encode(keyMaterial), "PBKDF2", false, ["deriveKey"]); return crypto.subtle.deriveKey( - { name: "PBKDF2", salt: SECRET_KDF_SALT, iterations: 100_000, hash: "SHA-256" }, + { name: "PBKDF2", salt, iterations: 100_000, hash: "SHA-256" }, baseKey, { name: "AES-GCM", length: 256 }, false, @@ -60,19 +67,33 @@ async function deriveSecretAesKey(keyMaterial: string): Promise { ); } -/** Encrypt a secret with AES-256-GCM. Returns base64 ciphertext (incl. auth tag) + base64 IV + version. */ -export async function encryptSecret(plaintext: string, keyMaterial: string): Promise<{ ciphertext: string; iv: string; version: number }> { +/** + * Encrypt a secret with AES-256-GCM. Returns base64 ciphertext (incl. auth tag) + base64 IV + the + * per-record salt (base64, null for the legacy v1 envelope) + envelope version. Production always uses + * the current envelope; `version` is parameterized only so tests can produce legacy v1 ciphertexts. + */ +export async function encryptSecret( + plaintext: string, + keyMaterial: string, + version: number = SECRET_KEY_VERSION_CURRENT, +): Promise<{ ciphertext: string; iv: string; salt: string | null; version: number }> { if (!keyMaterial) throw new Error("missing_encryption_secret"); - const key = await deriveSecretAesKey(keyMaterial); + const saltBytes = version >= 2 ? crypto.getRandomValues(new Uint8Array(16)) : SECRET_KDF_SALT_V1; + const key = await deriveSecretAesKey(keyMaterial, saltBytes); const iv = crypto.getRandomValues(new Uint8Array(12)); const encrypted = await crypto.subtle.encrypt({ name: "AES-GCM", iv }, key, new TextEncoder().encode(plaintext)); - return { ciphertext: base64Encode(new Uint8Array(encrypted)), iv: base64Encode(iv), version: SECRET_KEY_VERSION }; + return { ciphertext: base64Encode(new Uint8Array(encrypted)), iv: base64Encode(iv), salt: version >= 2 ? base64Encode(saltBytes) : null, version }; } -/** Decrypt a secret produced by {@link encryptSecret}. Throws if the secret/IV/ciphertext do not match. */ -export async function decryptSecret(ciphertext: string, iv: string, keyMaterial: string): Promise { +/** + * Decrypt a secret produced by {@link encryptSecret}. Pass the stored per-record `salt` for v2 rows; + * omit it (or pass null) for legacy v1 rows, which fall back to the constant salt. Throws if the + * secret/IV/salt/ciphertext do not match. + */ +export async function decryptSecret(ciphertext: string, iv: string, keyMaterial: string, salt?: string | null): Promise { if (!keyMaterial) throw new Error("missing_encryption_secret"); - const key = await deriveSecretAesKey(keyMaterial); + const saltBytes = salt ? base64ToBytes(salt) : SECRET_KDF_SALT_V1; + const key = await deriveSecretAesKey(keyMaterial, saltBytes); const decrypted = await crypto.subtle.decrypt({ name: "AES-GCM", iv: base64ToBytes(iv) }, key, base64ToBytes(ciphertext)); return new TextDecoder().decode(decrypted); } diff --git a/test/unit/ai-key-byok.test.ts b/test/unit/ai-key-byok.test.ts index 1ded870769..3c0d2899e8 100644 --- a/test/unit/ai-key-byok.test.ts +++ b/test/unit/ai-key-byok.test.ts @@ -10,20 +10,29 @@ import { createTestEnv } from "../helpers/d1"; const SECRET = "unit-test-encryption-secret-at-least-32-bytes-long"; describe("encryptSecret / decryptSecret (AES-256-GCM)", () => { - it("round-trips a secret and produces a fresh IV each time", async () => { + it("round-trips a secret with a fresh IV and a fresh per-record salt each time (v2 envelope)", async () => { const a = await encryptSecret("sk-ant-supersecret", SECRET); const b = await encryptSecret("sk-ant-supersecret", SECRET); expect(a.iv).not.toBe(b.iv); // random IV per encryption + expect(a.salt).not.toBe(b.salt); // random per-record salt per encryption expect(a.ciphertext).not.toBe(b.ciphertext); - expect(a.version).toBe(1); - await expect(decryptSecret(a.ciphertext, a.iv, SECRET)).resolves.toBe("sk-ant-supersecret"); + expect(a.version).toBe(2); + await expect(decryptSecret(a.ciphertext, a.iv, SECRET, a.salt)).resolves.toBe("sk-ant-supersecret"); + }); + + it("decrypts legacy v1 rows (no per-record salt) with the constant salt", async () => { + const legacy = await encryptSecret("sk-ant-legacy-key", SECRET, 1); + expect(legacy.version).toBe(1); + expect(legacy.salt).toBeNull(); + // A v1 row stores salt = NULL; decrypt without a salt falls back to the constant salt. + await expect(decryptSecret(legacy.ciphertext, legacy.iv, SECRET)).resolves.toBe("sk-ant-legacy-key"); }); it("fails to decrypt with the wrong secret and throws without a key", async () => { - const { ciphertext, iv } = await encryptSecret("sk-secret", SECRET); - await expect(decryptSecret(ciphertext, iv, "a-different-secret-of-sufficient-length-here")).rejects.toThrow(); + const { ciphertext, iv, salt } = await encryptSecret("sk-secret", SECRET); + await expect(decryptSecret(ciphertext, iv, "a-different-secret-of-sufficient-length-here", salt)).rejects.toThrow(); await expect(encryptSecret("x", "")).rejects.toThrow("missing_encryption_secret"); - await expect(decryptSecret(ciphertext, iv, "")).rejects.toThrow("missing_encryption_secret"); + await expect(decryptSecret(ciphertext, iv, "", salt)).rejects.toThrow("missing_encryption_secret"); }); }); @@ -33,12 +42,13 @@ describe("repository BYOK key storage", () => { await expect(getRepositoryAiKeyStatus(env, "acme/widgets")).resolves.toEqual({ configured: false }); const status = await upsertRepositoryAiKey(env, { repoFullName: "acme/widgets", provider: "anthropic", key: "sk-ant-abc123XYZ7890", model: "claude-3-5-sonnet-latest", createdBy: "maintainer" }); - expect(status).toEqual({ configured: true, provider: "anthropic", last4: "7890", model: "claude-3-5-sonnet-latest" }); + expect(status).toMatchObject({ configured: true, provider: "anthropic", last4: "7890", model: "claude-3-5-sonnet-latest", createdBy: "maintainer" }); + expect(status.configured && status.updatedAt).toMatch(/^\d{4}-\d{2}-\d{2}T/); - // Status surface never includes the key or ciphertext. + // Status surface never includes the key or ciphertext, but does surface who set it + when. const fetched = await getRepositoryAiKeyStatus(env, "acme/widgets"); expect(JSON.stringify(fetched)).not.toContain("sk-ant"); - expect(fetched).toMatchObject({ configured: true, last4: "7890" }); + expect(fetched).toMatchObject({ configured: true, last4: "7890", createdBy: "maintainer" }); // Decrypt only happens at call time. await expect(getDecryptedRepositoryAiKey(env, "acme/widgets")).resolves.toEqual({ provider: "anthropic", key: "sk-ant-abc123XYZ7890", model: "claude-3-5-sonnet-latest" }); @@ -59,6 +69,26 @@ describe("repository BYOK key storage", () => { await expect(getDecryptedRepositoryAiKey(env, "acme/widgets")).resolves.toBeNull(); }); + it("audits the key lifecycle (set → replace → delete) without ever recording key material", async () => { + const env = createTestEnv({ TOKEN_ENCRYPTION_SECRET: SECRET }); + await upsertRepositoryAiKey(env, { repoFullName: "acme/widgets", provider: "anthropic", key: "sk-ant-first-key-0000", createdBy: "alice" }); + await upsertRepositoryAiKey(env, { repoFullName: "acme/widgets", provider: "openai", key: "sk-openai-second-1111", createdBy: "bob" }); + await deleteRepositoryAiKey(env, "acme/widgets", "carol"); + + const events = await env.DB.prepare("select actor, status, model, metadata_json from ai_usage_events where feature = ? order by rowid asc").bind("ai_key_change").all<{ actor: string; status: string; model: string; metadata_json: string }>(); + expect(events.results.map((e) => e.status)).toEqual(["set", "replace", "delete"]); + expect(events.results.map((e) => e.actor)).toEqual(["alice", "bob", "carol"]); + // Audit rows never contain key material — only the display-only last4. + const blob = JSON.stringify(events.results); + expect(blob).not.toContain("sk-ant"); + expect(blob).not.toContain("sk-openai"); + + // A delete with no key present records nothing. + await deleteRepositoryAiKey(env, "acme/widgets", "carol"); + const after = await env.DB.prepare("select count(*) as n from ai_usage_events where feature = ?").bind("ai_key_change").first<{ n: number }>(); + expect(after?.n).toBe(3); + }); + it("stores real ISO timestamps when created_at/updated_at are omitted (no literal default)", async () => { const env = createTestEnv({ TOKEN_ENCRYPTION_SECRET: SECRET }); const db = getDb(env.DB); diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index c67d7b9dcc..f3cf6347cb 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -52,6 +52,27 @@ describe("runGittensoryAiReview gating", () => { await expect(runGittensoryAiReview(env, baseInput)).resolves.toMatchObject({ status: "quota_exceeded" }); expect(run).not.toHaveBeenCalled(); }); + + it("clamps a non-numeric AI_MAX_OUTPUT_TOKENS back to the default", async () => { + const run = vi.fn(async () => ({ response: reviewJson() })); + const env = createTestEnv({ AI: { run } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000", AI_MAX_OUTPUT_TOKENS: "not-a-number" }); + const result = await runGittensoryAiReview(env, baseInput); + expect(result.status).toBe("ok"); // NaN → clamped to the 256 floor, review still runs + }); + + it("does NOT count a BYOK advisory against the free neuron budget (it bills the maintainer)", async () => { + const fetchMock = vi.fn(async () => new Response(JSON.stringify({ content: [{ type: "text", text: reviewJson({ assessment: "BYOK advisory." }) }] }), { status: 200 })); + vi.stubGlobal("fetch", fetchMock); + const run = vi.fn(); + // Free budget is exhausted (1 neuron), but a BYOK advisory bills the maintainer's account, so it still runs. + const env = createTestEnv({ AI: { run } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "1" }); + const result = await runGittensoryAiReview(env, { ...baseInput, providerKey: { provider: "anthropic", key: "sk-ant-secret" } }); + expect(result.status).toBe("ok"); + expect(result.status === "ok" && result.advisoryNotes).toContain("BYOK advisory."); + expect(result.status === "ok" && result.estimatedNeurons).toBe(0); // advisory-only BYOK consumes no free budget + expect(fetchMock).toHaveBeenCalled(); + expect(run).not.toHaveBeenCalled(); + }); }); describe("runGittensoryAiReview advisory mode", () => {