Skip to content

feat(providers): store refresh credentials in credential drivers - #2801

Merged
johntmyers merged 5 commits into
NVIDIA:mainfrom
mrunalp:2797-vault-refresh-credentials/mrunalp
Aug 19, 2026
Merged

feat(providers): store refresh credentials in credential drivers#2801
johntmyers merged 5 commits into
NVIDIA:mainfrom
mrunalp:2797-vault-refresh-credentials/mrunalp

Conversation

@mrunalp

@mrunalp mrunalp commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

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

  • classify refresh material using caller markings, authoritative provider profiles, and strategy-defined secret fields
  • stage secret writes through the active credential driver and commit only opaque handles to refresh state
  • resolve gateway-only refresh inputs just in time for scheduled or forced mint operations
  • atomically persist issuer-rotated refresh tokens and retry cleanup of superseded handles
  • migrate legacy inline refresh secrets on worker startup while retaining legacy-driver resolution during transitions
  • preserve last-known-good state across concurrent reconfiguration, deletion, failed rotation, and gateway restart scenarios
  • hydrate authoritative resource versions on refresh-state list paths before compare-and-swap updates
  • extend protobuf storage state and regenerate the Go bindings
  • document storage ownership, migration safety, historical database retention, and operator workflows
  • update the OpenShell CLI skill with external-driver refresh behavior and recovery guidance

Testing

  • mise run pre-commit passes
  • mise run ci passes
  • Unit tests added and updated for storage, migration, rotation, concurrency, and cleanup
  • Vault driver tests pass: 16 passed
  • Server suite passes: 1,374 passed, 8 ignored
  • Manual Vault + Podman E2E completed with gog and OpenClaw

The 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_OK without receiving the refresh token.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp
mrunalp requested review from a team, derekwaynecarr and sjenning as code owners August 19, 2026 00:32
@mrunalp mrunalp added area:gateway Gateway server and control-plane work area:providers labels Aug 19, 2026
@copy-pr-bot

copy-pr-bot Bot commented Aug 19, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp mrunalp added the test:e2e Requires end-to-end coverage label Aug 19, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/2801 does not exist yet. A maintainer needs to comment /ok to test 0541d456d41b3937f92b8dba37c51441478529db to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

@mrunalp

mrunalp commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 0541d45

@sjenning

Copy link
Copy Markdown
Collaborator

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 resolve_provider_handles, whose registry accepts only the currently enabled external driver (plus the database driver) and whose runtime connects only configured drivers. After a deliberate backend transition such as Vault to Kubernetes Secrets—or default database storage to Vault without retaining an explicit credential_storage block—existing handle-only refresh states can no longer be resolved or deleted, so scheduled and forced refreshes fail once the current access token expires. The old driver must remain addressable until those handles are copied to the new storage owner and cleaned up.

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 0541d456d41b3937f92b8dba37c51441478529db; material residual gaps are the absence of a mixed-binary rollout test and a credential-driver switch/migration test.

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp

mrunalp commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@mrunalp

mrunalp commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 16005c6

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md and 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

Comment thread crates/openshell-server/src/grpc/provider.rs Outdated
@johntmyers johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Aug 19, 2026
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 547b3f0

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 receives ABORTED and 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.

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:approval-needed Gator completed review; maintainer approval needed and removed gator:in-review Gator is reviewing or awaiting PR review feedback gator:watch-pipeline Gator is monitoring PR CI/CD status labels Aug 19, 2026
@johntmyers
johntmyers added this pull request to the merge queue Aug 19, 2026
@johntmyers johntmyers added gator:merge-ready and removed gator:approval-needed Gator completed review; maintainer approval needed gator:merge-ready labels Aug 19, 2026
@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:merge-ready and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Aug 19, 2026
Merged via the queue into NVIDIA:main with commit 6e90f3d Aug 19, 2026
81 of 88 checks passed
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Monitoring Complete

Monitoring 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 gator:* label because there is nothing left for gator to monitor on this PR.

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

Labels

area:gateway Gateway server and control-plane work area:providers test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: store provider refresh credentials in credential drivers

3 participants