Replace IsEncrypted heuristic with explicit enc:v1: ciphertext marker (fixes auth-row bricking)

[joestump]

Implements the highest-severity finding of the remediation review. Part of epic #319. Governing: ADR-0006 (application-layer AES-256-GCM encryption), ADR-0021 (encryption key rotation).

What / Why

IsEncrypted() treated any std-base64-decodable string with >= 29 decoded bytes as ciphertext. A 40-char alphanumeric password (or base64-alphabet API token) false-positived: the write hook skipped encryption (plaintext stored), then every read hit the decrypt interceptor, GCM authentication failed, and the query itself errored — the auth row was bricked until manual DB surgery.

Changes

• Explicit versioned marker (internal/crypto/encrypt.go): new ciphertext is enc:v1: + base64(nonce||ct||tag). IsEncrypted() is now an exact marker-prefix check — never a heuristic. The marker alphabet (:) is disjoint from std base64, so marked values can never be confused with legacy ciphertext or plaintext. The old heuristic survives only as LooksLikeLegacyCiphertext(), explicitly documented as attempt-decryption-only, for the migration paths. Decrypt() accepts both marked and legacy (bare base64) input. Governing ADR-0006/ADR-0021 comments added throughout (previously the only major subsystem with zero).
• Self-healing read path (internal/database/hooks.go): interceptors resolve each stored value via resolveEncryptedField() — marker → decrypt (failure is a real error: wrong key/corruption); legacy base64 shape → attempt decrypt, on GCM failure treat as plaintext; in both legacy cases the row is best-effort re-encrypted with the marker (write hook sees the marker and stores it as-is). This path never errors the query. Plain plaintext keeps the existing encrypt-on-next-write behavior.
• Key rotation handles all formats (cmd/admin/main.go): rotate-key now resolves marked, legacy, and plaintext values and always writes back the marked format under the new key, so a rotated database is uniformly enc:v1:. Marked values that fail old-key decryption abort (definitive wrong key); ambiguous legacy-shaped values fall back to plaintext with a per-row warning. Pre-rotation old-key verification scans for a marked value first (definitive), then a decryptable legacy value.
• ADR-0021 postgres/mysql guard (cmd/admin/main.go): the "server not running" pre-rotation check was SQLite-only (BEGIN EXCLUSIVE probe). Postgres/MySQL now acquire a session-scoped advisory lock (pg_try_advisory_lock / GET_LOCK) to serialize rotations and refuse to run while other sessions are connected to the database. The pool is pinned to one connection (SetMaxOpenConns(1)) so session-scoped locks hold for the process lifetime. The connection check is documented as best-effort — stopping the server first remains mandatory per ADR-0021.

Test evidence

• make test: all 27 packages pass, zero failures.
• gofmt -l clean on internal/crypto, internal/database, cmd/admin.
• New tests:
  • TestBase64LookalikePasswordRoundTrip — a 40-char base64-alphabet password is stored encrypted (marker verified via raw SQL) and round-trips (acceptance a).
  • TestPlaintextLookalikeSelfHealsOnRead — a pre-existing plaintext-that-looks-like-base64 row reads back correctly instead of erroring, and the stored value self-heals to marked ciphertext (acceptance b).
  • TestLegacyCiphertextSelfHealsOnRead — pre-marker ciphertext decrypts on read and migrates to the marked format.
  • TestRunMixedMarkedLegacyAndPlaintextRows / TestRunLegacyCiphertextRotation — rotation across marked + legacy + plaintext rows on SQLite (acceptance c); the postgres/mysql guard is exercised structurally and documented (no pg/mysql instance in CI — see code comments in acquireRotationGuard).
  • Marker-format unit tests: TestEncryptProducesMarker, TestDecryptLegacyUnmarkedCiphertext, TestLooksLikeLegacyCiphertext, plus TestIsEncrypted regression cases for the false-positive inputs.

Deferred Design Doc Updates

docs/adrs/ is a protected path in this remediation wave, so the issue's "Amend ADR-0006 to document the marker format" requirement is deferred. Proposed amendments for a follow-up:

• ADR-0006: document the enc:v1: versioned marker format (enc:v1: + base64(nonce||ct||tag)); update the "IsEncrypted() heuristic" consequence bullets to reflect that detection is now an exact marker check, with LooksLikeLegacyCiphertext() reserved for the self-healing migration path; describe the read-path self-heal (legacy ciphertext and heuristic-lookalike plaintext both re-encrypted with marker on read).
• ADR-0021: document that rotation migrates legacy/plaintext values to the marked format under the new key, and the postgres/mysql advisory-lock + other-sessions pre-rotation guard (plus its best-effort nature).
• SPEC key-rotation: ROT-004 (pre-rotation validation) and ROT-040 (decrypt with old key) semantics now include the legacy/plaintext fallback behavior described above.

Closes #335

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Adversarial review — PR #353 (issue #335)

Verdict: CHANGES NEEDED

Independently verified against a detached checkout of origin/feature/335-encrypted-marker-fix-bricking (8ed8106). make test passes (27 packages); go test -race clean on internal/crypto, internal/database, cmd/admin. Every finding below was confirmed empirically with throwaway tests against the branch, not just by inspection. The core design is right — exact marker check, disjoint alphabet, self-heal that never errors the query — but three defects need fixing, two of them in the exact failure class this PR exists to eliminate.

Blocking

F1. HIGH — A user password beginning with enc:v1: is stored as plaintext at rest AND bricks the row on read.
internal/database/hooks.go:95 (and :184, :197, :306): the write hooks skip encryption whenever crypto.IsEncrypted(value) — which is now a bare prefix check on user-controlled input. internal/database/hooks.go:56-61: on read, a marked value that fails decrypt is a hard error. Confirmed on the branch: creating a NavidromeAuth with password enc:v1:hunter2 stores the literal string in the DB (confidentiality regression — before this PR that password was encrypted, since it isn't base64) and every subsequent Get fails with failed to decrypt password for user 1: invalid ciphertext: not base64 encoded — a bricked row, the exact #335 failure mode, now reachable by typing a password into a form. Same applies to "" → enc:v1: exactly, and marker+garbage.
Fix properly: stop using IsEncrypted as the write-hook skip signal for user input. Pass a context sentinel (e.g. ctx = database.WithPreEncrypted(ctx)) on the heal write-backs and in the hooks only skip encryption when that sentinel is present; user-supplied values then ALWAYS get encrypted (even marker-lookalikes round-trip via double encryption, which is fine). A cheaper partial fix — treating marked-but-invalid-base64 as plaintext on read — kills the brick for enc:v1:hunter2 but still leaves enc:v1: + valid-base64 passwords bricked and still stores marker-prefixed user secrets as plaintext; prefer the sentinel.

F2. HIGH — rotate-key with a wrong old key on an all-legacy database silently corrupts every credential and prints a full success summary.
cmd/admin/main.go:331-334 (verifyOldKey warn-and-proceed) + main.go:353-358 (resolveStoredValue legacy fallback). Confirmed on the branch: real key K1, legacy (unmarked) ciphertext rows, operator passes wrong old key K_wrong → run() returns nil, stdout prints Key rotation complete … Verification: PASSED … Update your environment variable, and every credential is now enc:v1: + enc(K_new, ). The server will then "successfully" decrypt credentials to base64 garbage and send them to Navidrome/Spotify/Last.fm — bricking traded for silent wrong-data, which the review brief correctly flags as worse. The PR's "recoverable but operator must heed warnings" claim is too generous: (a) the warnings go to stderr per-row while stdout ends in an unqualified success block; (b) recovery is NOT rotate-key in reverse — a reverse rotation double-wraps (marked outer layer decrypts to the legacy ct string, which is then re-encrypted, not restored); you need a bespoke unwrap script plus the correct old key. And this is the most likely real-world pre-upgrade state: every pre-marker database is all-legacy, so the first rotation after this PR ships has zero marked rows to trip the definitive abort path.
Fix: hard-abort when non-empty values exist but none positively verified the old key (i.e. foundValue && !verified), and also abort per-row in reencryptAll when a legacy-shaped value fails old-key decrypt, unless an explicit --force-treat-undecryptable-as-plaintext (or --force) flag is passed. The legit lookalike-plaintext case (#335 rows) is exactly what the flag is for; make the abort message name it. Warn-and-proceed as a default is a data-loss footgun in the PR whose purpose is preventing credential destruction.

F3. MEDIUM — Unconditional heal write-back can silently revert a concurrent legitimate credential update.
internal/database/hooks.go:165, :278-285, :375: the heal is UpdateOneID(id).SetPassword(healed).Exec(ctx) with no guard on the value it read. Confirmed deterministically on the branch with the interceptor's exact interleaving: read loads legacy value and computes healed; a legitimate UpdateOneID(...).SetPassword("new-user-password") commits; the delayed heal write then lands → final stored value decrypts to the OLD password. The user's password change is silently lost. Narrow window and only on not-yet-migrated rows, but these are credentials.
Fix: make the heal conditional on the original stored value — ent v0.14.5 supports it: client.NavidromeAuth.Update().Where(navidromeauth.ID(auth.ID), navidromeauth.Password(storedRaw)).SetPassword(healed).Exec(ctx) (return storedRaw from resolveEncryptedField or capture it at the call site). Zero rows affected on race = correct outcome; next read retries.

Non-blocking

F4. MEDIUM (pre-existing, but contradicts the issue's acceptance criteria) — rotation likely fails at runtime on postgres/mysql.
cmd/admin/main.go:387-440 (reencryptAll): tx.Exec is called while tx.Query rows are still open on the transaction's single connection. Fine on SQLite (tested), but go-sql-driver/mysql ("busy buffer"/commands-out-of-sync) and lib/pq do not support executing statements while a result set is open on the same conn — rotation would error mid-transaction (rollback, so no data harm, but the feature is broken). Pattern predates this PR, but issue #335's AC says "Key rotation works across marked + legacy rows on all three drivers" and only SQLite is exercised. Suggest buffering each table's rows into memory, closing rows, then updating — cheap and removes the driver dependency. Fine as an immediate follow-up issue if you'd rather keep this PR scoped.

F5. LOW — read-path silent wrong-key on legacy rows. With a misconfigured server key and legacy rows, resolveEncryptedField treats real ciphertext as plaintext, returns base64 garbage as the credential, and heals it wrapped under the wrong key — all silently (hooks.go has no logging at all). Accepted tradeoff of self-heal, but a WARN log on the legacy-GCM-failure branch would give operators their only signal.

F6. LOW — guard caveats. information_schema.processlist only shows other users' sessions if the CLI's MySQL user has the PROCESS privilege — without it the other-sessions check silently sees nothing. The pg/mysql advisory lock evaporates if the pinned connection is dropped and silently re-established mid-run. Both acceptable for a short-lived CLI given the documented best-effort stance; worth a sentence in the code comment. Otherwise the guard checks out: single-conn pinning is correct (and also fixes the SQLite probe's cross-connection PRAGMA hazard), lock IDs/queries are right.

F7. NIT — marked2 (cmd/admin/main_test.go:305) is an opaque helper name; encryptOrFatal or similar.

Verified good (adversarial checks that passed)

• Marker/legacy ambiguity is impossible: : is outside the std base64 alphabet, so legacy ciphertext can never begin with enc:v1:. Proven by construction.
• No double encryption on repeated saves; heal write-backs pass through the write hook unmodified; no interceptor recursion (heals are mutations via Exec, interceptors wrap queries only); -race clean including concurrent reads healing the same row (last-writer-wins with equivalent plaintext — harmless).
• Heal escaping a surrounding transaction (root client capture) is safe: heal of a row uncommitted in another tx fails NotFound/busy and is ignored; no corruption path found; failure just retries next read.
• verifyNewKey decrypts every non-empty value post-commit, which is exactly why plaintext rows must be encrypted during rotation — reencryptAll rewrites every non-empty value, so the pair is internally consistent; the behavior change (previously rotation hard-failed on a plaintext first row) is an improvement.
• PR Story #324: Consolidate configuration into Viper Config struct; make Lidarr optional #352 overlap: Story #324: Consolidate configuration into Viper Config struct; make Lidarr optional #352 rewrites main()'s config block (cmd/admin/main.go ~48-70); this PR's hunks all start inside run() (~115+). Non-overlapping hunks against the same base — expect a clean auto-merge whichever lands second; the --db-flag-overrides-DSN semantics are preserved by both.

F1-F3 are all small, well-localized fixes; happy to re-review quickly.

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Pushed f7d85b2 addressing all review findings from #353 (comment).

BLOCKING

1. enc:v1:-prefixed user input (HIGH) — write hooks now encrypt unconditionally; value-shape checks are gone from the write path. A new context sentinel (withSelfHeal/isSelfHeal in internal/database/hooks.go) is the only way to skip encryption, and only the interceptor self-heal write-backs use it. A password literally starting with enc:v1: is therefore always stored as real ciphertext and round-trips. Consequence documented in code: a marked value in the DB is now provably ciphertext, so marked-decrypt failure on read can only be a wrong key or corruption and still errors loudly (correct signal, never user data). New test: TestMarkerPrefixedPasswordRoundTrip (create + update paths, raw storage verified).

2. Wrong-old-key on all-legacy DB (HIGH) — verifyOldKey now returns (found, verified) and run() hard-aborts on found && !verified: on an all-legacy database a wrong old key is indistinguishable from all-plaintext data, so silently proceeding is never safe. New --force flag is the explicit escape hatch for genuinely pre-encryption plaintext databases, with a stderr warning when used. New tests: TestRunAbortsOnUnverifiedOldKey (asserts abort mentions --force and data is byte-identical after), TestRunForceTreatsUnverifiedAsPlaintext.

3. Heal clobbering concurrent updates (MEDIUM) — all three self-heal write-backs are now compare-and-swap: Update().Where(ID(id), Field(storedRawValue)).SetField(healed) (ent predicates), so a stale heal matches 0 rows if the credential changed in between. New deterministic test: TestSelfHealDoesNotClobberConcurrentUpdate.

RECOMMENDED

4. slog.Warn added on every legacy-GCM-failure read path (per entity + row id, hints at wrong SPOTTER_SECURITY_ENCRYPTION_KEY) — matching the rotation tool's per-row stderr warnings.
5. reencryptAll now buffers all rows per table before issuing tx.Exec UPDATEs — required with the single pinned connection on mysql/lib/pq, which cannot exec while a query result set is open.
6. Guard caveats documented in acquireRotationGuard: MySQL information_schema.processlist needs the PROCESS privilege to see other users' sessions; the advisory lock is silently lost if the pinned connection reconnects; lazy-connecting servers can slip the other-sessions check — stopping the server remains mandatory per ADR-0021.

Verification: make test green (27 packages, zero failures); go test -race clean on internal/crypto, internal/database, cmd/admin; gofmt -l clean on all changed packages.

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Re-review of f7d85b2 — delta from #353 (comment)

Verdict: LGTM

Re-verified empirically on a fresh detached checkout of f7d85b2, re-running my original attack reproductions against the fixed code (all now defeated) plus regression probes. make test green (27 packages); go test -race clean on internal/crypto, internal/database, cmd/admin.

• F1 (marker-prefixed user input) — FIXED. Re-ran my original attack (enc:v1:hunter2) plus bare enc:v1: and enc:v1: + valid base64, on both create and update paths: all round-trip, all stored as real marked ciphertext, nothing plaintext at rest, no bricked reads. The selfHealCtxKey sentinel is package-private, constructed only at the three heal write-backs in internal/database/hooks.go, and never escapes — grep confirms no other producer, and all external credential writers (internal/handlers/auth.go, spotify_auth.go, lastfm_auth.go) now always encrypt. Marked-decrypt failure on read is now provably wrong-key/corruption and still errors loudly — correct signal.
• F2 (wrong-old-key rotation) — FIXED. Re-ran my original all-legacy + wrong-key attack: hard abort mentioning --force, stored bytes untouched and still decryptable with the real key. --force on a genuine plaintext DB works (author's test + verified). Also probed the verifyOldKey signature change for regressions: mixed marked+legacy DB with the correct key still rotates without --force; empty DB still exits "nothing to rotate".
• F3 (heal clobber) — FIXED. I replayed my original deterministic clobber sequence through the actual decryptNavidromeAuth path with a stale entity: the CAS (Where(ID, Field(storedRaw))) matched 0 rows and the user's new password survived. Verified the CAS predicates use the raw stored value in all three heal sites (the auth.X = plaintext assignments come after the heal block). Live (non-stale) heal still persists marked ciphertext end-to-end. The author's TestSelfHealDoesNotClobberConcurrentUpdate is non-vacuous — its predicate matches hooks.go verbatim.
• F4 (mysql/pq exec-while-rows-open) — FIXED. reencryptAll now buffers each table's rows and closes the result set before any tx.Exec. Checked the other query paths: verifyOldKey, verifyNewKey, and acquireRotationGuard never exec while a result set is open. Buffering is complete.
• F5/F6 — addressed. slog.Warn with row id on every legacy-GCM-failure read path; guard caveats (PROCESS privilege, reconnect lock loss, lazy connections) documented in acquireRotationGuard.
• Refactor regression scan: verifyOldKey (bool, bool, error) callers all updated; resolveEncryptedField 4-value signature consistent at all call sites; hook restore-original-value logic unaffected by the sentinel change (Update-heal returns a row count, not an entity, so the post-mutation restore can't misfire). Nothing found.

Nice work — the sentinel approach is the right fix, and the abort-by-default rotation is exactly the posture this PR needed. Good to merge from my side (pending the known #352 rebase, which remains non-conflicting).

🤖 Posted on behalf of @joestump by Claude.