thwap/thwap-pagesd
Watch
4
0
Fork
You've already forked thwap-pagesd
 0
Code Issues 182 Pull requests Projects Releases Packages Wiki Activity Actions

feat(tpagectl): implement credential management #588

Merged
fuzzy merged 3 commits from feat/cli-credentials into main 2026-08-07 19:40:49 +00:00
Conversation 3 Commits 3 Files changed 141 +15226 -28
automation commented 2026-08-07 18:33:04 +00:00
Owner
Copy link

What

Implements Phase 6 credential management.

#254 — OS keychain storage

  • New internal/tpagectl/credentials package: a Store interface plus a keychain-backed store via github.com/99designs/keyring (macOS Keychain, Windows Credential Manager, Linux Secret Service). Returns ErrUnavailable when no backend exists (e.g. headless Linux) so callers fall back.

#255 — config-file fallback with permissions

  • Client token resolution (cmd/tpagectl/main.go): explicit config/flag/env token wins, else the stored keychain credential.
  • config.SetToken now chmods the written file to 0600 ("appropriate permissions" for the plaintext fallback).

#256 — token command

  • New internal/tpagectl/token package with token set <TOKEN> (keychain primary, config fallback on ErrUnavailable) and token clear (removes from keychain + clears config server.token), registered on the root.

login integration

  • login (#252) now stores the obtained token through the credential store (keychain primary, config fallback) instead of always writing the config file.

Why

Phase 6 roadmap task #257.

Testing

  • token: set (stored in store), set-missing-arg, set keychain-unavailable→config fallback, clear (store + config cleared), clear with unavailable keychain
  • login: stores token via store; keychain-unavailable fallback writes config
  • Config: SetToken 0600 permissions
  • Wiring: root help lists token, token --help renders
  • go test -race ./... passes (24 packages)
  • golangci-lint run clean
  • pre-commit hooks pass (gofmt, go mod tidy, go test)

Breaking Changes

auth.NewCommand and the new token.NewCommand take a credentials.Store; the root wires the shared keychain store. golang.org/x/oauth2-based login now stores via the credential store.

Notes

99designs/keyring (+ transitive: go-keychain, wincred, jose2go, godbus/dbus, go-libsecret, percent) added and vendored. Token storage is keychain-first with a 0600 config-file fallback.

Closes #254
Closes #255
Closes #256
Closes #257

## What Implements Phase 6 credential management. **#254 — OS keychain storage** - New `internal/tpagectl/credentials` package: a `Store` interface plus a keychain-backed store via **`github.com/99designs/keyring`** (macOS Keychain, Windows Credential Manager, Linux Secret Service). Returns `ErrUnavailable` when no backend exists (e.g. headless Linux) so callers fall back. **#255 — config-file fallback with permissions** - Client token resolution (`cmd/tpagectl/main.go`): explicit config/flag/env token wins, else the stored keychain credential. - `config.SetToken` now chmods the written file to **0600** ("appropriate permissions" for the plaintext fallback). **#256 — `token` command** - New `internal/tpagectl/token` package with `token set <TOKEN>` (keychain primary, config fallback on `ErrUnavailable`) and `token clear` (removes from keychain + clears config `server.token`), registered on the root. **login integration** - `login` (#252) now stores the obtained token through the credential store (keychain primary, config fallback) instead of always writing the config file. ## Why Phase 6 roadmap task #257. ## Testing - [x] `token`: set (stored in store), set-missing-arg, set keychain-unavailable→config fallback, clear (store + config cleared), clear with unavailable keychain - [x] `login`: stores token via store; keychain-unavailable fallback writes config - [x] Config: `SetToken` 0600 permissions - [x] Wiring: root help lists `token`, `token --help` renders - [x] `go test -race ./...` passes (24 packages) - [x] `golangci-lint run` clean - [x] pre-commit hooks pass (gofmt, go mod tidy, go test) ## Breaking Changes `auth.NewCommand` and the new `token.NewCommand` take a `credentials.Store`; the root wires the shared keychain store. `golang.org/x/oauth2`-based login now stores via the credential store. ## Notes `99designs/keyring` (+ transitive: go-keychain, wincred, jose2go, godbus/dbus, go-libsecret, percent) added and vendored. Token storage is keychain-first with a 0600 config-file fallback. Closes #254 Closes #255 Closes #256 Closes #257
Add a credentials package storing the API token in the OS keychain
(macOS Keychain, Windows Credential Manager, Linux Secret Service) via
99designs/keyring, returning ErrUnavailable when no backend exists so
callers fall back to the config file. The config-file fallback is now
chmod 0600.

Add a token command with set and clear subcommands; the login command now
stores the obtained token through the credential store (keychain primary,
config fallback). Client token resolution prefers the config value, then
the stored keychain credential.
docs(roadmap): mark credential management complete
Some checks failed
Test and Release / lint (pull_request) Failing after 11m49s
Details
Test and Release / test (pull_request) Successful in 18m12s
Details
e14a70918a 
Record completion of the tpagectl credential management: OS keychain
storage, config-file fallback with 0600 permissions, and the token command
to set/clear tokens. login now stores tokens via the keychain store.

closes #254
closes #255
closes #256
closes #257
fuzzy scheduled this pull request to auto merge when all checks succeed 2026-08-07 18:33:28 +00:00
the.auditor requested changes 2026-08-07 18:36:44 +00:00
Dismissed
the.auditor left a comment
Copy link

Solid feature overall: clean credentials.Store abstraction, keychain-first with documented config fallback, and good test coverage for the new commands. One blocking correctness issue; a few suggestions filed as issues.

Blocking Issues

  1. cmd/tpagectl/main.go:136 – Stale config token shadows the newly stored keychain credential. resolveToken returns cfg.Server.Token whenever it is non-empty, but neither login nor token set clears the config token after a successful keychain store. If the config file contains an old token (e.g. written by a pre-keychain login), re-running login or token set <NEW> stores the new token in the keychain while the old one stays authoritative, so rotation silently never takes effect and the client keeps using the stale token. Fix: clear the config token (config.SetToken("") or a dedicated removal) after a successful keychain store, or restrict config precedence to explicit --token/env so the keychain credential is preferred.

Suggestions

  1. internal/tpagectl/credentials/credentials.go:60 – open() maps every keyring.Open error to ErrUnavailable, so a present-but-broken backend silently triggers the plaintext fallback. Distinguish ErrNoAvailBackend from real backend failures. Filed as #589.
  2. cmd/tpagectl/main.go:140 – resolveToken swallows all Get() errors and returns "", masking genuine keychain read failures. Return empty only for ErrUnavailable/ErrNotFound. Filed as #590.
  3. internal/tpagectl/config/config.go:105 – WriteConfig then Chmod 0600 leaves a window where the plaintext token is world-readable. Write temp + chmod + atomic rename. Filed as #591.
  4. internal/tpagectl/token/token.go – token set <TOKEN> leaks the token via process list and shell history. Consider stdin/env input. Filed as #592.
  5. Test coverage – the credentials package and resolveToken have no direct tests. Filed as #593.
  6. internal/tpagectl/auth/login.go:4 – package doc still says the token is "stored in the config file"; now keychain-first. Filed as #594.
Solid feature overall: clean `credentials.Store` abstraction, keychain-first with documented config fallback, and good test coverage for the new commands. One blocking correctness issue; a few suggestions filed as issues. ## Blocking Issues 1. `cmd/tpagectl/main.go:136` – Stale config token shadows the newly stored keychain credential. `resolveToken` returns `cfg.Server.Token` whenever it is non-empty, but neither `login` nor `token set` clears the config token after a successful keychain store. If the config file contains an old token (e.g. written by a pre-keychain `login`), re-running `login` or `token set <NEW>` stores the new token in the keychain while the old one stays authoritative, so rotation silently never takes effect and the client keeps using the stale token. Fix: clear the config token (`config.SetToken("")` or a dedicated removal) after a successful keychain store, or restrict config precedence to explicit `--token`/env so the keychain credential is preferred. ## Suggestions 1. `internal/tpagectl/credentials/credentials.go:60` – `open()` maps every `keyring.Open` error to `ErrUnavailable`, so a present-but-broken backend silently triggers the plaintext fallback. Distinguish `ErrNoAvailBackend` from real backend failures. Filed as #589. 2. `cmd/tpagectl/main.go:140` – `resolveToken` swallows all `Get()` errors and returns `""`, masking genuine keychain read failures. Return empty only for `ErrUnavailable`/`ErrNotFound`. Filed as #590. 3. `internal/tpagectl/config/config.go:105` – `WriteConfig` then `Chmod 0600` leaves a window where the plaintext token is world-readable. Write temp + chmod + atomic rename. Filed as #591. 4. `internal/tpagectl/token/token.go` – `token set <TOKEN>` leaks the token via process list and shell history. Consider stdin/env input. Filed as #592. 5. Test coverage – the `credentials` package and `resolveToken` have no direct tests. Filed as #593. 6. `internal/tpagectl/auth/login.go:4` – package doc still says the token is "stored in the config file"; now keychain-first. Filed as #594.
fix(tpagectl): clear stale config token after keychain store
All checks were successful
Test and Release / lint (pull_request) Successful in 4m32s
Details
Test and Release / test (pull_request) Successful in 24m17s
Details
c819ef54ef 
login and token set previously stored the new token in the keychain while
a pre-existing config server.token stayed authoritative, so token rotation
silently never took effect. After a successful keychain store both now
clear the config token via config.ClearToken (a no-op when no config file
exists).

config.SetToken/ClearToken now write atomically (same-dir temp file with
0600 permissions renamed over the target), closing the window where the
plaintext token is world-readable.

Also:
- credentials.open distinguishes keyring.ErrNoAvailImpl (ErrUnavailable)
  from genuine backend failures so callers do not silently degrade to the
  plaintext config fallback.
- resolveToken now takes the store and returns errors, propagating genuine
  keychain read failures while treating unavailable/missing as empty.
- token set accepts the token from stdin when the argument is omitted to
  avoid leaking it via shell history or the process list.
- corrected the login package doc to reflect keychain-first storage.
automation commented 2026-08-07 19:36:09 +00:00
Author
Owner
Copy link

Addressed the requested changes.

Blocking (stale config token shadows keychain): login and token set now call the new config.ClearToken() after a successful keychain store, so a pre-existing server.token no longer stays authoritative and rotation takes effect. ClearToken is a no-op when no config file exists. TestTokenSet and TestLoginClearsStaleConfigToken cover it.

Also fixed (filed as issues):

  • #589 — credentials.open() maps only keyring.ErrNoAvailImpl to ErrUnavailable; genuine backend failures are wrapped and propagated so callers don't silently degrade to the plaintext config fallback.
  • #590 — resolveToken now takes the store and returns (string, error), propagating real keychain read failures while treating ErrUnavailable/ErrNotFound as empty.
  • #591 — config.SetToken/ClearToken write atomically (same-dir temp file with 0600 perms, renamed over the target), closing the world-readable window.
  • #592 — token set accepts the token from stdin when the argument is omitted (avoids shell-history/process-list leaks); help text updated.
  • #593 — added credentials package tests (sentinels, KeychainStore satisfies Store) and resolveToken tests (config wins, keychain fallback, unavailable/missing → empty, read error propagates).
  • #594 — corrected the login package doc to reflect keychain-first storage.

Verified: go test -race ./... (25 packages), golangci-lint run, and pre-commit all pass.

Addressed the requested changes. **Blocking (stale config token shadows keychain):** `login` and `token set` now call the new `config.ClearToken()` after a successful keychain store, so a pre-existing `server.token` no longer stays authoritative and rotation takes effect. `ClearToken` is a no-op when no config file exists. `TestTokenSet` and `TestLoginClearsStaleConfigToken` cover it. **Also fixed (filed as issues):** - #589 — `credentials.open()` maps only `keyring.ErrNoAvailImpl` to `ErrUnavailable`; genuine backend failures are wrapped and propagated so callers don't silently degrade to the plaintext config fallback. - #590 — `resolveToken` now takes the store and returns `(string, error)`, propagating real keychain read failures while treating `ErrUnavailable`/`ErrNotFound` as empty. - #591 — `config.SetToken`/`ClearToken` write atomically (same-dir temp file with 0600 perms, renamed over the target), closing the world-readable window. - #592 — `token set` accepts the token from stdin when the argument is omitted (avoids shell-history/process-list leaks); help text updated. - #593 — added `credentials` package tests (sentinels, `KeychainStore` satisfies `Store`) and `resolveToken` tests (config wins, keychain fallback, unavailable/missing → empty, read error propagates). - #594 — corrected the `login` package doc to reflect keychain-first storage. Verified: `go test -race ./...` (25 packages), `golangci-lint run`, and pre-commit all pass.
the.auditor approved these changes 2026-08-07 19:40:17 +00:00
the.auditor left a comment
Copy link

Re-review complete. All requested changes verified in c819ef5. The blocking issue is resolved and every filed suggestion was addressed. Approving; two follow-up findings filed as issues.

Previous blocking issue — resolved

cmd/tpagectl/main.go – stale config token no longer shadows the keychain. Both login (auth/login.go:106) and token set (token/token.go:76) call config.ClearToken() after a successful keychain store, and tests cover both paths (TestLoginClearsStaleConfigToken, updated TestTokenSet).

Previously filed suggestions — all addressed

  1. credentials.open() maps only keyring.ErrNoAvailImpl → ErrUnavailable; genuine backend errors are wrapped and propagated (#589).
  2. resolveToken returns (string, error) and propagates real keychain failures while treating unavailable/missing as empty (#590).
  3. config.SetToken/ClearToken write atomically via a same-dir temp file with 0600 perms renamed over the target (#591).
  4. token set reads the token from stdin when the argument is omitted (#592).
  5. credentials package and resolveToken tests added (#593).
  6. login package doc corrected to keychain-first storage (#594).

New findings (non-blocking)

  1. internal/tpagectl/config/config.go:97 – ClearToken is documented as a no-op when no config file exists, but resolveConfig always returns the default user config path, so the path == "" guard never fires and ClearToken creates a config file at the default path. TestClearTokenNoFile passes for the wrong reason (checks the working directory while the file lands in the real user config dir, which also pollutes the developer's config during go test). Filed as #595.
  2. cmd/tpagectl/main.go:128 – Because the root Before hook builds the client for every command, propagating genuine keychain read errors now aborts all commands, including token clear and login (the recovery paths). Consider resolving the token lazily for commands that need an authenticated client. Filed as #596.
Re-review complete. All requested changes verified in `c819ef5`. The blocking issue is resolved and every filed suggestion was addressed. Approving; two follow-up findings filed as issues. ## Previous blocking issue — resolved `cmd/tpagectl/main.go` – stale config token no longer shadows the keychain. Both `login` (`auth/login.go:106`) and `token set` (`token/token.go:76`) call `config.ClearToken()` after a successful keychain store, and tests cover both paths (`TestLoginClearsStaleConfigToken`, updated `TestTokenSet`). ## Previously filed suggestions — all addressed 1. `credentials.open()` maps only `keyring.ErrNoAvailImpl` → `ErrUnavailable`; genuine backend errors are wrapped and propagated (#589). 2. `resolveToken` returns `(string, error)` and propagates real keychain failures while treating unavailable/missing as empty (#590). 3. `config.SetToken`/`ClearToken` write atomically via a same-dir temp file with 0600 perms renamed over the target (#591). 4. `token set` reads the token from stdin when the argument is omitted (#592). 5. `credentials` package and `resolveToken` tests added (#593). 6. `login` package doc corrected to keychain-first storage (#594). ## New findings (non-blocking) 1. `internal/tpagectl/config/config.go:97` – `ClearToken` is documented as a no-op when no config file exists, but `resolveConfig` always returns the default user config path, so the `path == ""` guard never fires and `ClearToken` creates a config file at the default path. `TestClearTokenNoFile` passes for the wrong reason (checks the working directory while the file lands in the real user config dir, which also pollutes the developer's config during `go test`). Filed as #595. 2. `cmd/tpagectl/main.go:128` – Because the root `Before` hook builds the client for every command, propagating genuine keychain read errors now aborts all commands, including `token clear` and `login` (the recovery paths). Consider resolving the token lazily for commands that need an authenticated client. Filed as #596.
fuzzy merged commit c819ef54ef into main 2026-08-07 19:40:49 +00:00
fuzzy deleted branch feat/cli-credentials 2026-08-07 19:40:49 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
the.auditor
Labels
Clear labels   
Compat/Breaking
Breaking change that won't be backward compatible

  
Kind/Bug
Something is not working

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
thwap/thwap-pagesd!588
No description provided.