Skip to content

feat(control-plane): deliver a bootstrap secret into hosted tenant containers - #8253

Merged
JSONbored merged 1 commit into
mainfrom
feat/hosted-tenant-secret-bootstrap
Jul 23, 2026
Merged

feat(control-plane): deliver a bootstrap secret into hosted tenant containers#8253
JSONbored merged 1 commit into
mainfrom
feat/hosted-tenant-secret-bootstrap

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Resolves #8202 for its ORB scope (AMS split into #8246, see Boundaries below).

control-plane/src/secret-driver.ts could custody a tenant's secret in the broker but had no way to actually deliver it into a running container -- its own header called this "separate, not-yet-built infrastructure." This PR builds that delivery mechanism.

Why not push the real secret value? I read the real @cloudflare/containers SDK (not just this repo's own hand-rolled test interface) before picking a mechanism. createTenantContainer calls stub.start() exactly once, guarded by isProvisioned() -- there's no second call for injectSecrets to hook into as the issue's push option suggested. And a repeat start() against an already-running/starting container is a no-op or throws at the SDK level (envVars are only ever applied at an actual cold boot). So a literal "push the DB connection string through a second stub.start() call" isn't just worse than pull, it doesn't reliably work at all.

What this ships instead (pull, bootstrapped by a small reorder):

  • provisionTenant now runs provisionDatabase -> injectSecrets -> createContainer (was createContainer first) so a bootstrap credential exists before the container's one real cold-boot start() call.
  • injectTenantSecrets (secret-driver.ts) stops discarding the one-time exchange secret the broker already hands back on custody -- it's returned as bootstrapSecret and threaded by provisionTenant into the very next createContainer call.
  • createTenantContainer (container-driver.ts) delivers it as LOOPOVER_TENANT_SECRET_TOKEN, merged alongside the existing LOOPOVER_PINNED_VERSION into one envVars object. Product-agnostic naming (no ORB_/AMS_ prefix), matching PINNED_VERSION_ENV_VAR's own precedent -- both OrbTenantContainer and AmsTenantContainer will read the identical var name.
  • New fetchBrokeredStoredSecret (src/orb/broker-client.ts) lets a container exchange that bootstrap credential against the broker's already-wired POST /v1/orb/token stored-secret path (brokerOrbToken's ORB_SECRET_TYPE_TENANT_DB_CREDENTIAL branch, resolveStoredSecret -- already shipped server-side, just never had a client). OrbTenantContainer runs the unmodified self-host image, so ORB gets this for free once wired; AMS has no broker-aware code today, hence the split.

deprovisionTenant's own order is deliberately left unchanged -- "revoke the secret before the DB/container it belonged to is gone" is an independent security property, not a mechanical requirement to mirror provision's new order.

Tests

  • test/integration/orb-broker.test.ts: new test proving a revoked tenant_db_credential enrollment's secret can no longer be exchanged -- the exact "revocation actually removes access, not just broker custody" deliverable Deliver secrets/env vars into a running hosted tenant container (OrbTenantContainer/AmsTenantContainer) #8202 called for. (The existing revocation test used the GitHub-token enrollment type; this is the missing stored-secret combination.)
  • test/unit/orb-broker-client.test.ts: full coverage of fetchBrokeredStoredSecret (success, custom broker URL, shared URL-safety validation, non-OK response, missing secretValue, both ?? defensive branches).
  • control-plane/test/container-driver.test.ts: bootstrap-secret-only, both-pinned-and-bootstrap-merged, neither-set (byte-identical to pre-Build a fleet rollout mechanism #4898), and idempotent-repeat-create cases.
  • control-plane/test/provisioning.test.ts: new step-order assertion, plus a test proving bootstrapSecret actually threads from injectSecrets' result into the createContainer request (and an omission test for when the driver returns none).
  • control-plane/test/secret-driver.test.ts, driver-factory.test.ts: updated to expect the now-returned bootstrapSecret.
  • control-plane/test/provisioning-pagerduty.test.ts: fixed a real bug the reorder surfaced -- its driverThatThrowsOn helper's no-op stand-in for injectSecrets returned undefined (a Promise<void> slipping past injectSecrets' real return type via TypeScript's void-return special-casing). Under the old create-container-first order this was never exercised for createContainer-throws tests; the new order runs injectSecrets first, and its no-op return value gets destructured. Fixed to return a real {}.

Verified: full unsharded npm run test:coverage (20975/20996 passing, 0 failures), tsc --noEmit --incremental false clean, control-plane's own build + node:test suite (193/193) + real c8/lcov coverage (100% line+branch on every file this PR touches), git diff --check clean.

Boundaries

Scoped to the delivery mechanism + ORB, matching #8202's own explicit "needs a design decision, not a default extension" framing -- picking a mechanism was the hard part, and AMS's container-side wiring is genuinely new code (packages/loopover-miner has zero broker-aware code today), not a second call site for the same thing. Split into #8246 rather than bundled here.

Does not wire a runtime consumer for the resolved secret value into src/server.ts's boot sequence -- #8202's own deliverables are the delivery mechanism and proof it works (including revocation), not runtime consumption. That's #7858's hosted half, already listed as blocked-by #8202.

Test plan

  • npm run typecheck (forced non-incremental)
  • npm run test:coverage (full unsharded suite)
  • control-plane: npm run build && npm run test:node (193/193)
  • control-plane: real coverage via npm run control-plane:coverage -- 100% line+branch on every touched file
  • git diff --check

…ntainers (#8202)

Resolves push-vs-pull: a second stub.start() call from injectSecrets can't
reliably deliver a live secret (Cloudflare Containers only apply envVars at
an actual cold boot, and createContainer already owns the tenant's one real
start() call). Instead, provisionTenant now runs database -> secrets ->
container, so injectSecrets' one-time exchange secret (previously discarded)
rides the container's own cold-boot envVars as a bootstrap credential; the
container exchanges it for the real custodied value via the new
fetchBrokeredStoredSecret client against the broker's already-wired
stored-secret path.

Scoped to the mechanism + ORB (which reuses its unmodified self-host
broker-client code for free); AMS's container-side wiring is a real
separate lift and follow-up issue #8246.
@superagent-security

Copy link
Copy Markdown
Contributor

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

1 similar comment
@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 (a0e791d) to head (d8cbbd4).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8253      +/-   ##
==========================================
- Coverage   92.12%   91.60%   -0.53%     
==========================================
  Files         782      782              
  Lines       78531    78585      +54     
  Branches    23720    23728       +8     
==========================================
- Hits        72347    71986     -361     
- Misses       5062     5524     +462     
+ Partials     1122     1075      -47     
Flag Coverage Δ
control-plane 99.85% <100.00%> (+<0.01%) ⬆️
shard-1 59.36% <0.00%> (-0.59%) ⬇️
shard-2 46.64% <0.00%> (-0.57%) ⬇️
shard-3 52.85% <100.00%> (-0.67%) ⬇️

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

Files with missing lines Coverage Δ
control-plane/src/container-driver.ts 100.00% <100.00%> (ø)
control-plane/src/index.ts 100.00% <100.00%> (ø)
control-plane/src/provisioning.ts 100.00% <100.00%> (ø)
control-plane/src/secret-driver.ts 100.00% <100.00%> (ø)
control-plane/src/tenant-provisioning-driver.ts 100.00% <100.00%> (ø)
src/orb/broker-client.ts 99.21% <100.00%> (+0.06%) ⬆️

... and 3 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 23, 2026
@loopover-orb

loopover-orb Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-23 15:06:10 UTC

13 files · 1 AI reviewer · 1 blocker · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This PR wires a real delivery path for a tenant's bootstrap secret into its container's cold-boot env, reordering provisionTenant to database→injectSecrets→createContainer so the secret exists before the one real stub.start() call, and adds fetchBrokeredStoredSecret for the container's own exchange. The mechanism itself is sound and well-reasoned (correctly avoids a second stub.start() call, which the PR author verified against the real SDK doesn't reapply envVars), and test coverage is thorough for the new branches. However, the reorder introduces a genuine orphaned-secret regression: if createContainer fails after injectSecrets has already succeeded, the enrollment's secretRef is silently dropped instead of surfaced for cleanup/revocation.

Blockers

  • control-plane/src/provisioning.ts:107-118 (provisionTenant): `secretRef` is assigned inside the try block right after `injectSecrets` succeeds, but if the subsequent `createContainer` call throws (a real path — Cloudflare quota errors, transient container-API failures), control jumps straight to the catch/pageAndRethrow and the function never reaches its `return { ...secretRef }` line, so the broker enrollment that was just created is never handed back to the caller to persist and later revoke — it's now a permanently un-revocable dangling secret in the broker, a regression introduced specifically by moving injectSecrets ahead of createContainer; `pageAndRethrow`'s alert (provisioning.ts:69-71) also doesn't include `secretRef`, so even the PagerDuty page gives an operator nothing to manually revoke by.
Nits — 5 non-blocking
  • control-plane/src/secret-driver.ts:77-79 and broker-client.ts:100 call it a 'one-time' bootstrap credential, but the exchange itself isn't single-use (only revocation invalidates it, per the integration test at test/integration/orb-broker.test.ts:369-376) — worth softening the terminology so a future reader doesn't assume the broker enforces single-use.
  • No test exercises the createContainer-fails-after-injectSecrets-succeeds path this reorder newly creates (control-plane/test/provisioning.test.ts only covers the happy threading case and the pagerduty tests only fail one step at a time with every other step a no-op) — worth adding one that asserts what happens to the already-injected secretRef.
  • Thread `secretRef` (not just the raw error) into `pageAndRethrow`/the PagerDuty alert when a failure happens after `injectSecrets` has already run, so an operator has something to revoke manually.
  • Consider having `provisionTenant`'s catch block attempt a best-effort `revokeSecrets` when `secretRef` is set and a later step failed, mirroring the existing `onFailure` best-effort pattern, rather than leaving custody stranded in the broker.
  • Possible secret-shaped assignment in the diff (generic_secret_assignment) — Verify the value is not a real credential.

Concerns raised — review before merging

  • control-plane/src/provisioning.ts:107-118 (provisionTenant): `secretRef` is assigned inside the try block right after `injectSecrets` succeeds, but if the subsequent `createContainer` call throws (a real path — Cloudflare quota errors, transient container-API failures), control jumps straight to the catch/pageAndRethrow and the function never reaches its `return { ...secretRef }` line, so the broker enrollment that was just created is never handed back to the caller to persist and later revoke — it's now a permanently un-revocable dangling secret in the broker, a regression introduced specifically by moving injectSecrets ahead of createContainer; `pageAndRethrow`'s alert (provisioning.ts:69-71) also doesn't include `secretRef`, so even the PagerDuty page gives an operator nothing to manually revoke by.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. control-plane/src/provisioning.ts:107-118 \(provisionTenant\): \`secretRef\` is assigned inside the try block right after \`injectSecrets\` succeeds, but if the subsequent \`createContainer\` call throws \(a real path — Cloudflare quota errors, transient container-API failures\), control jumps straight to the catch/pageAndRethrow and the function never reaches its \`return \{ ...secretRef \}\` line, so the broker enrollment that was just created is never handed back to the caller to persist and later revoke — it's now a permanently un-revocable dangling secret in the broker, a regression introduced specifically by moving injectSecrets ahead of createContainer; \`pageAndRethrow\`'s alert \(provisioning.ts:69-71\) also doesn't include \`secretRef\`, so even the PagerDuty page gives an operator nothing to manually revoke by.

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8202
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 14 registered-repo PR(s), 14 merged, 255 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 14 PR(s), 255 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Partially addressed
The PR builds a real pull-based delivery mechanism (bootstrap secret via LOOPOVER_TENANT_SECRET_TOKEN, reordered provisionTenant, fetchBrokeredStoredSecret, and revocation tests), directly satisfying the issue's push/pull design ask and the revocation deliverable for the ORB path, with the container-driver code itself already product-agnostic (bindingFor branches on product, not the env var naming

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 14 PR(s), 255 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 23, 2026
@JSONbored
JSONbored merged commit 151bad8 into main Jul 23, 2026
14 checks passed
@JSONbored
JSONbored deleted the feat/hosted-tenant-secret-bootstrap branch July 23, 2026 15:08
JSONbored added a commit that referenced this pull request Jul 23, 2026
… bootstrap (#8246) (#8263)

Adds tenant-credential-resolution.ts, a duplicated (not imported -- cross-
package import from packages/loopover-miner into root src/ fails tsc with
TS6059, confirmed) exchange of LOOPOVER_TENANT_SECRET_TOKEN against the
broker, mirroring src/orb/broker-client.ts's fetchBrokeredStoredSecret for
ORB. hosted-entry.ts resolves it once per wake, best-effort -- no code in
this package consumes the resolved value yet (the miner's stores are
unconditionally local SQLite), so this proves the mechanism is wired for AMS
without making a scheduled cycle fragile against an unused value, mirroring
#8202/#8253's own "prove it works, defer consumption" precedent for ORB.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deliver secrets/env vars into a running hosted tenant container (OrbTenantContainer/AmsTenantContainer)

1 participant