Skip to content

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

Closed
lox wants to merge 1 commit into
openclaw:mainfrom
lox:lox/onepassword
Closed

feat(auth): Add 1Password keyring backend#891
lox wants to merge 1 commit into
openclaw:mainfrom
lox:lox/onepassword

Conversation

@lox

@lox lox commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Local and agent workflows currently have to choose between OS keychains, which can prompt repeatedly or behave poorly in headless contexts, and the encrypted file backend, which requires distributing GOG_KEYRING_PASSWORD. This adds a 1Password-backed option so gog auth material can live in a configured 1Password vault while local desktop workflows can use the 1Password app instead of a service account token.

This change adds a 1password keyring backend using the 1Password Go SDK. It supports desktop-app auth through onepassword_account / GOG_1PASSWORD_ACCOUNT, service-account auth through OP_SERVICE_ACCOUNT_TOKEN, and vault/item selection through config or environment. 1Password items are API Credential entries titled gogcli-keyring by default, with the keyring key in username and the encoded payload in the concealed credential field.

Example local setup:

gog auth keyring 1password
gog config set onepassword_auth desktop
gog config set onepassword_account xxx.1password.com
gog config set onepassword_vault <vault-id>

Repeated short-lived subprocesses can also opt in to a state-dir access-token cache:

gog config set access_token_cache true

The cache stores only short-lived Google access tokens, checks it before reopening the refresh-token keyring, and clears it when stored auth is removed. That keeps agent-style Gmail/Calendar loops from repeatedly asking for 1Password approval while preserving 1Password as the source of truth for refresh tokens.

A couple of guardrails are included for review:

  • 1Password only treats tagged gogcli-keyring API Credential items as managed, except for an empty starter template that is tagged on first use.
  • OP_SERVICE_ACCOUNT_TOKEN remains environment-only; non-secret selectors can live in config.json.
  • auth doctor reports the selected 1Password auth mode and missing vault/account/token configuration.

@clawsweeper

clawsweeper Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 7:11 AM ET / 11:11 UTC.

Summary
The PR adds a 1password keyring backend, related config/env keys, auth-doctor checks, optional access-token caching, tests, docs, and a new 1Password SDK dependency.

Reproducibility: not applicable. for the feature request itself; the patch bug is source-reproducible because the new cache hit returns before stored-grant validation and cache entries record requested scopes. I did not run a live Google/1Password setup in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Touched surface: 31 files, +2201/-49. The branch spans auth code, secrets storage, OAuth token sourcing, config, docs, tests, and dependencies.
  • Dependency delta: 1 direct SDK added, 7 indirect modules added in go.mod. The new dependency tree is in the credential-storage path and needs maintainer-visible supply-chain review.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
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 terminal output or a short recording showing a real gog auth keyring 1password setup storing and reading auth through 1Password.
  • [P1] Fix the access-token cache so cache hits cannot bypass stored-grant semantics.
  • Get maintainer/security acceptance or resolution for the Socket warnings on the new dependency chain.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body and comments do not include redacted terminal output, logs, a recording, or another artifact showing the new 1Password backend working in a real setup after the change. 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] The access-token cache can return before stored OAuth scope validation and currently records requested scopes, so an opt-in cache may mask insufficient-grant checks until the cached token expires.
  • [P1] The new 1Password SDK dependency chain sits directly on credential storage and has Socket warnings that need explicit maintainer/security acceptance.
  • [P1] No redacted real setup proof shows gog storing and reading auth through 1Password with either desktop-app or service-account auth.

Maintainer options:

  1. Fix cache scope validation first (recommended)
    Make cached access tokens prove observed granted scopes or defer cache hits until the stored-grant check has run, then add focused regression coverage.
  2. Accept the SDK surface explicitly
    Maintainers can proceed with the new 1Password SDK chain only after reviewing or triaging the Socket warnings for the credential-storage path.
  3. Split the cache from the backend
    If the cache semantics are not required for the first 1Password backend landing, pause that part and land a smaller backend-only PR after proof and dependency review.

Next step before merge

  • [P1] Maintainers need to decide dependency/security acceptance and the contributor must add real 1Password runtime proof; automation cannot supply those missing merge inputs.

Security
Needs attention: The diff adds a new credential-storage SDK dependency chain with Socket warnings that need maintainer/security review before merge.

Review findings

  • [P2] Preserve scope checks on cache hits — internal/googleapi/client_auth.go:229-231
Review details

Best possible solution:

Land only after the cache preserves stored-grant semantics, the PR includes redacted real 1Password auth proof, and maintainers explicitly accept the new SDK dependency surface.

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

Not applicable for the feature request itself; the patch bug is source-reproducible because the new cache hit returns before stored-grant validation and cache entries record requested scopes. I did not run a live Google/1Password setup in this read-only review.

Is this the best way to solve the issue?

No; the 1Password backend may be a useful direction, but the cache needs to preserve grant-check semantics and the new SDK dependency needs explicit acceptance before this is the best merge path.

Full review comments:

  • [P2] Preserve scope checks on cache hits — internal/googleapi/client_auth.go:229-231
    The new cache returns a token source before opening stored auth and before requireStoredGrant validation. Since cache entries are written with the requested scopes rather than scopes observed from the token response, a stale or over-broad cache entry can bypass the insufficient-grant error path and send callers to the API with a token that may not actually cover the requested service. Please cache/validate observed granted scopes, or do not use the cache before the stored-grant check can run.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal-priority auth backend feature with bounded but meaningful credential-storage review risk.
  • add merge-risk: 🚨 auth-provider: The PR changes OAuth token source behavior and can bypass stored-grant validation through the new access-token cache.
  • add merge-risk: 🚨 security-boundary: The PR adds a new third-party SDK chain to the path that stores and retrieves auth material.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • 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 body and comments do not include redacted terminal output, logs, a recording, or another artifact showing the new 1Password backend working in a real setup after the change. 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 auth backend feature with bounded but meaningful credential-storage review risk.
  • merge-risk: 🚨 auth-provider: The PR changes OAuth token source behavior and can bypass stored-grant validation through the new access-token cache.
  • merge-risk: 🚨 security-boundary: The PR adds a new third-party SDK chain to the path that stores and retrieves auth material.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body and comments do not include redacted terminal output, logs, a recording, or another artifact showing the new 1Password backend working in a real setup after the change. 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] Review the new 1Password SDK dependency chain — go.mod:8
    go.mod adds github.com/1password/onepassword-sdk-go plus transitive modules including Extism, wazero, and wabin; Socket flagged high obfuscated-code warnings for the SDK path and wazero plus a deprecated wabin dependency, so this credential-path supply-chain surface needs explicit acceptance or a narrower dependency strategy.
    Confidence: 0.86

What I checked:

  • Repository policy read: AGENTS.md was read fully; its PR review-mode and auth/security guidance applies to this read-only review. (AGENTS.md:34, 213ddb60d7d1)
  • Current main lacks the requested backend: Current main still depends directly on github.com/99designs/keyring and has no github.com/1password/onepassword-sdk-go dependency. (go.mod:8, 213ddb60d7d1)
  • Current backend choices are limited: Current main accepts only auto, keychain, and file backends through the existing keyring backend policy. (internal/secrets/backend.go:113, 213ddb60d7d1)
  • PR adds a new credential-path dependency tree: The branch adds github.com/1password/onepassword-sdk-go plus indirect modules including Extism, wazero, and wabin. (go.mod:8, e69384decbd1)
  • Access-token cache bypasses the stored-grant check: The PR checks the access-token cache before opening stored auth and before the requireStoredGrant scope validation path. (internal/googleapi/client_auth.go:229, e69384decbd1)
  • Cache records requested scopes: The new cache entry stores the requested scope list rather than scopes observed from the returned OAuth token. (internal/googleapi/access_token_cache.go:99, e69384decbd1)

Likely related people:

  • steipete: GitHub commit history shows recent work across the keyring backend, secrets store, auth dependency injection, and OAuth scope reconciliation paths touched or affected by this PR. (role: recent area contributor; confidence: high; commits: 716f6c2cf14a, 93d0c7c179df, 6bd333a1e141; files: internal/secrets/backend.go, internal/secrets/store.go, internal/googleapi/client_auth.go)
  • malob: Recent history shows keyring timeout and prompt behavior work in the same backend policy file that the PR extends. (role: recent keyring contributor; confidence: medium; commits: b3c285613b74; files: internal/secrets/backend.go)
  • TurboTheTurtle: Recent token alias repair work touched the secrets store behavior that this PR changes through trusted writes and the new backend. (role: adjacent token-store contributor; confidence: medium; commits: ebc8218b609a; files: internal/secrets/store.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.

@socket-security

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedgolang/​github.com/​1password/​onepassword-sdk-go@​v0.4.1-0.20260605221002-f1117e36ce0688100100100100

View full report

@socket-security

Copy link
Copy Markdown

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.

Action Severity Alert  (click "▶" to expand/collapse)
Warn High
Obfuscated code: golang github.com/1password/onepassword-sdk-go is 90.0% likely obfuscated

Confidence: 0.90

Location: Package overview

From: go.modgolang/github.com/1password/onepassword-sdk-go@v0.4.1-0.20260605221002-f1117e36ce06

ℹ Read more on: This package | This alert | What is obfuscated code?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore golang/github.com/1password/onepassword-sdk-go@v0.4.1-0.20260605221002-f1117e36ce06. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

Warn High
Obfuscated code: golang github.com/tetratelabs/wazero is 90.0% likely obfuscated

Confidence: 0.90

Location: Package overview

From: ?golang/github.com/1password/onepassword-sdk-go@v0.4.1-0.20260605221002-f1117e36ce06golang/github.com/tetratelabs/wazero@v1.11.0

ℹ Read more on: This package | This alert | What is obfuscated code?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore golang/github.com/tetratelabs/wazero@v1.11.0. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

Warn Medium
Deprecated by its maintainer: golang github.com/tetratelabs/wabin

Reason: Repository has been archived by the owner.

From: ?golang/github.com/1password/onepassword-sdk-go@v0.4.1-0.20260605221002-f1117e36ce06golang/github.com/tetratelabs/wabin@v0.0.0-20230304001439-f6f874872834

ℹ Read more on: This package | This alert | What is a deprecated package?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Research the state of the package and determine if there are non-deprecated versions that can be used, or if it should be replaced with a new, supported solution.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore golang/github.com/tetratelabs/wabin@v0.0.0-20230304001439-f6f874872834. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@lox

lox commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Could break this up into a smaller one @steipete if that makes it easier, but without the caching it's annoying to have at approve every cli invocation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 30, 2026
@lox

lox commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Going to break this up into smaller pieces.

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

Labels

rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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.

1 participant