Conversation
Add support for additional credential sources beyond pass: - 1Password Service Accounts (op://) with configurable token file - age encryption (age:) with embedded filippo.io/age library - macOS Keychain (keychain:) with keychain-setup CLI command - Bitwarden CLI (bw:) with session persistence All backends support jq extraction via pipe syntax (e.g., `bw:uuid | .login.password`). Security hardening: - Trusted binary lookup restricts CLI auto-detection to known paths - O_NOFOLLOW and permission validation for credential files - Owner UID validation for sensitive files - Debug logging for credential fetch failures (server-side only) New CLI commands: - `keychain-setup` (macOS only) - create keychain items with ACL New config options: - `op_token_file` - custom 1Password token location - `age_identity_file` - custom age identity location - `op_binary`, `bw_binary` - CLI path overrides
There was a problem hiding this comment.
Pull request overview
Adds multiple new credential backends (1Password service accounts, age-encrypted files, macOS Keychain, Bitwarden CLI) and introduces jq-based extraction across backends, along with hardened binary resolution and CLI/install improvements.
Changes:
- Implement new credential source parser + embedded jq evaluation, and wire jq extraction into Fetch flows.
- Add credential backends:
op://(1Password),age:,keychain:(macOS-only),bw:(Bitwarden with session persistence). - Harden binary auto-detection to trusted directories and extend CLI with
keychain-setupand saferinstallconflict behavior (--force).
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/paths/paths.go | Adds trusted binary lookup + defaults for age identity and OP token files. |
| internal/paths/paths_test.go | Tests for trusted binary lookup behavior (no PATH usage, separator rejection). |
| internal/daemon/executor.go | Passes OP/BW binary overrides into credential fetching. |
| internal/daemon/daemon.go | Configures credential backend paths on startup/reload; adds error logging for admin check. |
| internal/daemon/daemon_test.go | Tests backend path configuration and Bitwarden cleanup on startup failure. |
| internal/credentials/credentials.go | Refactors Fetch to use parsed sources; adds OP/BW options; routes to new backends + jq. |
| internal/credentials/credentials_test.go | Adds tests for new Fetch options and jq extraction for env backend. |
| internal/credentials/parser.go | New source parser supporting prefixes and optional jq pipe syntax. |
| internal/credentials/parser_test.go | Parser coverage across all backends and error cases. |
| internal/credentials/jq.go | Embedded jq evaluation via gojq with timeout and string conversion. |
| internal/credentials/jq_test.go | Extensive jq behavior tests (types, errors, timeout). |
| internal/credentials/onepassword.go | 1Password backend (trusted binary lookup, token sourcing, secure file reads). |
| internal/credentials/onepassword_test.go | Unit tests for OP parsing/token reading and direct fetch path. |
| internal/credentials/age.go | age backend using embedded filippo.io/age, identity discovery, decrypt + optional jq. |
| internal/credentials/age_test.go | Tests identity selection, decryption, security checks, and jq extraction. |
| internal/credentials/bitwarden.go | Bitwarden backend with session persistence, retries on auth errors, secure credential sourcing. |
| internal/credentials/bitwarden_test.go | Tests Bitwarden auth error detection, retry/backoff, cancellation, and integration skip path. |
| internal/credentials/binary_lookup_test.go | Tests override vs trusted lookup behavior for OP/BW binary resolution. |
| internal/credentials/keychain_darwin.go | macOS Keychain backend + KeychainSetup implementation. |
| internal/credentials/keychain_darwin_test.go | Keychain behavior tests, with integration tests gated by env var. |
| internal/credentials/keychain_stub.go | Non-darwin stubs for keychain backend/setup. |
| internal/config/config.go | Adds config fields and getters for OP/BW binaries and OP/age file paths. |
| internal/config/config_test.go | Tests for new config getters and path validation. |
| cmd/claw-wrap/main.go | Adds keychain-setup command and safer install behavior with conflicts/--force. |
| cmd/claw-wrap/keychain_darwin.go | Implements claw-wrap keychain-setup command (darwin-only). |
| cmd/claw-wrap/keychain_stub.go | Disables keychain-setup on non-darwin. |
| cmd/claw-wrap/install_test.go | Tests install conflict/idempotency/force-replace behavior. |
| docs/CONFIG.md | Documents new backends, jq extraction, and trusted lookup behavior/config keys. |
| docs/INSTALL.md | Documents install conflict handling, idempotency, and --force. |
| go.mod | Adds dependencies for age + gojq and updates direct/indirect requirements. |
| go.sum | Adds checksums for new dependencies. |
| .gitignore | Ignores /tmp/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/config/config.go
Outdated
| // GetOPBinary returns the configured 1Password CLI binary path or empty for PATH lookup. | ||
| // If configured, the returned path must be absolute. | ||
| func (c *Config) GetOPBinary() string { | ||
| if c.Proxy != nil && c.Proxy.OPBinary != "" { | ||
| if !filepath.IsAbs(c.Proxy.OPBinary) { | ||
| log.Printf("[WARN] op_binary %q is not absolute, using PATH lookup", c.Proxy.OPBinary) | ||
| return "" | ||
| } |
There was a problem hiding this comment.
GetOPBinary’s docstring and warning log say the empty return value triggers “PATH lookup”, but credential backends actually resolve binaries via paths.FindTrustedBinary (trusted dirs only), not $PATH. This wording is misleading for operators troubleshooting why op isn’t found; consider updating the comment/log to reflect trusted-directory lookup.
| // Configure credential backends | ||
| setAgeIdentityFileFunc(newCfg.GetAgeIdentityFile()) | ||
| setOPTokenFileFunc(newCfg.GetOPTokenFile()) | ||
|
|
There was a problem hiding this comment.
reloadConfig updates credential backend globals (age identity path / 1Password token file) while request handling goroutines can concurrently fetch credentials. Since credentials.SetAgeIdentityFile / SetOPTokenFile mutate package-level variables without synchronization, this can trigger Go data races (and undefined behavior under -race). Consider making these settings immutable after startup, or guard them with a mutex/atomic in the credentials package, or pass the paths via FetchOptions/context instead of global vars.
| func removeInstallTarget(path string, info os.FileInfo) error { | ||
| if info.IsDir() { | ||
| return os.RemoveAll(path) | ||
| } | ||
| return os.Remove(path) |
There was a problem hiding this comment.
removeInstallTarget deletes directories with os.RemoveAll when --force is used. If a tool name collides with a directory under the install dir, this can wipe arbitrary directory contents, which is unusually destructive for an install command. Consider refusing to remove directories (even with --force), or require an explicit separate flag for recursive removal.
| daemon Start the secrets daemon | ||
| list List configured tools | ||
| check Verify all credentials are accessible | ||
| install Create symlinks for all tools | ||
| %s version Show version | ||
| help Show this help |
There was a problem hiding this comment.
The CLI help output lists install but no longer documents its supported flags (--install-dir, --config, --force). This makes the new safer conflict behavior and override mechanism hard to discover. Consider updating the printHelp text to include the flags and brief descriptions.
- Fix misleading "PATH lookup" wording → "trusted-directory lookup" - Add mutex protection for credential path globals (data race fix) - Use io.ReadAll instead of single file.Read (partial read fix) - Refuse to remove directories in install --force (safety) - Document install flags in help text - Use strconv.Itoa for exit codes in test (>9 support)
Summary
Add support for additional credential sources beyond
pass:op://) - with configurable token file pathage:) - embedded filippo.io/age library, no external binarykeychain:) - withkeychain-setupCLI commandbw:) - with session persistence across requestsAll backends support jq extraction via pipe syntax:
Security hardening
/usr/bin,/usr/local/bin,/opt/homebrew/bin,/home/linuxbrew/.linuxbrew/bin)O_NOFOLLOWand permission validation (0600/0640) for credential filesNew CLI commands
keychain-setup <service-name>(macOS only) - create keychain items with ACL for claw-wrapNew config options
Test plan