Skip to content

fix(control-plane): revoke orphaned secrets when createContainer fails after injectSecrets - #8255

Merged
JSONbored merged 1 commit into
mainfrom
fix/provisioning-orphaned-secret-revoke
Jul 23, 2026
Merged

fix(control-plane): revoke orphaned secrets when createContainer fails after injectSecrets#8255
JSONbored merged 1 commit into
mainfrom
fix/provisioning-orphaned-secret-revoke

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Fixes a real regression in #8253 (already merged), caught by code review after merge.

#8253 reordered provisionTenant to database -> injectSecrets -> createContainer so a tenant's bootstrap secret exists before the container's one real cold-boot stub.start() call. That reorder opened a window that didn't exist before: injectSecrets can succeed (custodying a real secret + minting secretRef) and then createContainer can still fail right after it -- a real path (Cloudflare quota, a transient container-API error). Since provisionTenant always rethrows rather than returning on a step failure, secretRef never reached the caller to persist and later revoke via deprovisionTenant -- permanently orphaning a live, exchangeable credential in the broker with no code path left to clean it up.

This was unreachable before #8253: injectSecrets used to be the last of the three steps, so nothing after it could ever fail once secretRef was set.

Fix

  • provisionTenant's catch block now best-effort revokes the just-injected secret (when one was actually obtained) before rethrowing -- self-contained cleanup, doesn't depend on any caller doing the right thing with a value it was never going to receive.
  • The revoke attempt's own failure (e.g. broker unreachable) is swallowed and warn-logged, same posture as the existing onFailure hook -- it must never mask the real provisioning error.
  • secretRef is also threaded into the PagerDuty alert's customDetails (both provisionTenant and deprovisionTenant) as defense-in-depth for when the best-effort revoke itself fails -- an operator paged for the failure has something to manually revoke by instead of nothing.

Tests

  • Best-effort revoke actually fires with the right secretRef, and the original createContainer error is still what's rethrown.
  • A failure in the revoke itself doesn't mask the real error.
  • No revoke is attempted when no secretRef was ever obtained (provisionDatabase or injectSecrets itself failing) -- confirms the fix is correctly scoped, not a blanket "always try to revoke."
  • PagerDuty alert carries secretRef when present, omits it when absent (existing exact-equality test already covered the omission case).
  • deprovisionTenant's alert also carries the secretRef it was given.

Verified: control-plane build + full node:test suite (201/201) + real c8/lcov coverage (100% line+branch on both touched files), git diff --check clean.

Test plan

  • control-plane: npm run build && npm run test:node (201/201)
  • control-plane: real coverage via npm run control-plane:coverage -- 100% line+branch on provisioning.ts and pagerduty-notify.ts
  • git diff --check

…s 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).
@JSONbored JSONbored self-assigned this Jul 23, 2026
@JSONbored
JSONbored merged commit 3494297 into main Jul 23, 2026
6 checks passed
@JSONbored
JSONbored deleted the fix/provisioning-orphaned-secret-revoke branch July 23, 2026 15:14
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.60%. Comparing base (e4708ba) to head (2767522).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8255      +/-   ##
==========================================
- Coverage   92.13%   91.60%   -0.53%     
==========================================
  Files         783      783              
  Lines       78605    78645      +40     
  Branches    23733    23734       +1     
==========================================
- Hits        72421    72046     -375     
- Misses       5062     5524     +462     
+ Partials     1122     1075      -47     
Flag Coverage Δ
control-plane 99.85% <100.00%> (+<0.01%) ⬆️
shard-1 55.85% <ø> (-0.68%) ⬇️
shard-2 53.40% <ø> (-0.58%) ⬇️
shard-3 49.37% <ø> (-0.57%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
control-plane/src/pagerduty-notify.ts 100.00% <100.00%> (ø)
control-plane/src/provisioning.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant