feat: default coder.useKeyring to true and harden shared credential handling - #1107
feat: default coder.useKeyring to true and harden shared credential handling#1107EhabY wants to merge 2 commits into
Conversation
78489be to
7128c90
Compare
…andling Store session tokens in the OS keyring by default on macOS and Windows, passing --use-keyring explicitly to CLI 2.29 and later. Model the CLI store as shared (the CLI's own directory, or a user directory on 2.31+) or private (a file in the extension's per-deployment directory), and treat CODER_CONFIG_DIR like a user --global-config. Record who minted each stored token so logout runs coder logout against a shared store only for a token the extension created and the CLI still holds. Ask before adopting the CLI's session for a different user. Show an error with Open Settings when the CLI cannot store the token at login, and a Show Output button when logout cannot remove every credential. Closes #1106
7128c90 to
d38afa2
Compare
d38afa2 to
7b0efe7
Compare
The CLI keys keyring entries by host without the scheme, so an http lookup returns the https deployment's token. File reads are left to the CLI, whose file mode checks the stored URL against --url from 2.32.
7b0efe7 to
5000734
Compare
code-asher
left a comment
There was a problem hiding this comment.
Looks good to me. I think maybe we should change whether we also log out the cli, but not blocking imo.
tbh I have not actually tested the keyring, being on Linux and all, but might find some time to run a VM later.
| transport.kind === "keyring" ? "keyring" : "file", | ||
| ); | ||
| await this.cliLogin(transport, url, token, configs, options); | ||
| const cli = await this.resolveCli(url, configs); |
There was a problem hiding this comment.
When I sign in for the first time I get an error popup "failed to store credentials: no cli binary found". Maybe we should ignore ENOENT errors? Same if I log out without having connected once. Nothing is actually wrong at these points.
Arguably if the cli is deleted after a connect, then I could see considering it an error if it disappears, but at the same time we will download and configure it again, so maybe not a big deal.
I also get two log entries about it, one a warning (probably when trying to read the token), and then another after logging in (probably the write). I feel like these should be infos at most and be a bit more descriptive (something like "tried to read token, but the cli has not been downloaded or was deleted") at least if we have not connected once before.
| const token = await this.runTokenRead(transport.binPath, args, options); | ||
| if (!token) { | ||
| // Keyring entries drop the scheme, so an http lookup returns the https token. | ||
| if (cli.auth.useKeyring && !url.startsWith("https:")) { |
There was a problem hiding this comment.
I wonder if this check should be in the cli instead? If it spits out an incorrect token given some input, that feels like a cli bug.
Then again, people probably update the VS Code extension faster so if this is a pressing issue then it might make sense to have it in both places (and I suppose we may need it in Toolbox too).
| span.setProperty("store", cli.auth.store); | ||
| if (!(await this.ownsCliSession(cli, session, signal))) { | ||
| this.logger.info("Kept the CLI session for", url); | ||
| return true; |
There was a problem hiding this comment.
Should be return false I think? Because the doc comment on deleteToken says it returns true if every store was cleared, which is not the case here.
| const confirmed = await this.confirmSignIn( | ||
| deployment.url, | ||
| { | ||
| title: "Sign in with the Coder CLI session?", |
| private async cliLogin( | ||
| transport: CliTransport, | ||
| /** Reads the CLI's token via `coder login token` (CLI 2.32+). Undefined on any failure. */ | ||
| public async readToken( |
There was a problem hiding this comment.
Similar to my other comment, we get a warning in the log if the cli has no session token, wonder if we could suppress this.
Then again, maybe not worth parsing the error output of login token, that does seem kinda janky.
| if (!cli) { | ||
| return { kind: "none" }; | ||
| /** A shared store is ours only if the CLI still holds the token this extension created. */ | ||
| private async ownsCliSession( |
There was a problem hiding this comment.
This is neat but I feel like it is also unpredictable. When a user goes to log out, sometimes it will log out the cli, sometimes not, depending on what they did potentially a long time ago which they may have long forgotten.
Or, it seems possible that a user may log in with VS Code, then rely on that shared token for JetBrains, then one day log out of VS Code and be surprised JetBrains also lost its token, for example.
Maybe we can prompt the user about whether to also clear the token from the shared storage or keyring, warning that anything else relying on the token would also be logged out?
| return ( | ||
| isKeyringSupported() && configs.get<boolean>("coder.useKeyring", false) | ||
| ); | ||
| return isKeyringSupported() && configs.get<boolean>("coder.useKeyring", true); |
There was a problem hiding this comment.
Reasoning out loud about the effects of this change: for users currently logged in, they still get their token from the secret store, and when they connect we store it in the keyring and configure the cli to use the keyring, so they should not be suddenly logged out or anything like that. Does that sound right?
Or in other words, this only affects how we share tokens with the cli, it does not currently impact the plugin's own internal storage/copy of tokens.
The only potential breakage is if the keyring is misconfigured or something, then connecting could start failing despite no changes from the user. But that seems reasonable, and they can fix by opting out, we even have a notification for it.
|
|
||
| ### Changed | ||
|
|
||
| - Store session tokens in the OS keyring by default on macOS and Windows. The |
There was a problem hiding this comment.
The wording to me makes it sound like the keyring is the only place they are stored now but we do still store them in VS Code's secret storage from what I can tell (looking at persistSessionAuth).
Could just be a me issue though lol. But maybe "Additionally store session tokens in the OS keyring by default for use by the cli...does not change how the plugin stores its own tokens" or something
Closes #1106.
What changes
coder.useKeyringdefaults totrue. On macOS and Windows with Coder CLI 2.29 or later, the session token goes to the OS keyring through the CLI. Linux and older CLIs keep using a file.--use-keyring=<bool>, so the setting wins overCODER_USE_KEYRING.--global-configorCODER_CONFIG_DIR) or private (a file in the extension's per-deployment directory). A private store never asks for the keyring; theCliAuthtype enforces it. For a user directory the CLI decides between file and keyring (a file today, the keyring once feat(cli): allow keyring session token storage together with--global-configcoder#29105 lands), and the extension never guesses which.tokenSource(extensionorcli). Sessions stored before this field existed parse asextension.coder logoutagainst a shared store only for a token the extension created and the CLI still holds, verified with onecoder login tokencall on 2.32+; below that the stored provenance is trusted. A session borrowed from the CLI is removed from the extension without signing the CLI out.httpsURLs. The CLI keys them by host with the scheme dropped, so anhttplookup would return thehttpsdeployment's token and send it in cleartext. File reads are left to the CLI, which checks the stored URL against--urlfrom 2.32, sotokenReadmoves from 2.31.0 to 2.32.0 and 2.31 users get the login prompt instead of CLI token pickup.Flags per situation
--global-config <ext dir> --url <url>--url <url> --use-keyring=true--global-config <ext dir> --url <url> --use-keyring=false--global-config <ext dir> --url <url> --use-keyring=false--url <url> --use-keyring=false"Off" includes Linux. A private store drops the user's own
--global-configitem; a shared store keeps it, andCODER_CONFIG_DIRreaches the CLI through the environment.Trade-offs
coder.useKeyring: false.--use-keyringalongside--global-configmoves user-directory setups to keyring storage. Nothing breaks, but a terminal CLI still reading the file needs onecoder login.keyring_tokenlogin method folds intocli_token, and the credential telemetrycategoryattribute becomesstore(sharedorprivate).Testing
pnpm test,pnpm typecheck,pnpm lint, andpnpm formatpass. Developed on Linux; not yet verified on a keyring-capable machine: the shared token showing up incoder login token,CODER_CONFIG_DIRpass-through, and the locked-keychain error.🤖 Generated with Claude Code