feat(auth): Add 1Password keyring backend#894
Conversation
6582f40 to
73a3421
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 10:37 PM ET / 02:37 UTC. Summary Reproducibility: not applicable. this is a feature PR rather than a bug report. The relevant proof is after-fix runtime validation, which the PR body and contributor comment provide for the new backend paths. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Keep the branch open and require an explicit owner decision to accept this SDK-backed core backend as-is, request an Do we have a high-confidence way to reproduce the issue? Not applicable; this is a feature PR rather than a bug report. The relevant proof is after-fix runtime validation, which the PR body and contributor comment provide for the new backend paths. Is this the best way to solve the issue? Unclear as a product choice: the implementation appears coherent and current main has no equivalent backend, but maintainers still need to decide whether a core SDK-backed integration is preferable to a narrower AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 621757c686ee. Label changesLabel changes:
Label justifications:
Evidence reviewedSecurity concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
Fixed the namespace behavior in 335034e: the 1Password backend now receives the existing keyring service name from serviceNameFor(options), derives the default item title from it, and still lets GOG_1PASSWORD_ITEM_TITLE/config onepassword_item_title override that default.\n\nValidated locally with:\n\n |
|
Terminal output proof: |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Co-authored-by: Lachlan Donald <lachlan+codex@buildkite.com>
335034e to
7d72c83
Compare
|
Maintainer repair is complete at exact head The repaired branch passes full local CI, source-blind service-account import/read/doctor/remove/cleanup proof, static Linux build/tests, forced-GC lifecycle regression, and autoreview. It also fixes a real SDK ownership failure found during live proof ( Owner decision still required before merge: accept the unreleased 1Password SDK pseudo-version, embedded WASM/Extism dependency chain and Socket alerts, 15.2 MB (+27.3%) binary growth, API-terms boundary, and one-item-per-key/no-cache design; otherwise request a narrower |
|
@steipete did I miss a linter somewhere? |
gog currently supports OS keychain and encrypted file storage, but local agent workflows that rely on macOS Keychain can hit repeated approval prompts. This adds an opt-in 1Password keyring backend that stores gog secrets as 1Password API Credential items.
The backend supports desktop-app auth in compatible CGO builds and service-account auth for headless use. Non-secret settings are available through
gog config;OP_SERVICE_ACCOUNT_TOKENremains environment-only.This is the backend-only slice from the earlier cache-plus-backend proposal. It deliberately omits the access-token cache. Switching backends also does not migrate existing tokens, default-account state, or client credentials; export/import is required before switching.
Maintainer repairs
main; kept the contributor commits and credit.0.31.2 - Unreleasedand thanked @lox.Client.Items()does not retain*Client, so GC could release the core client ID and later operations failed withinvalid client id. The adapter now retains the parent client and has a forced-GC regression test. Upstream context: Client not able to retrieve secrets after a while 1Password/onepassword-sdk-go#210Exact-head proof
Candidate:
7d72c8377b4710e9bc445e7c47b5da8539e4e2d9make ci: pass.CGO_ENABLED=0 go test ./internal/secrets ./internal/cmd -count=1: pass.GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build ./cmd/gog: pass.--home; syntheticexample.invalidtoken; config/import/list/doctor/remove all passed; exact-title cleanup count zero.v0.31.1-13-g7d72c837, SHA-256b220c03f5b284e32dfbb68ae4193e69fcdeb4588dd2eacf15e16a899c201be2d.Owner decision required
The feature works, but the dependency/product tradeoff is material:
github.com/1password/onepassword-sdk-go@v0.4.1-0.20260605221002-f1117e36ce06; stablev0.4.0lacks the static-build fix used here, so this pins an unreleased pseudo-version.wabin. Socket flags the embedded/obfuscated-code shape and archived transitive dependency.Please explicitly choose one: accept this SDK/core-backend dependency and size/security tradeoff; request a narrower
opCLI or other design; or wait for a tagged SDK release/dependency reduction. This PR should remain unmerged until that owner decision.