feat(providers): store refresh credentials in credential drivers - #2801
Conversation
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
|
Label |
|
/ok to test 0541d45 |
|
Bot review from gpt-5.6-sol high. These concern the migration boundary from the old to new way. I'm not sure how much we care about that in alpha. [P1] Gate legacy migration before allowing mixed-version writers — crates/openshell-server/src/provider_refresh.rs:1637 The worker immediately migrates every inline refresh record and removes its secret values without any cluster-version check or operator-controlled migration gate. During a normal rolling upgrade, an older replica can then decode the migrated row while discarding fields 19 and 20 and persist a refresh/error update containing neither the inline secret nor its new handle, permanently breaking that grant. The documentation tells operators to stop old replicas, but the automatic first sweep does not enforce that prerequisite, so migration needs a rollout gate before it can rewrite records. [P1] Keep legacy credential drivers available while handles are migrated — crates/openshell-server/src/credentials.rs:466 Refresh material is resolved by feeding its persisted handles into Overall assessment: The same-version, single-driver lifecycle is carefully staged and has strong CAS/cleanup coverage, but the two transition paths above can lose refresh availability and should be fixed before merge. CI is green at reviewed head |
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
|
Thanks for flagging these. Agree these are migration-boundary concerns, but OpenShell does not intend to support refresh-state or credential-driver migrations before the 0.1.0 compatibility boundary. In 16005c6 I removed the automatic inline refresh-material migration from this PR, so the worker no longer rewrites legacy rows during an upgrade. I also documented that credential handles remain bound to the driver that created them: operators must remove or reconfigure affected credentials while the original driver is available before changing backends, and mixed gateway versions must not operate on the same refresh records. |
|
/ok to test 16005c6 |
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR implements accepted issue #2797 and is project-valid provider/gateway work.
Head SHA: 16005c676f057adda8a676306508b03d71924388
Base SHA: 8d67250a5d17348eb96c4fa46226b06d8041f2ba
Merge base SHA: 8d67250a5d17348eb96c4fa46226b06d8041f2ba
Patch ID: 0260c2a942a5cb7e1d4416c1fa8f274e484ace08
Gator payload: 4
Review mode: initial
Previous reviewed SHA: none
Review budget exhausted: no
Maintainer decision required: no
Thanks @mrunalp. I checked your migration-boundary response to @sjenning against current head: automatic inline migration is removed, and the published gateway/provider docs now require grant reconfiguration, original-driver cleanup, and no mixed gateway versions. Those earlier migration concerns do not remain blockers.
Blocking findings:
GATOR-16005c67-01: Concurrent first-time refresh configuration across gateway replicas can orphan staged long-lived secrets because the absent-state path uses a last-writer-wins upsert. See the inline finding.
Carried findings:
- None.
Non-blocking suggestions:
GATOR-16005c67-02: Align.agents/skills/openshell-cli/SKILL.mdand the proto comment with the current pre-0.1.0 policy: no automatic inline refresh migration; operators must reconfigure grants. Regenerate the Go protobuf if the proto comment changes.
Docs: Fern gateway/provider documentation covers the user-visible storage and migration behavior; the agent-skill wording above remains a non-blocking consistency fix.
Next state: gator:in-review
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
|
/ok to test 547b3f0 |
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR implements accepted issue #2797 and remains project-valid provider/gateway work.
Head SHA: 547b3f0a475c8d94d9b639c066326602a1f02f42
Base SHA: 8d67250a5d17348eb96c4fa46226b06d8041f2ba
Merge base SHA: 8d67250a5d17348eb96c4fa46226b06d8041f2ba
Patch ID: 808f99ab748b0608525acd701ead6e8e05411ae9
Gator payload: 4
Review mode: follow_up
Previous reviewed SHA: 16005c676f057adda8a676306508b03d71924388
Review budget exhausted: no
Maintainer decision required: no
Thanks @mrunalp. I checked the new atomic refresh-state create path, both persistence backends, losing-request secret cleanup, the two-replica race coverage, and the skill/protobuf migration wording.
Blocking findings:
- No blocking findings remain.
Carried findings:
GATOR-16005c67-01: resolved. The absent-state branch now uses an insert-only scoped write; a losing concurrent request receivesABORTEDand cleans up its staged handles, while the winner remains the sole durable state and backend object.
Docs: Fern gateway/provider documentation remains sufficient, and the latest delta aligns the CLI skill and protobuf comment with the documented pre-0.1.0 migration boundary.
Test dispatch: test:e2e is applied and /ok to test 547b3f0a475c8d94d9b639c066326602a1f02f42 has been posted. Current-head Branch Checks, Helm Lint, and E2E workflows are not yet visible as queued or running.
Next state: gator:in-review pending confirmed current-head test dispatch; advance to gator:watch-pipeline once the required workflows are queued, running, or complete.
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: Gator review completed with no remaining blocking findings, required checks passed, and maintainer approval was present before merge. I removed the active |
Summary
Store provider refresh secrets through the gateway's active credential driver instead of embedding them in refresh-state records. This keeps OAuth refresh tokens, client secrets, service-account private keys, temporary AWS source credentials, and minted access tokens behind driver-owned handles while preserving the existing refresh CLI workflow.
Related Issue
Closes #2797
Changes
Testing
mise run pre-commitpassesmise run cipassesgogand OpenClawThe manual E2E used a loopback-only Vault 1.20.4 dev container and an isolated Podman gateway. Existing refresh inputs migrated into Vault, forced rotation succeeded without accumulating stale access-token objects, the Gmail probe succeeded before and after rotation, and OpenClaw returned
EMAIL_ACCESS_OKwithout receiving the refresh token.Checklist