Secrets in use move to the current MASTER_KEY by themselves during a rotation (#211)
* Secrets in use move to the current MASTER_KEY by themselves during a rotation
- CI / Typecheck and bundleFailed
- CI / Unit tests (1/2)Failed
- CI / Unit tests (2/2)Failed
6 files changed+64−11
Files (6)
modifiedapps/diffinery/docs/self-hosting/configuration.md+3−3View file
| @@ -199,15 +199,15 @@ | ||
| 199 | 199 | From then on: |
| 200 | 200 | |
| 201 | 201 | - New and replaced secrets (repository, organization and environment secrets, model provider keys, webhook secrets, import tokens) are encrypted with the new key. Each stored value names the key it was encrypted with (`v2.<key id>.…`, where the key id is the first 8 hex characters of the key's SHA-256: `printf %s "$NEW_KEY" | base64 -d | sha256sum | cut -c1-8`). Values stored before key ids existed (`v1.…`) are tried with both keys. |
| 202 | -- Webhook secrets are re-encrypted with the new key the next time the webhook delivers, and the GitHub token of an import when the import is retried. Repository, organization and environment secrets and model provider keys stay encrypted with the old key until someone saves them again. | |
| 202 | +- Stored values move to the new key by themselves as they are used: repository, organization and environment secrets the next time a workflow job uses them, model provider keys the next time an agent task does, webhook secrets the next time the webhook delivers, and the GitHub token of an import when the import is retried. A value nothing uses stays encrypted with the old key until someone saves it again. | |
| 203 | 203 | - Unsubscribe links, invitation and verification emails, sign-up codes, GitHub linking codes and OAuth state, runtime tokens, and cache and artifact URLs signed with the old key keep working; new ones are signed with the new key. |
| 204 | 204 | |
| 205 | 205 | Remove `MASTER_KEY_PREV` (`pnpm exec wrangler secret delete MASTER_KEY_PREV -c wrangler.deploy.jsonc`) only when nothing you still need depends on the old key: |
| 206 | 206 | |
| 207 | -- Every repository, organization and environment secret and every model provider key has been saved again since the rotation. Their values can't be read back, so whoever manages each one re-enters it. One left behind can't be decrypted once `MASTER_KEY_PREV` is gone, and runs that use it fail. | |
| 207 | +- Every repository, organization and environment secret and every model provider key you still need has been used or saved again since the rotation. Secrets your workflows use every day move over by themselves; one that only a rare workflow uses (a release, a yearly job) may not have, and its value can't be read back, so whoever manages it re-enters it. One left behind can't be decrypted once `MASTER_KEY_PREV` is gone, and runs that use it fail. | |
| 208 | 208 | - Links already sent have expired, or you accept that they stop working: runtime tokens and cache and artifact URLs last about as long as their job, sign-up codes 10 minutes, verification links 24 hours, invitation emails until the invitation expires, and unsubscribe links up to a year. Someone with an old unsubscribe link can still change their email settings after signing in. |
| 209 | 209 | |
| 210 | -If the old key leaked, remove `MASTER_KEY_PREV` as soon as the secrets have been saved again: until then, anyone holding the old key can still forge what it signs, and every secret encrypted with it is only as safe as that key. | |
| 210 | +If the old key leaked, treat the secrets as exposed: replace their values at the source (new API tokens, new passwords) and save the new values, which are sealed with the new key. Then remove `MASTER_KEY_PREV` right away: until then, anyone holding the old key can still forge what it signs. | |
| 211 | 211 | |
| 212 | 212 | ## Bindings |
| 213 | 213 |
modifiedapps/diffinery/src/agentic/keys.ts+5−2View file
| @@ -1,5 +1,5 @@ | ||
| 1 | 1 | import type { Env } from "../env.js"; |
| 2 | -import { decryptSecret, encryptSecret } from "../lib/crypto.js"; | |
| 2 | +import { decryptAndReseal, encryptSecret } from "../lib/crypto.js"; | |
| 3 | 3 | import { HttpError, now } from "../lib/util.js"; |
| 4 | 4 | import { enqueueMail } from "../mail/send.js"; |
| 5 | 5 | |
| @@ -120,6 +120,9 @@ | ||
| 120 | 120 | /** The key itself: only for the model proxy, inside the request it forwards, while a task this account pays for runs. */ |
| 121 | 121 | async reveal(accountId: string, provider: Provider): Promise<string | null> { |
| 122 | 122 | const row = await this.env.DB.prepare("SELECT ciphertext FROM model_keys WHERE account_id = ? AND provider = ?").bind(accountId, provider).first<{ ciphertext: string }>(); |
| 123 | - return row ? decryptSecret(this.env, scope(accountId, provider), row.ciphertext) : null; | |
| 123 | + if (!row) return null; | |
| 124 | + return decryptAndReseal(this.env, scope(accountId, provider), row.ciphertext, (sealed) => | |
| 125 | + this.env.DB.prepare("UPDATE model_keys SET ciphertext = ? WHERE account_id = ? AND provider = ? AND ciphertext = ?").bind(sealed, accountId, provider, row.ciphertext).run(), | |
| 126 | + ); | |
| 124 | 127 | } |
| 125 | 128 | } |
modifiedapps/diffinery/src/lib/crypto.ts+19−0View file
| @@ -154,6 +154,25 @@ | ||
| 154 | 154 | } |
| 155 | 155 | |
| 156 | 156 | /** |
| 157 | + * Decrypts a stored secret for use and, when it was sealed with the previous key or as v1, stores it | |
| 158 | + * again sealed with the current MASTER_KEY through `store`, so secrets in use move to the current key | |
| 159 | + * by themselves during a rotation. `store` must only replace the row while it still holds `envelope` | |
| 160 | + * (compare and set), so a value saved meanwhile is never overwritten. A failed store is logged and | |
| 161 | + * otherwise ignored: using the secret never depends on it. | |
| 162 | + */ | |
| 163 | +export async function decryptAndReseal(env: MasterKeyEnv, scope: string, envelope: string, store: (sealed: string) => unknown): Promise<string> { | |
| 164 | + const s = await openSecret(env, scope, envelope); | |
| 165 | + if (!s.current) { | |
| 166 | + try { | |
| 167 | + await store(await encryptSecret(env, scope, s.plaintext)); | |
| 168 | + } catch (e) { | |
| 169 | + console.error("re-encrypting a secret with the current MASTER_KEY failed", scope, (e as Error).message); | |
| 170 | + } | |
| 171 | + } | |
| 172 | + return s.plaintext; | |
| 173 | +} | |
| 174 | + | |
| 175 | +/** | |
| 157 | 176 | * The envelope to store in place of `envelope` when a row is written anyway: the same secret sealed |
| 158 | 177 | * with the current MASTER_KEY when it was sealed with the previous key or as v1, otherwise `envelope` |
| 159 | 178 | * itself. One that doesn't decrypt is kept as it is, so writing a row never loses what it held. |
modifiedapps/diffinery/src/repo/actions.ts+10−3View file
| @@ -15,7 +15,7 @@ | ||
| 15 | 15 | type TriggerEvent, |
| 16 | 16 | type Workflow, |
| 17 | 17 | } from "@diffinery/workflow-yaml"; |
| 18 | -import { decryptSecret, encryptSecret } from "../lib/crypto.js"; | |
| 18 | +import { decryptAndReseal, encryptSecret } from "../lib/crypto.js"; | |
| 19 | 19 | import { Directory } from "../lib/directory.js"; |
| 20 | 20 | import { getFirst, headFirst, legacyArtifactKeys, legacyCacheKeys, logKeys } from "../lib/storage-keys.js"; |
| 21 | 21 | import { permissionsForJob, type JobPermissions } from "../lib/job-permissions.js"; |
| @@ -2351,7 +2351,10 @@ | ||
| 2351 | 2351 | private async repoSecrets(): Promise<Record<string, string>> { |
| 2352 | 2352 | const out: Record<string, string> = {}; |
| 2353 | 2353 | for (const r of this.sql.exec<{ name: string; ciphertext: string }>("SELECT name, ciphertext FROM secrets").toArray()) { |
| 2354 | - out[r.name] = await decryptSecret(this.env, `repo:${this.repo.meta.repoId}`, r.ciphertext); | |
| 2354 | + // Sealed with a previous MASTER_KEY: stored again with the current one (lib/crypto.ts), keeping updated_at, which is when a person set it. | |
| 2355 | + out[r.name] = await decryptAndReseal(this.env, `repo:${this.repo.meta.repoId}`, r.ciphertext, (sealed) => | |
| 2356 | + this.sql.exec("UPDATE secrets SET ciphertext = ? WHERE name = ? AND ciphertext = ?", sealed, r.name, r.ciphertext), | |
| 2357 | + ); | |
| 2355 | 2358 | } |
| 2356 | 2359 | return out; |
| 2357 | 2360 | } |
| @@ -2368,7 +2371,11 @@ | ||
| 2368 | 2371 | .bind(m.ownerId, m.repoId) |
| 2369 | 2372 | .all<{ name: string; ciphertext: string }>(); |
| 2370 | 2373 | const out: Record<string, string> = {}; |
| 2371 | - for (const r of rows.results) out[r.name] = await decryptSecret(this.env, `org:${m.ownerId}`, r.ciphertext); | |
| 2374 | + for (const r of rows.results) { | |
| 2375 | + out[r.name] = await decryptAndReseal(this.env, `org:${m.ownerId}`, r.ciphertext, (sealed) => | |
| 2376 | + this.env.DB.prepare("UPDATE org_secrets SET ciphertext = ? WHERE org_id = ? AND name = ? AND ciphertext = ?").bind(sealed, m.ownerId, r.name, r.ciphertext).run(), | |
| 2377 | + ); | |
| 2378 | + } | |
| 2372 | 2379 | return out; |
| 2373 | 2380 | } |
| 2374 | 2381 |
modifiedapps/diffinery/src/repo/deployments.ts+4−2View file
| @@ -1,5 +1,5 @@ | ||
| 1 | 1 | import type { Env } from "../env.js"; |
| 2 | -import { decryptSecret, encryptSecret } from "../lib/crypto.js"; | |
| 2 | +import { decryptAndReseal, encryptSecret } from "../lib/crypto.js"; | |
| 3 | 3 | import { HttpError, now } from "../lib/util.js"; |
| 4 | 4 | import type { RepoDO } from "./RepoDO.js"; |
| 5 | 5 | import { requiredCheck } from "./required.js"; |
| @@ -610,7 +610,9 @@ | ||
| 610 | 610 | const secrets: Record<string, string> = {}; |
| 611 | 611 | const rows = this.secretsAllowed(name, job.ref, job.actor.kind) ? this.sql.exec<{ name: string; ciphertext: string }>("SELECT name, ciphertext FROM environment_secrets WHERE environment = ?", name).toArray() : []; |
| 612 | 612 | for (const r of rows) { |
| 613 | - secrets[r.name] = await decryptSecret(this.env, this.scope(name), r.ciphertext); | |
| 613 | + secrets[r.name] = await decryptAndReseal(this.env, this.scope(name), r.ciphertext, (sealed) => | |
| 614 | + this.sql.exec("UPDATE environment_secrets SET ciphertext = ? WHERE environment = ? AND name = ? AND ciphertext = ?", sealed, name, r.name, r.ciphertext), | |
| 615 | + ); | |
| 614 | 616 | } |
| 615 | 617 | const vars = Object.fromEntries(this.sql.exec<{ name: string; value: string }>("SELECT name, value FROM environment_variables WHERE environment = ?", name).toArray().map((r) => [r.name, r.value])); |
| 616 | 618 | return { secrets, vars }; |
modifiedapps/diffinery/test/unit/master-key.test.ts+23−1View file
| @@ -1,6 +1,6 @@ | ||
| 1 | 1 | import { createHash, createHmac } from "node:crypto"; |
| 2 | 2 | import { afterEach, describe, expect, it, vi } from "vitest"; |
| 3 | -import { decryptSecret, encryptSecret, keyId, masterKeys, matchesAnyKey, openSecret, resealSecret } from "../../src/lib/crypto.js"; | |
| 3 | +import { decryptAndReseal, decryptSecret, encryptSecret, keyId, masterKeys, matchesAnyKey, openSecret, resealSecret } from "../../src/lib/crypto.js"; | |
| 4 | 4 | import { signBlobUrl, signRuntimeToken, verifyBlobUrl, verifyRuntimeToken } from "../../src/lib/runtime-auth.js"; |
| 5 | 5 | import { inviteProof, validInviteProof } from "../../src/auth/invite-proof.js"; |
| 6 | 6 | import { readUnsubToken, unsubToken } from "../../src/mail/unsubscribe.js"; |
| @@ -104,6 +104,28 @@ | ||
| 104 | 104 | // One that doesn't decrypt is kept as it was. |
| 105 | 105 | expect(await resealSecret(other, "repo:r1", V1)).toBe(V1); |
| 106 | 106 | }); |
| 107 | + | |
| 108 | + it("in use are stored again under the current key, and using them never depends on that", async () => { | |
| 109 | + const current = await encryptSecret(during, "repo:r1", "hunter2"); | |
| 110 | + const stored: string[] = []; | |
| 111 | + expect(await decryptAndReseal(during, "repo:r1", current, (s) => stored.push(s))).toBe("hunter2"); | |
| 112 | + expect(stored).toEqual([]); | |
| 113 | + for (const stale of [V1, await encryptSecret(before, "repo:r1", "hunter2")]) { | |
| 114 | + stored.length = 0; | |
| 115 | + expect(await decryptAndReseal(during, "repo:r1", stale, (s) => stored.push(s))).toBe("hunter2"); | |
| 116 | + expect(stored).toHaveLength(1); | |
| 117 | + expect(stored[0]!.split(".").slice(0, 2)).toEqual(["v2", kidOf(NEW)]); | |
| 118 | + expect(await decryptSecret(after, "repo:r1", stored[0]!)).toBe("hunter2"); | |
| 119 | + } | |
| 120 | + // A store that fails (or rejects) is logged, and the secret is still returned. | |
| 121 | + const err = vi.spyOn(console, "error").mockImplementation(() => {}); | |
| 122 | + expect(await decryptAndReseal(during, "repo:r1", V1, () => Promise.reject(new Error("D1 down")))).toBe("hunter2"); | |
| 123 | + expect(await decryptAndReseal(during, "repo:r1", V1, () => { throw new Error("sql busy"); })).toBe("hunter2"); | |
| 124 | + expect(err).toHaveBeenCalledTimes(2); | |
| 125 | + err.mockRestore(); | |
| 126 | + // One that doesn't decrypt still throws, as decryptSecret does, and stores nothing. | |
| 127 | + await expect(decryptAndReseal(other, "repo:r1", V1, (s) => stored.push(s))).rejects.toThrow(/decrypt/); | |
| 128 | + }); | |
| 107 | 129 | }); |
| 108 | 130 | |
| 109 | 131 | describe("signatures made with MASTER_KEY", () => { |