Add credstore package for keychain-backed credential storage#3
Open
emsearcy wants to merge 11 commits into
Open
Add credstore package for keychain-backed credential storage#3emsearcy wants to merge 11 commits into
emsearcy wants to merge 11 commits into
Conversation
Introduces internal/credstore, a credential storage abstraction for the LFX CLI backed by github.com/99designs/keyring: - Credentials (refresh token, cached access token + expiry) are stored in the system keychain (macOS Keychain, Windows Credential Manager, Linux Secret Service/KWallet) by default, with keyring's own passphrase-encrypted file backend used automatically when no system keychain is available. - When --insecure-storage is passed, the system keychain is bypassed entirely in favor of a plain (unencrypted), owner-only (0600) JSON file, for headless/CI use where a passphrase prompt is unacceptable. - Non-sensitive DeviceState (device ID, IdP domain) is always stored as plain JSON under the XDG state directory (~/.local/state/lfx-cli/ by default), per XDG Base Directory conventions. Wires the shared --insecure-storage flag onto the `lfx auth` command group; the login/token/status/logout actions remain stubs pending LFXV2-2515 and LFXV2-2516. Verified locally on macOS: real Keychain round-trip (save/load/delete, no passphrase prompt) and plain-file fallback round-trip both pass. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
![github-license-compliance[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds keychain-backed credential and device-state storage for future authentication commands.
Changes:
- Adds secure keyring and optional plaintext credential backends.
- Persists device state under the XDG state directory.
- Adds the shared
--insecure-storageauth flag and dependencies.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
internal/credstore/credstore.go |
Implements credential and device-state storage. |
internal/commands/auth.go |
Adds the insecure-storage flag. |
go.mod |
Adds keyring dependencies. |
go.sum |
Records dependency checksums. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Restrict keyring.Open to real system credential stores (macOS Keychain, Windows Credential Manager, Linux Secret Service/KWallet/pass), dropping keyring's automatic passphrase-encrypted file fallback. If none of those backends are available, New now returns an error directing the user to --insecure-storage instead of silently falling back to a file. - Add defensive os.ErrNotExist handling in keyringSecrets.Delete, since some backends (e.g. pass) may surface that instead of keyring.ErrKeyNotFound for a missing key. - Reject relative $XDG_STATE_HOME per the XDG Base Directory Specification, falling back to ~/.local/state/lfx-cli instead of resolving state paths relative to the process's working directory. - Document --insecure-storage usage in the README. Addresses Copilot review comments on PR #3. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
Run go get -u ./... && go mod tidy, picking up minor version bumps for transitive keyring dependencies (wincred, jose2go, x/sys, x/term, go-md2man). Also documents this as a required step before every PR in AGENTS.md's Contributing Guidelines. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
Fixes cspell warnings from make megalinter for words introduced by the credstore package and existing Makefile content: credstore, fprintln, gobin, gopath, mgechev, esac. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
Independently verified each of the following before fixing: - os.WriteFile only applies its mode argument when creating a new file, silently preserving the mode of one that already exists. A pre-existing 0644 credentials.json (or state.json) therefore stayed world-readable even after writing "owner-only" data. Added writeOwnerOnlyFile, which opens with O_TRUNC and explicitly chmods 0600 regardless of prior state, and use it in both plainFileSecrets.Save and SaveDeviceState. Verified with a standalone repro: a pre-existing 0644 file now ends up 0600 after a write. - keyring.KeyCtlBackend's opener calls getKeyringForScope(cfg.KeyCtlScope); with the zero-value scope this package leaves unconfigured, that always errors, and keyring.Open silently skips the backend on error. Confirmed against the vendored keyring v1.2.2 source. Removed it from systemBackends since it can never actually open. - keyring's pass backend namespaces entries via Config.PassPrefix, not ServiceName (confirmed against pass.go). Without it, our credentialsKey would be stored as a generic top-level "credentials" entry in the user's password store, risking collisions with unrelated pass entries. Set PassPrefix to serviceName. - Fixed stale doc comments in Store.SaveCredentials and Options.Insecure still describing an encrypted-file fallback that no longer exists. Re-verified the real macOS Keychain round-trip locally after these changes. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
Address review comments from copilot-pull-request-reviewer[bot]: - .goreleaser.yaml, .github/workflows/release-tag.yml: build darwin binaries natively on macos-latest with CGO_ENABLED=1 instead of cross-compiling from ubuntu-latest with CGO_ENABLED=0. keyring's KeychainBackend is compiled only under the "darwin && cgo" build tag, so released macOS binaries previously had no working Keychain backend. Darwin archives and checksums are uploaded as additional release assets after the main GoReleaser job. - internal/credstore/credstore.go: made writeOwnerOnlyFile atomic by writing to a temp file in the same directory and renaming it into place, instead of truncating the destination in place. A failed write can no longer erase a previously valid stored credentials file. - internal/credstore/credstore.go: corrected doc comments to note that Chmod(0600) does not enforce owner-only access on Windows (Go maps it to the read-only attribute, not a real ACL), rather than overpromising confidentiality there. - .cspell.json: added binpath, mktemp, pipefail to the word list for the new release-tag.yml darwin job. Resolves 3 review threads. Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
Contributor
Author
Review Feedback AddressedCommit: a0df0ec Changes Made
Threads Resolved3 of 3 unresolved Copilot review threads addressed in this iteration. Not AddressedThe 2 |
Address review comments from copilot-pull-request-reviewer[bot]: - README.md: documented that the owner-only insecure-storage file's 0600 permissions don't translate to a real ACL on Windows, so users there shouldn't assume the plaintext tokens are protected by file mode alone. - .github/workflows/release-tag.yml: made the darwin job's release asset upload retry-safe. Added --clobber to the archive upload so a rerun doesn't fail on already-existing assets, and filter out any previously-appended darwin checksum lines before re-appending them so reruns don't duplicate entries in checksums.txt. Resolves 2 review threads. Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
Contributor
Author
Additional Review Feedback AddressedCommit: 6b363b9 Copilot ran another pass after the previous push and found 2 more items: Changes Made
Threads Resolved2 of 2 new unresolved threads addressed in this iteration. Still Open
|
Address review comment from copilot-pull-request-reviewer[bot]: - .github/workflows/release-tag.yml: the previous full-line grep filter only removed a darwin checksums.txt entry if its hash exactly matched the newly built archive. Since rebuilt archives get new hashes on every run (from embedded timestamps), a rerun would leave the stale entry in place and append a conflicting one for the same filename. Filter by filename instead, so reruns always replace prior darwin entries rather than duplicating them. Resolves 1 review thread. Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
Contributor
Author
Additional Review Feedback AddressedCommit: 506d8fc Copilot found one more issue on the previous fix: Changes Made
Threads Resolved1 of 1 new unresolved thread addressed in this iteration. Still Open
|
GoReleaser's only remaining value in this pipeline (archive naming, checksums.txt, GitHub release asset upload) was already being hand-rolled for the darwin-native job added to fix the cgo/Keychain issue, plus a cross-job checksums.txt merge step. Its changelog generation was dead code: gh release create --generate-notes already sets the release body before the tag-push workflow runs, and GoReleaser's default keep-existing mode won't overwrite it. CGO_ENABLED=0 cross-compilation works from any host OS, so linux and windows binaries don't need ubuntu-latest specifically. This collapses the previous two-job split (ubuntu-latest running GoReleaser for linux/windows, macos-latest natively building darwin with a separate checksum-merge step) into a single macos-latest job that builds and archives all 5 targets and uploads them in one pass, with one checksums.txt and no merge logic needed. Also carries forward GoReleaser's own recommended reproducible-builds recipe (pin mtime to commit timestamp, -trimpath) for all 5 binaries, and extends it to the non-binary archived files (LICENSE, README): actions/checkout sets file mtimes to checkout time since git doesn't track per-file mtimes at all, so those need pinning too for consistent archives across runs. - .goreleaser.yaml: removed. - .github/workflows/release-tag.yml: single `release` job replacing the `goreleaser` + `darwin` jobs. - Makefile, AGENTS.md: removed GoReleaser references. - .cspell.json: updated word list for the new script's vocabulary. Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
bsdtar's built-in -z (gzip) writer embeds the current wall-clock time in the gzip header, independent of the archived files' own mtimes. Even with every archived file's mtime pinned to the commit timestamp, this made the tar.gz bytes differ on every run. Fixed by piping an uncompressed tar stream through `gzip -n`, which omits the timestamp (and original filename) from the gzip header. Verified locally: two separate builds from separate git clones (with a 2-second gap) now produce byte-identical tar.gz archives; zip archives were already reproducible without this change, since zip timestamps are stored per-entry rather than in a container-level header. Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
| return err | ||
| } | ||
|
|
||
| return os.Rename(tmpPath, path) |
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
Implements
internal/credstore, a credential storage abstraction for the LFX CLI backed bygithub.com/99designs/keyring, addressing LFXV2-2514.pass) by default.keyring's own file backend is deliberately excluded — it isn't a real system keychain, and its delete-on-missing-key behavior doesn't matchkeyring.ErrKeyNotFound. If no system backend is available,Newreturns an error directing the user to--insecure-storage.--insecure-storageis passed, the system keychain is bypassed entirely in favor of a plain (unencrypted), owner-only (0600 on POSIX; on Windows this relies on inherited directory permissions, sinceChmod(0600)doesn't install a real ACL there) JSON file — for headless/CI use where a passphrase prompt is unacceptable. The 0600 mode is explicitly enforced on every write (not just file creation), and writes are atomic (temp file + rename) so a failed write can't corrupt a previously valid file.DeviceState(device ID, IdP domain) is always stored as plain JSON under the XDG state directory (~/.local/state/lfx-cli/by default), per XDG Base Directory conventions. A relative$XDG_STATE_HOMEis ignored per spec.--insecure-storageflag onto thelfx authcommand group. Thelogin/token/status/logoutactions remain stubs, to be implemented in LFXV2-2515 and LFXV2-2516, which will consumecredstore.Store.Release pipeline changes
This PR also replaces GoReleaser with a single hand-rolled GitHub Actions job (
.github/workflows/release-tag.yml), triggered by review feedback that macOS release binaries had no working Keychain backend:KeychainBackendingithub.com/99designs/keyringis compiled only under thedarwin && cgobuild tag. The original GoReleaser config cross-compiled every platform fromubuntu-latestwithCGO_ENABLED=0, so released macOS binaries silently shipped without Keychain support. Fixing this requires a native macOS build with cgo enabled, at which point GoReleaser's remaining value (archive naming,checksums.txt, changelog, GitHub release upload) was either already being hand-rolled to support that native build, or dead weight — the changelog in particular was already inert, sincegh release create --generate-notessets the release body before the tag-push workflow runs, and GoReleaser's defaultkeep-existingmode won't overwrite it.CGO_ENABLED=0cross-compilation works from any host OS, all 5 targets (linux/amd64, linux/arm64, windows/amd64, darwin/amd64, darwin/arm64) now build in a single job onmacos-latest— linux/windows cross-compiled as before, darwin built natively withCGO_ENABLED=1. Onechecksums.txt, onegh release uploadcall, no cross-job artifact merging.-trimpathand every archived file (binary + LICENSE/LICENSE-docs/README.md) has its mtime pinned to the commit timestamp, sinceactions/checkoutsets file mtimes to checkout time (git doesn't track per-file mtimes at all). Byte-identical archive compression isn't attempted/guaranteed (gzip internals aren't necessarily deterministic across environments), but the binary content and all recorded metadata are.Testing
make check(fmt, vet, lint, revive) passes.security find-generic-passwordthatDeleteCredentialsproperly removes the Keychain item.KeyCtlBackendandpass/PassPrefixbehavior against the vendoredgithub.com/99designs/keyring@v1.2.2source.git cloneto confirm mtime pinning works without relying on pre-existing local file timestamps).Jira
LFXV2-2514
🤖 Generated with GitHub Copilot (via OpenCode)