Make miren login ephemeral by default#968
Conversation
Logging in registered a fresh ed25519 key with the cloud every time, which is great for long-lived sessions but puts real load on the backend for a feature most people never use. The cloud already grew the other half of the answer (the device flow now returns a refresh token, and /auth/refresh rotates a pair), so the client needs a way to hold a token instead of a key. Adds a "token" identity carrying an access and refresh token, and a single TokenForIdentity seam that resolves any identity to a bearer token: challenge-response for keypair, cached-or-refreshed for token. The refresh endpoint consumes the token it is handed, so two miren processes refreshing at once would leave one holding a spent token and a spurious logout. Refreshes therefore serialize on a file lock and re-read the identity inside it, which turns the loser of a race into a no-op instead of a 401. Only a definitive 401 means re-login; a 5xx or a network blip keeps the existing tokens rather than stampeding every CLI user into re-authenticating at once. Two incidental fixes fell out along the way: SetLeafConfig was dropping the Keys map when building its in-memory view, and leaf configs were written non-atomically, which is a torn read waiting to happen now that they are rewritten on a token refresh.
Eight call sites had each grown their own copy of "load the keypair, mint a JWT, use it as a bearer token", and several hard-rejected any identity that was not a keypair. Adding a token identity without touching them would have broken whoami, doctor, cluster switch and cluster add for anyone logging in fresh, so they all now go through TokenForIdentity and accept both kinds. With that in place, login stores the device flow tokens instead of registering a key. --persistent-key opts back into the old dance, and an identity that already has a key keeps it on re-login so we never silently orphan a key the server still knows about. If the cloud is old enough that it returns no refresh token, login falls back to registering a key rather than saving credentials that cannot renew. Logout now revokes the refresh token before deleting the local files, best effort, since a copied config would otherwise keep renewing for a week after the user thought they had logged out. A 401 also stops blaming permissions for what is usually just an expired session.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThe change adds typed token identity storage and shared token acquisition, including refresh rotation, persistence, locking, freshness checks, and revocation. Device-flow login now supports ephemeral token identities, persistent keys, and fallback when refresh tokens are absent. Cluster discovery, RBAC checks, debugging, diagnosis, and user lookup use the shared authentication path. Logout revokes refresh tokens best-effort, configuration writes are atomic, and related tests and documentation are added. Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit:
Review: Make miren login ephemeral by default
This is a well-designed change that introduces a new "token" identity type with automatic JWT refresh, cross-process locking, and atomic file writes. The core design decisions are sound, the test suite is genuinely good (the subprocess-based concurrency test for the single-use refresh token is particularly useful), and the code comments explain the less obvious choices clearly. That said, there are a couple of concrete issues worth knowing about before merging.
Correctness issue: persistIdentityTokens reads-then-writes under the lock, but readIdentityFromFile is a separate pass
In token.go, tokenForTokenIdentity holds the flock, re-reads the identity via readIdentityFromFile, and then calls persistIdentityTokens — which reads the file a second time before writing. That's three disk reads total per locked refresh, and between read 2 (inside readIdentityFromFile) and read 3 (inside persistIdentityTokens), a concurrent writer that isn't using the lock (e.g. miren login --force) could change the file. The comment says "the caller must hold the lock", but that invariant is only partially enforced. Minor, but worth noting — the locked re-read already validated freshness, so persistIdentityTokens only needs a write, not another full unmarshal-then-marshal round-trip. Not a blocker by itself, but it's a genuine TOCTOU window inside the locked section.
Real concern: identity in main config file can't refresh
tokenForTokenIdentity calls c.GetIdentitySource(name) and returns an error if it gets "". GetIdentitySource returns "" for identities that live directly in the main config file (not a leaf). The typical install path (saveTokenIdentityToConfig) always writes token identities as leaf configs in clientconfig.d/, so in practice this won't be hit — but a user who manually edits their clientconfig.yaml or who still has a main-config identity from an old version will see cannot locate config file for identity "cloud" on the first token refresh instead of the real auth error. The error message at token.go line 218 doesn't explain this, so it would be confusing.
RevokeRefreshToken called with a possibly-expired access token
In logout.go line 119, identity.Token is passed as the bearer credential. That's the cached access token, which may be expired by the time the user logs out. The server-side revoke endpoint is documented as best-effort and the logout continues regardless, but if the endpoint requires a valid bearer, the revoke will silently fail on every real-world logout (tokens expire). It would be more robust to attempt a refresh before revoking, or to note in the comment that an expired access token is expected here.
Minor: tmp.Chmod may not be portable
atomicWriteFile calls tmp.Chmod(perm) after writing but before renaming. On Linux this is fine. On macOS it's also fine. But (*os.File).Chmod is not guaranteed by the Go spec to be the same as os.Chmod(path, perm) on all platforms (Windows). This is a CLI tool that presumably runs only on Unix, so it's low risk, but using os.Chmod(tmpPath, perm) would be clearer and portable.
What's good
- The flock design — lock file left in place to avoid inode-racing — is correct and the comment explains exactly why.
- The fast-path (no lock, no network) for a still-fresh access token is properly placed before the flock acquisition.
- The 5-minute
tokenExpiryBufferconstant is a named, tested value rather than a magic number. - The backward-compatibility check for an existing keypair identity (so re-login doesn't silently orphan a registered server key) is thoughtful.
- The
TestTokenForIdentityConcurrentRefreshtest covering both goroutines and subprocesses is exactly what you want for a single-use rotating refresh token. - The distinction between
ErrLoginRequired(401 / definitive) and transient errors (5xx / network) is correct and tested.
I'm marking this CAVEATS rather than NOT_READY because the main-config identity issue is an edge case that won't affect the default install path, and the access-token-expiry on logout is best-effort by design. But both are worth addressing before shipping.
🍪 full review note · comment /biscuit review to run biscuit again.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
clientconfig/token.go (1)
230-234: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
TryLockContextfor the refresh lock
flock.Lock()can still block after the caller’s context is canceled or its deadline expires. The refresh path already honorsctx; useTryLockContext(ctx, retryDelay)here so lock acquisition stops on cancellation instead of waiting indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clientconfig/token.go` around lines 230 - 234, Update the refresh-lock acquisition in the token refresh path to call flock’s TryLockContext with the existing ctx and an appropriate retry delay instead of lock.Lock. Preserve the current error wrapping and deferred unlock behavior, while ensuring cancellation or deadline expiry stops acquisition promptly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/commands/login.go`:
- Around line 265-376: Update printUnsavedCredentials and its caller so
--no-save --persistent-key registers the generated or existing public key with
the cloud before printing the private key. Pass the required cloud URL and
access token into printUnsavedCredentials, invoke registerPublicKey using the
keypair and keyName, and return an error if registration fails; leave the
token-credential path unchanged.
In `@cli/commands/logout_revoke_test.go`:
- Around line 47-58: Synchronize request capture in the httptest handler by
replacing the unsynchronized gotAuth and gotRefresh variables with channel-based
delivery or a mutex-backed capture struct. Update the test’s later reads to use
the synchronized mechanism while preserving the existing revokeHits tracking and
captured header/body values.
In `@cli/commands/logout.go`:
- Around line 118-123: Update the revocation branch in the logout command around
RevokeRefreshToken to check that identity.Token is non-empty before reporting
server-side success. Treat a missing access token as a failed or bypassed
revocation, emit the existing warning instead of “Revoked refresh token,” and
preserve best-effort removal of local credentials.
---
Nitpick comments:
In `@clientconfig/token.go`:
- Around line 230-234: Update the refresh-lock acquisition in the token refresh
path to call flock’s TryLockContext with the existing ctx and an appropriate
retry delay instead of lock.Lock. Preserve the current error wrapping and
deferred unlock behavior, while ensuring cancellation or deadline expiry stops
acquisition promptly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dabb3ff8-0023-4679-a1c2-8fa7f574bee0
📒 Files selected for processing (22)
cli/commands/cluster_add.gocli/commands/cluster_switch.gocli/commands/config_bind_helpers.gocli/commands/debug_connection.gocli/commands/doctor.gocli/commands/global.gocli/commands/login.gocli/commands/login_deviceflow_test.gocli/commands/login_ephemeral_test.gocli/commands/logout.gocli/commands/logout_revoke_test.gocli/commands/whoami.goclientconfig/clientconfig.goclientconfig/leafwrite_test.goclientconfig/local.goclientconfig/setleafconfig_keys_test.goclientconfig/token.goclientconfig/token_identity_test.goclientconfig/token_refresh_concurrency_test.goclientconfig/token_test.godocs/docs/command/login.mdgo.mod
The refactor that split login into persist/print paths moved key registration inside the persisted branch, so --no-save --persistent-key printed a private key the cloud had never seen. Register before printing, since an unregistered key cannot authenticate anything. Logout revocation had a similar hole in practice: the stored access token lives an hour and is usually expired by the time anyone logs out, so the revoke call would quietly fail and the refresh token would stay live for its full week. Get a live token first, then revoke whatever refresh token that left current, and only claim success when a request actually succeeded. RevokeRefreshToken now says so rather than returning nil when it has no bearer to authorize with. Also take the refresh lock with the caller context so a cancelled command stops waiting, and synchronize the revoke test capture.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/commands/login_ephemeral_test.go`:
- Around line 162-163: Update the login test assertion near mc.keyBeginHits to
also verify the mock’s key registration completion counter, ensuring the printed
key is fully registered rather than only having the begin endpoint called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b696790-9a5c-477b-b02a-e15a672dd683
📒 Files selected for processing (6)
cli/commands/login.gocli/commands/login_ephemeral_test.gocli/commands/logout.gocli/commands/logout_revoke_test.goclientconfig/token.goclientconfig/token_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/commands/logout.go
- cli/commands/logout_revoke_test.go
- clientconfig/token_test.go
- clientconfig/token.go
- cli/commands/login.go
The mock cloud short-circuited key registration with a 409, so the tests only proved that registration began. Have it return a real envelope and challenge and check the signature comes back, then assert completion too, so the registration tests prove a key is actually usable rather than merely attempted.
|
/biscuit review |
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
Both concerns I raised in my previous review have been genuinely addressed.
Logout revocation — this was the most important fix, and it was done properly rather than just documented. revokeIdentitySession now calls TokenForIdentity first to get a live access token (fast-path if the cached token is still fresh, automatic refresh via /auth/refresh if it isn't), then re-reads the identity from disk to get the refresh token that is actually current (not the one a concurrent refresh may have just spent), and then calls RevokeRefreshToken with a valid bearer. RevokeRefreshToken now also returns an error instead of nil when accessToken == "", which closes the silent-false-success bug CodeRabbit also caught. The new test in logout_revoke_test.go is properly synchronized via a mutex-backed revokeCapture struct and covers both the happy path (correct bearer, correct refresh token) and the failure path (server 500 must not block local logout).
Error message for sourceless identity — the author's pushback was correct. GetIdentitySource does fall back to c.sourcePath for identities in the main config, so main-config token identities work fine. The "" case is genuinely only for in-memory configs that were never loaded from disk (e.g., DecodeConfig on a raw byte slice with no sourcePath), where there's nowhere to write rotated tokens. The new error message ("identity %q has no config file on disk to store refreshed tokens in; run 'miren login' to save it") is clear and actionable. I was wrong on the premise, and I'm satisfied the edge case is now documented rather than silently broken.
The TOCTOU note from my first review (two reads inside the locked section) is still present but remains a minor paper concern — both reads hold the lock, so the only risk is from a writer that deliberately ignores the lock, and persistIdentityTokens needs the full file contents to preserve other fields anyway.
This is ready to merge.
🍪 full review note · comment /biscuit review to run biscuit again.
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
LGTM, nice work — the bits that had to be careful (the single-use refresh race, only-a-401-logs-you-out) are, and the client lines up with the real cloud refresh handler on all of it: the 401/5xx split, single-use rotation, the sliding 7-day window. Bot catches look properly fixed too.
Dropped a handful of inline notes as I passed through. Only the ephemeral naming one is actually asking for anything — the rest are just spots where the identity code's got a bit of an ODOUR (under-typed, half-hydrated, a row of positional bools). None of it bites today and it's all right in the path of the RBAC / App-Auth pass (MIR-891), so leave it for that sweep — I'm flagging in situ mostly since it's what I'd be mumbling out loud if we were talking through this live.
--p+🤖
phinze noted the identity Type was a bare string, so the exhaustive linter could not watch the switches on it; adding "token" meant hand-auditing every arm for the missing-case bug a named type would catch for free. Give it a type and consts. The switches now fail lint if a future identity type forgets an arm. Also rename the one place "ephemeral" leaked to users, the --persistent-key flag help, to "renewable": "ephemeral" collides with the unrelated ephemeral-deploy / ephemeral-version concept. Internal naming is left alone. Regenerated login.md to match.
2 participants
miren loginregistered a brand new ed25519 key with the cloud on every single login. That persistent key mechanism is genuinely nice for long-lived sessions, but it means every routine login writes a new key server-side, and the vast majority of users never need that. It's a lot of backend load for a feature most people don't use.The cloud already grew the other half of the answer: the device flow returns a refresh token now, and
/auth/refreshrotates a pair. So the client just needs to be able to hold tokens instead of a key. Login now stores the access and refresh token it already receives and renews them in the background.--persistent-keyopts back into the old dance if you want it, and an identity that already has a key keeps it on re-login, so nobody's existing setup changes under them.The subtle part, and the bit most worth a reviewer's eye, is that the refresh endpoint consumes the token you hand it. Two
mirenprocesses refreshing at the same moment would leave one holding a spent token and a confusing "access denied" for a user who is perfectly logged in. So refreshes serialize on a file lock and re-read the identity from disk inside it, which turns the loser of a race into a cheap no-op rather than a 401. In the same spirit, only a definitive 401 counts as "session expired". A 5xx or a dropped connection keeps the existing tokens, because mapping a cloud blip to a logout would stampede every CLI user into re-authenticating at once, which is exactly the load we're trying to shed.Getting there needed a refactor first. Eight call sites had each grown their own copy of "load the keypair, mint a JWT, use it as a bearer token", and several outright rejected any identity that wasn't a keypair. Flipping the default without touching them would have broken
whoami,doctor,cluster switchandcluster addfor anyone logging in fresh, so the first commit routes them all through one seam.A couple of other things fell out along the way. Logout now revokes the refresh token before deleting the local files (best effort, it never blocks logout), since a copied config would otherwise keep renewing for a week after you thought you'd logged out. A 401 also stopped telling people to go ask their administrator for access when the real problem is usually just an expired session. And two incidental bugs got fixed:
SetLeafConfigwas silently dropping theKeysmap when building its in-memory view, and leaf configs were written non-atomically, which is a torn read waiting to happen once they're rewritten on every token refresh.Closes MIR-1385