Skip to content

feat(auth): Use lox keyring providers#898

Open
lox wants to merge 1 commit into
openclaw:mainfrom
lox:lox/keyring-v2-defaults
Open

feat(auth): Use lox keyring providers#898
lox wants to merge 1 commit into
openclaw:mainfrom
lox:lox/keyring-v2-defaults

Conversation

@lox

@lox lox commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

gog still depends on github.com/99designs/keyring, which is effectively abandoned and has accumulated unpatched security/dependency issues. The maintained fork is github.com/lox/keyring/v2, so this moves gog onto that supported module path instead of carrying risk from the old upstream.

gog's auth secret store also carried the old keyring adapter shape locally: provider ordering, file fallback setup, operation timeout wrapping, and the 99designs-compatible method surface. That made it harder to use the shared lox keyring packages without copying app-specific glue around.

This moves gog onto github.com/lox/keyring/v2, github.com/lox/keyring-defaults, and the external github.com/lox/keyring-1password provider. gog keeps the pieces that are actually app policy: service name selection, the encrypted file fallback prompt/dir, legacy file-key compatibility, locking, and token/default-account semantics.

For users, the existing auto, keychain, and file behavior is preserved, while gog auth keyring 1password and GOG_KEYRING_BACKEND=1password are available through the shared provider. The macOS Keychain trust behavior remains explicit via defaults.KeychainTrustApplication(false), and operation timeouts now use keyring.Timeout from core.

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 4, 2026, 7:44 AM ET / 11:44 UTC.

Summary
The PR migrates gog from 99designs/keyring to lox keyring packages, preserves file/keychain policy through the new provider API, and adds 1password as an accepted keyring backend.

Reproducibility: not applicable. This PR proposes a dependency/provider migration and a new accepted GOG_KEYRING_BACKEND=1password value, not a reproduced bug report. The relevant verification is upgrade and runtime behavior proof for the auth secret store.

Review metrics: 3 noteworthy metrics.

  • Patch surface: 38 files, +379/-507. The diff is a broad auth secret-store migration across dependencies, backend code, CLI help, docs, and tests.
  • Credential-path modules added: 3 direct lox modules, 7 indirect provider modules. The dependency set sits in OAuth token and client-secret storage, so provenance and upgrade behavior need maintainer attention.
  • Current proof artifacts: 0 real behavior proofs in PR body/comments. CI is green, but contributor-visible runtime proof is still missing for the changed auth backends.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup 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 output or logs for upgrade-style file/keychain token access and gog auth keyring 1password in a real setup; updating the PR body should trigger a fresh review, or a maintainer can comment @clawsweeper re-review.
  • Get an explicit maintainer decision on accepting the lox keyring and 1Password provider dependency boundary versus splitting or redirecting the 1Password work.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Only CI/check output is visible; the PR needs redacted terminal output, logs, or live output showing existing file/keychain auth and the new 1Password backend after the change, with private data redacted before posting. 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 move OAuth refresh-token storage and backend routing onto new lox keyring modules, including a 1Password provider dependency, without maintainer-visible runtime proof in the PR.
  • [P1] Because this is credential-path code, green CI does not settle dependency trust, upgrade behavior, or whether the 1Password provider belongs in core versus the narrower open 1Password PR.
  • [P1] Existing users' stored tokens depend on file/keychain compatibility behavior; the source looks intentional, but upgrade proof for existing file/keychain stores is still missing.

Maintainer options:

  1. Approve the lox keyring migration with proof (recommended)
    Accept the new credential-path dependency set only after maintainers review the supply-chain boundary and the contributor adds redacted runtime proof for file, keychain, and 1Password behavior.
  2. Split 1Password from the core migration
    Ask for a narrower PR that migrates the maintained keyring fork while leaving 1Password backend direction to the existing open 1Password discussion.
  3. Pause in favor of the existing 1Password PR
    If maintainers prefer the dedicated backend design, keep this PR paused or close it after the overlapping direction is resolved in feat(auth): Add 1Password keyring backend #897.

Next step before merge

  • [P1] The remaining blocker is maintainer dependency/security acceptance plus contributor-provided runtime proof, not a narrow mechanical repair ClawSweeper should queue.

Security
Needs attention: The diff changes the credential storage boundary by adding lox keyring packages and a 1Password provider path, so maintainers should explicitly approve the dependency trust model before merge.

Review details

Best possible solution:

Land only after maintainers explicitly accept the lox keyring dependency set and the PR includes redacted real-run proof for existing file/keychain access plus the new 1Password backend, or split the work so dependency migration and 1Password product direction can be decided separately.

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

Not applicable: this PR proposes a dependency/provider migration and a new accepted GOG_KEYRING_BACKEND=1password value, not a reproduced bug report. The relevant verification is upgrade and runtime behavior proof for the auth secret store.

Is this the best way to solve the issue?

Unclear: the source-level migration is coherent and current main has no lox keyring implementation, but the credential-path dependency change and overlapping 1Password direction require maintainer approval before this can be considered the best solution.

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 auth-provider improvement with meaningful credential-path risk but no current user regression.
  • add merge-risk: 🚨 compatibility: The PR changes file/keychain storage implementation details that existing users depend on during upgrade.
  • add merge-risk: 🚨 auth-provider: The PR changes keyring backend routing used for OAuth refresh tokens, default account state, and client secrets.
  • add merge-risk: 🚨 security-boundary: The PR introduces new third-party modules into the credential storage boundary, including a 1Password provider path.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; 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: Only CI/check output is visible; the PR needs redacted terminal output, logs, or live output showing existing file/keychain auth and the new 1Password backend after the change, with private data redacted before posting. 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-provider improvement with meaningful credential-path risk but no current user regression.
  • merge-risk: 🚨 auth-provider: The PR changes keyring backend routing used for OAuth refresh tokens, default account state, and client secrets.
  • merge-risk: 🚨 security-boundary: The PR introduces new third-party modules into the credential storage boundary, including a 1Password provider path.
  • merge-risk: 🚨 compatibility: The PR changes file/keychain storage implementation details that existing users depend on during upgrade.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; 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: Only CI/check output is visible; the PR needs redacted terminal output, logs, or live output showing existing file/keychain auth and the new 1Password backend after the change, with private data redacted before posting. 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 new credential-path dependencies — go.mod:9
    go.mod replaces the old keyring module with lox keyring packages and brings a 1Password provider into the OAuth token storage path; no malicious behavior is evident, but this is supply-chain-sensitive and needs maintainer trust review.
    Confidence: 0.88

What I checked:

  • Repository policy read: AGENTS.md was read fully and its PR review-mode instruction to inspect via gh pr view/gh pr diff without changing branches was applied. (AGENTS.md:34, c0aac99f6be6)
  • Current main still uses old keyring module: Current main still requires/imports github.com/99designs/keyring, so this PR's central dependency migration is not implemented on the default branch. (go.mod:8, c0aac99f6be6)
  • PR adds credential-path modules: The PR head adds direct lox keyring modules and an indirect native 1Password SDK dependency in the auth secret-storage dependency path. (go.mod:9, 8a5f2fae03ca)
  • PR exposes 1Password backend selection: The PR head updates gog auth keyring help and accepted values to include 1password. (internal/cmd/auth_keyring.go:72, 8a5f2fae03ca)
  • Sensitive implementation path inspected: The PR head routes allowed backends through github.com/lox/keyring-1password, keyring-defaults, and keyring/v2, while preserving app policy around service name, file fallback, and timeouts. (internal/secrets/backend.go:117, 8a5f2fae03ca)
  • Related open PR found: A related search found feat(auth): Add 1Password keyring backend #897, an open 1Password backend PR by the same contributor that already has a needs-proof/security-sensitive review, so it is overlapping context rather than a safe superseding close target. (7fe601bd4647)

Likely related people:

  • steipete: Current-main history shows repeated work on keyring backend policy, auth store injection, file-key compatibility, and release updates in the touched auth/secrets area. (role: recent area contributor; confidence: high; commits: f20a26cd9e26, 92abba72531b, 716f6c2cf14a; files: internal/secrets/backend.go, internal/secrets/file_keyring_safe.go, internal/secrets/store.go)
  • lox: Beyond authoring this PR, lox authored merged keyring prompt-storm work and the related open 1Password backend feature line. (role: prior keyring contributor and feature proposer; confidence: high; commits: 8f614ae204c0, 8a5f2fae03ca, 66a7b9f76839; files: internal/secrets/token.go, internal/secrets/timeout_keyring.go, internal/secrets/backend.go)
  • malob: Recent merged work added configurable keyring open timeouts and the macOS 30s default that this PR migrates onto the lox timeout API. (role: adjacent keyring timeout contributor; confidence: medium; commits: b3c285613b74; files: internal/secrets/backend.go, internal/secrets/timeout_keyring.go, docs/spec.md)
  • salmonumbrella: Prior merged work handled headless Linux keyring fallback behavior in the same backend selection area affected by the migration. (role: adjacent backend-selection contributor; confidence: medium; commits: 64aa17f16772; files: internal/secrets/backend.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: 🧂 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. 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
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: 🧂 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