feat(sidecar): production keyring backend support (closes #162)

[bdchatham]

Summary

• Implements Phase 1 step 2 of the in-pod governance signing workstream (design: docs/design/in-pod-governance-signing.md, Component B).
• Adds three envs read at startup: `SEI_KEYRING_BACKEND` (`test`/`file`/`os`, empty = governance disabled), `SEI_KEYRING_DIR` (defaults to `$SEI_HOME/keyring-file`), `SEI_KEYRING_PASSPHRASE` (required when backend=file).
• New `sidecar/server/keyring.go`: `OpenKeyring` factory + `SmokeTestKeyring` with bounded retry + defensive `redactPassphrase`.
• Passphrase wiped from `os.Environ()` unconditionally before any error check via `os.Unsetenv`.
• Engine gains `ExecutionConfig.Keyring` accessible via Set/Get on `*Engine`. No handler reads it in this PR — that wiring lands in feat(sidecar): sign-tx task family (gov vote / submit-proposal / deposit) #163 (sign-tx task family).
• Genesis isolation preserved: `generate_gentx.go` continues to construct its own `BackendTest` keyring locally; does NOT consume the shared `Engine.Keyring`.
• New `sidecar/docs/keyring.md` covers env contract + trust model + genesis-path isolation.

Discretionary decisions flagged for review

These weren't fully prescribed; cross-reviewers should sanity-check:

1. *`ExecutionConfig` on `Engine` with Set/Get accessors, not as a `NewEngine` constructor argument. Rationale: ~16 NewEngine call sites in tests; signature change would force churn for a field no handler reads yet. Mutex-guarded accessor pair added. Alternative considered: `NewEngineWithConfig` second constructor — rejected (duplicates constructor surface). Wiring to handlers belongs to feat(sidecar): sign-tx task family (gov vote / submit-proposal / deposit) #163.

2. `OpenKeyring` strips a trailing `keyring-file` segment from the supplied dir. The SDK's file backend internally appends `keyring-file` to the rootDir, so a caller passing `/sei/keyring-file` would land at `/sei/keyring-file/keyring-file`. The strip is a guardrail matching both possible operator mental models. Design's sketch used `filepath.Dir(dir)` unconditionally; I made it conditional on the suffix to keep `test`/`os` backends (which receive an arbitrary dir) unaffected.

3. `SmokeTestKeyring` recovers from `panic` in the underlying 99designs/keyring lib. The lib panics on a non-directory FileDir (discovered during testing). Recovery wraps the panic into a smoke-test error so a bad mount path produces fail-fast logging rather than a SIGSEGV.

4. Redaction = verbatim `strings.ReplaceAll` of the passphrase at the error-return boundary inside `OpenKeyring`. Defensive against future upstream regressions; the SDK doesn't echo the passphrase in any current code path. Wraps with `%s` (not `%w`) at that boundary to sever any chain that might carry the passphrase via a `%v` of an internal struct.

5. Passphrase `os.Unsetenv` happens before the error check, on every code path through `OpenKeyring`. Deferred-unset alternative would risk leaking the value across an early-return.

6. No stdout-capturing test for the `keyring opened` log line. The single log statement contains backend + dir only — no passphrase. The contract is asserted indirectly via code review of all call sites plus the `redactPassphrase` unit test. A captured-output test would be brittle against `seilog`'s writer wiring.

Spec gaps surfaced

• Issue body AC item 2 contradicts the merged design. Issue says "Shared keyring factory consumed by `generate_gentx.go` (refactored, not duplicated)" but design Component B says "Refactor `generate_gentx.go` to take its keyring backend from a shared sidecar-level factory rather than hardcoding `BackendTest`. The factory reads from envs above. Genesis path continues to use `BackendTest` when configured." On re-reading both, the design is consistent with EITHER reading — but the orchestrator's brief explicitly directed "Do NOT consume the shared `cfg.Keyring` for the gentx flow" to preserve genesis isolation as a hard property. Followed the brief; documented the deliberate isolation in `generate_gentx.go`. Worth confirming in review.
• Design code sketch `openKeyring` shows `codec.NewProtoCodec(...)` as a parameter. Real `keyring.New` signature is `(appName, backend, rootDir, userInput, opts...)` — no codec. Followed real SDK; design sketch is stale.

Test plan

• `go build ./...` clean
• `go test ./...` all packages pass
• `golangci-lint run --new-from-rev=origin/main ./...` reports 0 issues
• Review: confirm passphrase never appears in logs, error strings, or task results
• Review: confirm `generate_gentx` genesis path is unchanged (BackendTest hardcoded locally; isolation comment present)
• Review: confirm `os.Unsetenv` happens before any path that could surface the passphrase

Cross-review

This PR is being reviewed in parallel by platform-engineer, kubernetes-specialist, and security-specialist before being marked ready for merge. Findings will be synthesized into a single resolution comment.

🤖 Generated with Claude Code

---

Note

Medium Risk
Introduces new startup-time secret/env handling and Cosmos SDK keyring initialization, which can affect sidecar boot behavior and touches sensitive passphrase redaction/wiping logic. Also changes engine startup sequencing (explicit stale-task rehydration) which could impact crash-recovery behavior if misordered.

Overview
Adds opt-in production keyring backend support for the sidecar: serve now reads SEI_KEYRING_BACKEND/SEI_KEYRING_DIR/SEI_KEYRING_PASSPHRASE, opens and smoke-tests a Cosmos SDK keyring, and wipes the passphrase env to reduce exposure in /proc/<pid>/environ.

Introduces engine.ExecutionConfig (currently holding Keyring) and stores it on Engine, and adjusts engine startup so stale-task recovery is triggered explicitly via RehydrateStaleTasks() after config is installed.

Adds server.OpenKeyring + SmokeTestKeyring utilities with backend validation, directory normalization for the file backend, bounded retry, panic recovery, and passphrase redaction; includes unit tests plus new operator-facing docs in sidecar/docs/keyring.md. Also documents that generate-gentx continues using an isolated BackendTest keyring (no production keyring reuse).

^{Reviewed by Cursor Bugbot for commit 7e8670d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[bdchatham]

Cross-review complete — findings integrated

3-specialist parallel review against commit `750db33` (platform-engineer / kubernetes-specialist / security-specialist).

Verdicts: platform-engineer LGTM-with-fixes • kubernetes-specialist Approve-with-non-blocking-follow-ups • security-specialist LGTM-with-fixes (no code blockers).

Addressed in commit `afa9cac`

#	Source	Item	Fix
1	platform N1	Mutex on `SetExecutionConfig` was theater (field is set-once during single-threaded startup)	Dropped the mutex; doc comment explicitly states "not safe to call concurrently with task execution"
2	security #2	`os.Unsetenv` happened after the passphrase-required check, leaving a future-fragile window	Moved to immediately after `os.Getenv`; unconditional on every return path
3	platform N4 + security #3	`%w`-wrapping the SDK error preserved the chain — a typed field embedding the passphrase could resurface via a future caller's `%w` or `%v` of a wrapped struct	Use `errors.New(redactPassphrase(...))` to sever the chain; also removed `%w` wrap at the `buildExecutionConfig` call site
4	platform N3 + k8s n2	Suffix-strip on `SEI_KEYRING_DIR` was hidden behavior; home-dir resolution was misdescribed in docs	Documented suffix-strip explicitly; clarified `--home` flag (defaulting to `$SEI_HOME`) rather than implying direct `$SEI_HOME` read
5	platform N5	`SmokeTestKeyring` panic-recovery comment overstated the fail-fast property	Tightened: real reason is so the bounded retry loop can run
6	security nit	No explicit comment on why `kr.List()` discards the slice	Added: "List() decrypts the index, not individual keys — strongest non-destructive check"

Deferred to follow-ups (clearly out of #162 scope, surfaced for tracking)

• Rehydration-vs-config ordering trap (k8s N4): `NewEngine` rehydrates stale tasks BEFORE `SetExecutionConfig` runs. If a future sign-tx task is in 'running' state across a pod restart, rehydration would execute it with a zero-value config (nil Keyring). No bug today (no sign-tx handlers exist yet). Will fix in feat(sidecar): sign-tx task family (gov vote / submit-proposal / deposit) #163 by either rehydrating after the config is installed OR by having sign-tx handlers read the engine's config at execution time (not goroutine-launch time).
• `ExecutionConfig` blast radius / per-handler capabilities (security Bump version to 0.0.2 in prep for release #7): `*Engine.ExecutionConfig()` is package-exported and any future task handler can pull the unlocked keyring. Right scoping for MVP; consider an opaque `SignerProvider` interface in a future hardening pass.
• Log-line redaction test (platform Fix multi-line read and typos in config #6, security Fix multi-line read and typos in config #6): seilog caches its writer at init time, so an `os.Stdout` redirect during a test doesn't capture the actual log output. A runtime test would trivially pass-through rather than catch regressions. Deferred to code-inspection at the single log site (`serve.go:185` — `serveLog.Info("keyring opened", "backend", backend, "dir", dir)` — passphrase NOT in the arglist).
• `shareProcessNamespace` doc-vs-code consistency (security Integrate UCI lint, versioning and go check #1): the security reviewer flagged that the prompt claims `true` while the design doc text at one point says `false`. The CONTROLLER's actual pod-spec keeps `shareProcessNamespace=true` (deferred per Harden sidecar/seid trust boundary against /proc-based leakage sei-k8s-controller#221). This is a meta-issue, not a vulnerability in this PR; will reconcile when #221 lands.

Discretionary decisions reviewers ratified

• `OpenKeyring` suffix-strip of trailing `/keyring-file`: all three reviewers accepted as correct ergonomics; documented in docs now.
• Smoke test scope (`kr.List()` only, no specific key-name validation): all three accepted; permitted-empty-keyring posture documented in design Component B.
• 3 × 2s smoke-test retry window: k8s reviewer confirmed it's appropriate for kubelet Secret-mount races (typical race is ~100-500ms; 6s comfortable; chronic infra problems still CrashLoopBackOff in 6s as desired).
• Genesis isolation (generate_gentx continues to use `BackendTest` locally): all three reviewers approved the deliberate separation; the comment at `generate_gentx.go` explaining "why" is load-bearing.

Gates

• `go build ./...` clean
• `go test ./...` all packages pass
• `golangci-lint run --new-from-rev=origin/main ./...` — 0 issues

PR is ready for human review.

[bdchatham]

Addressed Cursor + line comments (commit `bb26ba6`)

Source	Pin	Resolution
Cursor bugbot	`serve.go:158` — passphrase not wiped on unsupported-backend path	Moved `os.Getenv` + `os.Unsetenv` for `SEI_KEYRING_PASSPHRASE` to the very top of `buildExecutionConfig`. Every return path (unset backend, unsupported backend, missing passphrase, OpenKeyring error, smoke-test failure, success) now leaves `/proc//environ` clean.
User	`engine.go:234` — "Any reason to not just export the field?"	Dropped `SetExecutionConfig`/`ExecutionConfig()` getter+setter pair. Now an exported `Engine.Config` field with a doc comment capturing the set-once-before-Submit contract. Call site at `serve.go:127` simplified to `eng.Config = execCfg`.
User	`engine/types.go:92` — "make more concise"	`ExecutionConfig` godoc trimmed from 10 lines to 2 — just the carrier intent + the nil-when-unconfigured field contract.
User	`server/keyring.go:25` — "make all comments more concise"	Tightened every comment in the file: backend aliases (4→2), AllowedBackends (3→1), smoke-test retry (5→2), OpenKeyring godoc (12→4), SmokeTestKeyring godoc (7→4), redactPassphrase (4→2), kr.List() inline (3→1). All WHYs preserved; padding removed. Net −46 lines in the file.

Gates

• `go build ./...` clean
• `go test ./...` all packages pass
• `golangci-lint run --new-from-rev=origin/main ./...` — 0 issues

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit bb26ba6. Configure here.}

[bdchatham]

Addressed Cursor finding (commit `7e8670d`)

"Config write races with rehydration goroutines from NewEngine" — real, well-spotted. Same trap the k8s-specialist reviewer flagged earlier as a deferred-to-#163 follow-up. Locking it in here is cheaper than carrying the future-race forward.

Fix

`NewEngine` was spawning rehydration goroutines as a side effect of construction; the caller then wrote `eng.Config = execCfg` after the goroutines were already live. Today the handlers don't touch `Config` so no race manifested, but #163 will wire sign-tx handlers that read `Config.Keyring` — the race becomes real then.

Made rehydration explicit:

• `NewEngine` is now a pure constructor (no goroutine spawn)
• `RehydrateStaleTasks` is now an exported method the caller invokes after installing `Config`. Doc comment says "must be called only after Config is installed."
• `serve.go:127` now reads:
  ```go
  eng := engine.NewEngine(ctx, handlers, store)
  eng.Config = execCfg
  eng.RehydrateStaleTasks()
  ```
  The `go` statement establishes a happens-before edge with the prior field write, so rehydrated handlers see the installed Config without any synchronization.

Call-site impact

• 1 production call site: `serve.go` (updated)
• 2 tests that depended on auto-rehydration (`engine_test.go` + `engine_e2e_test.go` stale-task cases): updated to call `RehydrateStaleTasks()` explicitly
• 14 other `NewEngine` call sites: unaffected — they use empty stores so rehydration was already a no-op

Gates

• `go build ./...` clean
• `go test ./...` all packages pass
• `go test -race ./sidecar/engine/...` clean
• `golangci-lint run --new-from-rev=origin/main ./...` — 0 issues

Net diff: +15 / -10.