refactor!: use ONEBOX_ for product-facing environment variables - #107
Conversation
115f543 to
b935e76
Compare
|
Review found a genuine blocker in the upgrade path. Fixed, plus two should-fixes and a nit. Force-pushed as Blocker: an upgrade could silently disable backup encryption
The wrapper then hit this: if [ -n "${ONEBOX_REPOSITORY_KEY-}" ]; then
WALG_LIBSODIUM_KEY="$ONEBOX_REPOSITORY_KEY"
export WALG_LIBSODIUM_KEY
fiUnset entry, guard skips, The guard predates this PR, but the rename is what arms it on a routine upgrade. A declared entry is a required one — : "${ONEBOX_REPOSITORY_KEY:?is not set in the credential file on this host — re-run `ob backup enable` for this service}"Three new tests execute the rendered wrapper against a stubbed wal-g rather than asserting on its text: it must refuse without the repository key, refuse without either destination credential, and pass the key through when all are present. All three fail against the old wrapper. The migration guide now has a section saying protected services need Should-fix: three migration-table rows were wrong
Should-fix: the namespace check could pass falselyCorrect, and my first attempt at fixing it did not go far enough — capturing the last command's status still hides a failure in the first stage. Verified by running it outside a git repository: it printed a clean result. Rebuilt so each stage is checked on its own — the file listing is materialised and asserted non-empty, the exemption is asserted not to have matched everything, and only grep's real no-match codes ( NitThe guide said a stray old name "gets an empty string". Hooks inherit On
|
Onebox's exported environment namespace moves from OB_ to ONEBOX_. The executable stays `ob`, the project file stays `ob.yml`, and the API version stays `onebox.run/v1`. Those three are typed constantly, so brevity earns its keep. An environment variable is not typed — it is written once into a CI settings page, a secrets store, or a hook script, and then read by someone who may not know what produced it. There, `OB_` is a puzzle: unsearchable, and indistinguishable from any other two-letter prefix in the same environment. The product calls itself Onebox everywhere else, and the sibling product already uses FANOUT_. Application-owned variables are untouched. A workload expecting FANOUT_*, DATABASE_URL, or OTEL_* receives exactly those keys — Onebox passes workload environment through rather than namespacing it. No aliases, no fallback reads, no dual writes. Carrying two namespaces would make every lookup a question about which one won, permanently, to spare a one-line search and replace once. A stray old name is simply never read: a hook using $OB_SERVER gets an empty string. The rename is anchored on a word boundary, because the unanchored pattern also matches the tail of identifiers like JOB_SECRET and POST_RELEASE_JOB_MARKER, which have nothing to do with the namespace. The encrypted backup fixture was decrypted, rekeyed, and re-encrypted rather than edited in place, so its MAC stays valid; the secret values are byte-identical. A `just env-namespace` check, wired into `just ci`, fails on any surviving OB_ reference outside the migration table that documents them. Documentation gains an environment-variables guide: what Onebox injects into local hooks, the job result protocol, backup helper keys, what it reads from a shell or CI, and the full before/after migration table. Closes #105
b935e76 to
b63a352
Compare
1 participant
Closes #105. Stacked on #106 — base is
feat/ssh-jump-host, so the rename coversOB_SSH_JUMPin the same sweep and the new namespace check is true the moment it lands. Merge #106 first; GitHub will retarget this tomain.Breaking change, no compatibility window.
ob,ob.yml, andonebox.run/v1are unchanged. Those are typed constantly, so brevity earns its keep. An environment variable is not typed — it is written once into a CI settings page, a secrets store, or a hook script, then read by someone who may not know what produced it.Application-owned variables are untouched: a workload expecting
FANOUT_*,DATABASE_URL, orOTEL_*receives exactly those keys.No aliases
No fallback reads, no dual writes. A stray old name is simply never read — a hook using
$OB_SERVERgets an empty string rather than an error. Confirmed against the built binary:Two things worth reviewing closely
The rename is word-boundary anchored. The unanchored pattern also matches the tail of
JOB_SECRET,POST_RELEASE_JOB_MARKER, andMANUAL_JOB_MARKERin the engine and onebox tests — a bulk replace would have producedJONEBOX_SECRET. Verified afterwards that no such identifier exists in the tree.The encrypted backup fixture was rekeyed properly.
e2e/testdata/postgres/secrets/backup.envis SOPS dotenv: keys are plaintext, values are encrypted, and a MAC covers the file — so editing the key name in place would have invalidated it. It was decrypted with the adjacent age key, renamed, and re-encrypted to the same recipient. The decrypted contents hash identically before and after, so no secret value was rotated or exposed.Regression check
just env-namespace, wired intojust ci:It exempts only the migration table that documents the old names, and was mutation-checked by planting a stray
OB_SOMETHING— it fails and names the file and line.Documentation
New guide
guides/environment-variables: what Onebox injects into local hooks, the job result protocol, backup helper keys, what it reads from a shell or CI, and the full before/after table. Linked from the jump-host guide; the install, backup, and migration guides carry the renamed variables.Scope
39 files. Runtime injection and consumption moved together — local hook metadata, the job result protocol, backup helper environment, CLI/build/install/release controls, e2e and contributor tooling, corpus fixtures, scripts, and site content. Frozen corpus verdicts and generated reference pages regenerated.
Purity is intact: generation still reads no process environment, so
ONEBOX_*cannot reach a generated runtime (TestGenerationIgnoresTheProcessEnvironmentcovers the renamed keys).Testing
just check— passjust lint— passjust env-namespace— passgo test -race ./...— pass (1603 tests)ONEBOX_E2E=1 go test ./e2e/with Docker — pass (287s)🤖 Generated with Claude Code
Post-review fix: an upgrade could have silently disabled backup encryption
Review found this and it is the most important thing in the PR.
ONEBOX_REPOSITORY_KEYis a fixed, Onebox-owned name (internal/app/backup_walg.go:59), and the wal-g wrapper is re-rendered and re-staged on every deploy (internal/engine/services.go:93-104). The decrypted credential file on the host, however, is written once byob backup enableand would still have definedOB_REPOSITORY_KEYafter an upgrade.ValidateWalgCredentialsruns only at enable time, so nothing on the deploy path noticed.The wrapper's guard then skipped the missing entry:
wal-g without
WALG_LIBSODIUM_KEYdoes not fail — it writes the backup unencrypted. The S3 entries are user-named, so they still resolved and archiving would have continued, unencrypted, with no error.The guard predates this PR; the rename is what would have armed it on a routine upgrade. A declared entry is a required one —
WalgCredentialEntriesis the same list enable-time validation insists on — so the wrapper now refuses rather than skipping:Three new tests execute the rendered wrapper against a stubbed wal-g instead of asserting on its text: refuse without the repository key, refuse without either destination credential, pass the key through when all are present. All three fail against the old wrapper.
The migration guide gains a section stating that protected services need
ob backup enablere-run — renaming the key inside your encrypted file does nothing on its own, because the host copy is what every deploy reads.Other post-review fixes
ONEBOX_VERSIONis the version stringjust buildstamps in (Justfile:14-17), not an install-script fetch — there is no install script.ONEBOX_RELEASE_REPOSITORYis whatscripts/release.sh:119queries to confirm the previous release run finished.ONEBOX_INSTALL_DIRbelongs tojust install.just env-namespacecould pass falsely. Capturing only the last command's status still hid a failure in the first stage — verified by running it outside a git repository, where it reported a clean tree. Each stage is now checked on its own: the file listing is materialised and asserted non-empty, the exemption is asserted not to have matched everything, and only grep's real no-match codes count as clean. Clean tree passes, a plantedOB_SOMETHINGfails with file:line, and a broken listing fails instead of reporting success.os.Environ()(internal/engine/recreate.go:136), so a shell or CI still exportingOB_SERVERpasses that stale value through. Now stated.Verification after those fixes
just check,just lint,just env-namespace— passgo test -race ./...— pass, 1608 testsONEBOX_E2E=1 go test ./e2e/with Docker — pass (291s)just server-e2eagainst a Lima VM over real SSH — pass (688s). This is the suite that exercises the re-encrypted SOPS fixture end to end:ob backup enable postgressucceeded, the wal-g runtime staged, and real backups ran through the hardened wrapper.