diff --git a/cmd/sherlock/main.go b/cmd/sherlock/main.go index a90140b..2e7c623 100644 --- a/cmd/sherlock/main.go +++ b/cmd/sherlock/main.go @@ -57,19 +57,19 @@ func main() { return } - if err := keyring.Probe(); err != nil { + store, err := keyring.Open() + if err != nil { fmt.Fprintln(os.Stderr, "sherlock:", err) - fmt.Fprintln(os.Stderr, "Hint:", keyring.RemediationHint()) os.Exit(exitKeyring) } switch sub { case "login": - runLogin(rest) + runLogin(store, rest) case "logout": - runLogout(rest) + runLogout(store, rest) case "status": - runStatus(rest) + runStatus(store, rest) case "run": if len(rest) == 0 { fmt.Fprintln(os.Stderr, "sherlock: run requires an agent name") @@ -122,7 +122,7 @@ func loadAuthnConfig() authn.Config { } } -func runLogin(args []string) { +func runLogin(store keyring.Store, args []string) { fs := flag.NewFlagSet("login", flag.ExitOnError) timeout := fs.Duration("timeout", 5*time.Minute, "how long to wait for the browser callback") _ = fs.Parse(args) @@ -154,23 +154,22 @@ func runLogin(args []string) { fmt.Fprintln(os.Stderr, "sherlock: login:", err) os.Exit(exitAuth) } - if err := keyring.NewStore().SetAuthentikTokens(res.Tokens); err != nil { + if err := store.SetAuthentikTokens(res.Tokens); err != nil { fmt.Fprintln(os.Stderr, "sherlock: persist tokens:", err) os.Exit(exitKeyring) } fmt.Printf("Logged in as %s <%s>.\n", res.Tokens.Name, res.Tokens.Email) } -func runLogout(_ []string) { - if err := keyring.NewStore().ClearAuthentikTokens(); err != nil { +func runLogout(store keyring.Store, _ []string) { + if err := store.ClearAuthentikTokens(); err != nil { fmt.Fprintln(os.Stderr, "sherlock: logout:", err) os.Exit(exitKeyring) } fmt.Println("Logged out.") } -func runStatus(_ []string) { - store := keyring.NewStore() +func runStatus(store keyring.Store, _ []string) { ts, err := store.GetAuthentikTokens() switch { case errors.Is(err, keyring.ErrNoTokens): diff --git a/docs/agents.md b/docs/agents.md index c00deb1..0a92611 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -25,7 +25,7 @@ bare alias is the daily-driver form. ## What sherlock does per spawn -1. `keyring.Probe()` — fail fast if the OS keyring isn't available. +1. `keyring.Open()` — fail fast if the OS keyring isn't available (returns `*UnavailableError` with a remediation `Hint` field). 2. Resolve the agent binary on `$PATH`. Friendly error if missing. 3. Render the per-agent MCP config to `$XDG_RUNTIME_DIR/sherlock/.mcp.json` (0600). In Phase 1 the diff --git a/docs/storage.md b/docs/storage.md index 22fa35e..1365a58 100644 --- a/docs/storage.md +++ b/docs/storage.md @@ -7,7 +7,7 @@ How sherlock persists secrets and runtime state. | Topic | Decision | |---|---| | Token storage | OS keyring via [`github.com/zalando/go-keyring`](https://github.com/zalando/go-keyring). No on-disk credential files, no `age` blobs, no plaintext. | -| Pre-flight | `sherlock` calls `keyring.Probe()` at startup of every subcommand. A missing/locked keyring fails fast with a platform-appropriate hint and exit code `3`. | +| Pre-flight | The only constructor is `keyring.Open()`, which probes the keyring before returning a Store. There is no probe-less escape hatch. A missing/locked keyring returns `*UnavailableError` (with a populated `Hint` field) and exits 3. | | Service name | `sherlock` for real tokens; `sherlock-preflight` for the probe sentinel. | | Account key | The Authentik `sub` claim once logged in, `default` before. | | Runtime files | `$XDG_RUNTIME_DIR/sherlock.sock`, `sherlock.pid`, `sherlock.log`, `sherlock/.mcp.json`. All `0600` (files) / `0700` (dirs). | @@ -37,19 +37,19 @@ secrets. ## Pre-flight semantics -`keyring.Probe()`: +`keyring.Open()`: -1. Writes a fixed sentinel value under service `sherlock-preflight`, - account `probe`. -2. Reads it back, verifies the round-trip. -3. Deletes it. +1. Probes the keyring: writes a fixed sentinel value under service + `sherlock-preflight` / account `probe`, reads it back, deletes it. +2. On success: returns a live `Store` backed by the OS keyring. +3. On failure: returns `*keyring.UnavailableError` whose `Cause` field + wraps the underlying error and whose `Hint` field carries a + per-OS one-line remediation. `Error()` includes both, so callers + normally just print the error and move on; `keyring.IsUnavailable(err)` + is the type-predicate for branching. -If any step fails the call returns `keyring.ErrUnavailable` wrapping -the underlying cause. `keyring.IsUnavailable(err)` is the test; -`keyring.RemediationHint()` returns a per-OS one-liner. - -CLI behaviour: any failure prints the wrapped error and the hint, then -exits with code `3`. +CLI behaviour: any failure prints the error (which already includes +the hint) and exits with code `3`. ## Platform notes diff --git a/internal/keyring/keyring.go b/internal/keyring/keyring.go index 14301f7..84b5151 100644 --- a/internal/keyring/keyring.go +++ b/internal/keyring/keyring.go @@ -4,10 +4,11 @@ // Linux, Keychain on macOS, Credential Manager on Windows). No // plaintext or age-encrypted blobs on disk. // -// The CLI MUST call Probe at startup and exit early (with -// platform-specific guidance) if the keyring is unavailable. Without -// the keyring, refresh tokens would be lost on every command — better -// to fail loudly. +// Open is the only public way to obtain a Store; it probes the OS +// keyring and fails fast with platform-specific guidance if the +// keyring is missing or locked. There is deliberately no escape hatch +// that constructs a Store without probing — without the keyring, +// sherlock would silently lose refresh tokens on every command. package keyring import ( @@ -21,7 +22,7 @@ import ( ) // Service is the keyring "service" namespace under which sherlock -// stores secrets. Per-key labels live in TokenSet's helpers. +// stores secrets. Per-key labels live as untyped constants below. const Service = "sherlock" const ( @@ -32,56 +33,84 @@ const ( keyAuthentikTokens = "authentik-tokens" ) -// ErrUnavailable is returned by Probe when the OS keyring is missing -// or non-responsive. Treat it as fatal; the CLI surfaces an actionable -// hint per platform. -type ErrUnavailable struct{ Cause error } - -func (e *ErrUnavailable) Error() string { - return fmt.Sprintf("os keyring unavailable: %v", e.Cause) +// UnavailableError is returned by Open when the OS keyring is missing +// or non-responsive. Hint is platform-specific remediation text, +// populated by the constructor — callers don't need to look it up +// themselves, just read e.Hint or print e.Error() (which includes it). +type UnavailableError struct { + Cause error + Hint string } -func (e *ErrUnavailable) Unwrap() error { return e.Cause } +func (e *UnavailableError) Error() string { + if e.Hint == "" { + return fmt.Sprintf("os keyring unavailable: %v", e.Cause) + } + return fmt.Sprintf("os keyring unavailable: %v\n hint: %s", e.Cause, e.Hint) +} -// IsUnavailable is the convenience predicate over ErrUnavailable. +func (e *UnavailableError) Unwrap() error { return e.Cause } + +// IsUnavailable reports whether err wraps an *UnavailableError. func IsUnavailable(err error) bool { - var u *ErrUnavailable + var u *UnavailableError return errors.As(err, &u) } -// RemediationHint returns a single-line, platform-aware hint that the -// CLI prints when Probe fails. -func RemediationHint() string { +// newUnavailable wraps cause with the per-OS remediation hint. +func newUnavailable(cause error) *UnavailableError { + return &UnavailableError{Cause: cause, Hint: hintForGOOS()} +} + +// hintForGOOS returns a single-line, platform-aware remediation hint. +// Internal — exposed only through UnavailableError.Hint. +func hintForGOOS() string { switch runtime.GOOS { case "linux": - return "Linux: install/start a Secret Service provider (gnome-keyring, KWallet, or keepassxc with Secret Service enabled). See https://specifications.freedesktop.org/secret-service/latest/." + return "install/start a Secret Service provider (gnome-keyring, KWallet, or keepassxc with Secret Service enabled). See https://specifications.freedesktop.org/secret-service/latest/." case "darwin": - return "macOS: the system Keychain must be unlocked. Check `security list-keychains`." + return "the system Keychain must be unlocked. Check `security list-keychains`." case "windows": - return "Windows: the Credential Manager service must be running." + return "the Credential Manager service must be running." default: - return runtime.GOOS + ": no Secret Service implementation is wired in sherlock for this OS." + return "no Secret Service implementation is wired in sherlock for " + runtime.GOOS } } -// Probe asserts that the OS keyring is reachable by performing a -// write+read+delete of a sentinel value. Returns *ErrUnavailable on -// any failure. Safe to call repeatedly. -func Probe() error { +// Open is the only way to obtain a Store. It probes the OS keyring +// (write+read+delete of a sentinel value) and returns the live Store +// on success, or *UnavailableError on any failure. +// +// We deliberately do not expose a constructor that skips the probe. +// Every production code path needs the fail-fast guarantee, and an +// "I know what I'm doing" escape hatch would invite real bugs where +// refresh tokens silently vanish into a dead keyring. Tests that need +// a Store without a real keyring use the in-memory keyring/fake.Store. +// +// Safe to call repeatedly. Each call performs a fresh probe; the +// returned Store carries no per-instance state. +func Open() (Store, error) { + if err := probe(); err != nil { + return nil, err + } + return realStore{}, nil +} + +func probe() error { if err := gokeyring.Set(probeService, probeAccount, probeValue); err != nil { - return &ErrUnavailable{Cause: fmt.Errorf("set: %w", err)} + return newUnavailable(fmt.Errorf("set: %w", err)) } got, err := gokeyring.Get(probeService, probeAccount) if err != nil { _ = gokeyring.Delete(probeService, probeAccount) - return &ErrUnavailable{Cause: fmt.Errorf("get: %w", err)} + return newUnavailable(fmt.Errorf("get: %w", err)) } if got != probeValue { _ = gokeyring.Delete(probeService, probeAccount) - return &ErrUnavailable{Cause: fmt.Errorf("readback mismatch: %q", got)} + return newUnavailable(fmt.Errorf("readback mismatch: %q", got)) } if err := gokeyring.Delete(probeService, probeAccount); err != nil { - return &ErrUnavailable{Cause: fmt.Errorf("delete: %w", err)} + return newUnavailable(fmt.Errorf("delete: %w", err)) } return nil } @@ -95,7 +124,7 @@ type TokenSet struct { AccessToken string `json:"access_token,omitempty"` RefreshToken string `json:"refresh_token,omitempty"` IDExpiresAt time.Time `json:"id_expires_at"` - RefreshExpAt time.Time `json:"refresh_expires_at,omitempty"` + RefreshExpAt time.Time `json:"refresh_expires_at"` Issuer string `json:"issuer"` ClientID string `json:"client_id,omitempty"` Scopes []string `json:"scopes,omitempty"` @@ -107,25 +136,22 @@ type TokenSet struct { // Empty reports whether the TokenSet carries no usable session. func (t TokenSet) Empty() bool { return t.IDToken == "" && t.RefreshToken == "" } -// Store is the typed wrapper sherlock uses. Implementations are -// expected to be safe for concurrent use. +// Store is the typed wrapper sherlock uses. Implementations must be +// safe for concurrent use. type Store interface { GetAuthentikTokens() (TokenSet, error) SetAuthentikTokens(TokenSet) error ClearAuthentikTokens() error } -// realStore is the production implementation backed by zalando/go-keyring. -type realStore struct{} - -// NewStore returns the real keyring-backed Store. Call Probe before -// constructing a Store in production code paths. -func NewStore() Store { return &realStore{} } - // ErrNoTokens is returned by GetAuthentikTokens when nothing is stored. var ErrNoTokens = errors.New("keyring: no authentik tokens stored") -func (r *realStore) GetAuthentikTokens() (TokenSet, error) { +// realStore is the production Store backed by zalando/go-keyring. +// Unexported on purpose — callers go through Open. +type realStore struct{} + +func (realStore) GetAuthentikTokens() (TokenSet, error) { raw, err := gokeyring.Get(Service, keyAuthentikTokens) if err != nil { if errors.Is(err, gokeyring.ErrNotFound) { @@ -140,7 +166,7 @@ func (r *realStore) GetAuthentikTokens() (TokenSet, error) { return ts, nil } -func (r *realStore) SetAuthentikTokens(ts TokenSet) error { +func (realStore) SetAuthentikTokens(ts TokenSet) error { b, err := json.Marshal(ts) if err != nil { return fmt.Errorf("keyring: encode tokens: %w", err) @@ -151,7 +177,7 @@ func (r *realStore) SetAuthentikTokens(ts TokenSet) error { return nil } -func (r *realStore) ClearAuthentikTokens() error { +func (realStore) ClearAuthentikTokens() error { if err := gokeyring.Delete(Service, keyAuthentikTokens); err != nil { if errors.Is(err, gokeyring.ErrNotFound) { return nil diff --git a/internal/keyring/keyring_test.go b/internal/keyring/keyring_test.go index e0e7d2d..c0a0d9e 100644 --- a/internal/keyring/keyring_test.go +++ b/internal/keyring/keyring_test.go @@ -3,33 +3,41 @@ package keyring import ( "errors" "os" + "strings" "testing" ) -func TestRemediationHint_NonEmpty(t *testing.T) { - if RemediationHint() == "" { - t.Fatal("empty hint") +func TestUnavailableError_HintPopulated(t *testing.T) { + err := newUnavailable(errors.New("boom")) + if err.Hint == "" { + t.Fatal("Hint should be populated by constructor") + } + if !strings.Contains(err.Error(), err.Hint) { + t.Fatalf("Error() should include Hint:\n %s\n hint=%q", err.Error(), err.Hint) } } func TestIsUnavailable(t *testing.T) { - var err error = &ErrUnavailable{Cause: errors.New("boom")} - if !IsUnavailable(err) { - t.Fatal("IsUnavailable false for *ErrUnavailable") + if !IsUnavailable(newUnavailable(errors.New("boom"))) { + t.Fatal("IsUnavailable false for *UnavailableError") } if IsUnavailable(errors.New("other")) { t.Fatal("IsUnavailable true for plain error") } } -// TestProbe_HappyPath runs against the real OS keyring. It's noisy on +// TestOpen_HappyPath runs against the real OS keyring. It's noisy on // machines without a session keyring (e.g. CI), so we gate it on an // explicit opt-in env var. -func TestProbe_HappyPath(t *testing.T) { +func TestOpen_HappyPath(t *testing.T) { if os.Getenv("TEST_KEYRING") == "" { - t.Skip("set TEST_KEYRING=1 to run the live keyring probe") + t.Skip("set TEST_KEYRING=1 to run the live keyring open") } - if err := Probe(); err != nil { - t.Fatalf("Probe failed on a machine that opted into TEST_KEYRING=1: %v", err) + store, err := Open() + if err != nil { + t.Fatalf("Open failed on a machine that opted into TEST_KEYRING=1: %v", err) + } + if store == nil { + t.Fatal("Open returned a nil Store on success") } } diff --git a/plan.md b/plan.md index 265fc28..190696c 100644 --- a/plan.md +++ b/plan.md @@ -234,7 +234,7 @@ Reuses the existing gssh server (`gssh.alexandru.macocian.me`) — sherlock does - **SSH backend:** reuse the existing gssh server. `gssh-mcp` is a thin JWT-authenticated HTTP+WS client; no local SSH-cert work in sherlock. See [docs/gssh-integration.md](docs/gssh-integration.md). - **Wrapper extensibility:** agent-agnostic. Each supported agent CLI (Copilot, Claude Code, Aider, …) is a single Go file under `internal/agent/` that registers itself with the package via `init()`. `sherlock copilot` and `sherlock claude` are sugar for `sherlock run copilot` / `sherlock run claude`. Adding a new agent is a small code change, by design — per-CLI quirks (auth subcommands, MCP config schema, env-var quirks) need a real code home. The TOML-overlay design from the Phase 0 plan was tried in Phase 1 and dropped. See [docs/agents.md](docs/agents.md). - **Docs convention:** `README.md` is TOC only; every topic lives as its own file under `docs/`. New concerns get new files, never appended sections. -- **Token persistence:** OS keyring via `zalando/go-keyring`. Strict pre-flight (`keyring.Probe()`) at startup of every CLI invocation; fail fast with exit code 3 and a per-OS hint if Secret Service / Keychain / Credential Manager is unavailable. See [docs/storage.md](docs/storage.md). +- **Token persistence:** OS keyring via `zalando/go-keyring`. The only public constructor is `keyring.Open()` — it probes the keyring (write+read+delete of a sentinel) and returns the live Store or an `*UnavailableError` whose `Hint` field carries platform-specific remediation. Fails fast with exit code 3 if Secret Service / Keychain / Credential Manager is unavailable. There is no probe-less escape hatch — the API itself enforces the fail-fast invariant. See [docs/storage.md](docs/storage.md). - **Loopback redirect port:** `127.0.0.1:6990` (unassigned per IANA). Bound only for the duration of `sherlock login`. The actual Authentik OAuth2 provider config is deferred — sherlock ships with empty defaults and `sherlock login` returns `not_configured` until `SHERLOCK_AUTHENTIK_ISSUER` / `SHERLOCK_AUTHENTIK_CLIENT_ID` are set. End-to-end flow is exercised by an integration test against a stub Authentik (`internal/authn/login_test.go`). - **No daemon (reverses Phase 1 #9 + #10):** the Phase 1 design had a `sherlock-broker` daemon with UDS RPC (`docs/rpc.md`), JSON-over-newline framing, fork+setsid lifecycle, PID-file flock, and idle-timeout watcher. It was scrapped in the post-Phase-1 refactor — see [docs/architecture.md#why-there-is-no-daemon](docs/architecture.md#why-there-is-no-daemon). Multi-process refresh races are serialised via an exclusive `flock()` on `$XDG_RUNTIME_DIR/sherlock.refresh.lock` in `internal/authn/refresh.go`. Net: -800 LoC, no `internal/{broker,socket,rpc}/` packages, no `cmd/sherlock-broker/` binary.