From 40a779d4a8cd873180ac4573b7e54ec979676e5a Mon Sep 17 00:00:00 2001 From: Alexandru Macocian Date: Sat, 13 Jun 2026 13:06:07 +0200 Subject: [PATCH] Fix login lock --- docs/architecture.md | 2 +- docs/auth-model.md | 12 ++++ internal/authn/ensure.go | 42 ++++++++++---- internal/authn/loginlock.go | 70 ++++++++++++++++++++++++ internal/authn/loginlock_test.go | 94 ++++++++++++++++++++++++++++++++ 5 files changed, 209 insertions(+), 11 deletions(-) create mode 100644 internal/authn/loginlock.go create mode 100644 internal/authn/loginlock_test.go diff --git a/docs/architecture.md b/docs/architecture.md index 3b8384c..e8690de 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -54,7 +54,7 @@ The Phase 1 design had a separate `sherlock-broker` daemon (forked-child, UDS RP Reasoning: -- **No daemon needed.** The OS keyring is already the single source of truth across processes. Cross-process refresh races are solved by an exclusive `flock()` on `$XDG_RUNTIME_DIR/sherlock.refresh.lock` with the canonical "take lock → re-read → maybe refresh" pattern in `internal/authn/refresh.go`. +- **No daemon needed.** The OS keyring is already the single source of truth across processes. Cross-process refresh races are solved by an exclusive `flock()` on `$XDG_RUNTIME_DIR/sherlock.refresh.lock` with the canonical "take lock → re-read → maybe refresh" pattern in `internal/authn/refresh.go`. Concurrent *fresh logins* are serialised by a sibling `flock()` on `$XDG_RUNTIME_DIR/sherlock.login.lock` (`internal/authn/loginlock.go`) so they don't collide on the fixed loopback port. - **No master session.** Caddy + each downstream service have their own Authentik OAuth providers with their own audiences, scopes, and consent screens. A single master token cannot satisfy all of them. Per-service tokens are the right granularity, and per-service tokens are best fetched on demand by the MCP that needs them. - **No `sherlock login` step.** Pre-authenticating to N services up-front for the case where the user only ends up using one of them is wasted browser flows. Lazy-on-first-use is the right default; the wallet caches the rest. diff --git a/docs/auth-model.md b/docs/auth-model.md index ee1ebe2..093c0e0 100644 --- a/docs/auth-model.md +++ b/docs/auth-model.md @@ -37,6 +37,18 @@ The first time an operator runs `sherlock copilot` in a fresh environment, the agent spawns its MCPs; each MCP triggers its own browser flow as it needs one. Subsequent runs are silent until the refresh window opens. +### Concurrent first-use is serialised + +Because the loopback port (`127.0.0.1:6990`) is a single machine-wide resource, +two MCPs hitting their first tool call at the same moment would otherwise both +try to bind it and one would fail with `EADDRINUSE`. `Ensure` wraps the fresh +login in an exclusive flock on `$XDG_RUNTIME_DIR/sherlock.login.lock` (sibling of +the refresh lock, but separate so a long, human-in-the-loop login never blocks a +quick refresh). The flows queue; after the first one authenticates, the rest +reuse the operator's existing Authentik browser session and complete without a +second click. Lock acquisition is context-cancellable, so a waiting flow gives up +cleanly if its request is cancelled. See `internal/authn/loginlock.go`. + ## Token renewal Authentik access tokens are short-lived (the `sherlock-cli` provider issues diff --git a/internal/authn/ensure.go b/internal/authn/ensure.go index 901c078..23c058e 100644 --- a/internal/authn/ensure.go +++ b/internal/authn/ensure.go @@ -13,7 +13,9 @@ import ( // override the browser-open behaviour. type EnsureOptions struct { // LockPath is the cross-process flock file used to serialise - // refreshes (and, in the future, fresh OAuth flows). Required. + // refreshes. The sibling login lock (same directory) serialises + // fresh OAuth flows so concurrent first-use logins don't collide on + // the fixed loopback port. Required. LockPath string // Login lets the operator know what URL their browser is opening. @@ -72,15 +74,35 @@ func Ensure(ctx context.Context, store keyring.Store, service string, cfg Config // refresh token, …) also forces a fresh flow. } - res, err := Login(ctx, cfg, LoginOptions{ - OnAuthURL: opts.OnAuthURL, - Discoverer: opts.Discoverer, + // Fresh login. Serialise across processes: Login binds the fixed + // loopback port (127.0.0.1:6990) for the OAuth callback, so two MCPs + // hitting their first tool call at once would otherwise collide with + // EADDRINUSE. The login lock lets them queue; the second flow reuses + // the operator's existing Authentik browser session, so it's instant. + var out keyring.TokenSet + lockErr := withLoginLock(ctx, loginLockPathFor(opts.LockPath), func() error { + // Re-check under the lock: a concurrent flow may have already + // authenticated this same service while we waited. + if fresh, e := EnsureFresh(ctx, store, service, EnsureFreshOptions{ + LockPath: opts.LockPath, + Discoverer: opts.Discoverer, + }); e == nil { + out = fresh + return nil + } + + res, e := Login(ctx, cfg, LoginOptions{ + OnAuthURL: opts.OnAuthURL, + Discoverer: opts.Discoverer, + }) + if e != nil { + return e + } + out = res.Tokens + return store.Set(service, res.Tokens) }) - if err != nil { - return keyring.TokenSet{}, err + if lockErr != nil { + return out, lockErr } - if err := store.Set(service, res.Tokens); err != nil { - return res.Tokens, err - } - return res.Tokens, nil + return out, nil } diff --git a/internal/authn/loginlock.go b/internal/authn/loginlock.go new file mode 100644 index 0000000..dff552b --- /dev/null +++ b/internal/authn/loginlock.go @@ -0,0 +1,70 @@ +//go:build unix + +package authn + +import ( + "context" + "fmt" + "os" + "path/filepath" + "syscall" + "time" +) + +// loginLockPoll is how often withLoginLock retries the non-blocking +// flock while another process holds it. Small enough to feel instant +// once the holder releases, large enough not to spin the CPU while a +// human is clicking through a browser login. +const loginLockPoll = 200 * time.Millisecond + +// loginLockName is the lock file that serialises fresh OAuth flows +// across processes. It lives beside the refresh lock (same dir) but is +// a *separate* lock: refreshes are short and must not be blocked by a +// long, human-in-the-loop login holding the loopback port. +const loginLockName = "sherlock.login.lock" + +// loginLockPathFor derives the login-lock path from the refresh-lock +// path (a sibling in the same runtime dir). Keeping it derived means +// MCP call sites don't have to thread a second path through. +func loginLockPathFor(refreshLockPath string) string { + return filepath.Join(filepath.Dir(refreshLockPath), loginLockName) +} + +// withLoginLock runs fn while holding an exclusive, cross-process flock +// on path. Only one fresh OAuth flow runs at a time machine-wide, so +// concurrent first-use logins from different MCPs don't collide on the +// fixed loopback port (127.0.0.1:6990). +// +// Acquisition is cancellable: because a login can block on a human, we +// poll a non-blocking LOCK_EX|LOCK_NB and honour ctx between attempts +// rather than parking forever in a blocking Flock. The first waiter in +// usually wins; ordering beyond that is not guaranteed (nor needed). +func withLoginLock(ctx context.Context, path string, fn func() error) error { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return fmt.Errorf("authn: mkdir login-lock dir: %w", err) + } + f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return fmt.Errorf("authn: open login lock: %w", err) + } + defer func() { _ = f.Close() }() + + for { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + break + } + if err != syscall.EWOULDBLOCK { + return fmt.Errorf("authn: flock login lock: %w", err) + } + // Held by another process — wait, but stay cancellable. + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(loginLockPoll): + } + } + defer func() { _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) }() + + return fn() +} diff --git a/internal/authn/loginlock_test.go b/internal/authn/loginlock_test.go new file mode 100644 index 0000000..63bbe08 --- /dev/null +++ b/internal/authn/loginlock_test.go @@ -0,0 +1,94 @@ +//go:build unix + +package authn + +import ( + "context" + "errors" + "sync" + "testing" + "time" +) + +func TestWithLoginLock_SerialisesAndRuns(t *testing.T) { + lock := t.TempDir() + "/sherlock.login.lock" + + var mu sync.Mutex + active := 0 + maxActive := 0 + var ran int + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _ = withLoginLock(context.Background(), lock, func() error { + mu.Lock() + active++ + if active > maxActive { + maxActive = active + } + ran++ + mu.Unlock() + + time.Sleep(10 * time.Millisecond) // hold the lock briefly + + mu.Lock() + active-- + mu.Unlock() + return nil + }) + }() + } + wg.Wait() + + if ran != 8 { + t.Fatalf("fn ran %d times, want 8", ran) + } + if maxActive != 1 { + t.Fatalf("max concurrent critical-section entries = %d, want 1 (lock not mutually exclusive)", maxActive) + } +} + +func TestWithLoginLock_PropagatesFnError(t *testing.T) { + lock := t.TempDir() + "/sherlock.login.lock" + sentinel := errors.New("boom") + err := withLoginLock(context.Background(), lock, func() error { return sentinel }) + if !errors.Is(err, sentinel) { + t.Fatalf("err = %v, want sentinel", err) + } +} + +func TestWithLoginLock_CancellableWhileHeld(t *testing.T) { + lock := t.TempDir() + "/sherlock.login.lock" + + // Hold the lock in a background goroutine until told to release. + holding := make(chan struct{}) + release := make(chan struct{}) + go func() { + _ = withLoginLock(context.Background(), lock, func() error { + close(holding) + <-release + return nil + }) + }() + <-holding // ensure the lock is held + + // A second acquisition with a cancellable context must give up when + // the context is cancelled, not block forever. + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + start := time.Now() + err := withLoginLock(ctx, lock, func() error { + t.Fatal("fn should not run: lock is held by another goroutine") + return nil + }) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("err = %v, want context.DeadlineExceeded", err) + } + if time.Since(start) > time.Second { + t.Fatal("acquisition did not bail out promptly on cancellation") + } + close(release) +}