Conversation
EhabY
force-pushed
the
add-keyring-support
branch
3 times, most recently
from
bbe05fe to
f5ec9a3
Compare
On macOS and Windows with CLI >= 2.29.0, write session tokens to the OS
keyring (Keychain / Credential Manager) instead of plaintext files.
The CLI reads from the keyring when invoked with --url instead of
--global-config. Falls back to file storage on Linux, older CLIs,
or if the keyring write fails.
Key changes:
- Add KeyringStore wrapping @napi-rs/keyring with the CLI's credential
format (JSON map keyed by host, base64 on macOS, raw bytes on Windows)
- Add CliAuth discriminated union ("global-config" | "url") threaded
through proxy command building and workspace state machine
- Add shouldUseKeyring() as single source of truth gating on CLI version,
platform, and coder.useKeyring setting
- Restructure remote.ts setup() to call configure() after featureSet is
known, so the keyring decision can be made
- Add keyring read fallback in LoginCoordinator for tokens written by
`coder login` from the terminal
- Add vendor-keyring.mjs build script to copy native binaries into
dist/node_modules/ for VSIX packaging (vsce can't follow pnpm symlinks)
- Harden file fallback with mode 0o600
EhabY
force-pushed
the
add-keyring-support
branch
from
f5ec9a3 to
45383e8
Compare
| const nativePackages = [ | ||
| "keyring-darwin-arm64", | ||
| "keyring-darwin-x64", | ||
| "keyring-win32-arm64-msvc", | ||
| "keyring-win32-x64-msvc", | ||
| ]; |
There was a problem hiding this comment.
To make a universal VSIX, we have to package all of these (x64 and arm64 for macOS and Windows). Potentially we can split this into separate VSIXs (esp. when we have linux which has even more).
The universal VSIX went from 1.02MB to 1.80MB. Almost doubled but still acceptable IMO
| return; | ||
| } catch (error) { | ||
| this.output.warn( | ||
| "Keyring write failed, falling back to file storage", |
There was a problem hiding this comment.
I think we should not silently fallback to plain text storage.
I debated doing the fallback in the CLI, but after considering we should try to be secure by default and looking into what the GitHub CLI did I decided against. Here's two examples: https://redirect.github.com/cli/cli/issues/8954 https://redirect.github.com/cli/cli/issues/10108
Having some explicit feedback to the user about the keyring failure and an explicit way for the user to opt-out of keyring use would be best.
| return ( | ||
| featureSet.keyringAuth && | ||
| isKeyringSupported() && | ||
| vscode.workspace.getConfiguration().get<boolean>("coder.useKeyring", true) |
There was a problem hiding this comment.
Similar functions in this file seem to parameterize this configuration. Should we do the same here?
| try { | ||
| keyringToken = this.keyringStore.getToken(deployment.safeHostname); | ||
| } catch (error) { | ||
| this.logger.warn("Failed to read token from keyring", error); |
There was a problem hiding this comment.
Will this be noisy on Linux? Should we gate keyring use with isKeyringSupported() here?
| public async clearCredentials(safeHostname: string): Promise<void> { | ||
| if (isKeyringSupported()) { | ||
| try { | ||
| this.keyringStore.deleteToken(safeHostname); |
There was a problem hiding this comment.
It seems like we aren't consistent on the host passed to deleteToken() and setToken(). Here it seems like safeHostname has any port stripped, but setToken() in configure() gets the raw URL (host+port). I imagine we would want to always use host+port?
2 participants
Summary
--urlinstead of--global-configso the CLI reads tokens from the keyring0o600permissions) on Linux, older CLIs, or if the keyring write failscoder loginin the terminal are picked up automaticallyHow it works
KeyringStorewraps@napi-rs/keyringusing the exact credential format the CLI expects (JSON map keyed by host, base64 on macOS, raw UTF-8 bytes on Windows)CliAuthdiscriminated union ("global-config"|"url") is threaded through proxy command building and the workspace state machineshouldUseKeyring()gates on CLI version, platform, andcoder.useKeyringsettingremote.tssetup()is restructured soconfigure()runs after the CLI feature set is knownvendor-keyring.mjscopies native binaries intodist/node_modules/at build time for VSIX packaging (vsce can't follow pnpm symlinks)