feat: store auth token in native OS keychain by default#8273
Draft
serhalp wants to merge 1 commit into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e71f961 to
50665d0
Compare
The Netlify auth token is stored in plaintext in the global netlify config file (e.g. `~/Library/Preferences/netlify/config.json` on macOS), which is readable by any process running as the same user. Malicious or compromised tools on developer machines may attempt to steal these tokens. This change adds support for storing and retrieving the token in the OS keychain (macOS Keychain, Windows Credential Manager, Linux Secret Service via libsecret). This change is enabled by default, with an opt-out available (someday we may remove support for plaintext storage). On `netlify login`, the token now goes to the keychain via `@napi-rs/keyring`. If the keychain isn't available for any reason, we fall back to the plaintext file and warn so the user knows. For users who already have a token in the plaintext config, the first time `getToken()` runs in an interactive shell we explicitly prompt them: ``` Your Netlify auth token is currently stored in plaintext at /Users/…/Library/Preferences/netlify/config.json. The CLI can move it to your OS keychain (more secure). Your operating system may prompt you to allow access. ? Move the token to the keychain now? (Y/n) ``` "Yes" migrates and clears the plaintext entry. "No" is persisted (`auth.keychainMigrationDeclined`) so we don't nag again, with a hint to run `netlify logout && netlify login` to revisit at a later time. `NETLIFY_USE_LEGACY_AUTH_STORAGE=1` skips the keychain entirely on both read and write. Something I was concerned about: Agents, scripts, and CI should never see an OS keychain dialog they didn't ask for, as they likely won't be able to respond interactively. Three gates for this: 1. The migration prompt is gated on `isInteractive()` (TTY + not CI). 2. The keychain write in `writeAuthTokenForStorage` is also gated on `isInteractive()`, so even a non-interactive login (unusual but possible; people do all sorts of things in their CI workflows) won't trigger the first-write OS dialog. 3. The keyring library itself fails closed; e.g. on Linux without a daemon, `setPassword` throws `AccessDenied` synchronously and we fall back. I added some output to `netlify status` (both human format and `--json`) that indicates the token storage mechanism and location. I added some telemetry so we can track migration progress: - `user_authTokenStored` (mode, migrated, keychainFailed): fires on login and on successful migration - `user_authTokenRead` (mode): fires when a stored token is returned - `user_authTokenMigrationDeclined`: user explicitly said no
50665d0 to
6a2cb79
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Netlify auth token is stored in plaintext in the global netlify config file (e.g.
~/Library/Preferences/netlify/config.jsonon macOS), which is readable by any process running as the same user. Malicious or compromised tools on developer machines may attempt to steal these tokens.This change adds support for storing and retrieving the token in the OS keychain (macOS Keychain, Windows Credential Manager, Linux Secret Service via libsecret).
This change is enabled by default, with an opt-out available (someday we may remove support for plaintext storage).
Behaviour
On
netlify login, the token now goes to the keychain via@napi-rs/keyring. If the keychain isn't available for any reason, we fall back to the plaintext file and warn so the user knows.For users who already have a token in the plaintext config, the first time
getToken()runs in an interactive shell we explicitly prompt them:"Yes" migrates and clears the plaintext entry. "No" is persisted (
auth.keychainMigrationDeclined) so we don't nag again, with a hint to runnetlify logout && netlify loginto revisit at a later time.NETLIFY_USE_LEGACY_AUTH_STORAGE=1skips the keychain entirely on both read and write.I added some output to
netlify status(both human format and--json) that indicates the token storage mechanism and location.Something I was concerned about: Agents, scripts, and CI should never see an OS keychain dialog they didn't ask for, as they likely won't be able to respond interactively. Three gates for this:
isInteractive()(TTY + not CI).writeAuthTokenForStorageis also gated onisInteractive(), so even a non-interactive login (unusual but possible; people do all sorts of things in their CI workflows) won't trigger the first-write OS dialog.setPasswordthrowsAccessDeniedsynchronously and we fall back.I added some telemetry so we can track migration progress:
user_authTokenStored(mode, migrated, keychainFailed): fires on login and on successful migrationuser_authTokenRead(mode): fires when a stored token is returneduser_authTokenMigrationDeclined: user explicitly said no