feat(seed): resolve seed connection credentials from HashiCorp Vault - #944
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
yusuf-gundogdu
left a comment
There was a problem hiding this comment.
Good work, and I want one change before this goes in.
I checked the lazy split by mutation rather than by reading, because that is the part the issue said would be the whole job. It holds. Listing hands the reference back untouched and reads nothing, opening one connection costs exactly one read, a second open inside the TTL costs none, and getSeedConnectionByIdUnfiltered stays unresolved so a caller being refused never causes a read. I also went looking for a path that opens a connection without going through resolveConnection, since that path would hand the literal ${vault:...} string to a driver as a password. There is none: query, multi-query, transaction, health, monitoring, maintenance, profile, pool-stats, test-connection, provider-meta, cancel, object-route, agent runs and fleet-health all go through it. fleet-health resolves inside a loop, but it genuinely opens each connection there and the cache covers the repeat, so that one is correct as written.
The one change. A Vault failure reaches the user as an internal server error. VaultError has no branch in src/lib/api/errors.ts, so it falls through to the generic handler at the bottom and is logged as "Unhandled error". Measured through createErrorResponse, next to the two classes it should sit beside:
VaultError 500 INTERNAL_ERROR
DatabaseConfigError 400 CONFIG_ERROR
SeedConnectionError 404 (its own status)
Every way a reference fails is an operator configuration fault: VAULT_ADDR unset, a v1 shaped path, a key that is not there, a policy that does not grant the path. None of those are the product failing, and the repo already classifies exactly this kind of fault as CONFIG_ERROR. As it stands, an operator who mistypes a path gets a 500 and an "Unhandled error" line in the log, which reads as a crash.
It matters more here than it would elsewhere, because of an asymmetry the lazy split creates. A broken ${ENV_VAR} never reaches a user at all: resolution happens on the list path, resolveAllCredentials drops that connection and the rest keep working. I measured it on your own fixture, VAULT_FIXTURE_ENV_PASSWORD set gives env-postgres, vault-mysql and vault-postgres, and unset gives vault-mysql and vault-postgres, with env-postgres gone. A broken ${vault:...} cannot behave that way and should not, so connect time is the only place a user ever sees one, and it is the only error class this feature has.
A branch for VaultError returning 400 with CONFIG_ERROR, plus a case pinning it, is all I am asking for. If you think an unreachable Vault deserves a 502 while a bad path stays 400, split it and say so, I have no objection to that shape. What I do not want is the generic branch.
Three things you do not need to touch. The eager and lazy split is right and the existing env-var tests pass unmodified, which was the point. The tests measure rather than assert shape: request counts, an injected clock for the TTL boundary, captured log output for the secret rather than a reading of the code, and the Kubernetes lease renewed before it lapses. And the secret never appears in a VaultError message, so nothing leaks through the generic branch today either, it is only classified wrong.
One housekeeping item while you are pushing. The branch is four commits behind main. None of them touch your eleven files, so there is no conflict, but please rebase in the same push so CI runs against current main before I merge.
Done when a failed reference returns a 4xx with a code rather than the generic 500, a case pins it, and the branch is on current main.
|
Backing Yusuf's request, and adding one measurement that points the same way. I ran this against a real Vault and PostgreSQL before reading his review. Same seed file, same database, two builds side by side: the released one fails with password authentication failed for user "demo", this branch returns the connection's stats. The feature works, and the resolved value and the token appear nowhere in the container log. What I want to add to his point is that a failing vault reference currently does not say it is a vault reference. He found the in-version case, a 500 logged as "Unhandled error". There is a second one: on a Studio that predates this resolver the reference is not recognised at all, so the literal string reaches the driver as a password and the operator sees password authentication failed. I measured both today. That one is ours to handle in a release note, not yours, but it is why I think his classification matters more here than it looks. On the box you left unchecked: gitleaks over your commit range is clean against our config, and the merged coverage gate is 58003/58003 lines at 100.00%. So Yusuf's item is the only one open. |
…ibredb#937) Add a `${vault:<mount>/data/<path>#<key>}` scheme to the seed credential resolver, alongside the existing `${ENV_VAR}` one. A reference is read from Vault's KV v2 API when a connection is opened and cached per path for `VAULT_CACHE_TTL_MS`, so a rotated secret is picked up without a restart. `${ENV_VAR}` resolution stays eager and synchronous. Only a `${vault:...}` reference is passed through the connections list unresolved, and resolved once, for one connection, after the access check in `resolveConnection`, so listing connections reads no secret. Vault credentials come from the environment (`VAULT_ADDR`, `VAULT_TOKEN` or `VAULT_ROLE` with the projected service account token, `VAULT_NAMESPACE`); the client is `fetch` only, so no dependency is added. Every `VAULT_*` variable is optional and an unset `VAULT_ADDR` fails a reference with a message naming it. Also adds `docker-compose.vault-demo.yml` - Studio, PostgreSQL, dev-mode Vault and a one-shot init container that writes the secret and the seed file - with the rotation walkthrough in `docs/SEED_CONNECTIONS.md`.
A `${vault:...}` reference that cannot be resolved is an operator
configuration fault - an unset VAULT_ADDR, a v1-shaped path, a missing key, a
policy that does not grant the path, or a Vault this deployment cannot reach -
so it should not fall through to the generic 500 INTERNAL_ERROR. The message
already names the variable or the path an operator has to fix.
`createErrorResponse` now maps `VaultError` to 400 CONFIG_ERROR, the class
`DatabaseConfigError` already uses. A malformed reference raises `VaultError`
too, rather than a bare `Error`, so every failed reference takes that branch.
Pinned through `createErrorResponse` in tests/unit/api-errors.test.ts, which
returns the generic 500 before the mapping.
f1667cc to
ee19235
Compare
|
Done. I kept one class instead of the 502/400 split. Every one of these is the operator's Vault config — the address, the path, the policy, or bringing Vault back — and nothing on the client acts on a One case the review didn't name was in the same hole: a reference with no
Rebased onto |
|
Everything Yusuf asked for is in, and the branch is on current |
|
great, this is a big PR, congrats, merging shortly |
Description
Adds a second reference scheme to the seed credential resolver:
password: "${vault:secret/data/prod/postgres#password}"reads the value from HashiCorp Vault's KV v2 API instead of an environment variable. The gain is rotation without a restart — today a Vault value arrives through External Secrets Operator and an env var, so the process reads it once.The trap in this issue is the list path.
getManagedConnectionsresolves every connection's credentials on everyGET /api/connections/managed, which is a free dictionary lookup for${ENV_VAR}and a Vault read per secret for a reference. So${ENV_VAR}stays exactly where it was — eager and synchronous — and only${vault:...}is deferred: passed through the list path untouched and resolved once, for one connection, inresolveConnectionafter the role check.getSeedConnectionByIdUnfilteredstays unresolved too, so a caller on the way to a 403 reads nothing.Vault credentials come from the environment and never from the seed file. The client is
fetchonly, so no dependency is added; KV v2 (data.data.<key>) is the only shape supported. EveryVAULT_*variable is optional: with none set and no reference in the seed file, nothing is constructed at module load, no request is made and nothing about Vault is logged.Every way a reference fails is an operator configuration fault — an unset
VAULT_ADDR, a v1-shaped path, a missing key, a policy that does not grant the path, a Vault this deployment cannot reach — so a failed reference is answered as 400CONFIG_ERROR, with the message naming the variable or the path, rather than falling through to the generic 500.Type of Change
Related Issue
Closes #937
Changes Made
src/lib/seed/vault-client.ts(new): KV v2 reads overfetch, withVAULT_ADDR,VAULT_TOKENor Kubernetes auth (VAULT_ROLEplus the projected service account token atVAULT_K8S_TOKEN_PATH, whose lease is tracked and re-logged-in before it lapses),VAULT_NAMESPACEand a per-path cache underVAULT_CACHE_TTL_MS. A 403 is never reported as a missing secret, and the resolved value is never logged or put in an error message.src/lib/seed/credential-resolver.ts: recognises${vault:<mount>/data/<path>#<key>}, leaves it unresolved on the eager path without the plaintext-password warning, addsresolveVaultCredentialsfor the lazy read, and raises aVaultError— not a bareError— for a malformed reference, so it is classified with the rest.src/lib/seed/resolve-connection.ts: resolves the references after the access decision.src/lib/api/errors.ts: maps aVaultErrorto 400CONFIG_ERROR, the classDatabaseConfigErroralready uses, instead of the generic 500INTERNAL_ERRORwith its "Unhandled error" log.docs/SEED_CONNECTIONS.md: the scheme, the variables, the rotation window, the demo walkthrough, and the Vault row of the deployment table..env.example: the sixVAULT_*variables.docker-compose.vault-demo.yml(new): Studio, PostgreSQL, a dev-mode Vault, and a one-shot init container that writes the secret into Vault and the seed file into the volume Studio mounts. It pullsghcr.io/libredb/libredb-studio:latest, and its own header says the Vault is dev mode and not a production configuration.tests/unit/seed/vault-client.test.ts,tests/unit/seed/credential-resolver-vault.test.ts,tests/integration/seed/vault-seed-pipeline.test.ts, a case intests/api/seed/managed-route.test.ts, andtests/fixtures/seed-connections/vault-config.yaml.Testing
Failing first. With
credential-resolver.tsandresolve-connection.tsreverted tomain,tests/unit/seed/credential-resolver-vault.test.tscannot importresolveVaultCredentialsandtests/integration/seed/vault-seed-pipeline.test.tshas three failures — and the base also treats each reference as a plaintext password:The review follow-up was pinned the same way:
tests/unit/api-errors.test.tsgained twocreateErrorResponsecases forVaultError, and both came back 500INTERNAL_ERRORbefore the mapping was added (25 tests: 23 pass, 2 fail). After it they are 400CONFIG_ERROR.Rebased onto
mainatfa554877(19 commits behind) with no conflicts, and the whole gate below was re-run on the new base.Local gate:
bun run format— cleanbun run lint— 0 errors, 225 warnings, the same warning set asmain; the one new warning this change introduced was removedbun run typecheck— cleanbun run knip— cleanbun run chart:check,bun run channels:showcase:check,bun run readme:check,bun run security:check— all OKbun run test— 563 of 564 files pass. The exception istests/unit/launcher-utils.test.ts(5 tests), which fails identically on unmodifiedmainin this environment: two areEXDEVon a cross-device rename, and three are startup-URL assertions. Nothing here touches the launcher.bun run test:coverage && bun run coverage:check—58020/58020 lines (100.00%). The launcher file is the one excluded from that run because it cannot pass in this sandbox; it exercisesbin/, notsrc/.helm dependency build charts/libredb-studio, which is what the runner needs to include them.bun run build:lib,bun run attw,bun run build— pass.Live Vault dev container (
hashicorp/vault:latest, dev mode, real KV v2 read through the app code):docker compose -f docker-compose.vault-demo.yml up, then opening the seeded connection (the local image is this branch built from the Dockerfile, since the published:latestpredates the feature):PostgreSQL in that file is
POSTGRES_HOST_AUTH_METHOD=scram-sha-256with--auth-host=scram-sha-256, so the password is actually what authenticates; without it the official image trusts every connection and the demo proves nothing.Test Environment
mainatfa554877after the review rebase)Checklist
bun run test:coverageandbun run coverage:checklocally: 100.00%)src/lib/db/providers/, I updated the matchingdocs/providers/documentation andtests/integration/db/tests in the same PR — not applicable, no provider changedAdditional Notes
VAULT_ADDRis read when a reference is resolved, never at module load, and there is no boot-time probe, health check or warning.${vault:...}reference with no#keyis not rejected on the list path — that path does no parsing beyond a prefix match — and fails with an explicit error when the connection is opened. It is aVaultError, so it takes the same 400CONFIG_ERRORbranch as every other failed reference, and is never used as a literal password.tests/api/seed/managed-route.test.tsinstead of a new file on purpose: that suite already carries the@/lib/authstub, and a new file would have made it a 38th hand-copied copy of that mock, which D85 indocs/BACKLOG.mdtracks. The existing cases in that file and intests/integration/seed/seed-pipeline.test.tsare unchanged.src/lib/storage/connection-secrets.ts,Dockerfile,charts/and the other compose files are untouched.