feat(config,db): load YAML configuration and connect to Postgres - #6
Merged
Merged
Conversation
The two things every remaining piece of work was blocked on. Neither the
auth engine nor any /v1 route could be built while --config was parsed and
ignored and nothing ever opened a database connection.
Nothing here makes authentication work. The router is still /healthz plus
the Stripe-shaped 404: no /v1 route, no /dash/v1 route, no client store,
no ClientAssertionStore, no kill-switch check, no signing-key generation
or rotation. authkestra-op remains a dev-dependency of the integration
test crate alone. The schema and the config loader are real and tested;
the auth is not built, and docs/status.md says so plainly.
Configuration (vpay-config, library only — not yet wired into either
binary, so --config is still ignored at runtime):
* Config::load layers application.yml over application-{profile}.yml,
resolves ${ENV} placeholders, then validates.
* ${ENV} interpolation is hand-rolled: figment's Env provider reads
variables into the tree but does not interpolate inside YAML scalars.
Resolution runs before typed deserialization, so an unresolved
placeholder is fatal for any field, as docs/flows/configuration.md
requires — not just for credentials.
* The env lookup is injected rather than read from the process, because
std::env::set_var is unsafe in edition 2024 and the workspace forbids
unsafe with no test carve-out. That makes the failure path testable.
* Merchant and dashboard OAuth clients are deliberately NOT modelled —
ADR-0010 was being written concurrently and a guessed client shape
would be fabrication. Two boot-guard rules stay unimplemented as a
result and are recorded as gaps.
* 5 -> 36 tests.
Persistence (new vpay-db crate):
* connect, run_migrations (sqlx::migrate!), check_connection, DbError.
* Both binaries now require a database at boot, run migrations before
binding, and /healthz performs a real SELECT 1 returning 503 when the
database is unreachable.
* Liveness and readiness deliberately not split: no k8s manifests exist
to justify the distinction yet.
Schema cutover to the settled auth decisions (0009-0012):
* merchant_api_keys dropped — merchants authenticate with
client_credentials + private_key_jwt (ADR-0010), so there is no key to
store.
* oauth_signing_keys reshaped: private_key_pem removed entirely,
public_jwk JSONB added, id renamed kid. No secret material is stored
anywhere in the schema now. The private PEM comes from a Kubernetes
Secret at boot; TokenManager::new_asymmetric parses it once at
construction and retains only derived keys, so the database never
needs it again.
* oauth_client_assertion_jtis for private_key_jwt replay protection —
the primary key is the atomic guard. No cleanup job exists yet, so it
grows unbounded; that is recorded, not handled.
* disabled_clients, the kill switch: YAML stays authoritative for
identity, but an operator can revoke instantly without a deploy. It
stores no credential. The cost is that config is no longer the sole
authority, so a runbook must read both.
* oauth_refresh_tokens and oauth_device_codes are kept deliberately.
OpStore's supertrait bound forces concrete types in those slots; "no
refresh tokens, no device flow" is enforced by never mounting those
handlers, not by the schema.
Two defects fixed after review:
* Secret redaction. ProviderHost and CommonArgs now have hand-written
Debug impls; a derived one would have printed rail credentials and the
database password on any tracing::debug!(?args). Config, ServerArgs and
WorkerArgs keep derive — it delegates to each field's own impl — and a
test proves the composition holds.
* TLS roots. sqlx is pinned to tls-rustls-ring (vendored webpki-roots),
not native-roots. A licence failure on webpki-roots' CDLA-Permissive-2.0
was first worked around by switching to native roots, which reads the OS
trust store — and the runtime image is FROM scratch (ADR-0004), so there
is none. That would have failed only in the shipped image, while passing
locally and in CI where connections are plain. The licence is allowed in
deny.toml instead, with justification: it covers Mozilla's CA data, not
code, and is permissive. rustls-native-certs now reaches the graph only
through bollard -> testcontainers -> vpay-testkit [dev-dependencies].
Verified: just ci exits 0. 105 passed / 3 skipped, up from 80/3; the 3
skipped are the unchanged adapter-conformance ignores. cargo deny clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The two things every remaining piece of work was blocked on: configuration loading and a database connection. Neither the OpenID Provider nor any
/v1route could be built while--configwas parsed and ignored and nothing ever opened a pool.Nothing here makes authentication work, and
docs/status.mdis written to make that unmissable. The router is still/healthzplus the Stripe-shaped 404: no/v1route, no/dash/v1route, no client store, noClientAssertionStore, no kill-switch check, no signing-key generation or rotation.authkestra-opremains a dev-dependency of the integration test crate alone. The schema and config loader are real and tested; the auth is not built.Source of truth: ADR-0003 (administration is YAML in git; a profile selects a config file, never a code path), ADR-0004 (static musl into
scratch— load-bearing for the TLS fix below), and the new ADR-0010, recording the maintainer's decision to move merchant auth off API keys.Scope
Configuration —
Config::loadlayersapplication.ymloverapplication-{profile}.yml, resolves${ENV}, then validates via the existing tested guard rules plusgarde.vpay-configwent 5 → 36 tests. It is a library only; neither binary calls it yet, so--configis still ignored at runtime.Persistence — new
vpay-db:connect,run_migrations,check_connection. Both binaries require a database at boot, run migrations before binding, and/healthzperforms a realSELECT 1.Schema cutover (
0009–0012) —merchant_api_keysdropped;oauth_signing_keysreshaped topublic_jwk JSONBwithprivate_key_pemremoved entirely;oauth_client_assertion_jtisanddisabled_clientsadded. No secret material is stored anywhere in the schema now.ADR-0010 — merchants authenticate with
client_credentials+private_key_jwtfrom YAML, public JWK only. Device flow dropped, no refresh tokens, dashboard read-only with one scope.README.md,docs/api/README.mdandexamples/**corrected: Stripe-shaped now means object model, idempotency and webhook signatures, explicitly not auth — no Stripe SDK can authenticate against vpay.Verification
cargo nextest run --workspacejust cicargo deny checkThe 3 skipped are unchanged adapter-conformance ignores. Every new constraint has a test that inserts a violating row and asserts
db_err.constraint()names that specific constraint.Risk Assessment
Two defects were found and fixed during review — both are the interesting part of this PR.
1. TLS roots would have failed only in production. A licence failure on
webpki-roots'CDLA-Permissive-2.0was first worked around by switching sqlx totls-rustls-ring-native-roots. That reads the OS trust store — and the runtime image isFROM scratchper ADR-0004, so there is none. TLS to Postgres would have failed in the shipped image while passing locally and in CI, where connections are plain. Reverted to vendored roots; the licence is allow-listed indeny.tomlwith justification (it covers Mozilla's CA data, not code, and is permissive).rustls-native-certsnow reaches the graph only viabollard → testcontainers → vpay-testkit [dev-dependencies].2. Credentials would have leaked into logs.
ProviderHost.credentialsandCommonArgs.database_urlwere reachable through derivedDebug— onetracing::debug!(?args)from live rail secrets and a Postgres password. Hand-written redacting impls now.Config/ServerArgs/WorkerArgskeep derive, which is safe because derive delegates to each field's own impl; a test proves the composition rather than asserting it.Known gaps, recorded rather than hidden:
--configis still inert.docs/flows/configuration.mdare unimplemented because merchant clients are deliberately unmodelled — ADR-0010 was in flight and a guessed shape would have been fabrication.oauth_client_assertion_jtishas no cleanup job (the worker loop is still ⛔), so it grows unbounded.--database-urland/healthzreturning 503 against a dead database both lack tests. Onlycheck_connection()'s failure path is unit-tested.settingsandcredentialsare bothBTreeMap<String, String>and onlycredentialsis redacted — that boundary is convention, not type. ASecret<String>newtype would make it structural.AI Usage Declaration
AI (Claude Opus 5 via Claude Code) performed this work, delegating five features to parallel Sonnet sub-agents on disjoint file sets. The orchestrating session re-ran every gate and verified load-bearing claims independently — which caught the TLS regression, the credential-leak hazard, and three stale doc claims (including a self-contradictory module comment introduced by the TLS revert). One agent correctly downgraded a row from ✅ to 🟡 against my own brief, on the grounds that two claims I stated had no test; I accepted its judgement.
NotImplementedtokens are unchanged.Reviewer Focus
deny.toml's newCDLA-Permissive-2.0allowance — a licence-policy decision, and the alternative silently breaks thescratchimage.0010_reshape-oauth-signing-keys.sql— is public-JWK-in-DB plus PEM-from-Secret the shape you want before any key is ever generated?--configinert), but it is the one place this PR adds capability nothing calls.