Skip to content

Store credentials in system keychain by default#289

Open
michael-webster wants to merge 9 commits into
mainfrom
webster/keychain-credential-storage
Open

Store credentials in system keychain by default#289
michael-webster wants to merge 9 commits into
mainfrom
webster/keychain-credential-storage

Conversation

@michael-webster
Copy link
Copy Markdown
Contributor

@michael-webster michael-webster commented Apr 28, 2026

Summary

Stores credentials (CircleCI token, Anthropic API key, GitHub token) in the system keychain by default instead of the plaintext config file.

  • `chunk auth set` writes to the system keychain unless `--insecure-storage` is passed
  • `--insecure-storage` falls back to the config file (existing behavior), with a warning that the credential is stored on disk
  • Keychain failures are now fatal — no silent fallback to plaintext. Users on systems without a working keychain must pass `--insecure-storage` explicitly
  • `chunk auth status` warns when any credential is coming from the config file and suggests migrating with `chunk auth set`
  • Adds `keyring.MockInit()` so tests that exercise keychain paths use an in-memory backend

Migration

Existing users with credentials in the config file continue to work — credentials are still read from both sources. `chunk auth status` will show a warning and prompt them to re-run `chunk auth set` to move credentials to the keychain.

Test plan

  • `chunk auth set circleci` saves token to keychain, not config file
  • `chunk auth set circleci --insecure-storage` saves to config file and prints on-disk warning
  • `chunk auth status` shows warning for any credential sourced from config file
  • On a system without keychain (or with keychain locked), `chunk auth set` without `--insecure-storage` fails with a clear error
  • `task test` passes

@michael-webster michael-webster force-pushed the webster/keychain-credential-storage branch 2 times, most recently from b8d373c to a4d446b Compare April 28, 2026 13:29
@michael-webster michael-webster marked this pull request as ready for review April 28, 2026 13:36
Add keychain-backed credential storage via go-keyring (com.circleci.cli
service). Credentials are saved to the system keychain by default and
fall back to the config file when the keychain is unavailable. The
--insecure-storage flag on chunk auth set opts into plaintext file
storage. CircleCI tokens are keyed by host so different instances stay
separate.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@michael-webster michael-webster force-pushed the webster/keychain-credential-storage branch from a4d446b to f40c742 Compare April 28, 2026 13:54
Comment thread acceptance/auth_test.go
}

// auth status reads key from config file when no env var is set
func TestAuthStatusFromConfigFile(t *testing.T) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to revert these if people think the fallback resolution tests are of dire importance. But a) we want to ditch config file as even an option and b) the keychain dep is harder to test and the code around it is simple.

@michael-webster michael-webster force-pushed the webster/keychain-credential-storage branch from e07610f to 17c448f Compare April 28, 2026 14:40
…ests

Tests that resolved credentials from config file are removed — env var
coverage is sufficient. isolateConfig and setupTempConfig now set dummy
credential env vars so Resolve() never falls through to the system keychain
during local test runs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@michael-webster michael-webster force-pushed the webster/keychain-credential-storage branch from 17c448f to 991b238 Compare May 7, 2026 15:57
Resolves conflicts in auth.go, go.mod, and acceptance/auth_test.go.
Combines --insecure-storage (keychain fallback) with --force/-f flag
and non-interactive handling from main. Updates docs to reflect both
new flags and keychain-first storage behavior.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@michael-webster michael-webster marked this pull request as draft May 13, 2026 17:11
webster and others added 3 commits May 13, 2026 14:17
Verifies all four goreleaser targets (linux/amd64, linux/arm64,
darwin/amd64, darwin/arm64) build cleanly with CGO_ENABLED=0.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Matches how goreleaser actually builds the binary and ensures the
smoke test catches any CGO dependency introduced accidentally.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@michael-webster michael-webster marked this pull request as ready for review May 18, 2026 12:58
@michael-webster michael-webster requested a review from z00b May 19, 2026 13:58
@schurchleycci
Copy link
Copy Markdown
Contributor

Claude:

  1. Silent keychain failures (authprompt/authprompt.go:334)

if err := keyring.Set(keyring.CircleCITokenKey(baseURL), token); err == nil {
return true, nil
}
// silently falls through to config file
If keychain storage fails (e.g., locked keychain, Linux with no D-Bus), the error is swallowed and the credential silently ends up in the plaintext config file. The user sees “saved to user config” but doesn’t know why the keychain was skipped. This should at least log a warning like “Keychain unavailable, saving to config file instead.”

  1. Potential “already stored” check uses resolved config, not keychain truth

In auth.go:595:

if rc.CircleCIToken != "" && !envSet {
io.Printf("A CircleCI token is already stored.\n")
rc.CircleCIToken could be non-empty because the token came from the keychain or from the config file. That’s fine. But it also means if a user has a token only in the keychain, they’ll get the “already stored” prompt — which is correct behavior. Just worth confirming the wording is intentional (“already stored” instead of the old “already stored in config”).

When insecureStorage is false (the default), a keychain write failure
now returns an error rather than transparently saving to the plaintext
config file. Users on systems without a working keychain must pass
--insecure-storage explicitly.

Adds keyring.MockInit() so tests that exercise keychain code paths can
use an in-memory backend without touching the real system keychain.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@michael-webster
Copy link
Copy Markdown
Contributor Author

Thanks for the review Scott!

  1. Fixed — keychain failures are now fatal rather than silently falling back to the config file. Users on systems without a working keychain need to pass --insecure-storage explicitly. Also added keyring.MockInit() to the keyring package so tests that exercise these paths use an in-memory backend instead of the real system keychain.

  2. The wording is intentional — "already stored" is deliberately source-agnostic since the token could be in either the keychain or config file depending on how it was saved.

Adds a warning in printSaved when --insecure-storage is used, and in
auth status for each credential that comes from the config file, with
a suggestion to re-run 'chunk auth set' to migrate to the keychain.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@michael-webster michael-webster force-pushed the webster/keychain-credential-storage branch from 180a8ee to a47faa2 Compare May 19, 2026 20:27
…nt wording

- auth remove now checks both keychain and config file for stored credentials
  instead of only the config file, so keychain-stored tokens are actually cleared
- Resolve() returns early if config.Load() fails rather than proceeding with a
  zero-value UserConfig and making unnecessary keychain calls
- printSaved warning now says 'chunk auth set <provider>' (runnable command)
- TestAuthRemoveWithStoredKey verifies config file via os.ReadFile instead of
  config.Load() to avoid coupling acceptance test to internal resolution logic

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants