mosaicstack/stack
1
0
Fork 0
Code Issues 58 Pull Requests Actions Packages Projects Releases 15 Wiki Activity

docs(#771): remediate database role split review

Browse Source
This commit is contained in:
Hermes Agent
2026-07-15 00:43:44 -05:00
parent da742ca2da
commit 45ba3d6ad4
8 changed files with 311 additions and 136 deletions
Download Patch File Download Diff File Expand all files Collapse all files

109
docs/reports/native-kanban-sot/kbn-101-contract-security-review-82ce325.md Normal file
View File

@@ -0,0 +1,109 @@
# KBN-101 contract independent security/architecture review
**Verdict: REQUEST CHANGES**
## Review identity and scope
- **Exact reviewed head:** `da742ca2da4a2ff466916c818fe275c4f7ffd384` (`docs(#771): record role-split review evidence`)
- **Required comparison:** `origin/main...da742ca2da4a2ff466916c818fe275c4f7ffd384`
- **Range:** `82ce3252df38a687c50485f8d048b53ca8db5989` is an ancestor of the reviewed head; the final head adds the scratchpad evidence commit and was reviewed.
- **Changed docs:** `docs/PRD.md`, `docs/SITEMAP.md`, `docs/native-kanban-sot/{INDEX.md,KBN-101-DB-ROLE-SPLIT.md,SHARED-CONTRACT.md,TASKS.md}`, and `docs/scratchpads/771-kbn101-db-role-split.md` (300 additions / 25 deletions).
- **Reviewed inputs:** issue #771; current DB/Gateway/storage/config/wizard/installer/compose/Portainer/CI sources; all current migration/DDL references; KBN-010, rc.4/rc.5 shared contract, requirements/canon, KBN-100 #769 branch context, and the final scratchpad.
- **Repository/provider state:** not modified. The pre-existing `.mosaic/orchestrator/*` dirt was not touched.
The role graph itself is sound in principle: a NOLOGIN platform database owner, separate NOLOGIN schema owner, NOINHERIT migrator which explicitly `SET ROLE`s, and runtime membership only in a capability role with `SET FALSE` does not create circular privilege or application-created login roles. The split of foundation certification before KBN-100 and immutable-operation certification after KBN-100 is also correctly ordered.
## Findings
### HIGH — DDL/migration control plane is not closed at every current entrypoint
The contract requires an explicit, locked migration phase and forbids Gateway/runtime DDL (`KBN-101-DB-ROLE-SPLIT.md:34-39`), but its KBN-101-02 result merely says migration-capable commands use the migration DTO (`:109`). It does not prohibit or route every existing bypass through that one command.
Current bypasses include:
- `runMigrations()` falls back from an argument to `DATABASE_URL` and a hard-coded URL (`packages/db/src/migrate.ts:24-35`), while `drizzle.config.ts` likewise uses `DATABASE_URL` plus a default (`packages/db/drizzle.config.ts:3-9`).
- Package scripts expose direct `drizzle-kit migrate` **and** `drizzle-kit push` (`packages/db/package.json:23-26`); `db:push` bypasses the planned journal/fingerprint/lock entirely.
- `mosaic storage migrate --run` shells out to the direct `db:migrate` script (`packages/storage/src/cli.ts:413-452`).
- The federated integration test can create types, tables, and indexes directly against `DATABASE_URL` and intentionally operates without a Drizzle ledger (`packages/db/src/federation.integration.test.ts:28-30,46-134`).
**Failure mode:** a runtime or CI environment with only `DATABASE_URL`, or an operator invoking an existing command, can apply unverified DDL outside the lock, `SET ROLE` preflight, exact-ledger gate, and deployment sequencing. This breaks the requested fail-closed split even if Gateway startup is repaired.
**Required remediation:** amend KBN-101-02/03/06 to enumerate these entrypoints and make the dedicated migrator runner the only PostgreSQL DDL path. Production-like `db:push` must be removed/blocked; `db:migrate`, `storage migrate --run`, and migration tests must invoke the same migration runner with `DATABASE_MIGRATION_URL`, lock, identity preflight, and ledger verification. Tests needing schema must consume a pre-migrated disposable database, or be explicitly run only by that migration phase. Add negative tests showing each command refuses `DATABASE_URL`-only execution and cannot reach DDL.
### HIGH — TLS requirement has no deployable server/bootstrap contract
The contract correctly requires a mounted CA and hostname-verified TLS (`KBN-101-DB-ROLE-SPLIT.md:25,28,93-95`). However KBN-101-05 promises only a “migration phase and secret binding boundary” (`:112`), not PostgreSQL server TLS, certificate issuance/SANs, CA distribution, startup ordering, or the fresh/existing-database bootstrap trust path.
Current standalone and federated compose expose plain PostgreSQL with no server TLS configuration or CA mount (`docker-compose.yml:2-14`; `docker-compose.federated.yml:27-44`). The Portainer test stack passes a single plaintext in-network URL and uses the same database login for Gateway and database bootstrap (`deploy/portainer/federated-test.stack.yml:51-60,110-117`).
**Failure mode:** enforcing the mandatory CA makes current local standalone/federated topologies unable to start; relaxing it to make bootstrap work silently violates K101-REQ-03. A first database cannot be safely migrated until the server certificate, its SAN for the actual service/DNS name, and trusted CA are provisioned, but this lifecycle is not owned or tested.
**Required remediation:** add a concrete KBN-101-00/05 TLS bootstrap sub-contract: issuer/CA owner; server key/cert and SAN inputs; secure storage/mount permissions; `postgresql.conf`/container TLS enablement; migration and runtime CA mounts; hostname used by each compose/Swarm service; readiness only after TLS authentication; CA overlap rotation; and an existing-database transition. Require a disposable standalone and federated/Swarm test to prove verified TLS succeeds and missing CA, wrong CA, wrong SAN, and `sslmode` downgrade fail before readiness. Do not merge KBN-101-05 with an implicit plaintext exception.
### HIGH — exact ledger fingerprint and historical 0009 repair are underspecified for existing databases
The contract requires an “ordered complete set” and rejection of out-of-order rows (`KBN-101-DB-ROLE-SPLIT.md:36-38`), but does not define the canonical serialized tuple, ledger ordering source, or safe upgrade rule for a historical ledger. The current ledger stores only `id`, `hash`, and `created_at` (`packages/db/src/migrate.ts:70-82,105-107`). Its journal is demonstrably non-monotonic: `0008` has `when=1776822435828`, followed by `0009` at `1745280000000` (`packages/db/drizzle/meta/_journal.json:62-79`); the existing PostgreSQL runner documents that this causes skipping (`packages/db/src/migrate.ts:29-35`).
**Failure mode:** an implementation can either reject a legitimate historical database after correcting 0009, or accept a reordered/duplicated ledger because no precise comparison rule exists. A count/hash-set implementation would fail to detect the condition that this contract explicitly calls unsafe; physical `id` order is not an adequate substitute after historical repair.
**Required remediation:** freeze a versioned manifest algorithm before implementation: canonical record fields (at least journal index/tag, corrected logical order, migration content hash, and an explicit migration-manifest version), canonical byte serialization, SHA-256 input, and exact observed-ledger mapping. State whether physical ledger insertion order is normative; if not, compare hash-to-manifest tuples rather than timestamps. Add an idempotent migrator-only 0009 existing-database remediation/reconciliation procedure with backup/rollback evidence. Require clean, pre-0009, 0009-skipped, 0009-applied-late, duplicate, unknown, missing, corrupt-pair, and stale-replica cases. No manual ledger insertion is an acceptable production recovery path.
### MEDIUM — advisory-lock namespace is collision-prone and lacks a fixed identifier contract
The specified lock is `pg_try_advisory_lock(hashtext('mosaic-schema-migration-v1'))` (`KBN-101-DB-ROLE-SPLIT.md:34`). `hashtext` produces a 32-bit key. Session ownership/crash behavior is otherwise correctly stated (one session, same-session release, connection-close release), but an unrelated database user can accidentally collide or deliberately hold the key and force `DATABASE_MIGRATION_LOCKED`.
**Failure mode:** avoidable migration denial of service in a shared PostgreSQL database. The current repository already uses separate `hashtext` advisory-lock names for migrate-tier, demonstrating the need for a documented namespace rather than a collision-prone implicit one.
**Required remediation:** freeze a two-int advisory-lock namespace (fixed documented class/object values) or a documented 64-bit `hashtextextended` key with fixed seed; keep acquisition, migration, verification, and release on the single `max:1` migrator session. Add tests for concurrent migration, connection loss/crash release, readiness while the lock holder is active, and an unrelated lock-key non-interference case.
### MEDIUM — identifier safety and `search_path` verification need executable constraints
The contract rightly requires `pg_catalog, <mosaic_application_schema>` and rejects writable paths (`KBN-101-DB-ROLE-SPLIT.md:54,70-77`), but uses dynamic placeholders for database/schema and does not state how migration/bootstrap SQL will avoid identifier interpolation. Existing code has raw-SQL facilities (`packages/storage/src/migrate-tier.ts` uses `.unsafe`), so this is not merely theoretical.
**Failure mode:** a future operator-configured database/schema value that reaches bootstrap or `SET search_path` through raw string construction can inject DDL, or a pooled connection can retain a mutable search path.
**Required remediation:** require fixed allowlisted identifiers or server-side identifier quoting (`format('%I', ...)`) only; never interpolate URL/config values into SQL. Set and verify the trusted path per connection/session before any query (`SET LOCAL` inside transactions where applicable), forbid `public`/`$user` additions, and add injection-shaped identifier and pooled-connection reset negatives. Include this in KBN-101-00/01 tests.
## Acceptance and threat traceability
| Requirement / threat | Review result | Evidence or blocking finding |
| --- | --- | --- |
| K101-REQ-01 / AC-K101-01 split runtime/migration URLs | Partial | Role/DTO boundary is coherent; HIGH DDL-path finding requires all current commands to be closed. |
| K101-REQ-02 / AC-K101-02 explicit migration/readiness | Blocked | HIGH ledger definition and HIGH DDL-bypass findings. |
| K101-REQ-03 / AC-K101-03 least privilege, TLS, grants | Partial | Role model, default privileges, ledger read-only, TEMP/function checks are well specified (`KBN-101...:47-56,70-79`); HIGH TLS bootstrap and MEDIUM identifier constraints remain. |
| K101-REQ-04 / AC-K101-04 immutable relations | Correctly deferred | KBN-101-09 after KBN-100 is the correct serial gate (`KBN-101...:58-66,115-118`); no synthetic-only certification claim found. |
| K101-REQ-05 / AC-K101-05 N-1, secrets, rollback | Partial | No owner-runtime exception and rollback keeps migration URL out of Gateway (`:83-95`); deployable TLS and full command inventory are missing. |
| K101-REQ-06 / AC-K101-07 KBN gates and DAG | Structurally sound | DAG is acyclic: 00→01/{03}; 02→06; 00/01/03→05; 00/04/05/06→07→08→KBN-100→09→KBN-105. KBN-100’s current branch contains docs-only baseline tracking, not schema implementation. |
| T: runtime DDL / migration fallback | Blocked | HIGH finding 1. Current Gateway/storage, CLI, direct Drizzle scripts, and integration DDL require explicit closure. |
| T: race/crash/readiness | Partial | Same-session nonblocking lock and replica-unready rules are present (`:34-38`); lock namespace remediation required. |
| T: immutable evidence rewrite | Correctly staged | Explicit INSERT/SELECT-only matrix and RESTRICT retention are retained; proof is properly after table creation. |
| T: secret leakage / TLS downgrade | Partial | Redaction and distinct Vault paths are specified (`:93-97`), but no server TLS/bootstrap implementation contract exists. |
## Unresolved assumptions
1. `standalone` and `federated` are the complete PostgreSQL production-like set (K101-A1).
2. Each eligible deployment can execute a dedicated migration Job/one-shot phase (K101-A2).
3. Vault path names are targets, not verified existing paths; deployment ownership remains to be established.
4. PostgreSQL 17 is available for the selected membership and advisory-lock implementation.
5. The required server-side TLS issuer/certificate lifecycle and Swarm/compose secret transport have not been decided; this is blocking, not a permissible implicit plaintext bootstrap.
6. Historical databases containing the 0009 journal/ledger anomaly have no frozen reconciliation procedure.
## Independent test and consistency evidence
Read-only checks run in this review:
| Check | Result |
| --- | --- |
| `git diff --check origin/main...da742ca2...` | PASS |
| `pnpm exec prettier --check` on all seven changed docs | PASS |
| `pnpm exec tsc --noEmit -p docs/native-kanban-sot/tsconfig.json` | PASS |
| `docker compose -f docker-compose.yml config --quiet` (isolated test ports) | PASS |
| `docker compose -f docker-compose.federated.yml --profile federated config --quiet` (isolated test ports) | PASS |
| Static journal inspection | FAILS the required monotonic ordering premise: 0008 → 0009 `when` decreases; current runner documents skipping behavior. |
| Static DDL-entrypoint inventory | Found direct Drizzle scripts, storage CLI shell-out, runtime extension/migration calls, fleet backlog migration, tier probe extension creation, and a direct-DLL federated integration test. |
No live database, Vault, CI, deployment, issue, PR, or repository mutation was performed. The pass results validate documentation syntax/contract compilation and compose syntax only; they do **not** certify the proposed security behavior.
## Conclusion
Do not merge this frozen contract as implementation-ready until the HIGH findings are corrected and independently re-reviewed. The central role ownership/default-privilege design, immutable-table staging, and KBN-100/KBN-105 serial gating should be retained; they are not the reason for this REQUEST CHANGES verdict.