Skip to content

reduce macOS keychain prompts for OAuth MCP servers#2580

Merged
dgageot merged 3 commits intodocker:mainfrom
dgageot:board/reducing-keychain-password-prompts-for-m-45b7e857
Apr 28, 2026
Merged

reduce macOS keychain prompts for OAuth MCP servers#2580
dgageot merged 3 commits intodocker:mainfrom
dgageot:board/reducing-keychain-password-prompts-for-m-45b7e857

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Apr 28, 2026

Problem

When using OAuth-protected MCP servers, macOS prompted the user for keychain access many times — once per server on first launch, again on every HTTP request that needed to read a token, and yet again on every token refresh. Even after clicking Always Allow, a rebuild (which changes the binary's signature) brought the prompts back.

The root cause was the storage layout: every token was a separate keychain item, and every keychain item carries its own ACL on macOS.

Fix

Store all OAuth tokens for all MCP servers in a single bundled keychain item, cached in memory after the first read.

  • The OS prompts the user at most once per process, regardless of how many MCP servers are configured.
  • Token reads after the first one are served from memory and don't touch the keychain at all.
  • Token writes (refreshes, new flows) all target the same item the user has already authorised, so a single Always Allow keeps applying.

The store is exposed as a process-wide singleton via sync.OnceValue, so multiple OAuth-capable toolsets share the same in-memory cache and never reopen the keyring on construction.

Backwards compatibility

On first load, any tokens written by the previous one-item-per-token layout are migrated into the bundle, and the legacy entries (including the index) are removed. Migration is best-effort: failures are logged but never block startup.

Public API

Unchanged. NewKeyringTokenStore, OAuthTokenEntry, ListOAuthTokens, and RemoveOAuthToken keep the same signatures, so mcp.go, oauth_login.go, and cmd/root/debug_oauth.go work untouched.

Tests

New tests cover:

  • Reads collapsing to a single underlying keyring Get regardless of how many resource URLs are looked up.
  • All Set calls targeting the single bundle item.
  • Legacy migration (with the legacy index removed).
  • Corrupt-bundle recovery.
  • Denied keychain access not snowballing into repeat prompts.
  • Concurrent access under the race detector.

Validation

  • go build ./...
  • go vet ./...
  • go test ./... ✓ (full suite, including -race)
  • golangci-lint run ./... ✓ (0 issues)

Possible follow-up

Code-sign release builds with a stable Developer ID so macOS recognises the binary across upgrades and never re-prompts after Always Allow. Today's dev/ad-hoc-signed builds change identity on every rebuild, which forces another prompt no matter how good the storage layout is. That's outside this PR's scope.

dgageot added 3 commits April 28, 2026 17:36
Verifies that the mutex protection correctly handles concurrent reads,
writes, and deletes without data races. The test exercises 10 goroutines
performing 100 operations each across 3 different resource URLs.
@dgageot dgageot requested a review from a team as a code owner April 28, 2026 16:05
Copy link
Copy Markdown
Contributor

@aheritier aheritier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tokens will be stored in. a different location after restart but not really worth documenting it

Copy link
Copy Markdown
Contributor

@aheritier aheritier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tokens will be stored in. a different location after restart but not really worth documenting it

@dgageot dgageot merged commit a722363 into docker:main Apr 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants