Skip to content

feat(seed): resolve seed connection credentials from HashiCorp Vault - #944

Merged
cevheri merged 2 commits into
libredb:mainfrom
tunglambk:feat/vault-seed-credential-resolution
Sep 18, 2026
Merged

cevheri merged 2 commits into
libredb:mainfrom
tunglambk:feat/vault-seed-credential-resolution

Conversation

@tunglambk

@tunglambk tunglambk commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

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. getManagedConnections resolves every connection's credentials on every GET /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, in resolveConnection after the role check. getSeedConnectionByIdUnfiltered stays 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 fetch only, so no dependency is added; KV v2 (data.data.<key>) is the only shape supported. Every VAULT_* 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 400 CONFIG_ERROR, with the message naming the variable or the path, rather than falling through to the generic 500.

Type of Change

  • New feature (non-breaking change which adds functionality)

Related Issue

Closes #937

Changes Made

  • src/lib/seed/vault-client.ts (new): KV v2 reads over fetch, with VAULT_ADDR, VAULT_TOKEN or Kubernetes auth (VAULT_ROLE plus the projected service account token at VAULT_K8S_TOKEN_PATH, whose lease is tracked and re-logged-in before it lapses), VAULT_NAMESPACE and a per-path cache under VAULT_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, adds resolveVaultCredentials for the lazy read, and raises a VaultError — not a bare Error — 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 a VaultError to 400 CONFIG_ERROR, the class DatabaseConfigError already uses, instead of the generic 500 INTERNAL_ERROR with 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 six VAULT_* 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 pulls ghcr.io/libredb/libredb-studio:latest, and its own header says the Vault is dev mode and not a production configuration.
  • Tests: 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 in tests/api/seed/managed-route.test.ts, and tests/fixtures/seed-connections/vault-config.yaml.

Testing

Failing first. With credential-resolver.ts and resolve-connection.ts reverted to main, tests/unit/seed/credential-resolver-vault.test.ts cannot import resolveVaultCredentials and tests/integration/seed/vault-seed-pipeline.test.ts has three failures — and the base also treats each reference as a plaintext password:

[WARN ] {route=seed/credential-resolver, connId=vault-postgres} Seed connection has plaintext password, use ${ENV_VAR} syntax
[WARN ] {route=seed/credential-resolver, connId=vault-mysql} Seed connection has plaintext password, use ${ENV_VAR} syntax
2 files: 0 passed, 2 failed  |  6 tests: 3 pass, 3 fail

The review follow-up was pinned the same way: tests/unit/api-errors.test.ts gained two createErrorResponse cases for VaultError, and both came back 500 INTERNAL_ERROR before the mapping was added (25 tests: 23 pass, 2 fail). After it they are 400 CONFIG_ERROR.

Rebased onto main at fa554877 (19 commits behind) with no conflicts, and the whole gate below was re-run on the new base.

Local gate:

  • bun run format — clean
  • bun run lint — 0 errors, 225 warnings, the same warning set as main; the one new warning this change introduced was removed
  • bun run typecheck — clean
  • bun run knip — clean
  • bun run chart:check, bun run channels:showcase:check, bun run readme:check, bun run security:check — all OK
  • bun run test — 563 of 564 files pass. The exception is tests/unit/launcher-utils.test.ts (5 tests), which fails identically on unmodified main in this environment: two are EXDEV on a cross-device rename, and three are startup-URL assertions. Nothing here touches the launcher.
  • bun run test:coverage && bun run coverage:check58020/58020 lines (100.00%). The launcher file is the one excluded from that run because it cannot pass in this sandbox; it exercises bin/, not src/.
  • The 12 Helm chart tests (215 tests) — pass after 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):

list (no Vault read): [ { id: 'vault-postgres', password: '${vault:secret/data/prod/postgres#password}' } ]
open #1: pg-secret
open #2 within TTL: pg-secret
rotating secret in Vault... rotate status: 200
open before TTL expiry: pg-secret
open after TTL expiry: rotated-secret
now pointing VAULT_ADDR at a closed port...
open fails with: Vault is unreachable for "http://127.0.0.1:8299/v1/secret/data/prod/postgres": Unable to connect. Is the computer able to access the url?

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 :latest predates the feature):

open with the password that lives only in Vault:
{"success":true,"message":"Connection successful","latency":9}

rotate (the documented two commands):
vault kv put secret/prod/postgres password=rotated
ALTER ROLE

open before VAULT_CACHE_TTL_MS expires:
{"error":"Failed to connect to PostgreSQL: password authentication failed for user \"demo\"","code":"CONNECTION_ERROR","statusCode":503,"retryable":true}

wait 12s, open again:
{"success":true,"message":"Connection successful","latency":7}

container restart count: 0

PostgreSQL in that file is POSTGRES_HOST_AUTH_METHOD=scram-sha-256 with --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

  • LibreDB Studio Version: 0.16.0 (branch off main at fa554877 after the review rebase)
  • OS: Linux x64
  • Node.js/Bun Version: Bun 1.4.2, Node 22 (the repo's floor is 24; used 22 locally, CI uses its pinned runtime)
  • Database Type: PostgreSQL 18 in the demo

Checklist

  • My code follows the project's code style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes (see the launcher exception above, which is pre-existing)
  • The required CI test job passes the 100% line-coverage gate (bun run test:coverage and bun run coverage:check locally: 100.00%)
  • If I changed src/lib/db/providers/, I updated the matching docs/providers/ documentation and tests/integration/db/ tests in the same PR — not applicable, no provider changed
  • Any dependent changes have been merged and published

Additional Notes

  • VAULT_ADDR is read when a reference is resolved, never at module load, and there is no boot-time probe, health check or warning.
  • A ${vault:...} reference with no #key is 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 a VaultError, so it takes the same 400 CONFIG_ERROR branch as every other failed reference, and is never used as a literal password.
  • The route case went into tests/api/seed/managed-route.test.ts instead of a new file on purpose: that suite already carries the @/lib/auth stub, and a new file would have made it a 38th hand-copied copy of that mock, which D85 in docs/BACKLOG.md tracks. The existing cases in that file and in tests/integration/seed/seed-pipeline.test.ts are unchanged.
  • Out of scope, per the issue: Vault as a connection type, references on user-created connections, Azure Key Vault and AWS Secrets Manager, and any boot-time Vault check. src/lib/storage/connection-secrets.ts, Dockerfile, charts/ and the other compose files are untouched.

@cevheri cevheri added enhancement New feature or request security Supply-chain, auth, or hardening work dependencies Dependency version updates core-capabilities labels Sep 17, 2026
@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@yusuf-gundogdu yusuf-gundogdu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cevheri

cevheri commented Sep 17, 2026

Copy link
Copy Markdown
Member

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.

@cevheri cevheri added the loop:needs-info Maintainer-loop task blocked on human-reviewed clarification label Sep 17, 2026
Tung Lam added 2 commits September 18, 2026 06:09
…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.
@tunglambk
tunglambk force-pushed the feat/vault-seed-credential-resolution branch from f1667cc to ee19235 Compare September 18, 2026 06:13
@tunglambk

Copy link
Copy Markdown
Contributor Author

Done. VaultError maps to 400 CONFIG_ERROR now, and a test pins it through createErrorResponse.

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 retryable flag for these responses, so the split would be a distinction the response can't spend. DatabaseConfigError already means this class, and the message names the variable or the path (Vault address is not configured: set VAULT_ADDR ..., Vault has no secret at "secret/data/prod/postgres"), which is the part you and cevheri measured was missing.

One case the review didn't name was in the same hole: a reference with no #key raised a bare Error, so it would have missed the new branch too. It raises VaultError now, same message.

tests/unit/api-errors.test.ts has two new createErrorResponse cases (unset VAULT_ADDR, unreachable Vault). Both came back 500 INTERNAL_ERROR before the change and 400 CONFIG_ERROR after. Everything you listed to leave alone is untouched — the eager/lazy split, the request-count and injected-clock tests, the captured-log secret check and the Kubernetes lease.

Rebased onto main (fa554877) in the same push; it had grown to 19 commits behind by then, no conflicts. Re-ran the gate on the new base: format, lint (0 errors), typecheck, knip, all four drift guards, bun run test (563/564 files, only the sandbox-broken launcher-utils.test.ts), coverage 58020/58020 lines (100.00%), the 12 Helm chart files, and build:lib/attw/build. Head is ee19235.

@tunglambk

Copy link
Copy Markdown
Contributor Author

Everything Yusuf asked for is in, and the branch is on current main (ee19235c). The repo's own CI, CodeQL, Security Scan and Platform Integration workflows are still action_required on that head — this is the account's first contribution here, so GitHub hasn't started them. A maintainer approving those runs is the only thing left before the required checks can confirm the change.

@cevheri

cevheri commented Sep 18, 2026

Copy link
Copy Markdown
Member

great, this is a big PR, congrats, merging shortly
it is not urgent, but a section like "run one-commang vault demo stack etc.." coult be added to the readme.md file; feel free to implement that and submit it if you'd like

@cevheri
cevheri merged commit 8bf3dc0 into libredb:main Sep 18, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core-capabilities dependencies Dependency version updates enhancement New feature or request loop:needs-info Maintainer-loop task blocked on human-reviewed clarification security Supply-chain, auth, or hardening work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resolve seed connection passwords from HashiCorp Vault at connect time

3 participants