fix(storage): harden the KVS-backed identity and wallet stores

[AkramBitar]

Fixes #2041

Six fixes in token/services/storage/db/kvs:

1. Vault key collisions — composite-key components are percent-escaped, so an empty component, a / inside one (base64 identity hashes have them) or a ./.. can no longer make two different keys point at the same secret.
2. Iterator — a key deleted between the list and its read is skipped, not yielded as an empty record.
3. StoreIdentity — the entry IdentityExists reads is written last and every write is idempotent, so a partial failure reads as "not stored" and a retry converges.
4. tracker.go — history entries are snapshots, not the caller's pointer.
5. fscKVS.Close() — returns the store's close error instead of only logging it.
6. Test helper — closes the HTTP response body on success.

Also fixed in the same code: a nil iterator for an empty list (panicked in IdentityConfigurationsIterator), an unchecked value.(string), and an unclosed iterator in GetWalletIDs.

Warning

Storage-format change: Vault entries whose key components contain / or % now live at a different path and are not found by the new code. See docs/services/storage/kvs.md.

IdentityExists returns (bool, error)

#2409 has landed, so the WalletStoreService.IdentityExists signature is already on main and what remains here is only the KVS side of it: a failed lookup is no longer indistinguishable from "the binding does not exist". The interface, sql/common/wallet.go, the mock and the test callers all came from #2409 and are not in this diff.

The entry is read with kvs.Get rather than probed with kvs.Exists, the way GetWalletID already does. FSC implements Exists as len(GetExisting(...)) > 0 and GetExisting drops the underlying store error, so a transient failure would masquerade as "not bound" — the confusion the error return exists to remove. Get is the only method on the KVS surface that propagates the store error, so absence is classified from the error instead: an authoritative "not found" is a miss, everything else reaches the caller.

Registry.ContainsIdentity cannot grow an error return (it backs driver.Wallet.Contains, a plain bool), so it logs the failure before reporting false.

Second commit: every Vault absence shape is a miss

From review round 4. read() reported a missing secret as absent, but answered a secret with no data — or a data field that was missing, empty or not a map — with an error, which aborts the entire prefix scan behind GetConfID and GetWalletIDs. Exists already classified all three shapes as absent, so the two disagreed on what exists within the same file.

It is reachable without a malformed secret: KV v1 has one mount per writer-set, so anything else writing under it (an operator running vault kv put, another application sharing the mount) leaves an entry with no data map of ours that any scan over that mount reaches. One of those under the prefix hid every real entry below it.

read() now mirrors Exists and the iterator's existing skip path handles the result. A secret that does carry a data map is this KVS' own entry, so a value inside it that is missing, not a string or not decodable stays an error — that entry is corrupt, not absent.

This removes the only place in the tree that produced state of id [...] does not exist, the message WalletStore's anchored not-found match looks for on this backend. The head stays listed because hashicorp is a module of its own, versioned independently of the root module, so a deployment can still pair this store with a release that returns it.

The issue's perf suggestion (hash-first keys) is left out — it needs a secondary index plus a scan fallback for existing data, so it belongs in its own change.

Tests: unit + fuzz for the key/path round trip (in the nightly fuzz matrix), a Vault-backed case for /, empty, .. and % components plus a key deleted after the list, a tracker regression test, and dbtest's wallet suite wired to the in-memory and Vault-backed KVS (it previously ran against the SQL drivers only, which is why the GetConfID and GetWalletIDs defects went unnoticed). From round 4: testForeignSecretIsAbsentNotAnError, and TestIsNotFoundErrMatchesLiveBackend, which classifies the error the in-tree KVS really returns so a rewording upstream fails in CI rather than in a deployment. New page docs/services/storage/kvs.md.

[Effi-S]

Some Findings:

1. Backward-incompatible Vault storage format — Medium

Location: token/services/storage/db/kvs/hashicorp/vault_kvs.go — NormalizeID
Keys with / or % (identity hashes routinely contain /) now map to different Vault
paths, so existing deployments stop finding previously written identity/wallet data. Called
out in a doc WARNING but there is no migration tooling — upgraders must re-register affected
identities manually. Confirm this is acceptable / has a migration story.

2. KVS IdentityExists cannot surface read failures — Medium

Location: token/services/storage/db/kvs/walletdb.go — IdentityExists
The new (bool, error) return only ever carries a key-construction error; kvs.Exists still
swallows Vault read errors (FSC GetExisting), so a transient failure reads as "not bound"
for the KVS backend. The hardening goal (fault ≠ non-membership) is only actually met by the
SQL backend. Documented, but the KVS half of the change is effectively inert.

3. Context threaded but not used for Vault I/O — Low

Location: token/services/storage/db/kvs/hashicorp/vault_kvs.go — read (ignores ctx),
Get, iterator
ctx is now plumbed through but read calls client.Logical().Read(...), not
ReadWithContext. Cancellation/deadlines are not honored. Cosmetic threading; switch to the
*WithContext Vault calls to make it real.

4. StoreIdentity intermediate inconsistency window — Low

Location: token/services/storage/db/kvs/walletdb.go — StoreIdentity
The GetWalletID key [tmsID, roleID, idHash] is written before the IdentityExists key.
A failure between them leaves GetWalletID returning a wallet while IdentityExists reports
false. Retries converge (writes are idempotent), so low impact, but the two reads disagree
in the window. Acceptable if IdentityExists is the sole commit marker.

[AkramBitar]

Thanks @Effi-S. All four addressed; squashed into 5ff00ea.

1. Vault format break — acknowledged, no migration tool. Intentional; doc note expanded. The old mapping escaped nothing, so it wasn't injective: <mount>/a/b/c doesn't say whether it was written under ["a","b","c"] or ["a/b","c"]. Keys can't be read back off the paths, only reconstructed from the known layout — hence re-register rather than a mechanical rewrite. The backend is a separate module with no in-tree consumer, so it's opt-in. Happy to add a migration command as a follow-up if you think deployments need one.

2. IdentityExists inert — fixed. You were right. It now reads the entry with kvs.Get instead of probing with kvs.Exists, the way GetWalletID already does: "does not exist" is an authoritative miss, every other error propagates. One wrinkle — the Vault backend reports a missing id as a nil error with the destination untouched, so an empty value is also a miss. That's safe because GetWalletID already reserves "" for "no binding". Added a round-trip test and a fault-injection test.

3. Context unused — fixed. read, Exists, Put, Delete and the iterator's list now use the *WithContext calls, so cancellation and deadlines are real. Containerised Vault tests pass. Also bumped Exists's swallowed read error from Debugf to Errorf, since its interface can't return one.

4. StoreIdentity window — no change, by design. IdentityExists is the sole commit marker, as you suspected. The ordering is deliberate and documented, every write is idempotent so retries converge, and Registry.ContainsIdentity already treats a failed lookup as false.

---

Out of scope, found while fixing #2: IdentityStore.ConfigurationExists (identitydb.go:208) has the identical inert pattern — pre-existing, so I didn't widen the diff. Happy to open an issue. The Vault test suite also leaves its containers behind.

[Effi-S]

@AkramBitar Looks good. Only minor findings