mptyl/ThothII
1
0
Fork 0
Code Issues 25 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

docs: record DWH auth implementation evidence

Browse Source
This commit is contained in:
User
2026-08-21 13:34:36 +02:00
parent 0c4ff3750d
commit 7118950416
6 changed files with 807 additions and 322 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.superpowers/sdd/task-2-report.md
+308 -160
View File
@@ -1,177 +1,325 @@
# Task 2 report — root Compose startup
# Task 2 report — protected atomic registry
Status: DONE
## Scope and commit
Implemented the root Compose defaults and the single bundle declaration:
- added `.env.example` with automatic Compose defaults (`COMPOSE_FILE=compose.yaml`, an empty
profile, and the relative `THT_SECRETS_FILE` path);
- removed the mandatory `external` profile from `core` and `frontend`;
- mounted `deploy/secrets/thothii.secrets` at `/run/secrets/thothii.secrets` and passed only the
mounted path into the core container;
- changed the production overlay to inherit that bundle instead of declaring per-secret mounts;
- removed the local overlay's legacy `env_file` dependency;
- added the versioned bundle template and `.gitignore` exception;
- updated deployment security checks and added `scripts/test-default-compose.sh`.
Focused verification:
```text
./scripts/test-default-compose.sh # default Compose contract passed.
./scripts/test-container-deployment.sh # container deployment security contract passed.
./scripts/test-preprocess-compose-config.sh # preprocess compose config: ok
docker compose --env-file .env.example config --quiet
(with a temporary mode-0600 bundle via THT_SECRETS_FILE)
git diff --check
```
The local-vector and preprocess service secret declarations remain for Task 3, which converts
those services to the same bundle helper. Documentation and smoke command migration is reserved
for Task 4.
---
# Task 2 report — PostgreSQL session repository
## Scope delivered
- Added `PostgresSessionRepository`, implementing the Task 1 repository contract with a
direct PostgreSQL SQLAlchemy connection, transaction-local RLS context, UUIDv4 validation,
current artifacts (including `cte_sql:<name>`), append-only decisions, preferences, and
content-free deletion tombstones.
- Added `tht session migrate --database-url URL [--status] --json` and a checksum-protected,
advisory-transaction-locked migration runner.
- Added server session configuration selection. `session_storage.connection` uses direct
PostgreSQL TLS modes `verify-ca` or `verify-full`; it does not use PostgREST.
- Updated packaging and `.gitignore` so session migrations are present in the built wheel.
- Did not alter Task 3 workflow commands, Pi gate code, or backend code.
- Commit: `541ef45 feat: add protected DWH credential registry`
- Committed files only:
- `tools/dwh-auth/internal/securefile/securefile_linux.go`
- `tools/dwh-auth/internal/securefile/securefile_linux_test.go`
- `tools/dwh-auth/internal/registry/store.go`
- `tools/dwh-auth/internal/registry/store_test.go`
- No server, Nginx, systemd, Docker stack, real registry, secrets, or legacy ThothII files were
read or changed. Tests use `t.TempDir` and synthetic record digests only.
## TDD evidence
### RED
Command:
```sh
cd harness && .venv/bin/pytest tests/test_postgres_session_repository.py tests/test_session_migrate_cmd.py -q
```
Result before production implementation: `1 failed, 4 errors in 3.89s`.
- Four setup errors were `ModuleNotFoundError: No module named
'tht.session.postgres_repository'`.
- The migration CLI test failed because `tht session migrate` did not exist (`No such command
'migrate'`).
### GREEN
Initial focused suite after implementation: `5 passed in 4.18s`.
Final focused verification:
```sh
cd harness && .venv/bin/pytest \
tests/test_session_repository.py \
tests/test_postgres_session_repository.py \
tests/test_session_migrate_cmd.py \
tests/test_vector_migration_packaging.py -q
```
Result: `12 passed in 5.80s`.
Changed-file lint verification:
```sh
cd harness && .venv/bin/ruff check \
tht/session/postgres_repository.py tht/migrations/sessions tht/config.py \
tht/session/repository.py tht/cli/session_cmd.py \
tests/test_postgres_session_repository.py tests/test_session_migrate_cmd.py \
tests/test_vector_migration_packaging.py
```
Result: `All checks passed!`.
## Migration and role policy choices
`001_schema.sql` creates only private `thoth_sessions` tables:
- `principals` and `principal_preferences`;
- `sessions`, with `session_artifacts` and `review_decisions` cascading on session deletion;
- `audit_log`, which deliberately has no content/detail/metadata column and keeps only action,
session UUID, actor identity, owner identity, and timestamp.
`002_security.sql` creates separate `thoth_sessions_runtime` and
`thoth_sessions_migrator` group roles, explicitly `NOLOGIN NOBYPASSRLS NOSUPERUSER`, revokes
public access, gives the runtime role only the operations required by the adapter, and enables
and forces RLS on every table. Owner/admin policies read only transaction-local settings:
`thoth_sessions.actor_issuer`, `thoth_sessions.actor_subject`, and
`thoth_sessions.is_admin`. The adapter starts every operation in a transaction, switches to the
restricted runtime role, sets those settings with `set_config(..., true)`, and uses advisory
transaction locks for migrations and per-session mutations.
The runtime role remains a `NOLOGIN` group role by design. Deployment must provision a dedicated
non-superuser LOGIN role and grant it membership, for example:
```sql
CREATE ROLE thoth_sessions_app LOGIN NOINHERIT PASSWORD '<secret>';
GRANT thoth_sessions_runtime TO thoth_sessions_app;
```
This avoids embedding an environment-specific login name or credential in versioned SQL. The
new integration test proves that this non-superuser membership path can create and read a
session while the adapter executes as `thoth_sessions_runtime`.
## Security/self-review
- Owner isolation and admin cross-owner reads run against disposable PostgreSQL containers,
not Supabase.
- No table or column includes `embedding`; repository code imports no embedding/vector code;
the regression test writes a session artifact under a monkeypatched embedding sentinel.
- An unauthorized owner receives the same `SessionError` as an absent session, preserving the
future backend's 404 mapping boundary.
- The audit row is inserted before deleting the parent session, so cascades remove all artifact
and decision content while the tombstone survives.
- A security review found and this task fixed the initial `.gitignore` rule that would have
excluded `migrations/sessions/*.sql` from Git/wheels. The wheel test now asserts both session
migration files and checks both the existing vector CLI and the new session CLI.
- The review also highlighted runtime login provisioning. It is covered by a non-superuser
regression test and documented above; concrete credential/role deployment belongs to Task 7.
## Remaining concerns
- Full `harness/.venv/bin/pytest -q` could not complete in this execution environment: the
runner terminated the command after roughly 30 seconds. Captured output reached 44% with no
failures before termination; `pgrep` confirmed no pytest process remained. The Task 2 focused
suites above completed successfully.
- `harness/.venv/bin/ruff check .` currently reports 34 pre-existing violations in unrelated
test files (for example unused imports in `tests/l0/test_db_connection.py` and semicolon style
in `tests/test_phase_effective.py`). The changed-file Ruff command is clean.
- Task 7 must safely provision the dedicated runtime login/membership and inject its TLS
credentials/CA; this task intentionally does not create a deployment-specific LOGIN role or
password.
## Review follow-up — unavailable migration database JSON contract
All Go commands ran in the required official `golang:1.26.5` container with only this linked
worktree bind-mounted at `/work`. The container image reports `go version go1.26.5 linux/amd64`.
### RED
Command:
Before either Task 2 production file existed, the focused command was run inside the container:
```sh
cd harness && .venv/bin/pytest \
tests/test_session_migrate_cmd.py::test_session_migrate_status_database_failure_is_pristine_json -q
```text
go test ./internal/securefile ./internal/registry -count=1
```
Result: `1 failed in 0.46s`. The unreachable direct PostgreSQL URL exited with code 1 but left
stdout empty, so `json.loads(result.stdout)` raised `JSONDecodeError`.
It failed non-zero for the expected absent implementation symbols, including `undefined: OpenDir`,
`undefined: ReadSecret`, `undefined: Open`, `undefined: State`, `undefined: PublicRecord`, and
`undefined: Store`.
### GREEN
The session migration CLI now catches `SQLAlchemyError` at the same command boundary as its
migration/domain errors and emits only `{"error": ...}` on stdout for `--json`.
After the minimal implementation and formatting:
```sh
cd harness && .venv/bin/pytest tests/test_session_migrate_cmd.py -q
cd harness && .venv/bin/ruff check tht/cli/session_cmd.py tests/test_session_migrate_cmd.py
```text
go test ./internal/securefile ./internal/registry -count=1
```
Result: `2 passed in 3.60s`; Ruff: `All checks passed!`.
Result:
```text
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/securefile
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/registry
```
### Race verification
The required race command completed successfully:
```text
go test -race ./internal/securefile ./internal/registry -count=1
```
Result:
```text
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/securefile 1.027s
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/registry 1.179s
```
Additional scoped verification:
```text
go vet ./internal/securefile ./internal/registry
go test ./... -count=1
git diff --cached --check
```
The Task 2 vet command completed with no findings; all four DWH-auth packages passed the full
module test run; the staged-diff check completed with no output.
## Delivered behavior
- `securefile` is Linux-only and traverses absolute paths through descriptor-anchored
`syscall.Open`/`Openat` calls with `O_NOFOLLOW|O_CLOEXEC`; protected roots, child directories,
records, and secret files are regular/directories only and are checked against `Lstat` after
`Fstat`.
- Protected reads reject special, group-writable, or world-writable modes, cap record reads at
4096 bytes, read at most one extra byte, and reject file-size changes or short/partial reads.
Secret ingress additionally requires exact `0600`.
- Secret output uses `O_CREAT|O_EXCL|O_NOFOLLOW`, exact `0600`, and an absolute protected parent.
- `registry.Open` creates protected `active` and `revoked` subdirectories under an existing safe
root. Record enumeration rejects unexpected entries, unsafe files, symlinks, oversized files,
bad filenames, malformed JSON, unknown JSON fields, duplicate JSON fields, and trailing JSON.
- `Add` validates Task 1 records, writes canonical JSON plus one newline through an exclusive
temporary file, sets final mode `0640`, syncs the file, renames under a protected per-root writer
lock, then syncs the directory.
- `Revoke` writes and syncs a valid revoked record before unlinking and syncing the active record.
`Find` checks revoked first; `List` resolves an active/revoked overlap to the revoked public
record. `FindLegacy` scans fail-closed and permits only the reserved legacy record state.
- `PublicRecord` deliberately omits `secret_sha256`; the redaction is regression-tested.
## Security-test coverage
- protected normal files and canonical record publication;
- symlinked roots, registry directories, records, and secret input;
- unsafe root/directory/record/secret modes;
- bounded/oversized record input;
- unknown, duplicate, trailing, and partial JSON;
- filename mismatch and multiple legacy-record integrity failures;
- revoked-state precedence when both active and revoked files exist;
- concurrent adds and concurrent reads during revocation, including the race detector.
## Self-review
Reviewed all syscall, path, mode, and error paths after the final race run:
- Directory traversal never follows a supplied component; later operations use retained directory
descriptors, not re-opened untrusted prefixes.
- `Fstat` validates the opened object and `Lstat` must identify the same inode/device; the direct
child name grammar refuses separators, dot components, and NUL.
- File validation occurs before and after reads; mode/type/size checks fail closed. Directory
listing obtains a fresh `openat(dirfd, ".")` descriptor so scans do not share a mutable directory
offset.
- Writer serialization protects the check-then-rename no-replace sequence. Failed temporary
cleanup leaves an unexpected entry that later scans reject rather than silently accepting it.
- State-specific validation rejects revocation metadata in active records and requires it in
revoked records. Revoked files are consulted before active files so interruption after revoked
publication cannot reactivate a credential.
- All functionality uses only Go standard-library packages and Linux `syscall`; no CGO, SQLite,
or third-party module was added.
## Concerns
- The optional whole-module `go vet ./...` reports a pre-existing Task 1 test warning at
`internal/credential/credential_test.go:86` (`append` with no variadic values). The identical
line is present in approved HEAD `1e82fd3`, outside this task’s authorized files. Focused Task 2
vet passes, and all module tests pass.
- The official image's login shell resets `PATH` and hides `/usr/local/go/bin`; all evidence uses
direct `go`/`gofmt` container entrypoints, which preserves the image’s Go 1.26.5 environment.
- The generic `apply_patch` helper intermittently failed before file access with a sandbox network
namespace error. Exact scoped corrections were applied through the shared worktree workflow;
this did not affect the final staged file set or verification evidence.
## Review remediation — 2026-08-21
### Scope and fix commit
- Review-fix commit: `971a0e6 fix: harden DWH credential registry reads`.
- Committed files only:
- `tools/dwh-auth/internal/registry/store.go`
- `tools/dwh-auth/internal/registry/store_test.go`
- The separate Task 1 vet correction is the independent preceding commit `d2415b5`; it is not
included in this Task 2 fix commit. No filesystem primitive, server, Nginx, service, registry,
secret, Docker stack, or legacy ThothII file was changed.
### Strict TDD evidence
All commands again used the official `golang:1.26.5` image with only this linked worktree mounted
at `/work`.
#### RED
The first focused command was run after the new regression tests and before production changes:
```text
go test ./internal/securefile ./internal/registry -count=1
```
It failed as intended. The three case-variant aliases (`SECRET_SHA256`, `Secret_SHA256`, and
`Schema_Version`) were accepted; past expiry returned active records from both `Find` and
`FindLegacy`; a revocation snapshot let readers return active data before publication; a temporary
file let `List`, `Check`, and `FindLegacy` observe false integrity failures; and the original
concurrent-read regression observed `ErrNotFound` during revocation.
The deterministic exact-expiry test was then added before the clock implementation. Its focused
run failed as intended with:
```text
internal/registry/store_test.go:572:10: store.now undefined
```
#### GREEN and verification
After the minimum implementation and `gofmt`:
```text
go test ./internal/securefile ./internal/registry -count=1
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/securefile 0.014s
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/registry 0.684s
go test -race ./internal/securefile ./internal/registry -count=1
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/securefile 1.022s
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/registry 1.711s
go vet ./internal/securefile ./internal/registry
```
The focused vet output was empty (success). Additional final checks passed:
```text
go test ./... -count=1
ok internal/credential
ok internal/record
ok internal/registry
ok internal/securefile
go vet ./...
go test -race ./internal/registry -count=10
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/registry 8.127s
git diff --cached --check
```
### Remediated security invariants
- `Find` and `FindLegacy` now deny an active record when `ExpiresAt <= now.UTC()`, returning the
existing non-disclosing `ErrNotFound`. The unexported per-Store `now` function is the minimal
deterministic clock seam; past, exact-equality, and future cases are covered for v1 and legacy
records. A revoked record is still consulted before expiry and therefore remains authoritative.
- One per-Store `sync.RWMutex` creates an in-process consistent snapshot. `Add` and `Revoke` hold
it exclusively for their full writer-lock lifetime, including temporary-file publication and
revoked-then-active removal. `Find`, `FindLegacy`, `List`, and `Check` hold a shared lock; their
bodies delegate only to unlocked helpers, preventing nested-lock deadlocks. `Close` also takes
the exclusive lock before closing descriptors.
- Deterministic regression tests hold the writer path at the revocation publication/unlink and
temporary-file stages. They prove public readers wait, then see either the final revoked state or
a clean directory, eliminating the `Names`-to-load/unlink and temporary-entry false failures
within the Store contract.
- Before struct decoding, the outer record JSON object now requires exactly spelled keys from the
schema allowlist and rejects duplicate literal keys. `Decoder.DisallowUnknownFields`, recursive
duplicate detection, trailing-value rejection, record validation, filename matching, no-follow
reads, modes, and durability ordering remain intact.
### Self-review and concerns
- Reviewed the new lock boundaries, error returns, revoked-first ordering, clock fallback,
JSON-token consumption, and every unchanged `securefile` syscall/path/mode boundary. The change
adds only standard-library `sync`; it does not relax existing fail-closed behavior.
- The synchronized snapshot is intentionally per `Store`, matching the requested in-process
contract. The existing protected advisory lock continues to serialize writers across Store
instances/processes; no cross-process reader snapshot is claimed by this fix.
- The historical whole-module vet concern in the original Task 2 report is now resolved by the
independent Task 1 commit `d2415b5`; complete module vet passes in the final evidence above.
## Cross-Store snapshot remediation — 2026-08-21
### Scope and TDD evidence
This third Task 2 fix wave changes only the protected lock primitive and registry snapshot code:
- `tools/dwh-auth/internal/securefile/securefile_linux.go`
- `tools/dwh-auth/internal/securefile/securefile_linux_test.go`
- `tools/dwh-auth/internal/registry/store.go`
- `tools/dwh-auth/internal/registry/store_test.go`
All commands used the official `golang:1.26.5` image with only this linked worktree mounted at
`/work`.
The test-only red patch initially tried to inspect the unexported `securefile.Dir.fd` through the
registry package and therefore did not compile. That assertion was removed without production
changes: the registry tests still create writer Store A and reader Store B through two independent
`Open(root)` calls, while the securefile test proves separate descriptors directly in its own
package. The subsequent behavioral RED run, before the production change, was:
```text
go test ./internal/securefile ./internal/registry -count=1
FAIL TestLockSharedAllowsReadersAndBlocksExclusiveWriter: Dir lacks shared advisory locking
FAIL TestCrossStoreReadersWaitAcrossRevokePublicationAndUnlink:
Find, List, Check, and FindLegacy completed during Store A's revocation snapshot
FAIL TestCrossStoreScanReadersWaitForWriterTemporaryFile:
Store B's List, Check, and FindLegacy observed `.tmp-regression`
```
### Delivered synchronization contract
- `securefile.Dir.LockShared` now acquires `LOCK_SH` on the same protected, no-follow, exact-0600
root lock file used by `Lock`, which continues to acquire `LOCK_EX`. The lock file is still
opened/created, mode-validated, inode-checked, and closed through the existing Linux syscall
path.
- Every public snapshot reader (`Find`, `FindLegacy`, `List`, and `Check`) takes its Store
`RLock`, then a shared advisory lock on root `.writer.lock`, and retains both through the whole
revoked/active lookup or directory scan/load. `Add` and `Revoke` retain Store `Lock`, then the
same root lock under `LOCK_EX`, over their full operation.
- The lock order is universally Store mutex then root advisory lock. Public methods delegate only
to unlocked helpers, so neither reader nor writer paths recursively acquire the Store mutex.
`Close` retains its exclusive Store mutex, preventing descriptor closure from racing any locked
reader or writer.
- Revoked-first precedence, expiry denial, exact JSON validation, no-follow checks, record modes,
temporary-file durability, and all previous behavior remain unchanged.
### GREEN and repeated verification
```text
go test ./internal/securefile ./internal/registry -count=1
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/securefile 0.019s
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/registry 0.711s
go test -race ./internal/securefile ./internal/registry -count=1
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/securefile 1.032s
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/registry 1.748s
go vet ./internal/securefile ./internal/registry
go test ./... -count=1
ok internal/credential
ok internal/record
ok internal/registry
ok internal/securefile
go vet ./...
go test -race ./internal/registry \
-run 'TestCrossStoreReadersWaitAcrossRevokePublicationAndUnlink|TestCrossStoreScanReadersWaitForWriterTemporaryFile' -count=20
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/registry 9.880s
go test -race ./internal/securefile \
-run TestLockSharedAllowsReadersAndBlocksExclusiveWriter -count=20
ok github.com/aritmolab/thothii/tools/dwh-auth/internal/securefile 1.063s
git diff --check
```
Both vet commands and the whitespace check produced no output. The securefile regression opens
three protected directory descriptors, proves they are distinct, permits two independent shared
holders, proves a third descriptor cannot take `LOCK_EX|LOCK_NB`, then proves exclusive acquisition
succeeds after shared release. The registry regressions deterministically block Store B readers
while Store A holds the exclusive root lock and verify only final revoked/clean states afterward.
### Self-review and concerns
- Reviewed lock creation/reopen races, no-follow flags, exact lock-file mode validation, lock
release, descriptor lifetime, lock ordering, error wrapping, and the unlocked-helper call graph.
No public reader invokes another public reader or writer while holding a Store lock.
- Advisory synchronization necessarily covers cooperating registry Store instances/processes;
arbitrary external filesystem mutation remains fail-closed through the existing integrity
checks rather than being silently accepted.
- No known concerns within the registry's cooperating-process contract.