Skip to content

feat(auth): surface credential-store failures and add ggshield auth status#1268

Open
AGallouin wants to merge 1 commit into
GitGuardian:mainfrom
AGallouin:agallouin/keyring-failure-feedback
Open

feat(auth): surface credential-store failures and add ggshield auth status#1268
AGallouin wants to merge 1 commit into
GitGuardian:mainfrom
AGallouin:agallouin/keyring-failure-feedback

Conversation

@AGallouin

Copy link
Copy Markdown

Context

When the ggshield binary path changes (mise/asdf reshim, pyenv/pipx reinstall, Homebrew upgrade…), the macOS Keychain entry created by the previous binary can no longer be overwritten: writes fail with -25244 (errSecInvalidOwnerEdit) because the entry's ACL is tied to the old path. ggshield silently fell back to storing the token in cleartext in auth_config.yaml, with only a debug-level log. The availability probe could not catch this, since it writes a fresh key with no ACL conflict.

Fixes #1267.

What has been done

  • Failed credential-store writes are now surfaced to the user: AuthConfig.save() prints a warning with a humanized error (the -25244 case is translated to plain language), states that the token will stay in cleartext, and gives the copy-pasteable recovery commands (security delete-generic-password … + ggshield auth login). The fix advice is gated on the active keyring backend (not the OS), so a custom backend on macOS doesn't get Keychain-specific commands.
  • Successful cleartext → credential-store migrations are confirmed with an info message. Routine re-saves and fresh logins stay silent to avoid noise; the prior on-disk state is used to tell the two apart.
  • New ggshield auth status command diagnosing token storage, in text and --json:
    • reports the credential-store backend (macOS Keychain, Windows Credential Locker, Linux Secret Service, KWallet…), whether it is reachable, and where each instance's token actually lives (ok / failed / plaintext / disabled / skipped), with fix commands when actionable;
    • is strictly read-only: it never writes to the credential store (no token migration as a side effect, and no write-probe — a new is_reachable() read-only check is used instead of is_available());
    • never touches the credential store at all when GGSHIELD_NO_KEYRING is set (reachable is reported as null);
    • the diagnosis is read-centric: a token that reads back fine is ok even if overwriting its entry would fail, because reads are what every command needs at runtime — write failures are surfaced at save time instead.
    • JSON output has a stable shape (credential_store + per-instance {instance, status, message, fix}, with null for non-applicable fields) intended to be parseable by scripts; tests freeze the contract.

Differences from the issue's proposed fixes: instead of making is_available() probe real instance keys (which would put writes on every command's startup path), the conflict is detected where it matters — the save path warns on the actual failed write, and auth status verifies each real instance key with reads only. The diagnostic command lives under ggshield auth status rather than ggshield config keyring-status, and its output is ASCII-only for legacy Windows terminals.

Validation

Unit tests cover the new behavior (failure warning, migration message, silence on re-saves, read-only guarantee, JSON contract). To validate manually on macOS:

  • Authenticate: ggshield auth login, then run ggshield auth status — the instance should report token_storage: ok with the macOS Keychain location.
  • Simulate the ACL conflict: re-create the Keychain entry from another binary path (or reinstall ggshield via mise/brew so its path changes), then re-trigger a save (ggshield auth login) — a warning should explain the failure and print the security delete-generic-password fix; auth status should report plaintext with the same fix.
  • Apply the fix commands — auth status should report ok again, and the save should print the migration confirmation.
  • GGSHIELD_NO_KEYRING=1 ggshield auth status --json should report "reachable": null and not prompt for Keychain access.

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

🤖 Generated with Claude Code

…tatus

When writing a token to the OS credential store failed (e.g. macOS error
-25244 after the ggshield binary path changed following a mise/asdf
reshim, a pyenv/pipx reinstall or a Homebrew upgrade), the token stayed
in cleartext in auth_config.yaml with only a hidden debug-level log.

- Saves now print an actionable warning with the humanized error and the
  commands fixing the stale Keychain entry
- Successful cleartext -> credential-store migrations are confirmed with
  an info message
- New `ggshield auth status` command (text and --json) diagnoses the
  credential-store backend, its reachability and where each instance
  token actually lives, without ever writing to the store

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AGallouin AGallouin requested a review from a team as a code owner June 11, 2026 09:28
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.

Keyring migration fails silently when Keychain entry ACL doesn't match current binary (e.g. mise installs)

1 participant