From 3aa8bd04ecf4220f5614349ffcac4ac59a60c254 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Fri, 25 Sep 2026 11:29:36 -0700 Subject: [PATCH 1/3] refactor(accounts): make API keys opaque secrets the proxy resolves by hash ADR-013 as revised (source-cooperative/data.source.coop#234): a key is `sck_` + 32 random bytes, stored only as its SHA-256, which is now the record's partition key; a random public `key_id` is the handle the UI, revoke and expiry actions use. Nothing signs a key and nothing calls the proxy to issue one, so the Ory ID-token round-trip, the compensating delete and `proxy-keys.ts` go, and `getOryIdToken` returns to private. The exchanges route becomes `POST /api/v1/service-account-keys/exchanges`, keyed by hash in the body. The proxy calls it before it knows which account a key belongs to, so it authenticates as itself: `verifyProxyAssertion`, extracted from `authenticateWithOidcToken`, checks the assertion, and the route accepts only the sentinel subject `urn:source:data-proxy`, which `authenticateWithOidcToken` now refuses outright so it can never become a session. Unknown, revoked, expired and disabled keys all answer `active: false` with no account, and a live key answers with its account and `key_id`; last use is recorded best-effort so a throttled write cannot refuse a live key. Pages strip the hash with `publicKey` before a record reaches the client component. The dialog's hint describes the stock-SDK setup, and the Storybook mock returns a key of the real shape. Co-Authored-By: Claude Fable 5.1 --- deploy/lib/database-construct.ts | 5 +- scripts/init-local.ts | 4 +- .../[service_account_id]/page.tsx | 5 +- .../[account_id]/service-accounts/page.tsx | 4 +- .../[jti]/exchanges/route.test.ts | 48 ----------- .../[jti]/exchanges/route.ts | 54 ------------ .../exchanges/route.test.ts | 74 +++++++++++++++++ .../service-account-keys/exchanges/route.ts | 82 +++++++++++++++++++ .../IssueApiKeyDialog.stories.tsx | 7 +- .../service-accounts/IssueApiKeyDialog.tsx | 7 +- .../ServiceAccountDetail.stories.tsx | 4 +- .../service-accounts/ServiceAccountDetail.tsx | 4 +- .../ServiceAccountList.stories.tsx | 4 +- .../actions/__mocks__/service-account-keys.ts | 9 +- src/lib/actions/proxy-credentials.ts | 2 +- src/lib/actions/service-account-keys.test.ts | 63 +++++++------- src/lib/actions/service-account-keys.ts | 63 +++++++------- src/lib/api/oidc.test.ts | 46 ++++++++++- src/lib/api/oidc.ts | 45 ++++++++-- .../clients/database/service-account-keys.ts | 35 +++----- src/lib/services/proxy-keys.ts | 38 --------- src/types/service-account-key.ts | 28 +++++-- 22 files changed, 362 insertions(+), 269 deletions(-) delete mode 100644 src/app/api/v1/service-account-keys/[jti]/exchanges/route.test.ts delete mode 100644 src/app/api/v1/service-account-keys/[jti]/exchanges/route.ts create mode 100644 src/app/api/v1/service-account-keys/exchanges/route.test.ts create mode 100644 src/app/api/v1/service-account-keys/exchanges/route.ts delete mode 100644 src/lib/services/proxy-keys.ts diff --git a/deploy/lib/database-construct.ts b/deploy/lib/database-construct.ts index 35e848933..b599a6480 100644 --- a/deploy/lib/database-construct.ts +++ b/deploy/lib/database-construct.ts @@ -77,8 +77,9 @@ export class DatabaseConstruct extends Construct { this.serviceAccountKeysTable = this.createTable({ name: "service-account-keys", stage, - // The key's jti; the key itself is never stored. - partitionKey: "jti", + // SHA-256 of the key, which the proxy presents to look it up; the key + // itself is never stored. + partitionKey: "key_hash", indexes: [ { // fetch the keys of a service account diff --git a/scripts/init-local.ts b/scripts/init-local.ts index 2ede27256..c2fff4bb1 100644 --- a/scripts/init-local.ts +++ b/scripts/init-local.ts @@ -178,10 +178,10 @@ async function createTables() { new CreateTableCommand({ TableName: getTableName("service-account-keys"), AttributeDefinitions: [ - { AttributeName: "jti", AttributeType: "S" }, + { AttributeName: "key_hash", AttributeType: "S" }, { AttributeName: "account_id", AttributeType: "S" }, ], - KeySchema: [{ AttributeName: "jti", KeyType: "HASH" }], + KeySchema: [{ AttributeName: "key_hash", KeyType: "HASH" }], GlobalSecondaryIndexes: [ { IndexName: "account_id", diff --git a/src/app/(app)/edit/account/[account_id]/service-accounts/[service_account_id]/page.tsx b/src/app/(app)/edit/account/[account_id]/service-accounts/[service_account_id]/page.tsx index 74b103ddd..312003663 100644 --- a/src/app/(app)/edit/account/[account_id]/service-accounts/[service_account_id]/page.tsx +++ b/src/app/(app)/edit/account/[account_id]/service-accounts/[service_account_id]/page.tsx @@ -14,7 +14,7 @@ import { CONFIG } from "@/lib/config"; import { getPageSession } from "@/lib/api/utils"; import { managedServiceAccount } from "@/lib/accounts/service-accounts"; import { editAccountServiceAccountsUrl } from "@/lib/urls"; -import { MembershipState } from "@/types"; +import { MembershipState, publicKey } from "@/types"; export const metadata: Metadata = { title: "Service account" }; @@ -47,7 +47,8 @@ export default async function ServiceAccountPage({ params }: PageProps) { account, trusts, grants: memberships.filter((m) => m.state === MembershipState.Member), - keys, + // The hash stays on the server; the client component sees the rest. + keys: keys.map(publicKey), }} products={products.map(({ product_id, title }) => ({ product_id, title }))} proxyOrigin={CONFIG.storage.endpoint} diff --git a/src/app/(app)/edit/account/[account_id]/service-accounts/page.tsx b/src/app/(app)/edit/account/[account_id]/service-accounts/page.tsx index 6667b87f2..9a70b3288 100644 --- a/src/app/(app)/edit/account/[account_id]/service-accounts/page.tsx +++ b/src/app/(app)/edit/account/[account_id]/service-accounts/page.tsx @@ -14,7 +14,7 @@ import { import { getPageSession } from "@/lib/api/utils"; import { canManageAccount } from "@/lib/api/authz"; import { createServiceAccountUrl } from "@/lib/urls"; -import { MembershipState, type ServiceAccountSummary } from "@/types"; +import { MembershipState, publicKey, type ServiceAccountSummary } from "@/types"; export const metadata: Metadata = { title: "Service accounts" }; @@ -38,7 +38,7 @@ export default async function ServiceAccountsPage({ params }: PageProps) { grants: (await membershipsTable.listByUser(account.account_id)).filter( (m) => m.state === MembershipState.Member ), - keys: await serviceAccountKeysTable.listByAccount(account.account_id), + keys: (await serviceAccountKeysTable.listByAccount(account.account_id)).map(publicKey), })) ); diff --git a/src/app/api/v1/service-account-keys/[jti]/exchanges/route.test.ts b/src/app/api/v1/service-account-keys/[jti]/exchanges/route.test.ts deleted file mode 100644 index eb009a6c7..000000000 --- a/src/app/api/v1/service-account-keys/[jti]/exchanges/route.test.ts +++ /dev/null @@ -1,48 +0,0 @@ -/** @jest-environment node */ -import { NextRequest } from "next/server"; -import { serviceAccountKeysTable } from "@/lib/clients/database"; -import { getApiSession } from "@/lib/api/utils"; -import { isAdmin } from "@/lib/api/authz"; - -jest.mock("@/lib/clients/database", () => ({ - serviceAccountKeysTable: { fetchByJti: jest.fn(), set: jest.fn() }, -})); -jest.mock("@/lib/api/utils", () => ({ getApiSession: jest.fn() })); -jest.mock("@/lib/api/authz", () => ({ isAdmin: jest.fn() })); - -const { POST } = require("./route"); - -const key = { jti: "j1", account_id: "acme--nightly-sync", label: "HPC", expires_at: null }; -const params = { params: { jti: "j1" } }; -const req = () => new NextRequest("http://localhost/api/v1/service-account-keys/j1/exchanges", { method: "POST" }); - -describe("POST /api/v1/service-account-keys/[jti]/exchanges", () => { - beforeEach(() => { - jest.resetAllMocks(); - (serviceAccountKeysTable.fetchByJti as jest.Mock).mockResolvedValue(key); - (getApiSession as jest.Mock).mockResolvedValue({ account: { account_id: "acme--nightly-sync" } }); - (isAdmin as jest.Mock).mockReturnValue(false); - }); - - test("answers active for a live key, as the account it belongs to, and records the use", async () => { - const res = await POST(req(), params); - expect(res.status).toBe(200); - await expect(res.json()).resolves.toMatchObject({ account_id: "acme--nightly-sync", active: true }); - expect(serviceAccountKeysTable.set).toHaveBeenCalledWith("j1", "last_used_at", expect.any(String)); - }); - - test("answers inactive for a revoked or expired key, without recording a use", async () => { - (serviceAccountKeysTable.fetchByJti as jest.Mock).mockResolvedValue({ ...key, revoked_at: "2026-01-01T00:00:00Z" }); - await expect((await POST(req(), params)).json()).resolves.toMatchObject({ active: false }); - (serviceAccountKeysTable.fetchByJti as jest.Mock).mockResolvedValue({ ...key, expires_at: "2020-01-01T00:00:00Z" }); - await expect((await POST(req(), params)).json()).resolves.toMatchObject({ active: false }); - expect(serviceAccountKeysTable.set).not.toHaveBeenCalled(); - }); - - test("is 401 for any other account, 404 for an unknown jti", async () => { - (getApiSession as jest.Mock).mockResolvedValue({ account: { account_id: "someone-else" } }); - expect((await POST(req(), params)).status).toBe(401); - (serviceAccountKeysTable.fetchByJti as jest.Mock).mockResolvedValue(null); - expect((await POST(req(), params)).status).toBe(404); - }); -}); diff --git a/src/app/api/v1/service-account-keys/[jti]/exchanges/route.ts b/src/app/api/v1/service-account-keys/[jti]/exchanges/route.ts deleted file mode 100644 index b84e74f13..000000000 --- a/src/app/api/v1/service-account-keys/[jti]/exchanges/route.ts +++ /dev/null @@ -1,54 +0,0 @@ -/** - * @openapi - * /service-account-keys/{jti}/exchanges: - * post: - * tags: [Accounts] - * summary: Record that an API key was presented, and say whether it may be exchanged - * description: | - * Called by the data proxy at `/.sts` when a token carrying this `jti` is presented, as the service account the key belongs to. Records the use and answers whether the key is still active — not revoked, not expired. The proxy caches the answer briefly, so revocation takes effect for new exchanges within that TTL. - * parameters: - * - in: path - * name: jti - * required: true - * schema: - * type: string - * responses: - * 200: - * description: The key's standing - * 401: - * description: Unauthorized - * 404: - * description: Not Found - */ -import { NextRequest, NextResponse } from "next/server"; -import { StatusCodes } from "http-status-codes"; -import { getApiSession } from "@/lib/api/utils"; -import { isAdmin } from "@/lib/api/authz"; -import { serviceAccountKeysTable } from "@/lib/clients/database"; -import { isKeyActive } from "@/types"; - -export async function POST( - request: NextRequest, - { params }: { params: Promise<{ jti: string }> } -) { - const session = await getApiSession(request); - const { jti } = await params; - const key = await serviceAccountKeysTable.fetchByJti(jti); - if (!key) { - return NextResponse.json({ error: "No such key" }, { status: StatusCodes.NOT_FOUND }); - } - // Only the account the key belongs to — the proxy, calling as it — may ask. - if (!session?.account || (session.account.account_id !== key.account_id && !isAdmin(session))) { - return NextResponse.json({ error: "Unauthorized" }, { status: StatusCodes.UNAUTHORIZED }); - } - const active = isKeyActive(key); - if (active) { - await serviceAccountKeysTable.set(jti, "last_used_at", new Date().toISOString()); - } - return NextResponse.json({ - account_id: key.account_id, - active, - expires_at: key.expires_at, - revoked_at: key.revoked_at ?? null, - }); -} diff --git a/src/app/api/v1/service-account-keys/exchanges/route.test.ts b/src/app/api/v1/service-account-keys/exchanges/route.test.ts new file mode 100644 index 000000000..38d39ebd9 --- /dev/null +++ b/src/app/api/v1/service-account-keys/exchanges/route.test.ts @@ -0,0 +1,74 @@ +/** @jest-environment node */ +import { NextRequest } from "next/server"; +import { accountsTable, serviceAccountKeysTable } from "@/lib/clients/database"; +import { verifyProxyAssertion } from "@/lib/api/oidc"; + +jest.mock("@/lib/clients/database", () => ({ + serviceAccountKeysTable: { fetchByHash: jest.fn(), set: jest.fn() }, + accountsTable: { fetchById: jest.fn() }, +})); +jest.mock("@/lib/api/oidc", () => ({ + verifyProxyAssertion: jest.fn(), + PROXY_SELF_SUBJECT: "urn:source:data-proxy", +})); + +const { POST } = require("./route"); + +const HASH = "a".repeat(64); +const key = { key_hash: HASH, key_id: "k1", account_id: "acme--nightly-sync", label: "HPC", expires_at: null }; +const req = (body: unknown = { key_hash: HASH }) => + new NextRequest("http://localhost/api/v1/service-account-keys/exchanges", { + method: "POST", + headers: { authorization: "Bearer proxy", "x-request-id": "r1" }, + body: JSON.stringify(body), + }); + +describe("POST /api/v1/service-account-keys/exchanges", () => { + beforeEach(() => { + jest.resetAllMocks(); + (verifyProxyAssertion as jest.Mock).mockResolvedValue({ sub: "urn:source:data-proxy" }); + (serviceAccountKeysTable.fetchByHash as jest.Mock).mockResolvedValue(key); + (serviceAccountKeysTable.set as jest.Mock).mockResolvedValue(undefined); + (accountsTable.fetchById as jest.Mock).mockResolvedValue({ account_id: "acme--nightly-sync", disabled: false }); + }); + + test("answers active with the account for a live key, and records the use", async () => { + const res = await POST(req()); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toEqual({ account_id: "acme--nightly-sync", key_id: "k1", active: true }); + expect(serviceAccountKeysTable.fetchByHash).toHaveBeenCalledWith(HASH); + expect(serviceAccountKeysTable.set).toHaveBeenCalledWith(HASH, "last_used_at", expect.any(String)); + }); + + test("still answers active when recording the use fails", async () => { + (serviceAccountKeysTable.set as jest.Mock).mockRejectedValue(new Error("throttled")); + await expect((await POST(req())).json()).resolves.toMatchObject({ active: true }); + }); + + test("answers inactive, naming no account, for a revoked, expired, disabled or unknown key, without recording a use", async () => { + (serviceAccountKeysTable.fetchByHash as jest.Mock).mockResolvedValue({ ...key, revoked_at: "2026-01-01T00:00:00Z" }); + await expect((await POST(req())).json()).resolves.toEqual({ active: false }); + (serviceAccountKeysTable.fetchByHash as jest.Mock).mockResolvedValue({ ...key, expires_at: "2020-01-01T00:00:00Z" }); + await expect((await POST(req())).json()).resolves.toEqual({ active: false }); + (serviceAccountKeysTable.fetchByHash as jest.Mock).mockResolvedValue(key); + (accountsTable.fetchById as jest.Mock).mockResolvedValue({ account_id: "acme--nightly-sync", disabled: true }); + await expect((await POST(req())).json()).resolves.toEqual({ active: false }); + (serviceAccountKeysTable.fetchByHash as jest.Mock).mockResolvedValue(null); + await expect((await POST(req())).json()).resolves.toEqual({ active: false }); + expect(serviceAccountKeysTable.set).not.toHaveBeenCalled(); + }); + + test("is 401 for any subject but the proxy's own, or no assertion", async () => { + (verifyProxyAssertion as jest.Mock).mockResolvedValue({ sub: "acme--nightly-sync" }); + expect((await POST(req())).status).toBe(401); + (verifyProxyAssertion as jest.Mock).mockResolvedValue(null); + expect((await POST(req())).status).toBe(401); + expect(serviceAccountKeysTable.fetchByHash).not.toHaveBeenCalled(); + }); + + test("is 400 for a body without a hex SHA-256", async () => { + expect((await POST(req({ key_hash: "sck_notahash" }))).status).toBe(400); + expect((await POST(req({}))).status).toBe(400); + expect(serviceAccountKeysTable.fetchByHash).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/v1/service-account-keys/exchanges/route.ts b/src/app/api/v1/service-account-keys/exchanges/route.ts new file mode 100644 index 000000000..579e91444 --- /dev/null +++ b/src/app/api/v1/service-account-keys/exchanges/route.ts @@ -0,0 +1,82 @@ +/** + * @openapi + * /service-account-keys/exchanges: + * post: + * tags: [Accounts] + * summary: Say whether an API key may be exchanged, and which service account it belongs to + * description: | + * Called by the data proxy at `/.sts` when an API key is presented, authenticated as the proxy itself: the key is opaque, so nothing names an account before this lookup. The proxy sends the key's SHA-256; this answers whether the key is active — known, not revoked, not expired, and its service account not disabled — and, if so, which account it is. An unknown hash is answered as inactive, indistinguishable from a revoked one. Records last use. The proxy caches the answer briefly, so revocation takes effect for new exchanges within that TTL. + * requestBody: + * required: true + * content: + * application/json: + * schema: + * type: object + * required: [key_hash] + * properties: + * key_hash: + * type: string + * description: Hex SHA-256 of the key + * responses: + * 200: + * description: The key's standing + * 400: + * description: Bad Request + * 401: + * description: Unauthorized + */ +import { NextRequest, NextResponse } from "next/server"; +import { StatusCodes } from "http-status-codes"; +import { z } from "zod"; +import { PROXY_SELF_SUBJECT, verifyProxyAssertion } from "@/lib/api/oidc"; +import { accountsTable, serviceAccountKeysTable } from "@/lib/clients/database"; +import { LOGGER } from "@/lib/logging"; +import { isKeyActive } from "@/types"; + +const BodySchema = z.object({ key_hash: z.string().regex(/^[0-9a-f]{64}$/) }); + +export async function POST(request: NextRequest) { + // Only the proxy, as itself. Never a session: the sentinel subject resolves + // to no account, and a cookie must not reach this route. + const assertion = await verifyProxyAssertion( + request.headers.get("authorization"), + new URL(request.url).origin + ); + if (assertion?.sub !== PROXY_SELF_SUBJECT) { + return NextResponse.json({ error: "Unauthorized" }, { status: StatusCodes.UNAUTHORIZED }); + } + const parsed = BodySchema.safeParse(await request.json().catch(() => null)); + if (!parsed.success) { + return NextResponse.json({ error: "key_hash is required" }, { status: StatusCodes.BAD_REQUEST }); + } + + const request_id = request.headers.get("x-request-id") ?? undefined; + const key = await serviceAccountKeysTable.fetchByHash(parsed.data.key_hash); + const account = key ? await accountsTable.fetchById(key.account_id) : null; + const reason = !key + ? "unknown" + : key.revoked_at + ? "revoked" + : !isKeyActive(key) + ? "expired" + : !account || account.disabled + ? "disabled" + : null; + const meta = { request_id, key_id: key?.key_id, account_id: key?.account_id, reason }; + if (!key || reason) { + LOGGER.warn("API key not exchangeable", { operation: "serviceAccountKeyExchange", metadata: meta }); + return NextResponse.json({ active: false }); + } + + // Best-effort: a throttled write must not refuse a live key. + await serviceAccountKeysTable + .set(key.key_hash, "last_used_at", new Date().toISOString()) + .catch((error: unknown) => + LOGGER.warn("Could not record API key use", { + operation: "serviceAccountKeyExchange", + metadata: { ...meta, error: error instanceof Error ? error.message : String(error) }, + }) + ); + LOGGER.info("API key exchanged", { operation: "serviceAccountKeyExchange", metadata: meta }); + return NextResponse.json({ account_id: key.account_id, key_id: key.key_id, active: true }); +} diff --git a/src/components/features/service-accounts/IssueApiKeyDialog.stories.tsx b/src/components/features/service-accounts/IssueApiKeyDialog.stories.tsx index 62f108f8e..d0aa69832 100644 --- a/src/components/features/service-accounts/IssueApiKeyDialog.stories.tsx +++ b/src/components/features/service-accounts/IssueApiKeyDialog.stories.tsx @@ -5,9 +5,10 @@ import { IssueApiKeyDialog } from "./IssueApiKeyDialog"; * Issuing an API key: a label and an expiry, then the key — shown once, with * the copy affordance and the warning that says so. * - * `issueApiKey` is mocked in `.storybook/preview.tsx` and resolves as though - * the proxy had signed a key, so **submitting the form shows the show-once - * view**. Open the dialog, give it a label, and issue. + * `issueApiKey` is mocked in `.storybook/preview.tsx` and resolves with a key + * of the real shape, `sck_` and 43 characters of nothing secret, so + * **submitting the form shows the show-once view**. Open the dialog, give it + * a label, and issue. */ const meta = { title: "Features/Service accounts/IssueApiKeyDialog", diff --git a/src/components/features/service-accounts/IssueApiKeyDialog.tsx b/src/components/features/service-accounts/IssueApiKeyDialog.tsx index bab779f85..d0eaba726 100644 --- a/src/components/features/service-accounts/IssueApiKeyDialog.tsx +++ b/src/components/features/service-accounts/IssueApiKeyDialog.tsx @@ -60,9 +60,10 @@ export function IssueApiKeyDialog({ accountId }: { accountId: string }) { - Put it in AWS_WEB_IDENTITY_TOKEN_FILE and point - the SDK at the data proxy's STS endpoint. Revoke it here if it - leaks; only a hash of nothing is kept — the key itself is not stored. + Save it to a file, point AWS_WEB_IDENTITY_TOKEN_FILE{" "} + at that file, and set AWS_ROLE_ARN and the data + proxy's STS endpoint; a stock AWS SDK does the rest. Revoke it here + if it leaks. Only its hash is stored — the key itself is not. diff --git a/src/components/features/service-accounts/ServiceAccountDetail.stories.tsx b/src/components/features/service-accounts/ServiceAccountDetail.stories.tsx index b6ef0795c..a173a6da5 100644 --- a/src/components/features/service-accounts/ServiceAccountDetail.stories.tsx +++ b/src/components/features/service-accounts/ServiceAccountDetail.stories.tsx @@ -82,7 +82,7 @@ const summary: ServiceAccountSummary = { ], keys: [ { - jti: "k1", + key_id: "k1", account_id: "nightly-sync", label: "HPC cron job", created_at: "2026-03-12T00:00:00Z", @@ -91,7 +91,7 @@ const summary: ServiceAccountSummary = { last_used_at: "2026-03-20T00:00:00Z", }, { - jti: "k2", + key_id: "k2", account_id: "nightly-sync", label: "Old laptop", created_at: "2025-03-12T00:00:00Z", diff --git a/src/components/features/service-accounts/ServiceAccountDetail.tsx b/src/components/features/service-accounts/ServiceAccountDetail.tsx index 74dc84586..b63be42a9 100644 --- a/src/components/features/service-accounts/ServiceAccountDetail.tsx +++ b/src/components/features/service-accounts/ServiceAccountDetail.tsx @@ -208,7 +208,7 @@ export function ServiceAccountDetail({ const marker = keyMarker(key); return ( {key.label} @@ -224,7 +224,7 @@ export function ServiceAccountDetail({ !key.revoked_at && (
- + diff --git a/src/components/features/service-accounts/ServiceAccountList.stories.tsx b/src/components/features/service-accounts/ServiceAccountList.stories.tsx index 71b697aab..ef2e41000 100644 --- a/src/components/features/service-accounts/ServiceAccountList.stories.tsx +++ b/src/components/features/service-accounts/ServiceAccountList.stories.tsx @@ -63,7 +63,7 @@ const summary = ( ], keys: [ { - jti: `k1-${account_id}`, + key_id: `k1-${account_id}`, account_id, label: "HPC cron job", created_at: "2026-03-12T00:00:00Z", @@ -72,7 +72,7 @@ const summary = ( last_used_at: "2026-03-20T00:00:00Z", }, { - jti: `k2-${account_id}`, + key_id: `k2-${account_id}`, account_id, label: "Old laptop", created_at: "2025-03-12T00:00:00Z", diff --git a/src/lib/actions/__mocks__/service-account-keys.ts b/src/lib/actions/__mocks__/service-account-keys.ts index cd96e55c7..30f3f2ad1 100644 --- a/src/lib/actions/__mocks__/service-account-keys.ts +++ b/src/lib/actions/__mocks__/service-account-keys.ts @@ -4,8 +4,9 @@ import type { ApiKeyActionState } from "@/types"; /** * Storybook stand-in for the API-key server actions, redirected to by - * `sb.mock()` in `.storybook/preview.tsx`. `issueApiKey` resolves as though a - * key were signed, so the show-once view is reachable by submitting the dialog. + * `sb.mock()` in `.storybook/preview.tsx`. `issueApiKey` resolves with a key + * of the real shape, so the show-once view is reachable by submitting the + * dialog. */ const idle = (): ApiKeyActionState => ({ message: "", success: false }); @@ -14,9 +15,9 @@ export const issueApiKey: typeof Real.issueApiKey = fn( message: "", success: true, issued: { - key: "sck_eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImsxIn0.eyJpc3MiOiJodHRwczovL2RhdGEuc291cmNlLmNvb3AiLCJzdWIiOiJuaWdodGx5LXN5bmMiLCJqdGkiOiIxIiwidHlwZSI6ImFwaV9rZXkifQ.signature", + key: "sck_storyFixtureNotARealKey0123456789abcdefghij", record: { - jti: "6f1c2a3b-4d5e-4f60-8a9b-0c1d2e3f4a5b", + key_id: "6f1c2a3b-4d5e-4f60-8a9b-0c1d2e3f4a5b", account_id: "nightly-sync", label: String(formData.get("label") || "CI"), created_at: "2026-03-12T00:00:00Z", diff --git a/src/lib/actions/proxy-credentials.ts b/src/lib/actions/proxy-credentials.ts index 77fa1c900..f619080cf 100644 --- a/src/lib/actions/proxy-credentials.ts +++ b/src/lib/actions/proxy-credentials.ts @@ -96,7 +96,7 @@ export async function getProxyCredentials( * If the OAuth2 client has skip_consent enabled (recommended), the consent * step is skipped and the flow completes in 4 HTTP calls instead of 6. */ -export async function getOryIdToken(identityId: string): Promise { +async function getOryIdToken(identityId: string): Promise { const { api: { backendUrl }, accessToken: adminApiKey, diff --git a/src/lib/actions/service-account-keys.test.ts b/src/lib/actions/service-account-keys.test.ts index 47d490a1c..c2826a084 100644 --- a/src/lib/actions/service-account-keys.test.ts +++ b/src/lib/actions/service-account-keys.test.ts @@ -1,21 +1,19 @@ +import { createHash } from "crypto"; import { issueApiKey, revokeApiKey, setApiKeyExpiry } from "./service-account-keys"; import { serviceAccountKeysTable } from "../clients"; import { getPageSession } from "../api/utils"; import { managedServiceAccount } from "@/lib/accounts/service-accounts"; -import { mintApiKey } from "@/lib/services/proxy-keys"; -import { AccountType, type Account, type ApiKeyActionState, type UserSession } from "@/types"; +import { API_KEY_PATTERN, AccountType, type Account, type ApiKeyActionState, type UserSession } from "@/types"; jest.mock("../clients", () => ({ - serviceAccountKeysTable: { create: jest.fn(), delete: jest.fn(), fetchByJti: jest.fn(), set: jest.fn() }, + serviceAccountKeysTable: { create: jest.fn(), listByAccount: jest.fn(), set: jest.fn() }, })); jest.mock("../api/utils", () => ({ getPageSession: jest.fn() })); jest.mock("@/lib/accounts/service-accounts", () => ({ managedServiceAccount: jest.fn() })); -jest.mock("@/lib/services/proxy-keys", () => ({ mintApiKey: jest.fn() })); jest.mock("next/cache", () => ({ revalidatePath: jest.fn() })); const keys = serviceAccountKeysTable as jest.Mocked; const managed = managedServiceAccount as jest.MockedFunction; -const mint = mintApiKey as jest.MockedFunction; const IDLE: ApiKeyActionState = { message: "", success: false }; const bot = { @@ -32,24 +30,34 @@ const form = (fields: Record) => { for (const [k, v] of Object.entries(fields)) data.set(k, v); return data; }; +const sha256 = (s: string) => createHash("sha256").update(s).digest("hex"); beforeEach(() => { jest.clearAllMocks(); (getPageSession as jest.Mock).mockResolvedValue(session); managed.mockResolvedValue(bot as never); keys.create.mockImplementation(async (k) => k); - mint.mockResolvedValue("sck_eyJ.signed.key"); }); describe("issueApiKey", () => { - it("records the key, has the proxy sign it, and returns it once", async () => { + it("stores only the key's hash and returns the key once", async () => { const result = await issueApiKey(IDLE, form({ account_id: "acme--nightly-sync", label: "HPC", expires_in_days: "90" })); expect(result.success).toBe(true); - expect(result.issued?.key).toBe("sck_eyJ.signed.key"); - const record = keys.create.mock.calls[0][0]; - expect(record).toMatchObject({ account_id: "acme--nightly-sync", label: "HPC", created_by: "alice" }); - expect(record.expires_at).not.toBeNull(); - expect(mint).toHaveBeenCalledWith("ory-alice", { account_id: "acme--nightly-sync", jti: record.jti, expires_at: record.expires_at }); + const key = result.issued!.key; + expect(key).toMatch(API_KEY_PATTERN); + const stored = keys.create.mock.calls[0][0]; + expect(stored).toMatchObject({ key_hash: sha256(key), account_id: "acme--nightly-sync", label: "HPC", created_by: "alice" }); + expect(stored.expires_at).not.toBeNull(); + expect(JSON.stringify(stored)).not.toContain(key); + // The record handed back is the public one: no hash, and the same handle. + expect(result.issued!.record).not.toHaveProperty("key_hash"); + expect(result.issued!.record.key_id).toBe(stored.key_id); + }); + + it("issues a different key every time", async () => { + const a = await issueApiKey(IDLE, form({ account_id: "acme--nightly-sync", label: "a" })); + const b = await issueApiKey(IDLE, form({ account_id: "acme--nightly-sync", label: "b" })); + expect(a.issued!.key).not.toBe(b.issued!.key); }); it("issues a key with no expiry when asked", async () => { @@ -58,13 +66,6 @@ describe("issueApiKey", () => { expect(keys.create.mock.calls[0][0].expires_at).toBeNull(); }); - it("removes the record when the proxy will not sign, so no row looks like a live key", async () => { - mint.mockRejectedValue(new Error("502")); - const result = await issueApiKey(IDLE, form({ account_id: "acme--nightly-sync", label: "HPC" })); - expect(result.success).toBe(false); - expect(keys.delete).toHaveBeenCalledWith(keys.create.mock.calls[0][0].jti); - }); - it("refuses a disabled service account, even to its manager", async () => { managed.mockResolvedValue({ ...bot, disabled: true } as never); const result = await issueApiKey(IDLE, form({ account_id: "acme--nightly-sync", label: "HPC", expires_in_days: "90" })); @@ -79,27 +80,27 @@ describe("issueApiKey", () => { expect((await issueApiKey(IDLE, form({ account_id: "acme--nightly-sync", label: "" }))).success).toBe(false); expect((await issueApiKey(IDLE, form({ account_id: "acme--nightly-sync", label: "x", expires_in_days: "0" }))).success).toBe(false); expect(keys.create).not.toHaveBeenCalled(); - expect(mint).not.toHaveBeenCalled(); }); }); describe("revokeApiKey and setApiKeyExpiry", () => { - const record = { jti: "j1", account_id: "acme--nightly-sync", label: "HPC", expires_at: null } as never; + const record = { key_hash: "h1", key_id: "k1", account_id: "acme--nightly-sync", label: "HPC", expires_at: null } as never; it("revokes only a key on a service account the caller manages", async () => { - keys.fetchByJti.mockResolvedValue(record); - expect((await revokeApiKey(IDLE, form({ account_id: "acme--nightly-sync", jti: "j1" }))).success).toBe(true); - expect(keys.set).toHaveBeenCalledWith("j1", "revoked_at", expect.any(String)); + keys.listByAccount.mockResolvedValue([record]); + expect((await revokeApiKey(IDLE, form({ account_id: "acme--nightly-sync", key_id: "k1" }))).success).toBe(true); + expect(keys.set).toHaveBeenCalledWith("h1", "revoked_at", expect.any(String)); - keys.fetchByJti.mockResolvedValue({ ...(record as object), account_id: "other" } as never); - expect((await revokeApiKey(IDLE, form({ account_id: "acme--nightly-sync", jti: "j1" }))).success).toBe(false); + // A key the account does not hold is not found, whatever id is given. + keys.listByAccount.mockResolvedValue([]); + expect((await revokeApiKey(IDLE, form({ account_id: "acme--nightly-sync", key_id: "k1" }))).success).toBe(false); }); it("changes expiry after issuance, including to never", async () => { - keys.fetchByJti.mockResolvedValue(record); - await setApiKeyExpiry(IDLE, form({ account_id: "acme--nightly-sync", jti: "j1", expires_in_days: "30" })); - expect(keys.set).toHaveBeenCalledWith("j1", "expires_at", expect.any(String)); - await setApiKeyExpiry(IDLE, form({ account_id: "acme--nightly-sync", jti: "j1", expires_in_days: "" })); - expect(keys.set).toHaveBeenLastCalledWith("j1", "expires_at", null); + keys.listByAccount.mockResolvedValue([record]); + await setApiKeyExpiry(IDLE, form({ account_id: "acme--nightly-sync", key_id: "k1", expires_in_days: "30" })); + expect(keys.set).toHaveBeenCalledWith("h1", "expires_at", expect.any(String)); + await setApiKeyExpiry(IDLE, form({ account_id: "acme--nightly-sync", key_id: "k1", expires_in_days: "" })); + expect(keys.set).toHaveBeenLastCalledWith("h1", "expires_at", null); }); }); diff --git a/src/lib/actions/service-account-keys.ts b/src/lib/actions/service-account-keys.ts index 49c4a6b23..66a0920d2 100644 --- a/src/lib/actions/service-account-keys.ts +++ b/src/lib/actions/service-account-keys.ts @@ -1,17 +1,18 @@ "use server"; import { revalidatePath } from "next/cache"; -import { randomUUID } from "crypto"; +import { createHash, randomBytes, randomUUID } from "crypto"; import { LOGGER } from "@/lib/logging"; import { - ServiceAccountKeySchema, + API_KEY_PREFIX, + publicKey, + ServiceAccountKeyRecordSchema, type ApiKeyActionState, - type ServiceAccountKey, + type ServiceAccountKeyRecord, } from "@/types"; import { getPageSession } from "../api/utils"; import { serviceAccountKeysTable } from "../clients"; import { managedServiceAccount } from "@/lib/accounts/service-accounts"; -import { mintApiKey } from "@/lib/services/proxy-keys"; import { editAccountServiceAccountsUrl, editServiceAccountUrl } from "@/lib/urls"; const outcome = (message: string, success: boolean): ApiKeyActionState => ({ @@ -28,10 +29,13 @@ function expiryFrom(formData: FormData): string | null | undefined { return new Date(Date.now() + days * 86_400_000).toISOString(); } +/** Hex SHA-256 of a key: what the record holds, and what the proxy presents. */ +const hashApiKey = (key: string) => createHash("sha256").update(key).digest("hex"); + /** - * Issues an API key: records it here, has the data proxy sign it, and returns - * the key once. The key's subject is the service account's own id, which the - * API resolves directly (ADR-014); nothing else is written for it. + * Issues an API key: an opaque secret (ADR-013) whose hash is the record's + * key. Nothing signs it and nothing but this response ever carries it. The + * proxy resolves it to this service account by asking the API. */ export async function issueApiKey( _prev: ApiKeyActionState, @@ -39,7 +43,7 @@ export async function issueApiKey( ): Promise { const session = await getPageSession(); const account = await managedServiceAccount(session, String(formData.get("account_id") ?? "")); - if (!account || !session?.identity_id || !session.account) { + if (!account || !session?.account) { return outcome("You do not manage that service account", false); } // Disabled means frozen: no new key until it is enabled again. @@ -47,9 +51,12 @@ export async function issueApiKey( const expires_at = expiryFrom(formData); if (expires_at === undefined) return outcome("Expiry must be between 1 and 3650 days", false); + // 32 random bytes in base64url: fixed length, no bias, all entropy. + const key = API_KEY_PREFIX + randomBytes(32).toString("base64url"); const now = new Date().toISOString(); - const parsed = ServiceAccountKeySchema.safeParse({ - jti: randomUUID(), + const parsed = ServiceAccountKeyRecordSchema.safeParse({ + key_hash: hashApiKey(key), + key_id: randomUUID(), account_id: account.account_id, label: String(formData.get("label") ?? "").trim(), created_at: now, @@ -57,42 +64,28 @@ export async function issueApiKey( expires_at, }); if (!parsed.success) return outcome("Give the key a label of up to 64 characters", false); - const record: ServiceAccountKey = parsed.data; + const record: ServiceAccountKeyRecord = parsed.data; await serviceAccountKeysTable.create(record); - let key: string; - try { - key = await mintApiKey(session.identity_id, { - account_id: account.account_id, - jti: record.jti, - expires_at: record.expires_at, - }); - } catch (error) { - // No record without a key: the row would look like a live credential. - await serviceAccountKeysTable.delete(record.jti); - LOGGER.error("API key minting failed", { - operation: "issueApiKey", - metadata: { account_id: account.account_id, jti: record.jti }, - error: error instanceof Error ? error : new Error(String(error)), - }); - return outcome("The data proxy could not sign the key. Try again.", false); - } - LOGGER.info("Issued API key", { operation: "issueApiKey", - metadata: { account_id: account.account_id, jti: record.jti, expires_at }, + metadata: { account_id: account.account_id, key_id: record.key_id, expires_at }, }); revalidatePath(editAccountServiceAccountsUrl(account.owner_account_id)); revalidatePath(editServiceAccountUrl(account.owner_account_id, account.account_id)); - return { ...outcome("", true), issued: { key, record } }; + return { ...outcome("", true), issued: { key, record: publicKey(record) } }; } +/** The key `key_id` names, if it belongs to a service account the caller manages. */ async function ownKey(formData: FormData) { const session = await getPageSession(); const account = await managedServiceAccount(session, String(formData.get("account_id") ?? "")); if (!account) return null; - const key = await serviceAccountKeysTable.fetchByJti(String(formData.get("jti") ?? "")); - return key && key.account_id === account.account_id ? { account, key } : null; + const key_id = String(formData.get("key_id") ?? ""); + const key = (await serviceAccountKeysTable.listByAccount(account.account_id)).find( + (k) => k.key_id === key_id + ); + return key ? { account, key } : null; } /** Revocation takes effect for new exchanges within the proxy's cache TTL. */ @@ -103,7 +96,7 @@ export async function revokeApiKey( const own = await ownKey(formData); if (!own) return outcome("No such key on a service account you manage", false); if (own.key.revoked_at) return outcome("Already revoked", false); - await serviceAccountKeysTable.set(own.key.jti, "revoked_at", new Date().toISOString()); + await serviceAccountKeysTable.set(own.key.key_hash, "revoked_at", new Date().toISOString()); revalidatePath(editAccountServiceAccountsUrl(own.account.owner_account_id)); revalidatePath(editServiceAccountUrl(own.account.owner_account_id, own.account.account_id)); return outcome("Key revoked", true); @@ -118,7 +111,7 @@ export async function setApiKeyExpiry( if (!own) return outcome("No such key on a service account you manage", false); const expires_at = expiryFrom(formData); if (expires_at === undefined) return outcome("Expiry must be between 1 and 3650 days", false); - await serviceAccountKeysTable.set(own.key.jti, "expires_at", expires_at); + await serviceAccountKeysTable.set(own.key.key_hash, "expires_at", expires_at); revalidatePath(editAccountServiceAccountsUrl(own.account.owner_account_id)); revalidatePath(editServiceAccountUrl(own.account.owner_account_id, own.account.account_id)); return outcome(expires_at ? "Expiry updated" : "Key no longer expires", true); diff --git a/src/lib/api/oidc.test.ts b/src/lib/api/oidc.test.ts index 7120f51f4..d17c0d115 100644 --- a/src/lib/api/oidc.test.ts +++ b/src/lib/api/oidc.test.ts @@ -10,7 +10,12 @@ import { type FlattenedJWSInput, type JWSHeaderParameters, } from "jose"; -import { authenticateWithOidcToken, _setJwks } from "./oidc"; +import { + authenticateWithOidcToken, + verifyProxyAssertion, + PROXY_SELF_SUBJECT, + _setJwks, +} from "./oidc"; // Generate a test RSA key pair let privateKey: CryptoKey; @@ -316,4 +321,43 @@ describe("authenticateWithOidcToken", () => { const token = await createToken({ sub: "alice" }); expect(await authenticateWithOidcToken(`Bearer ${token}`, AUDIENCE)).toBeNull(); }); + + test("never makes a session for the proxy's own subject, even if an account had that id", async () => { + (accountsTable.fetchByOryId as jest.Mock).mockResolvedValue(null); + (accountsTable.fetchById as jest.Mock).mockResolvedValue({ + account_id: PROXY_SELF_SUBJECT, + type: "service", + owner_account_id: "acme", + disabled: false, + flags: [], + }); + + const token = await createToken({ sub: PROXY_SELF_SUBJECT }); + expect(await authenticateWithOidcToken(`Bearer ${token}`, AUDIENCE)).toBeNull(); + expect(accountsTable.fetchById).not.toHaveBeenCalled(); + }); +}); + +describe("verifyProxyAssertion", () => { + beforeEach(() => { + _setJwks(async () => importJWK(publicJwk, "RS256")); + }); + afterAll(() => _setJwks(null)); + + test("returns the claims of a valid assertion without resolving anyone", async () => { + const token = await createToken({ sub: PROXY_SELF_SUBJECT }); + const payload = await verifyProxyAssertion(`Bearer ${token}`, AUDIENCE); + expect(payload?.sub).toBe(PROXY_SELF_SUBJECT); + expect(payload?.iss).toBe(ISSUER); + expect(accountsTable.fetchById).not.toHaveBeenCalled(); + expect(accountsTable.fetchByOryId).not.toHaveBeenCalled(); + }); + + test("returns null for a wrong issuer, a wrong audience, an expired token, or no token", async () => { + expect(await verifyProxyAssertion(`Bearer ${await createToken({ sub: "x" }, { issuer: "https://evil" })}`, AUDIENCE)).toBeNull(); + expect(await verifyProxyAssertion(`Bearer ${await createToken({ sub: "x" }, { audience: "https://other" })}`, AUDIENCE)).toBeNull(); + expect(await verifyProxyAssertion(`Bearer ${await createToken({ sub: "x" }, { expiresIn: "-1m" })}`, AUDIENCE)).toBeNull(); + expect(await verifyProxyAssertion(null, AUDIENCE)).toBeNull(); + expect(await verifyProxyAssertion("Basic abc", AUDIENCE)).toBeNull(); + }); }); diff --git a/src/lib/api/oidc.ts b/src/lib/api/oidc.ts index 2d501fb71..85e2c93b9 100644 --- a/src/lib/api/oidc.ts +++ b/src/lib/api/oidc.ts @@ -3,6 +3,7 @@ import { createRemoteJWKSet, decodeJwt, decodeProtectedHeader, + type JWTPayload, } from "jose"; import { CONFIG } from "@/lib/config"; import { accountsTable, membershipsTable } from "@/lib/clients/database"; @@ -59,14 +60,21 @@ async function serviceAccountById(account_id: string) { } /** - * Authenticates using a signed JWT from the data proxy's OIDC provider. - * Validates the token signature, issuer, audience, and expiry, then - * resolves the subject claim to a UserSession. + * The subject the data proxy signs with when it calls as itself rather than + * on behalf of an account (ADR-013, amending ADR-005): only the API-key + * standing lookup accepts it. A URN, so no account id can ever equal it. */ -export async function authenticateWithOidcToken( +export const PROXY_SELF_SUBJECT = "urn:source:data-proxy"; + +/** + * Verifies a proxy-signed assertion — signature against the proxy's JWKS, + * issuer, audience, RS256, expiry — and returns its claims, or null. Says + * nothing about who the subject is; callers decide what a subject may do. + */ +export async function verifyProxyAssertion( authorization: string | null, audience: string, -): Promise { +): Promise { if (!authorization || !authorization.toLowerCase().startsWith("bearer ")) { // A missing or non-Bearer Authorization header is an expected, normal input // (e.g. legacy clients still sending an API key), not an anomaly — log at @@ -83,7 +91,6 @@ export async function authenticateWithOidcToken( }); const token = authorization.slice(7); - let payload; try { const result = await jwtVerify(token, getJwks(), { // We assume that the data proxy is going to be hosting the JWKS @@ -95,7 +102,7 @@ export async function authenticateWithOidcToken( algorithms: ["RS256"], clockTolerance: 30, }); - payload = result.payload; + return result.payload; } catch (error) { // `Error` objects serialize to `{}`, hiding the cause. jose attaches a // machine-readable `code` (e.g. ERR_JWT_CLAIM_VALIDATION_FAILED, @@ -145,6 +152,19 @@ export async function authenticateWithOidcToken( } return null; } +} + +/** + * Authenticates using a signed JWT from the data proxy's OIDC provider. + * Validates the token signature, issuer, audience, and expiry, then + * resolves the subject claim to a UserSession. + */ +export async function authenticateWithOidcToken( + authorization: string | null, + audience: string, +): Promise { + const payload = await verifyProxyAssertion(authorization, audience); + if (!payload) return null; const oryId = payload.sub; if (!oryId) { @@ -154,8 +174,17 @@ export async function authenticateWithOidcToken( return null; } + // The proxy calling as itself is not a session anywhere; the one route that + // takes it checks the subject directly. + if (oryId === PROXY_SELF_SUBJECT) { + LOGGER.warn("OIDC token carries the proxy's own subject; no session for it", { + operation: "authenticateWithOidcToken", + }); + return null; + } + // The token subject is whatever the data proxy authenticated: a person's Ory - // identity id, or a service account's own id — for an API key it signed + // identity id, or a service account's own id — for an API key it resolved // (ADR-013), or a workload the account trusts (ADR-014), which names the // account it wants when it exchanges its token. The two namespaces are read // together; should a subject ever name both a person and a service account, diff --git a/src/lib/clients/database/service-account-keys.ts b/src/lib/clients/database/service-account-keys.ts index a93d4cf89..5f9d3ffce 100644 --- a/src/lib/clients/database/service-account-keys.ts +++ b/src/lib/clients/database/service-account-keys.ts @@ -1,24 +1,19 @@ -import { - DeleteCommand, - GetCommand, - PutCommand, - QueryCommand, - UpdateCommand, -} from "@aws-sdk/lib-dynamodb"; -import type { ServiceAccountKey } from "@/types"; +import { GetCommand, PutCommand, QueryCommand, UpdateCommand } from "@aws-sdk/lib-dynamodb"; +import type { ServiceAccountKeyRecord } from "@/types"; import { BaseTable } from "./base"; export class ServiceAccountKeysTable extends BaseTable { model = "service-account-keys"; - async fetchByJti(jti: string): Promise { + /** The exchange path: one key get by the hash the proxy presents. */ + async fetchByHash(key_hash: string): Promise { const result = await this.cachedSend( - new GetCommand({ TableName: this.table, Key: { jti } }) + new GetCommand({ TableName: this.table, Key: { key_hash } }) ); - return (result.Item as ServiceAccountKey | undefined) ?? null; + return (result.Item as ServiceAccountKeyRecord | undefined) ?? null; } - async listByAccount(account_id: string): Promise { + async listByAccount(account_id: string): Promise { const result = await this.cachedSend( new QueryCommand({ TableName: this.table, @@ -27,15 +22,15 @@ export class ServiceAccountKeysTable extends BaseTable { ExpressionAttributeValues: { ":account_id": account_id }, }) ); - return (result.Items ?? []) as ServiceAccountKey[]; + return (result.Items ?? []) as ServiceAccountKeyRecord[]; } - async create(key: ServiceAccountKey): Promise { + async create(key: ServiceAccountKeyRecord): Promise { await this.client.send( new PutCommand({ TableName: this.table, Item: key, - ConditionExpression: "attribute_not_exists(jti)", + ConditionExpression: "attribute_not_exists(key_hash)", }) ); return key; @@ -47,25 +42,21 @@ export class ServiceAccountKeysTable extends BaseTable { * Only an existing record is written: nothing may resurrect a deleted key. */ async set( - jti: string, + key_hash: string, field: "revoked_at" | "expires_at" | "last_used_at", value: string | null ): Promise { await this.client.send( new UpdateCommand({ TableName: this.table, - Key: { jti }, + Key: { key_hash }, UpdateExpression: "SET #f = :v", ExpressionAttributeNames: { "#f": field }, ExpressionAttributeValues: { ":v": value }, - ConditionExpression: "attribute_exists(jti)", + ConditionExpression: "attribute_exists(key_hash)", }) ); } - - async delete(jti: string): Promise { - await this.client.send(new DeleteCommand({ TableName: this.table, Key: { jti } })); - } } export const serviceAccountKeysTable = new ServiceAccountKeysTable({}); diff --git a/src/lib/services/proxy-keys.ts b/src/lib/services/proxy-keys.ts deleted file mode 100644 index 4906c6f0f..000000000 --- a/src/lib/services/proxy-keys.ts +++ /dev/null @@ -1,38 +0,0 @@ -import "server-only"; - -import { CONFIG } from "@/lib/config"; -import { getOryIdToken } from "@/lib/actions/proxy-credentials"; -import { API_KEY_PREFIX } from "@/types"; - -/** - * Asks the data proxy to sign an API key for `account_id` under `jti` — the - * proxy holds the signing key (ADR-013), this app holds the record. The call - * is made as `identityId`, whoever is issuing the key; the proxy checks with - * this API that they manage the account before it signs anything. - * - * SECURITY: `identityId` is trusted as-is, exactly as in getProxyCredentials. - * Pass an identity from a verified session, never from request input. - */ -export async function mintApiKey( - identityId: string, - key: { account_id: string; jti: string; expires_at: string | null } -): Promise { - const idToken = await getOryIdToken(identityId); - const resp = await fetch(`${CONFIG.storage.endpoint}/.keys`, { - method: "POST", - headers: { - authorization: `Bearer ${idToken}`, - "content-type": "application/json", - }, - body: JSON.stringify(key), - signal: AbortSignal.timeout(15_000), - }); - if (!resp.ok) { - throw new Error(`Key minting failed: ${resp.status}`); - } - const { key: minted } = (await resp.json()) as { key?: unknown }; - if (typeof minted !== "string" || !minted.startsWith(API_KEY_PREFIX)) { - throw new Error("Key minting returned no key"); - } - return minted; -} diff --git a/src/types/service-account-key.ts b/src/types/service-account-key.ts index 2622809b2..28b983181 100644 --- a/src/types/service-account-key.ts +++ b/src/types/service-account-key.ts @@ -4,13 +4,13 @@ import { z } from "zod"; extendZodWithOpenApi(z); /** - * The record behind an API key. The key itself — a JWT the data proxy signs - * with this record's `jti` as its id (ADR-013) — is shown once at issue and - * never stored; this row is what revocation and expiry act on. + * What the app and the UI see of an API key: everything but the hash. The + * key itself is an opaque secret (ADR-013) shown once at issue and never + * stored; `key_id` is its public handle for listing, revoking and expiry. */ export const ServiceAccountKeySchema = z .object({ - jti: z.string().uuid(), + key_id: z.string().uuid(), account_id: z.string(), label: z.string().min(1).max(64), created_at: z.string().datetime(), @@ -25,11 +25,25 @@ export const ServiceAccountKeySchema = z export type ServiceAccountKey = z.infer; /** - * Every key starts with this. A JWT always begins `eyJ`, so a leaked key - * matches `sck_eyJ[\w-]+\.[\w-]+\.[\w-]+` — the pattern to register with - * secret scanners. The proxy strips the prefix before verifying. + * The stored row: the public fields plus `key_hash`, the table's partition + * key — hex SHA-256 of the key — which the data proxy presents to ask whether + * a key may be exchanged. It never leaves the server; pages strip it with + * `publicKey` before handing a record to a client component. + */ +export const ServiceAccountKeyRecordSchema = ServiceAccountKeySchema.extend({ + key_hash: z.string().regex(/^[0-9a-f]{64}$/), +}); + +export type ServiceAccountKeyRecord = z.infer; + +export const publicKey = ({ key_hash: _, ...key }: ServiceAccountKeyRecord): ServiceAccountKey => key; + +/** + * Every key is `sck_` + 32 random bytes in base64url: a fixed 47 characters, + * all entropy after the prefix. The pattern is what secret scanners register. */ export const API_KEY_PREFIX = "sck_"; +export const API_KEY_PATTERN = /^sck_[A-Za-z0-9_-]{43}$/; /** Whether a key may still be exchanged: not revoked, and not past its expiry. */ export const isKeyActive = (key: ServiceAccountKey, now = Date.now()): boolean => From 2d62011de886bafa1d339e44ad86a37a0f3d4164 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Fri, 25 Sep 2026 13:37:15 -0700 Subject: [PATCH 2/3] chore(auth): tag verifyProxyAssertion's logs with its own operation The verify step now serves the API-key exchanges route directly as well as authenticateWithOidcToken, so its log lines carry their own operation tag; a proxy self-assertion failure is then separable from an end-user token failure when filtering logs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R1eiTse4416N6uTgAy4Ddd --- src/lib/api/oidc.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib/api/oidc.ts b/src/lib/api/oidc.ts index 85e2c93b9..292a77bcd 100644 --- a/src/lib/api/oidc.ts +++ b/src/lib/api/oidc.ts @@ -80,13 +80,13 @@ export async function verifyProxyAssertion( // (e.g. legacy clients still sending an API key), not an anomaly — log at // debug so it doesn't flood warn-level logs on every such request. LOGGER.debug("No Bearer token for OIDC authentication, skipping", { - operation: "authenticateWithOidcToken", + operation: "verifyProxyAssertion", }); return null; } LOGGER.debug("Authenticating with OIDC token", { - operation: "authenticateWithOidcToken", + operation: "verifyProxyAssertion", metadata: { audience }, }); const token = authorization.slice(7); @@ -132,7 +132,7 @@ export async function verifyProxyAssertion( // Leave the failure marker. } const logPayload = { - operation: "authenticateWithOidcToken", + operation: "verifyProxyAssertion", metadata: { error_name: e?.name, error_code: e?.code, From e703e7d170dd03740bd9ce395ed95159445f6531 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Fri, 25 Sep 2026 13:56:42 -0700 Subject: [PATCH 3/3] feat(accounts): print SDK variables at issue, change key expiry, point to Disable Opening the issue dialog threw: its "Never" expiry option had an empty value, which Radix Select refuses, so the dialog crashed as soon as it rendered. The expiry select is now `ApiKeyExpiryField`, which stands "never" for the empty `expires_in_days` the key actions read as no expiry; its stories are rendered by the smoke test, which skips the dialogs that contain it. The show-once view prints the five variables a stock AWS SDK or the AWS CLI needs to use the key from a file (`AWS_ROLE_ARN`, `AWS_WEB_IDENTITY_TOKEN_FILE`, `AWS_ENDPOINT_URL_STS`, `AWS_ENDPOINT_URL_S3`, `AWS_REGION`), filled in for this environment's proxy, with the same `role/FullAccess` ARN form and region as the GitHub workflow snippet; the dialog is 640px wide so the ARN line fits. `setApiKeyExpiry` had no caller: each live key now has a "Change expiry" dialog using the same field, starting at "never" for a key that never expires. The key list says to disable the account to stop every key at once, and the danger zone says what disabling does to credentials already issued and warns that enabling lets unrevoked keys sign in again. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R1eiTse4416N6uTgAy4Ddd --- .../ApiKeyExpiryField.stories.tsx | 21 +++++ .../ApiKeyExpiryField.test.tsx | 28 ++++++ .../service-accounts/ApiKeyExpiryField.tsx | 39 +++++++++ .../IssueApiKeyDialog.stories.tsx | 5 +- .../service-accounts/IssueApiKeyDialog.tsx | 87 ++++++++++--------- .../service-accounts/ServiceAccountDetail.tsx | 67 +++++++++++--- 6 files changed, 194 insertions(+), 53 deletions(-) create mode 100644 src/components/features/service-accounts/ApiKeyExpiryField.stories.tsx create mode 100644 src/components/features/service-accounts/ApiKeyExpiryField.test.tsx create mode 100644 src/components/features/service-accounts/ApiKeyExpiryField.tsx diff --git a/src/components/features/service-accounts/ApiKeyExpiryField.stories.tsx b/src/components/features/service-accounts/ApiKeyExpiryField.stories.tsx new file mode 100644 index 000000000..a79b38f06 --- /dev/null +++ b/src/components/features/service-accounts/ApiKeyExpiryField.stories.tsx @@ -0,0 +1,21 @@ +import type { Meta, StoryObj } from "@storybook/nextjs-vite"; +import { ApiKeyExpiryField } from "./ApiKeyExpiryField"; + +/** + * How long an API key lasts, counted from now: 30 days, 90 days, a year, or + * until it is revoked. Issuing a key starts at 90 days; changing the expiry of + * a key that never expires starts at never. + */ +const meta = { + title: "Features/Service accounts/ApiKeyExpiryField", + component: ApiKeyExpiryField, + parameters: { layout: "padded" }, +} satisfies Meta; + +export default meta; +type Story = StoryObj; + +export const Default: Story = { args: { id: "expiry" } }; + +/** A key that already never expires. */ +export const Never: Story = { args: { id: "expiry", never: true } }; diff --git a/src/components/features/service-accounts/ApiKeyExpiryField.test.tsx b/src/components/features/service-accounts/ApiKeyExpiryField.test.tsx new file mode 100644 index 000000000..7bd44e7f7 --- /dev/null +++ b/src/components/features/service-accounts/ApiKeyExpiryField.test.tsx @@ -0,0 +1,28 @@ +import { render } from "@testing-library/react"; +import { Theme } from "@radix-ui/themes"; +import { ApiKeyExpiryField } from "./ApiKeyExpiryField"; + +// jsdom has no ResizeObserver, and Radix's Select measures its trigger with one. +global.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + +// The field's one job beyond looks: submit `expires_in_days` the way the key +// actions read it. "Never" can't be an empty Select value, so it must reach the +// form as an empty string, which the actions take as no expiry. +const submitted = (ui: React.ReactElement) => + (render({ui}).container.querySelector( + 'input[name="expires_in_days"]' + ) as HTMLInputElement).value; + +describe("ApiKeyExpiryField", () => { + it("submits 90 days by default", () => { + expect(submitted()).toBe("90"); + }); + + it("submits an empty value for a key that never expires", () => { + expect(submitted()).toBe(""); + }); +}); diff --git a/src/components/features/service-accounts/ApiKeyExpiryField.tsx b/src/components/features/service-accounts/ApiKeyExpiryField.tsx new file mode 100644 index 000000000..3e09a2526 --- /dev/null +++ b/src/components/features/service-accounts/ApiKeyExpiryField.tsx @@ -0,0 +1,39 @@ +"use client"; + +import React, { useState } from "react"; +import { Select } from "@radix-ui/themes"; +import { Field } from "@/components/core"; + +// Select forbids an empty item value, so "never" stands for the empty +// `expires_in_days` the key actions read as no expiry. +const NEVER = "never"; + +const EXPIRIES = [ + { value: "30", label: "30 days" }, + { value: "90", label: "90 days" }, + { value: "365", label: "A year" }, + { value: NEVER, label: "Never — until revoked" }, +]; + +/** + * How long an API key lasts, counted from now: the `expires_in_days` field + * that both issuing a key and changing its expiry submit. + */ +export function ApiKeyExpiryField({ id, never = false }: { id: string; never?: boolean }) { + const [expiry, setExpiry] = useState(never ? NEVER : "90"); + return ( + + + + + + {EXPIRIES.map((option) => ( + + {option.label} + + ))} + + + + ); +} diff --git a/src/components/features/service-accounts/IssueApiKeyDialog.stories.tsx b/src/components/features/service-accounts/IssueApiKeyDialog.stories.tsx index d0aa69832..00eefcb65 100644 --- a/src/components/features/service-accounts/IssueApiKeyDialog.stories.tsx +++ b/src/components/features/service-accounts/IssueApiKeyDialog.stories.tsx @@ -3,7 +3,8 @@ import { IssueApiKeyDialog } from "./IssueApiKeyDialog"; /** * Issuing an API key: a label and an expiry, then the key — shown once, with - * the copy affordance and the warning that says so. + * the copy affordance, the warning that says so, and the variables that point + * any AWS SDK or the AWS CLI at it. * * `issueApiKey` is mocked in `.storybook/preview.tsx` and resolves with a key * of the real shape, `sck_` and 43 characters of nothing secret, so @@ -20,5 +21,5 @@ export default meta; type Story = StoryObj; export const Default: Story = { - args: { accountId: "miskatonic--nightly-sync" }, + args: { accountId: "miskatonic--nightly-sync", proxyOrigin: "https://data.source.coop" }, }; diff --git a/src/components/features/service-accounts/IssueApiKeyDialog.tsx b/src/components/features/service-accounts/IssueApiKeyDialog.tsx index d0eaba726..9bb4d8592 100644 --- a/src/components/features/service-accounts/IssueApiKeyDialog.tsx +++ b/src/components/features/service-accounts/IssueApiKeyDialog.tsx @@ -1,36 +1,45 @@ "use client"; -import React, { useActionState, useState } from "react"; -import { - Button, - Callout, - Code, - Dialog, - Flex, - Select, - Text, - TextField, -} from "@radix-ui/themes"; +import React, { useActionState } from "react"; +import { Box, Button, Callout, Code, Dialog, Flex, Text, TextField } from "@radix-ui/themes"; import { ExclamationTriangleIcon, PlusIcon } from "@radix-ui/react-icons"; import { CopyToClipboard } from "@/components/core/CopyToClipboard"; import { Field } from "@/components/core"; import { issueApiKey } from "@/lib/actions/service-account-keys"; import { IDLE_API_KEY_ACTION_STATE } from "@/types"; +import { ApiKeyExpiryField } from "./ApiKeyExpiryField"; -const EXPIRIES = [ - { value: "30", label: "30 days" }, - { value: "90", label: "90 days" }, - { value: "365", label: "A year" }, - { value: "", label: "Never — until revoked" }, -]; +/** + * What a stock AWS SDK or the AWS CLI needs to sign in with a key saved to a + * file: it reads the file, exchanges the key at the proxy's STS endpoint and + * refreshes on its own, so nothing else runs beside it. A key names its own + * account, so the role ARN's account segment is ignored; it is filled in to + * match the workflow snippet. + */ +const sdkEnvironment = (proxyOrigin: string, accountId: string) => + [ + `export AWS_ROLE_ARN=arn:aws:iam::${accountId}:role/FullAccess`, + "export AWS_WEB_IDENTITY_TOKEN_FILE=/path/to/the/saved/key", + `export AWS_ENDPOINT_URL_STS=${proxyOrigin}/.sts`, + `export AWS_ENDPOINT_URL_S3=${proxyOrigin}`, + "export AWS_REGION=us-west-2", + ].join("\n"); /** - * Issues an API key for a service account and shows it once. There is no - * second look: the key is not stored, only its record. + * Issues an API key for a service account and shows it once, with the + * variables an SDK needs to use it. There is no second look: the key is not + * stored, only its record. */ -export function IssueApiKeyDialog({ accountId }: { accountId: string }) { +export function IssueApiKeyDialog({ + accountId, + proxyOrigin, +}: { + accountId: string; + /** The data proxy the key signs in to; without it, no variables are shown. */ + proxyOrigin?: string; +}) { const [state, formAction, pending] = useActionState(issueApiKey, IDLE_API_KEY_ACTION_STATE); - const [expiry, setExpiry] = useState("90"); + const environment = proxyOrigin && sdkEnvironment(proxyOrigin, accountId); return ( @@ -39,7 +48,7 @@ export function IssueApiKeyDialog({ accountId }: { accountId: string }) { Issue an API key - + Issue an API key {state.issued ? ( @@ -59,11 +68,23 @@ export function IssueApiKeyDialog({ accountId }: { accountId: string }) { + {environment && ( + + + Save it to a file, then point any AWS SDK or the AWS CLI at it: + + + + 
+                    
+                      {environment}
+                    
+
+ + + )} - Save it to a file, point AWS_WEB_IDENTITY_TOKEN_FILE{" "} - at that file, and set AWS_ROLE_ARN and the data - proxy's STS endpoint; a stock AWS SDK does the rest. Revoke it here - if it leaks. Only its hash is stored — the key itself is not. + Revoke it here if it leaks. Only its hash is stored — the key itself is not. @@ -74,7 +95,6 @@ export function IssueApiKeyDialog({ accountId }: { accountId: string }) { ) : ( - For environments without OIDC: a server, a scheduler, an @@ -84,18 +104,7 @@ export function IssueApiKeyDialog({ accountId }: { accountId: string }) { - - - - - {EXPIRIES.map((option) => ( - - {option.label} - - ))} - - - + {state.message && ( {state.message} diff --git a/src/components/features/service-accounts/ServiceAccountDetail.tsx b/src/components/features/service-accounts/ServiceAccountDetail.tsx index b63be42a9..1447ad699 100644 --- a/src/components/features/service-accounts/ServiceAccountDetail.tsx +++ b/src/components/features/service-accounts/ServiceAccountDetail.tsx @@ -26,7 +26,7 @@ import { setServiceAccountDisabled, } from "@/lib/actions/service-accounts"; import { githubWorkflowStep } from "@/lib/services/github-workflow"; -import { revokeApiKey } from "@/lib/actions/service-account-keys"; +import { revokeApiKey, setApiKeyExpiry } from "@/lib/actions/service-account-keys"; import { GITHUB_ACTIONS_ISSUER, IDLE_API_KEY_ACTION_STATE, @@ -41,6 +41,7 @@ import { AddGithubTrustDialog } from "./AddGithubTrustDialog"; import { ProductAccessList, type ProductAccess } from "./ProductAccessList"; import { WorkflowSnippet } from "./WorkflowSnippet"; import { IssueApiKeyDialog } from "./IssueApiKeyDialog"; +import { ApiKeyExpiryField } from "./ApiKeyExpiryField"; const issuerLabel = (issuer: string) => issuer === GITHUB_ACTIONS_ISSUER ? "GitHub Actions" : issuer; @@ -83,6 +84,44 @@ function ExampleUsage({ subject, step }: { subject: string; step: string }) { ); } +/** + * A new expiry for a live key, counted from now, in a modal: longer for a + * workload that needs it, shorter during an incident, or never. + */ +function ChangeExpiry({ accountId, apiKey }: { accountId: string; apiKey: ServiceAccountKey }) { + const [state, action, saving] = useActionState(setApiKeyExpiry, IDLE_API_KEY_ACTION_STATE); + return ( + + + + + + When should {apiKey.label} expire? + + + + + + + + + + + + + + + + + ); +} + /** * One service account, and every control over it: the workflows it trusts, * each with its example usage; its API keys; the products it reaches, changed, removed or @@ -195,8 +234,8 @@ export function ServiceAccountDetail({ } + description="For environments without OIDC. Each is shown once, when it is issued. Revoke a key that leaks; to stop every key at once, disable the account below." + rightButton={} > {keys.length === 0 ? ( @@ -222,13 +261,17 @@ export function ServiceAccountDetail({ ].join(" · ")} actions={ !key.revoked_at && ( -
- - - -
+ + + {/* A flex box, so the button centres on the row like the one beside it. */} +
+ + + +
+ ) } /> @@ -258,8 +301,8 @@ export function ServiceAccountDetail({ {account.disabled - ? "Disabled: nothing can sign in as it. Its trusts and grants are kept." - : "Disabling stops every sign-in and keeps its trusts and grants."} + ? "Disabled: nothing can sign in as it. Enabling it lets its trusted workflows and unrevoked keys sign in again, so revoke any key that leaked first." + : "Disabling stops every sign-in, by workflow or by key, and within five minutes cuts credentials it already holds back to public data. Its trusts, keys and grants are kept."}