326 lines
15 KiB
Markdown
326 lines
15 KiB
Markdown
# Task 2 report — protected atomic registry
|
||
|
||
## Scope and commit
|
||
|
||
- 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
|
||
|
||
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
|
||
|
||
Before either Task 2 production file existed, the focused command was run inside the container:
|
||
|
||
```text
|
||
go test ./internal/securefile ./internal/registry -count=1
|
||
```
|
||
|
||
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
|
||
|
||
After the minimal implementation and formatting:
|
||
|
||
```text
|
||
go test ./internal/securefile ./internal/registry -count=1
|
||
```
|
||
|
||
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.
|