feat(config): add optional system keyring credential storage#135
Merged
Conversation
Add opt-in OS keyring storage for API keys and passwords via zalando/go-keyring, keeping the plaintext config file the default. A profile opts in with credential_store: keyring; the secret is then written to the OS keyring (macOS Keychain, Linux Secret Service, Windows Credential Manager) and stripped from the YAML file. Credentials resolve in order CLI flag, environment variable, keyring, plaintext file, so env vars and flags always win and are resolved before the keyring is ever consulted. Any keyring failure degrades gracefully and never aborts, and REDMINE_NO_KEYRING=1 hard-disables all keyring access, keeping CI and headless usage working unchanged. Existing plaintext profiles are unaffected and never consult the keyring. auth login gains a --keyring flag and a TTY-gated prompt; auth logout removes keyring entries. Docs updated in English and zh-cn.
Address three keyring credential-store issues: - Key keyring entries by an opaque per-profile keyring_id instead of the profile name. Two --config files with a same-named profile no longer collide and clobber each other's credential, and the id is stable across profile renames. The id is generated on first keyring save, inherited on re-login, and persisted in the config file. - auth logout no longer reports success when the keyring rejects deletion. DeleteProfile removes the stored secret before touching the config and returns an actionable error on a real backend failure, so the profile stays intact and logout is retryable. - Switching a profile from keyring back to the plaintext file now removes its stored secret instead of orphaning it in the keyring.
…secrets - Delete a profile's keyring secret only after the config change is durably committed. Previously DeleteProfile and the keyring-to-plaintext transition removed the stored secret first, so a failing os.Remove or SaveProfiles left a profile pointing at a destroyed credential. Config now commits first; a keyring cleanup failure is surfaced but never destroys a still-referenced secret. - Run the missing-keyring-secret check regardless of the no-active-profile path. CLI overrides are already applied at that point, so a valid --api-key still passes, but supplying only --server against an unreachable keyring now returns a clear error instead of a misleading downstream 401.
- Commit config file before writing secrets to the keyring so a backend failure never orphans a secret under an unrecorded keyring id; the retry reuses the persisted id. - Keep existing keyring profiles on the keyring during re-login: inherit the choice without a TTY and fail clearly when the backend is unusable instead of silently downgrading to plaintext. - Remove the stale counterpart secret when a login switches auth methods. - Stop suggesting REDMINE_NO_KEYRING=1 as a standalone fix in the missing-secret error message.
- Roll the config file back to its previous contents when persisting secrets to the keyring fails, so a failed plaintext-to-keyring conversion keeps the working file credential instead of leaving a profile with no credential anywhere. - Delete keyring secrets before committing profile deletion or a plaintext switch. Since Delete maps not-found to nil this order is retryable in both failure modes and can no longer drop the only reference to the opaque keyring id, permanently orphaning the secret.
…on skipped cleanup Switching keyring to plaintext snapshots the old secret and restores it if the config write fails, so no state is left without a working, retryable credential. When REDMINE_NO_KEYRING skips cleanup on logout or plaintext switch, a warning tells the user where the credential remains instead of silently orphaning it.
Also remove the unused config.Save.
|
o_O |
This was referenced Jul 20, 2026
aarondpn
added a commit
that referenced
this pull request
Jul 20, 2026
…only) (#137) ## Summary Adds an opt-in **read-only mode** to the CLI, complementing the read-only gating that already exists in the MCP layer. When enabled, any mutating HTTP request is refused before it is sent, so the tool can be pointed at a production Redmine for safe browsing/scripting. ## How it works - Enforced at the **HTTP transport** (`readOnlyTransport` RoundTripper): only `GET`/`HEAD`/`OPTIONS` pass; anything else returns `ErrReadOnly` without hitting the network. A single choke point means new commands are covered automatically. - Three ways to enable, precedence **flag > env > config**: - `--read-only` global flag - `REDMINE_READ_ONLY` environment variable - per-profile `read_only:` in the config file - **Backward compatible**: default off; no behavior change for existing users. ## Tests - `internal/api/read_only_test.go` — transport refuses mutating methods, allows reads. - `internal/cmdutil/factory_test.go` — flag/env/config precedence. - `internal/config/config_test.go` — `REDMINE_READ_ONLY` override. `go build`, `go vet`, `gofmt`, and full `go test ./...` all pass on top of current `main` (including #134/#135). ## Notes The docs site (`docs/`) is not updated in this PR — happy to add a page if you'd like it. --------- Co-authored-by: Aaron Döppner <58708656+aarondpn@users.noreply.github.com>
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
2 participants
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
Adds optional, opt-in OS keyring storage for user credentials via
zalando/go-keyring, so API keys and passwords no longer have to live in plaintext in~/.redmine-cli.yaml. The plaintext file remains the default; nothing changes for existing users unless they opt in.How it works
credential_store: keyringmarker. Its secret is written to the OS keyring (macOS Keychain / Linux Secret Service / Windows Credential Manager) and stripped from the YAML file on save.internal/secretspackage provides a smallStoreinterface over go-keyring, so the backend is swappable and testable (MockInit).auth logingains a--keyringflag plus a TTY-gated "Store credentials in system keyring?" prompt (shown only when a usable backend is detected).auth logoutremoves the profile's keyring entries.Backward compatibility & CI safety (the design constraints)
REDMINE_API_KEYnever touches the keyring.REDMINE_NO_KEYRING=1hard-disables all keyring reads/writes.Scope
Applies to
api_keyandpasswordonly.mcp.auth_tokenis intentionally left in file/env (different lifecycle).Tests
New coverage in
internal/secretsandinternal/config: keyring round-trip,ErrNotFoundhandling, env-var-wins-over-keyring, env-wins-over-failing-keyring (core CI path), legacy-never-consults-keyring, no-plaintext-written-when-opted-in, headless-returns-clear-error,DeleteProfilecleanup, and--api-keyflag bypassing the keyring. Fullgo build/go vet/go test ./.../gofmtpass. Docs (English + zh-cn) validated withastro build.Notes for reviewers
service="redmine-cli",user="<profile>:<field>". These strings are an implicit upgrade-compatibility contract: they must stay stable so credentials survive binary upgrades.