feat: move authentication to Go, behind a feature flag

[pjcdawkins]

Moves authentication from the legacy PHP CLI to Go, behind a feature flag: <PREFIX>GO_AUTH=1 (e.g. UPSUN_CLI_GO_AUTH=1). Without it, the CLI behaves as before. This replaces #195, which made the switch unconditionally.

With the flag on:

• Go stores credentials (system keychain via go-keyring, or files under <writable dir>/auth/) and refreshes tokens under a per-session flock.
• login/auth:browser-login, auth:api-token-login, logout/auth:logout and auth:token are native Go commands, with the legacy options and messages.
• The legacy CLI gets tokens and auth state from Go via a hidden auth:internal command, and Go runs hidden PHP commands for SSH set-up after login (auth:post-login) and a one-time migration of the legacy credentials (auth:export-sessions).
• init uses Go auth.

Switching the flag off again means logging in again with legacy auth, as the migration deletes the legacy copies.

The commits are ordered for review: config, credential store, token refresh, Go commands, the PHP side, tests. On the PHP side the legacy code is unchanged, apart from being moved into methods where it now branches on the flag.

Verification:

• The integration tests run with legacy auth by default and with Go auth if TEST_CLI_GO_AUTH=1 is set; CI runs both.
• Tested manually on:
  • Windows 11 (Credential Manager) and in WSL2 (file storage): see the comment below.
  • MacOS (Intel) which used the keychain as expected.
  • Linux + GNOME on Wayland (including with a locked keyring).

🤖 Generated with Claude Code

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning · 🔵 1 minor point

🔍 Full review · 50 files reviewed

Verification

• Manager.refresh re-reads the store after taking the session flock, and saves rotated tokens before returning, so a rotated refresh token is never sent twice.
• None of the lock holders take the lock again: refresh, Save and logoutOne run migrate before locking, and loggedOut/Revoke take no session lock.
• PHP's legacy keychain key format (slug/<id> with cli- stripped, and slug/<id>/api-token) matches what ExportSessionsCommand parses back into session IDs.
• CLIWrapper.init releases its cache flock before running PHP, so the nested Go→PHP→Go auth:internal→PHP export chain cannot deadlock on it.
• auth:internal writes the login-required JSON as the last stderr line and exits with code 3 through exitError, which matches how GoAuth::run parses it.

The PR adds unit tests (manager_test.go, store_test.go, config/auth_test.go, commands/auth*_test.go) and integration tests (auth_flows_test.go, auth_go_test.go). CI's integration-test job now runs them a second time with TEST_CLI_GO_AUTH=1. No test covers a step-up challenge on init's organization request, so the broken retry in init.go goes unnoticed.

Review details

• Commit: 4154949
• Model: claude-opus-5-5

---

Review 1 of 10 for this pull request · View the full run

Superseded: the latest Upsun Dispatch review no longer requests changes.

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning

🔁 Incremental · 2 files reviewed

Verification

• keychainErrMu sits in a struct that is only used by pointer (*store.Store in manager.go and in the tests), so the mutex is never copied.
• Remembering failures stops a Forget followed by Save from prompting again: the first save's keychainSet returns the remembered error, so the session falls back to BackendFile.
• Because the error is remembered under the mutex, a new keychain call can no longer start while a call abandoned by a non-wait timeout is still running.

The diff adds TestStore_KeychainFailureRemembered and TestStore_FirstSaveAfterKeychainFailure. Neither covers an entry-level error such as ErrSetDataTooBig followed by a load of a different keychain session. The Go tests run in the CI workflow (.github/workflows/ci.yml).

Review details

• Commits: 68352b8..2460fd0
• Model: claude-opus-5-5

---

Review 4 of 10 for this pull request · View the full run

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning

🔁 Incremental · 2 files reviewed

Verification

• secretServiceAvailable uses SessionBusPrivateNoAutoStartup, so the Linux check never spawns a new session bus.
• The new test unsets SNAP_CONTEXT, container and DOCKER_IP when a case does not set them, so a CI runner's environment cannot leak into a case.
• KeychainSupported is only reached through UseKeychain in manager.go, so the relaxed check affects only where new sessions are first saved.

This pass adds TestKeychainSupported_Linux. Its expected value depends on the host's Secret Service, so on CI it most likely only checks the environment-variable exclusions. No test covers a locked or prompt-requiring keyring, and the PR description says the keychain still needs manual testing.

Review details

• Commits: b00c796..3e7d3b6
• Model: claude-opus-5-5

---

Review 6 of 10 for this pull request · View the full run

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning

🔁 Incremental · 5 files reviewed

Verification

• keychainUsableOnLinux returns false for a locked or merely activatable Secret Service with no DISPLAY or WAYLAND_DISPLAY, so a plain SSH session falls back to files.
• The GetProperty failure path is safe: v.Value() on a zero Variant returns nil, and the comma-ok type assertion then gives unlocked=false.
• keychain() now stops remembering keyring.ErrSetDataTooBig as well as ErrNotFound, so one oversized entry no longer disables the keychain for other sessions.
• api.Error now has Unwrap, and go.mod pins golang.org/x/oauth2 back to v0.37.0.

The new TestKeychainUsableOnLinux, TestHasDisplay and TestKeychainSupported_Container unit-test the pure decision function and the env checks. Nothing tests secretServiceStatus against a real or mock D-Bus, or the first Save now waiting with no time limit.

Review details

• Commits: 3e7d3b6..6c6f33f
• Model: claude-opus-5-5

---

Review 7 of 10 for this pull request · View the full run

[upsun-dispatch]

Warning

Changes suggested — 🟡 2 warnings

🔁 Incremental · 2 files reviewed

Verification

• keychain(false, …) maps to limit 0 and keychain(true, …) maps to limit -1, so existing reads and later writes behave as before this push.
• The bounded first write now falls back to a file after firstWriteTimeout, so Save no longer waits without limit on a new session.
• The onLate callback runs only when the abandoned call later returns a nil error, which TestStore_KeychainLimit exercises.

TestStore_KeychainLimit (added in this push) covers the limited wait and the late clean-up callback. Nothing tests the late write racing other processes, or a write that outlives the process. Both are left to the Go test job in .github/workflows/ci.yml.

Review details

• Commits: 6c6f33f..92a04a5
• Model: claude-opus-5-5

---

Review 8 of 10 for this pull request · View the full run

[upsun-dispatch]

Warning

Changes suggested — 🟡 2 warnings

🔁 Incremental · 6 files reviewed

Verification

• v.Value() on the zero dbus.Variant returned with a GetProperty error is nil, so secretServiceUnlocked returns false without panicking.
• Load, Save and Delete all use sf.Account for sessions in the keychain, so they agree on which item they read, write and remove.
• An abandoned first write now goes to a unique account, so it cannot overwrite the secret of a session saved later under the same id.
• The first write uses keychain(false, …), so it is bounded by KeychainTimeout and falls back to a file. Updates to existing keychain sessions still wait while the lock is held.

TestStore_KeychainAccountPerSession was added: it covers the per-session account and that a stray write under the bare id is ignored. Nothing tests the late-write path after a first-write timeout, or secretServiceUnlocked itself. The Linux display tests were removed, and only the container-env test remains.

Review details

• Commits: 92a04a5..fc61dcc
• Model: claude-opus-5-5

---

Review 9 of 10 for this pull request · View the full run

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning · ⚪ 1 nitpick

🔁 Incremental · 3 files reviewed

⚪ Nitpick

• internal/auth/store/store.go:294 — keychainCall repeats the whole body of keychain: the lock, the remembered-error check, and the rule for which errors are entry errors. keychain could simply return s.keychainCall(wait, fn, nil). As written, the two copies of the error-classification rule can drift apart.

Verification

• keyringCollection picks collection/login when the Service's Collections property lists it, otherwise aliases/default. That matches go-keyring's GetLoginCollection fallback.
• The onLate goroutine captures set/del locally, so the test's restoring of setSecret/deleteSecret cannot race with it.
• A file-mode session's Account survives re-saves (newFile.Account = sf.Account), so a later Delete still removes a late keychain write.
• Save records the account only when the error is errKeychainTimeout, not when it is ErrSetDataTooBig or another immediate failure.

The new TestStore_FirstSaveTimeout covers the in-process onLate deletion and the deletion of a recorded account by a later Delete. The Linux collection selection in keyringCollection has no test that touches D-Bus. CI runs the Go tests through .github/workflows/ci.yml.

Review details

• Commits: fc61dcc..67b1106
• Model: claude-opus-5-5

---

Review 10 of 10 for this pull request · View the full run

[pjcdawkins]

@upsun-dispatch review

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔁 Incremental · 2 files reviewed

🔵 Minor point

• internal/auth/store/store.go:304 — keychainCleanUp discards every failure, timeouts included, so the error is never stored in s.keychainErr. A timeout here usually means the keychain is locked and an unlock prompt is pending, which is exactly the case keychainErr exists for (its comment: "each try could prompt the user"). Example: logout → DeleteAll while the keychain is locked. The first file-mode session with Account set blocks for 10s on its clean-up delete and gives up. Each later such session blocks for another 10s and may prompt again. A keychain-backed session after it (ids are sorted) then calls keychain(true, …), which prompts again and waits with no upper bound. Before this change that session would have failed fast with the remembered error. Not remembering entry errors and hard failures is fine. A timeout should still be remembered, or at least end the clean-ups for the rest of the process.

Verification

• keychain now just calls keychainCall(wait, fn, nil), so there is one copy of the lock, the remembered-error check and the entry-error rule.
• keychainCleanUp takes keychainErrMu before calling keychainCallLocked, so it is serialized with other keychain calls and never writes keychainErr.
• Delete's file-mode branch for a recorded Account is now the only caller of keychainCleanUp, and the error it ignores is no longer stored in keychainErr.

The new test TestStore_CleanupFailureNotRemembered covers a clean-up that fails immediately and checks that keychainErr stays nil. No test covers a clean-up that times out. The Go unit tests run in CI (.github/workflows/ci.yml).

Review details

• Commits: 67b1106..2320ced
• Model: claude-opus-5-5

---

View the full run

[pjcdawkins]

Tested manually on Windows 11 (native, and WSL2), with UPSUN_CLI_GO_AUTH=1 unless noted.

Native Windows (PowerShell):

• Legacy login without the flag, then a PHP command with the flag: the session was migrated to Credential Manager (upsun-cli-auth:default-<suffix>), the session file holds only backend and account, the legacy credential was deleted and the SSH certificate kept.
• PHP commands with Go tokens: environments, ssh-cert:info, auth:info.
• Refresh: 4 Go (auth:token) and 4 PHP (auth:info) processes in parallel, within the 2-minute refresh window. The token was refreshed once; all 8 got the same new token, with no errors.
• logout: deleted the Credential Manager entry, the session file and the session's SSH key, certificate and config.
• auth:token when logged out offered a browser login; login then stored the session under a new keychain account.

WSL2 (Ubuntu 24.04, no Secret Service): login used file storage (auth/default.json, mode 600, in a 700 directory), PHP commands and the SSH config prompt worked, and logout removed the session files.

🤖 Generated with Claude Code

[pjcdawkins]

@upsun-dispatch review