From 276752274b1589df12a298b89bf7c3c6fb3e7be0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 23 Jul 2026 08:12:21 -0700 Subject: [PATCH] fix(control-plane): revoke orphaned secrets when createContainer fails after injectSecrets The #8202 reorder (database -> injectSecrets -> createContainer) opened a window where injectSecrets succeeds and mints secretRef, then createContainer fails right after -- since provisionTenant always rethrows rather than returning on a step failure, secretRef never reached the caller to persist and later revoke, permanently orphaning a live credential in the broker. Unreachable before the reorder, since injectSecrets used to be the last step. provisionTenant now best-effort revokes that secretRef itself before rethrowing, and the PagerDuty alert carries it too as a fallback for when the revoke itself fails (e.g. broker unreachable). --- control-plane/src/pagerduty-notify.ts | 15 ++- control-plane/src/provisioning.ts | 41 +++++- control-plane/test/pagerduty-notify.test.ts | 12 ++ .../test/provisioning-pagerduty.test.ts | 120 ++++++++++++++++++ 4 files changed, 180 insertions(+), 8 deletions(-) diff --git a/control-plane/src/pagerduty-notify.ts b/control-plane/src/pagerduty-notify.ts index 75be534a02..d59b429c02 100644 --- a/control-plane/src/pagerduty-notify.ts +++ b/control-plane/src/pagerduty-notify.ts @@ -50,12 +50,17 @@ function warnProvisioningPagerDutyFailed(tenantName: string, error: unknown): vo /** Build the alert payload for a provisioning or deprovisioning failure (#7667). Pure -- no IO. `error` is * coerced through the same {@link pagerDutyFailMessage} helper the IO path uses for its own failure logging, so - * the paged summary and any local warn log agree on the same truncated message. */ + * the paged summary and any local warn log agree on the same truncated message. `secretRef` (#8202, optional) + * is included in `customDetails` when the caller already had one at failure time -- provisionTenant's own + * best-effort revoke (provisioning.ts) is the primary defense against a dangling broker secret, but a revoke + * can itself fail (e.g. broker unreachable), so the page still needs to hand an operator something to manually + * revoke by rather than nothing at all. */ export function buildProvisioningPagerDutyAlert(input: { tenantName: string; product: string; phase: "provision" | "deprovision"; error: unknown; + secretRef?: string; }): ProvisioningPagerDutyAlert { const message = pagerDutyFailMessage(input.error); return { @@ -65,7 +70,13 @@ export function buildProvisioningPagerDutyAlert(input: { summary: `${input.product} tenant ${input.phase} failed for ${input.tenantName}: ${message}`, severity: "critical", dedupKey: `control_plane_${input.phase}_failed:${input.product}:${input.tenantName}`, - customDetails: { tenantName: input.tenantName, product: input.product, phase: input.phase, message }, + customDetails: { + tenantName: input.tenantName, + product: input.product, + phase: input.phase, + message, + ...(input.secretRef !== undefined ? { secretRef: input.secretRef } : {}), + }, }; } diff --git a/control-plane/src/provisioning.ts b/control-plane/src/provisioning.ts index c057712556..00eeb692cd 100644 --- a/control-plane/src/provisioning.ts +++ b/control-plane/src/provisioning.ts @@ -69,8 +69,9 @@ function pageAndRethrow( phase: "provision" | "deprovision", error: unknown, options: ProvisioningPagerDutyOptions, + secretRef?: string, ): never { - const alert = buildProvisioningPagerDutyAlert({ tenantName: tenant.name, product, phase, error }); + const alert = buildProvisioningPagerDutyAlert({ tenantName: tenant.name, product, phase, error, secretRef }); const notify = options.notify ?? notifyProvisioningFailure; const env = options.env ?? process.env; const warnNotifyFailed = (notifyError: unknown): void => { @@ -93,9 +94,13 @@ function pageAndRethrow( * just the tenant identity every other step operates on. `createContainer` is in turn called with `database` * still attached AND `bootstrapSecret` newly attached (#8202) whenever `injectSecrets` returned one -- a real * container driver delivers it into the container's own cold-boot environment. A step failure pages (#7667) and - * always rethrows — provisioning never fails silently. `onFailure` (#7677, optional) runs first in that failure - * path — the caller's seam for persisting the `"failed"` lifecycle state — and is best-effort: its own - * rejection is swallowed so it can never mask the step error. */ + * always rethrows — provisioning never fails silently. If `createContainer` is what failed AFTER `injectSecrets` + * already succeeded (#8202), this function best-effort revokes that just-injected secret itself before + * rethrowing -- since it always throws rather than returning on failure, no caller ever gets a chance to persist + * `secretRef` for a later `deprovisionTenant` otherwise, which would permanently orphan a live credential in the + * broker. `onFailure` (#7677, optional) runs after that in the same failure path — the caller's seam for + * persisting the `"failed"` lifecycle state — and, like the revoke attempt, is best-effort: neither's own + * rejection can mask the step error. */ export async function provisionTenant( tenant: Tenant, product: Product, @@ -112,12 +117,34 @@ export async function provisionTenant( secretRef = injected.secretRef; await driver.createContainer({ ...request, database, ...(injected.bootstrapSecret !== undefined ? { bootstrapSecret: injected.bootstrapSecret } : {}) }); } catch (error) { + // #8202: injectSecrets can succeed (custodying a real secret + minting secretRef) and createContainer can + // still fail right after it (Cloudflare quota, a transient container-API error) -- since this function + // always rethrows rather than returning on a step failure, secretRef would otherwise never reach the caller + // to persist and later revoke, permanently orphaning a live, exchangeable credential in the broker (this + // was unreachable before #8202: injectSecrets used to be the LAST step, so nothing after it could fail once + // secretRef was set). Best-effort revoke it here, before rethrowing, so this function cleans up after + // itself rather than counting on a caller that has no way to know the secret exists. Swallowed like + // onFailure below: a revoke failure (e.g. broker unreachable) must never mask the real provisioning error -- + // that's exactly why the PagerDuty alert below still carries secretRef, as an operator's last resort. + if (secretRef !== undefined) { + await driver.revokeSecrets({ ...request, secretRef }).catch((revokeError: unknown) => { + console.warn( + JSON.stringify({ + event: "provisioning_orphaned_secret_revoke_failed", + tenant: tenant.name, + product, + secretRef, + message: pagerDutyFailMessage(revokeError), + }), + ); + }); + } // #7677 (ratified 2026-07-21): give the caller its chance to transition the tenant's registry record to // "failed" BEFORE the rethrow, so a customer polling the read path sees a terminal "Setup failed" instead // of a record stuck at "provisioning" forever. Best-effort by design: a failure writing the failed state // must never mask the provisioning error itself, which still pages and rethrows exactly as before. if (onFailure) await onFailure().catch(() => undefined); - pageAndRethrow(tenant, product, "provision", error, pagerDuty); + pageAndRethrow(tenant, product, "provision", error, pagerDuty, secretRef); } return { tenant, product, state: "active", database, ...(secretRef !== undefined ? { secretRef } : {}) }; } @@ -141,7 +168,9 @@ export async function deprovisionTenant( await driver.dropDatabase(request); await driver.destroyContainer(request); } catch (error) { - pageAndRethrow(tenant, product, "deprovision", error, pagerDuty); + // secretRef is already known here (the caller's own input, not something this function minted) -- passed + // along so an operator paged for a deprovision failure doesn't have to go look it up separately. + pageAndRethrow(tenant, product, "deprovision", error, pagerDuty, secretRef); } return { tenant, product, state: "torn down" }; } diff --git a/control-plane/test/pagerduty-notify.test.ts b/control-plane/test/pagerduty-notify.test.ts index ad07a87e15..190713d7e0 100644 --- a/control-plane/test/pagerduty-notify.test.ts +++ b/control-plane/test/pagerduty-notify.test.ts @@ -43,6 +43,18 @@ test("buildProvisioningPagerDutyAlert: provision failure builds a critical alert }); }); +test("buildProvisioningPagerDutyAlert: includes secretRef in customDetails when given (#8202)", () => { + const alert = buildProvisioningPagerDutyAlert({ + tenantName: "acme", + product: "orb", + phase: "provision", + error: new Error("container quota exceeded"), + secretRef: "orbenr_abc", + }); + + assert.equal(alert.customDetails.secretRef, "orbenr_abc"); +}); + test("buildProvisioningPagerDutyAlert: deprovision failure coerces a non-Error thrown value (#7667)", () => { const alert = buildProvisioningPagerDutyAlert({ tenantName: "acme", diff --git a/control-plane/test/provisioning-pagerduty.test.ts b/control-plane/test/provisioning-pagerduty.test.ts index 7a5b2f6eef..62069b7470 100644 --- a/control-plane/test/provisioning-pagerduty.test.ts +++ b/control-plane/test/provisioning-pagerduty.test.ts @@ -12,6 +12,7 @@ import { type ProvisioningPagerDutyAlert, type Tenant, type TenantProvisioningDriver, + type TenantProvisioningRequest, } from "../dist/index.js"; /** A driver where exactly one named step throws `error`; every other step is a no-op success. */ @@ -153,3 +154,122 @@ test("deprovisionTenant defaults to the real notifyProvisioningFailure + process await assert.rejects(deprovisionTenant(tenant, "ams", driver), /db drop failed/); }); + +// #8202: injectSecrets moved ahead of createContainer, so secretRef can now be minted and THEN orphaned if +// createContainer fails right after -- provisionTenant always rethrows rather than returning, so no caller ever +// gets secretRef to persist and revoke later otherwise. These prove the fix: a best-effort self-revoke, safe +// even when that revoke itself fails, and correctly scoped to only fire once a real secretRef actually exists. + +test("#8202: provisionTenant best-effort revokes the just-injected secret when createContainer fails right after, before rethrowing", async () => { + const revokeCalls: TenantProvisioningRequest[] = []; + const driver: TenantProvisioningDriver = { + ...createFakeTenantProvisioningDriver(), + injectSecrets: async () => ({ secretRef: "orbenr_abc", bootstrapSecret: "orbsec_xyz" }), + createContainer: async () => { + throw new Error("container quota exceeded"); + }, + revokeSecrets: async (request) => { + revokeCalls.push(request); + }, + }; + const tenant: Tenant = { name: "acme" }; + + await assert.rejects(provisionTenant(tenant, "orb", driver), /container quota exceeded/); + + assert.equal(revokeCalls.length, 1); + assert.equal(revokeCalls[0]?.secretRef, "orbenr_abc"); +}); + +test("#8202: a failure in the best-effort revoke itself does not mask the real createContainer error", async () => { + const driver: TenantProvisioningDriver = { + ...createFakeTenantProvisioningDriver(), + injectSecrets: async () => ({ secretRef: "orbenr_abc" }), + createContainer: async () => { + throw new Error("container quota exceeded"); + }, + revokeSecrets: async () => { + throw new Error("broker unreachable"); + }, + }; + const tenant: Tenant = { name: "acme" }; + + await assert.rejects(provisionTenant(tenant, "orb", driver), /container quota exceeded/); +}); + +test("#8202: provisionTenant does NOT attempt a revoke when no secretRef was ever obtained (e.g. provisionDatabase itself failed)", async () => { + const revokeCalls: TenantProvisioningRequest[] = []; + const driver: TenantProvisioningDriver = { + ...driverThatThrowsOn("provisionDatabase", new Error("db provisioning failed")), + revokeSecrets: async (request) => { + revokeCalls.push(request); + }, + }; + const tenant: Tenant = { name: "acme" }; + + await assert.rejects(provisionTenant(tenant, "orb", driver), /db provisioning failed/); + + assert.equal(revokeCalls.length, 0); +}); + +test("#8202: provisionTenant does NOT attempt a revoke when injectSecrets itself is the step that failed", async () => { + const revokeCalls: TenantProvisioningRequest[] = []; + const driver: TenantProvisioningDriver = { + ...driverThatThrowsOn("injectSecrets", new Error("secret injection failed")), + revokeSecrets: async (request) => { + revokeCalls.push(request); + }, + }; + const tenant: Tenant = { name: "acme" }; + + await assert.rejects(provisionTenant(tenant, "orb", driver), /secret injection failed/); + + assert.equal(revokeCalls.length, 0); +}); + +test("#8202: the PagerDuty alert carries secretRef when injectSecrets had already succeeded before the failing step", async () => { + const calls: ProvisioningPagerDutyAlert[] = []; + const notify: NotifyProvisioningFailure = async (alert) => { + calls.push(alert); + }; + const driver: TenantProvisioningDriver = { + ...createFakeTenantProvisioningDriver(), + injectSecrets: async () => ({ secretRef: "orbenr_abc" }), + createContainer: async () => { + throw new Error("container quota exceeded"); + }, + }; + const tenant: Tenant = { name: "acme" }; + + await assert.rejects(provisionTenant(tenant, "orb", driver, { notify }), /container quota exceeded/); + await Promise.resolve(); + + assert.equal(calls[0]?.customDetails.secretRef, "orbenr_abc"); +}); + +test("#8202: the PagerDuty alert omits secretRef entirely when none was ever obtained", async () => { + const calls: ProvisioningPagerDutyAlert[] = []; + const notify: NotifyProvisioningFailure = async (alert) => { + calls.push(alert); + }; + const driver = driverThatThrowsOn("provisionDatabase", new Error("db provisioning failed")); + const tenant: Tenant = { name: "acme" }; + + await assert.rejects(provisionTenant(tenant, "orb", driver, { notify }), /db provisioning failed/); + await Promise.resolve(); + + assert.equal("secretRef" in (calls[0]?.customDetails ?? {}), false); +}); + +test("#8202: deprovisionTenant's PagerDuty alert carries the secretRef it was given, for operator convenience", async () => { + const calls: ProvisioningPagerDutyAlert[] = []; + const notify: NotifyProvisioningFailure = async (alert) => { + calls.push(alert); + }; + const driver = driverThatThrowsOn("dropDatabase", new Error("db drop failed")); + const tenant: Tenant = { name: "acme" }; + + await assert.rejects(deprovisionTenant(tenant, "ams", driver, { notify }, "orbenr_abc"), /db drop failed/); + await Promise.resolve(); + + assert.equal(calls[0]?.customDetails.secretRef, "orbenr_abc"); +});