feat(credentials): add provider credential storage drivers - #2437
feat(credentials): add provider credential storage drivers#2437sjenning wants to merge 14 commits into
Conversation
|
Obvious issue left over from the rebase in |
6a10141 to
0d42e4e
Compare
Fixed |
|
Hold on this. It is all messed up. |
0d42e4e to
d1e10d1
Compare
|
/ok to test d1e10d1 |
d1e10d1 to
052540f
Compare
|
/ok to test 052540f |
052540f to
1806c9b
Compare
|
/ok to test 1806c9b |
mrunalp
added
the
gator:in-review
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This is project-valid work for roadmap issue #1931 and implements its gateway-owned credential-storage scope.
Head SHA: 1806c9be07e0307137ac7c0b8cced209018e81fb
Base SHA: b1c7ff684e9b16f88d050c29b0093d07389fbbbb
Merge base SHA: b1c7ff684e9b16f88d050c29b0093d07389fbbbb
Patch ID: 8c7fdb202e6f6004ee9069a12d24c5bb02ffb8c7
Gator payload: 2
Review mode: initial
Previous reviewed SHA: none
Blocking findings:
GATOR-1806c9be-01(Critical): backend object identity is not workspace/provider-unique, allowing a cross-workspace Vault credential collision and disclosure.GATOR-1806c9be-02(Warning): handle-backed credentials bypass persisted provider expiration.GATOR-1806c9be-03(Warning): refresh overwrites the committed backend object before validation/CAS and can delete the still-referenced handle on failure.GATOR-1806c9be-05(Warning): the configured UDS startup timeout does not bound capability negotiation or runtime RPCs.
Carried findings:
- None.
Non-blocking suggestions:
- Add
openshell-driver-db-credstoreto theAGENTS.mdarchitecture inventory and document the new credential-driver Skaffold profiles in the Helm development skill.
Docs: Relevant published provider and gateway-configuration docs are updated.
Next state: gator:in-review
johntmyers
added
gator:blocked
Signed-off-by: Taylor Mutch <taylormutch@gmail.com>
Signed-off-by: Taylor Mutch <taylormutch@gmail.com>
… performance Address review findings from the credential storage drivers PR: - Route additional_credentials through the driver on refresh to prevent silent data loss for multi-credential providers (e.g. AWS STS) - Clean up stored credential handles on CAS failure during refresh to prevent orphaned secrets in external backends - Enforce namespace validation in the Kubernetes Secrets driver to prevent cross-namespace credential access when allow_reference_namespace is not enabled - Cache Vault Kubernetes auth tokens with 80% TTL to avoid re-authenticating on every credential operation - Parallelize resolve_credentials in all three drivers using try_join_all for faster sandbox startup - Add existingSecret support for the KEK Secret to fix helm template/GitOps workflows where lookup returns empty and regenerates the key - Document RBAC blast radius for the Kubernetes Secrets credential driver and recommend a dedicated namespace Signed-off-by: Varsha Prasad <varshaprasad96@gmail.com> Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
…rallelize operations Use resourceVersion optimistic concurrency with retry loop for K8s Secret ownership checks to prevent TOCTOU races. Switch Vault token cache from RwLock to Mutex with double-check pattern to prevent thundering herd on cache miss. Parallelize credential store and delete operations across independent keys using try_join_all. Signed-off-by: Varsha Prasad <varshaprasad96@gmail.com> Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
…ate cleanup Replace try_join_all with join_all in credential store/delete operations to handle partial failures — successfully-stored handles are cleaned up when another key fails. Add retry loop with conflict detection to db-credstore delete_credential, matching the K8s driver pattern. Consolidate 4 manual cleanup_pre_stored_provider_credentials call sites into a single error handler using an async block. Remove inconsistent .trim() from db-credstore validate_handle_owner. Signed-off-by: Varsha Prasad <varshaprasad96@gmail.com> Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Remove attempt-count guard from 409/Aborted match arms in retry loops so the post-loop Status::aborted error is reachable after exhausting retries. Previously, last-attempt conflicts fell through to the catch-all error arm, producing misleading Status::unavailable errors. Remove duplicate validation calls that ran after prepare_provider_credential_update but outside the cleanup-protected async block, which would leak pre-stored handles on failure. Signed-off-by: Varsha Prasad <varshaprasad96@gmail.com> Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
…aths Include workspace and provider ID in credential backend object paths to ensure cross-workspace uniqueness and prevent credential collision (GATOR-1806c9be-01). - Updated credential driver proto to include workspace and provider_id fields - Modified Vault driver to include workspace/provider_id in managed_secret_path - Modified Kubernetes Secrets driver to include workspace/provider_id in credential_owner_id and managed_secret_name - Updated all credential runtime calls to pass workspace/provider_id - Updated tests to use the new signatures This prevents two workspaces sharing the same external credential store from colliding on provider names, which was a critical security issue (CWE-639).
…d credentials Compute effective expiration from both provider and driver values using the earliest non-zero timestamp and skip expired values before insertion (GATOR-1806c9be-02). - Modified resolve_provider_handles to check provider credential_expires_at_ms - Skip expired credentials during resolution instead of returning them - Use effective expiration (min of provider and driver) in resolution results - Fix inference.rs to preserve earliest expiration when merging This ensures handle-backed credentials respect the same expiration semantics as inline credentials.
…dation Stage credential replacements under new immutable handles instead of reusing existing handles to prevent overwriting committed values before validation/CAS (GATOR-1806c9be-03). - Stage credentials with empty existing_handles map to force new handle creation - Validate and CAS before the new values are committed to backend storage - Delete old handles only after successful CAS - On CAS failure, delete only the newly staged handles - This prevents CWE-362/CWE-367 race conditions where failed refreshes could still modify or delete the active credential The fix ensures that a rejected refresh cannot modify the backend object still referenced by the committed provider record.
Apply configured timeouts to both startup capability negotiation and runtime RPCs to prevent indefinite hangs (GATOR-1806c9be-05). - Add DEFAULT_CREDENTIAL_DRIVER_RPC_TIMEOUT_SECS constant (30s) - Apply timeout to GetCapabilities during startup connection - Apply timeout to all runtime RPCs (store, delete, resolve) - Use tokio::time::timeout to bound the entire GetCapabilities operation during startup, not just the socket connection - Return contextual deadline errors on timeout This prevents a faulty or overloaded driver from hanging gateway operations indefinitely.
- Add missing workspace and provider_id parameters to test helpers - Update store_provider_credentials calls to include new parameters - Update delete_provider_credential_handles calls with new signature - Fix managed_secret_path call in vault driver test - Fix run_refresh_worker_tick call to include optional credentials param - Resolve collapsible_if clippy warning in provider_refresh.rs Signed-off-by: Seth Jennings <sjenning@redhat.com>
1806c9b to
18e3235
Compare
|
@johntmyers thanks for the review! All review feedback should be addressed by the new commits. I also rebased to resolve a conflict. |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @sjenning. I checked the five remediation commits on the rebased head, including the four threads you resolved. The workspace/provider identity and provider-expiration fixes address GATOR-1806c9be-01 and GATOR-1806c9be-02. The refresh-staging and local-timeout obligations remain open, and the latest test-fix commit leaves one deterministic Vault unit-test failure.
Validation: This remains project-valid work for roadmap issue #1931 and implements its gateway-owned credential-storage scope.
Head SHA: 18e32351d7eed3318fa657b68c35ef2deee47d4d
Base SHA: 489bb0d5dd68dfb5349891776015a2bb6f6577d6
Merge base SHA: 489bb0d5dd68dfb5349891776015a2bb6f6577d6
Patch ID: da49960d83d4707b6133dadbeea7881c188e33e7
Gator payload: 2
Review mode: follow_up
Previous reviewed SHA: 1806c9be07e0307137ac7c0b8cced209018e81fb
Blocking findings:
GATOR-18e32351-01(Warning): the Kubernetes-auth Vault resolve test constructs its managed path withtest-workspace/test-provider-idbut sendsdefault/prov-123, so identity validation rejects the request before the mocked GET.
Carried findings:
GATOR-1806c9be-03(Warning): refresh replacements still are not staged under distinct backend identities. Deterministic Vault paths overwrite the committed object, Kubernetes staging collides with the existing Secret, validation failures leak staged objects, and successful cleanup selects unchanged sibling handles and may delete the newly committed deterministic handle. Stage each replacement under a genuinely distinct immutable object, clean staged objects on every pre-CAS failure, and delete only the old handles for keys actually replaced after CAS.GATOR-1806c9be-05(Warning):Request::set_timeoutsupplies gRPC timeout metadata but does not locally bound a non-compliant or stalled UDS peer. Wrap effectful RPCs and prestarted-driverGetCapabilitiesin a local timeout and return a contextual deadline error.
Resolved findings:
GATOR-1806c9be-01: backend paths now incorporate workspace and provider UUID.GATOR-1806c9be-02: handle resolution now applies the earliest nonzero provider/driver expiration and skips expired values.
Docs: The remediation delta does not introduce a new direct UX contract; the PR already updates the relevant provider and gateway docs.
Next state: gator:in-review
| provider_name: "github-prod".to_string(), | ||
| credential_key: "GITHUB_TOKEN".to_string(), | ||
| handle: Some(handle(&format!("v1:{logical_path}"))), | ||
| workspace: "default".to_string(), |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-18e32351-01
Invariant: Credential-driver tests must construct handles and requests with the same workspace and provider identity enforced by the production driver.
Prerequisite: CI or an operator runs the openshell-driver-vault unit test suite.
Entry point → sink: cargo test -p openshell-driver-vault resolve_kubernetes_auth_kv2_secret → Vault resolve validates request identity against the handle path before issuing the mocked GET, then the test unwraps the error.
Base → head: Before identity enforcement, the handle path depended only on provider name and credential key and this test resolved successfully. The head builds logical_path with test-workspace / test-provider-id but sends default / prov-123, so validate_managed_secret_path returns InvalidArgument.
Impact: The Vault unit suite deterministically fails, blocking the repository test gate and preventing this test from covering Kubernetes-auth resolution.
Reproducer: Run cargo test -p openshell-driver-vault resolve_kubernetes_auth_kv2_secret; it fails with vault credential handle path does not match the managed path before the mocked Vault GET.
PR ownership: The author-only remediation delta adds workspace/provider UUID enforcement and supplies inconsistent fixture values here.
Requested change: Use test-workspace and test-provider-id in this request, or build logical_path with default and prov-123, so the handle and request share one identity.
johntmyers
added
gator:in-review
The Kubernetes auth Vault resolve test was constructing a managed path with test-workspace/test-provider-id but sending default/prov-123 in the request, causing validation to reject the request (GATOR-18e32351-01). - Update test to use test-workspace and test-provider-id in the request to match the logical_path construction - This ensures the test exercises the intended code path and validates Kubernetes auth resolution properly The test now passes and correctly validates identity enforcement.
Stage refresh replacements under genuinely distinct immutable handles using a unique staging ID to prevent overwriting committed values (GATOR-1806c9be-03). - Generate a unique staging ID using UUID for each refresh operation - Use this staging ID when storing credentials instead of the real provider ID - Pass the same staging ID during cleanup on failure to delete only staged objects - This ensures deterministic paths (Vault) and object names (K8s) don't collide with the committed provider's credentials The fix prevents failed refreshes from silently replacing active credentials or breaking providers by deleting still-referenced backend objects.
Add local tokio::time::timeout wrappers around credential driver RPCs to bound non-compliant or stalled UDS peers (GATOR-1806c9be-05). - Wrap StoreCredential, DeleteCredential, and ResolveCredentials in local timeouts - Return contextual deadline_exceeded errors when timeouts occur - Keep existing gRPC timeout metadata for compliant implementations - GetCapabilities during startup was already wrapped in previous commit This ensures a faulty local driver cannot hang gateway operations indefinitely, even if it accepts the connection but never responds to the RPC.
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Review Convergence Checkpoint
Thanks @sjenning. I checked the three remediation commits on this head. The Vault Kubernetes-auth test fixture now uses one consistent workspace/provider identity, and the remote StoreCredential, DeleteCredential, and ResolveCredentials calls now have local timeouts. Two carried invariant families remain only partially resolved.
Head SHA: 46de76958424427692351ae59edc9b4dc3d35093
Base SHA: 489bb0d5dd68dfb5349891776015a2bb6f6577d6
Merge base SHA: 489bb0d5dd68dfb5349891776015a2bb6f6577d6
Patch ID: 424ab18452d15c17dc5ffc2cb779cca49e471b99
Gator payload: 2
Review mode: follow_up
Previous reviewed SHA: 18e32351d7eed3318fa657b68c35ef2deee47d4d
This is the third finding-bearing review round, so Gator is pausing autonomous Warning-level review for a maintainer convergence decision.
Root-cause findings:
GATOR-1806c9be-03remains open: the unique refresh staging ID prevents overwrite, but it is persisted into Vault/Kubernetes handles that runtime resolution validates against the real provider ID. The pre-CAS validation error path still leaks staged objects, and old-handle cleanup still selects unchanged sibling handles. The governing invariant is to separate immutable provider ownership from per-write object identity, clean all staged objects on failure, and delete only the old handles actually replaced after CAS.GATOR-1806c9be-05remains open in part: the three effectful runtime RPCs are now locally bounded, but a configured prestarted UDS driver still reaches GetCapabilities without a local timeout, so a non-compliant peer can block gateway startup indefinitely.GATOR-18e32351-01is resolved by the consistent Vault test identity on this head.
Scope growth:
- Completing refresh staging now requires a clear owner-versus-object-generation identity contract across the default, Vault, and Kubernetes Secrets drivers. That remediation-driven contract decision is the appropriate maintainer checkpoint rather than another autonomous review round.
Reviewer-quality signals:
- No new finding IDs, duplicate invariants, waived re-raises, unchanged-code proposals, or missing-reproducer downgrades were produced in this round. The two items above preserve their original stable IDs.
Maintainer action: please choose whether to accept the current scope, split follow-up work, waive an obligation, or explicitly authorize another autonomous review round.
Next state: gator:blocked (reason: review_convergence_checkpoint)
johntmyers
added
gator:blocked
5 participants
Superseding #1986 while @TaylorMutch is away
Summary
Adds gateway-owned credential storage for Providers v2. New provider credential
writes now persist opaque handles instead of inline secret values, with default
encrypted database storage when no external credential backend is configured and
opt-in Kubernetes Secrets or Vault backends for deployments that want dedicated
secret storage.
Related Issue
Related to #1931
Changes
and capability calls.
credential driver, in-tree drivers, and UDS-connected external drivers.
openshell-driver-db-credstore.openshell-driver-kubernetes-secretsandopenshell-driver-vaultbinaries, plus in-tree gateway loading.through credential storage and persist internal
credential_handles.legacy inline credentials remain readable, config-only updates leave them
inline, and only newly submitted/rotated credential values move into credential
storage.
provider.credential_handleson provider create/update.provider refresh, and managed inference routes.
credential storage, backed by a retained key-encryption-key Secret injected
into the gateway.
Vault credential storage backends. The local Vault e2e path deploys OpenBao as
a Vault-compatible fixture.
tasks for credential driver validation.
Default Behavior
Credential drivers are opt-in for external backends. If
[openshell.gateway].credential_driversis omitted, the gateway uses the defaultencrypted database credential store. The submitted secret is encrypted into a
driver-owned credential object outside the provider record, and the provider
record stores only an opaque handle.
The explicit empty list form,
credential_drivers = [], is invalid. Operatorsshould omit the field for default encrypted DB storage or select exactly one
external backend such as
kubernetes-secretsorvault.Existing provider records that already contain inline plaintext credentials are
still resolved for upgrade compatibility, but new provider creates and credential
updates use credential storage handles.
Testing
cargo check -p openshell-driver-vault -p openshell-servermise run pre-commitmise run helm:testmise run e2e:kubernetes:credential-driversmise run e2e:rustmise run e2e:podman- provider/default credential coverage passed beforethe suite failed in
sandbox_create_with_no_keep_cleans_up_after_tty_command;isolated rerun of
OPENSHELL_E2E_PODMAN_TEST=sandbox_lifecycle mise run e2e:podmanreproduced the same Podman cleanup failure.Checklist