Skip to content

feat(auth): Add 1Password keyring backend#897

Open
lox wants to merge 6 commits into
openclaw:mainfrom
lox:lox/onepassword-backend
Open

feat(auth): Add 1Password keyring backend#897
lox wants to merge 6 commits into
openclaw:mainfrom
lox:lox/onepassword-backend

Conversation

@lox

@lox lox commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

The previous 1Password backend PR was closed because the official SDK added too much binary and dependency weight to the CLI. This keeps the same opt-in keyring backend behavior, but switches the implementation to github.com/lox/onepassword-sdk-native-go so gog does not embed the official SDK's WASM runtime stack.

Measured on this branch against the commit immediately before the SDK swap:

official SDK binary: 70,982,306 bytes
native SDK binary:   56,363,970 bytes
reduction:           14,618,336 bytes

official SDK module: 9,564 KiB
native SDK module:     512 KiB

official largest file: 9,496,396 bytes internal/wasm/core.wasm
native largest file:      43,834 bytes types.go

go.mod now has one new direct dependency on the native SDK and no longer needs github.com/1password/onepassword-sdk-go, github.com/tetratelabs/wazero, github.com/extism/go-sdk, github.com/tetratelabs/wabin, or the related WASM/Extism support packages.

The feature remains opt-in: users configure GOG_KEYRING_BACKEND=1password or gog auth keyring 1password, then set the 1Password vault/account/auth-mode config for either desktop app integration or service-account auth. Static/no-CGO builds still fail closed for desktop-app mode, while service-account mode remains available.

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 4, 2026, 5:49 AM ET / 09:49 UTC.

Summary
Adds an opt-in 1password keyring backend with config/env settings, auth doctor checks, docs, tests, and a new direct dependency on github.com/lox/onepassword-sdk-native-go.

Reproducibility: not applicable. this is a feature PR rather than a reported bug. The relevant check is after-fix runtime proof for the new native-SDK backend, and that proof is missing from the current PR.

Review metrics: 3 noteworthy metrics.

  • Patch surface: 23 files, +2032/-49. The PR spans backend code, config keys, auth doctor behavior, docs, tests, lint policy, and module metadata.
  • Direct dependency added: 1 new direct module. The new dependency is in the credential-storage path, so dependency provenance matters beyond ordinary feature code.
  • Current proof artifacts: 0 native-SDK live proofs in this PR. The PR body has size measurements, but no current-head runtime output for service-account or desktop-app storage behavior.

Root-cause cluster
Relationship: canonical
Canonical: #897
Summary: This PR is the open successor for the 1Password backend idea; the closed predecessor used the official SDK and was stopped for binary-size/product-direction reasons.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted live proof for the native-SDK branch, preferably terminal output for service-account auth and desktop-app auth if available.
  • Ask a maintainer to explicitly choose the native SDK path, an op CLI design, or closure of the SDK-backed direction.
  • [P2] Fix the 1Password default timeout so behavior matches the new docs on macOS.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR has size measurements and references prior work, but no terminal output, logs, screenshot, recording, or artifact showing the native-SDK backend working after this dependency swap. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging this PR would put an unofficial SDK, from a repository created on 2026-07-04 and owned by the PR author, directly in the token/default-account/client-secret storage path.
  • [P1] The predecessor PR was closed after maintainer feedback preferred shelling out to op; this branch reduces binary size versus the official SDK but still needs an explicit decision that the native SDK approach is acceptable.
  • [P1] The backend is opt-in, but users who switch to it do not get migration of existing tokens, defaults, or client credentials, so the upgrade guidance and failure mode need maintainer acceptance.
  • [P1] The current PR lacks live native-SDK behavior proof; prior proof from the closed official-SDK PR does not prove this dependency swap.

Maintainer options:

  1. Accept the native SDK backend deliberately
    A maintainer can approve the unofficial native SDK, API terms boundary, no-migration behavior, and credential-path dependency risk as an intentional opt-in backend tradeoff.
  2. Request the op CLI design
    Ask the contributor to replace the embedded SDK path with an op-backed integration, matching the direction recorded on the closed predecessor PR.
  3. Pause or close the SDK-backed approach
    If the dependency and product tradeoff is not acceptable for core, close this PR and invite a narrower design or external integration path.

Next step before merge

  • [P1] The remaining blocker is maintainer product/security acceptance plus contributor-provided real behavior proof, not a repair ClawSweeper can safely make automatically.

Security
Needs attention: The diff adds a security-sensitive unofficial SDK dependency to credential storage and needs explicit maintainer acceptance before merge.

Review findings

  • [P3] Use the platform timeout for 1Password defaults — internal/secrets/onepassword_keyring.go:330
Review details

Best possible solution:

Decide whether core should accept this native-SDK backend, ask for an op CLI based design, or close the SDK-backed direction; if it proceeds, require dependency/security acceptance plus redacted live proof for service-account and desktop-app behavior.

Do we have a high-confidence way to reproduce the issue?

Not applicable; this is a feature PR rather than a reported bug. The relevant check is after-fix runtime proof for the new native-SDK backend, and that proof is missing from the current PR.

Is this the best way to solve the issue?

Unclear; the implementation is focused and current main has no equivalent backend, but the recorded maintainer direction on the predecessor favored shelling out to op, so accepting a native SDK still needs product and security approval.

Full review comments:

  • [P3] Use the platform timeout for 1Password defaults — internal/secrets/onepassword_keyring.go:330
    When GOG_1PASSWORD_TIMEOUT and config are unset, this returns keyringOpenTimeout (10s) on every OS. The docs added by this PR say the default follows the platform keyring timeout, including 30s on macOS, so desktop-app calls can time out sooner than documented; pass the resolved open timeout into the 1Password config or use the platform default here.
    Confidence: 0.84

Overall correctness: patch is correct
Overall confidence: 0.82

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c0aac99f6be6.

Label changes

Label changes:

  • add P2: This is a normal-priority opt-in auth backend feature with meaningful security/product review needs but no current user regression.
  • add merge-risk: 🚨 compatibility: Switching backends does not migrate existing keyring entries, so users can lose access to stored credentials until they export/import or reauthorize.
  • add merge-risk: 🚨 auth-provider: The PR changes where gog stores OAuth tokens, default account state, and client secrets when users select the new backend.
  • add merge-risk: 🚨 security-boundary: The new backend adds an unofficial SDK dependency directly to secret storage and must be accepted as a credential-boundary tradeoff.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR has size measurements and references prior work, but no terminal output, logs, screenshot, recording, or artifact showing the native-SDK backend working after this dependency swap. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This is a normal-priority opt-in auth backend feature with meaningful security/product review needs but no current user regression.
  • merge-risk: 🚨 auth-provider: The PR changes where gog stores OAuth tokens, default account state, and client secrets when users select the new backend.
  • merge-risk: 🚨 security-boundary: The new backend adds an unofficial SDK dependency directly to secret storage and must be accepted as a credential-boundary tradeoff.
  • merge-risk: 🚨 compatibility: Switching backends does not migrate existing keyring entries, so users can lose access to stored credentials until they export/import or reauthorize.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR has size measurements and references prior work, but no terminal output, logs, screenshot, recording, or artifact showing the native-SDK backend working after this dependency swap. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

Security concerns:

  • [medium] Vet the unofficial SDK before credential-path use — go.mod:24
    go.mod adds github.com/lox/onepassword-sdk-native-go as a direct dependency for secret storage; the repository was created on 2026-07-04, is owned by the PR author, and its README says it is not supported by 1Password.
    Confidence: 0.9

What I checked:

  • Repository policy read: AGENTS.md was read in full; the read-only PR review flow and auth/secrets caution apply to this review. (AGENTS.md:34, c0aac99f6be6)
  • Current main has no 1Password backend: Current main documents and allows only auto, keychain, and file keyring backends in the existing backend path; rg found no existing onepassword/1password implementation on main. (internal/secrets/backend.go:113, c0aac99f6be6)
  • PR adds a new credential-path dependency: The PR adds github.com/lox/onepassword-sdk-native-go as a new direct dependency at a pseudo-version used by the keyring backend. (go.mod:24, 7fe601bd4647)
  • PR implements 1Password item storage: The new backend stores each gog keyring item in 1Password item fields with the key in username and a base64 credential payload in credential. (internal/secrets/onepassword_keyring.go:619, 7fe601bd4647)
  • Prior PR direction: The predecessor 1Password SDK PR was closed unmerged after a maintainer comment said the binary-size tradeoff meant gog should shell out to op; this PR changes SDK choice rather than adopting that design.
  • Dependency provenance check: The native SDK repository is public, MIT-licensed, owned by the PR author, created on 2026-07-04, and its README says it is unofficial and not supported by 1Password. (go.mod:24, 7fe601bd4647)

Likely related people:

  • steipete: Current-main history shows repeated work on keyring backend policy, auth store injection, and keyring open policy; the prior 1Password PR also records repair and product-direction review from this person. (role: recent area contributor and prior 1Password repair reviewer; confidence: high; commits: 716f6c2cf14a, 93d0c7c179df, 92abba72531b; files: internal/secrets/backend.go, internal/secrets/store.go, internal/cmd/auth_keyring.go)
  • lox: Beyond opening this PR, lox authored merged keyring prompt-storm work and authored the initial 1Password backend commits in this feature line. (role: prior keyring contributor and feature proposer; confidence: high; commits: c58b381c97ad, 66a7b9f76839, 7fe601bd4647; files: internal/secrets/backend.go, internal/secrets/onepassword_keyring.go, internal/cmd/auth_doctor.go)
  • malob: Recent merged work added configurable keyring open timeouts and the macOS 30s default that this PR’s new backend extends and partially bypasses. (role: adjacent keyring timeout contributor; confidence: medium; commits: b3c285613b74; files: internal/secrets/backend.go, internal/secrets/timeout_keyring.go, docs/spec.md)
  • TurboTheTurtle: Recent merged auth work repaired duplicate token alias writes in the same secrets store write path that this PR changes with trusted 1Password writes. (role: adjacent token-store contributor; confidence: medium; commits: ebc8218b609a; files: internal/secrets/store.go, internal/secrets/store_more_test.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 4, 2026
@lox

lox commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

@steipete totally happy to provide an option for just using op directly. The UX of this is slightly nicer IMO in terms of the clarity in what you are approving.

The other angle I am working on is I've forked the dead https://github.com/99designs/keyring that this project depends on (I was the original author at my first startup) into https://github.com/lox/keyring. It would provide a unified interface to the various keyring providers (including 1password) that this cli uses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants