feat(credentials): add provider credential storage drivers - #2437
feat(credentials): add provider credential storage drivers#2437sjenning wants to merge 6 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 |
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>
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
| let logical_path = if let Some(existing_handle) = request.existing_handle.as_ref() { | ||
| Self::logical_path_from_handle(existing_handle)? | ||
| } else { | ||
| managed_secret_path(&request.provider_name, &request.credential_key) |
There was a problem hiding this comment.
gator-agent
Critical — GATOR-1806c9be-01
Invariant: External credential objects must be owned by a gateway-generated provider identity that is unique across workspaces, not by caller-controlled provider names.
Prerequisite: Two supported workspaces share the same Vault/OpenBao or Kubernetes Secrets driver, and a caller can create a provider with the same name and credential key as a provider in another workspace.
Entry point → sink: Providers v2 CreateProvider/UpdateProvider → the driver derives a backend object solely from provider_name and credential_key; Vault then writes that shared path unconditionally.
Base → head: The base stored credentials inside each workspace-scoped provider record → the head creates a shared external path without the workspace or generated provider UUID.
Impact: With Vault, one workspace can retain a handle to a colliding path and read a credential written by another workspace, or replace that workspace's runtime credential (CWE-639). Kubernetes deterministically turns the same collision into a cross-workspace denial of service.
Reproducer: Configure one Vault driver for two workspaces. Create provider p with credential token=a in workspace A, then provider p with token=b in workspace B. Resolve A's existing handle and observe b. With Kubernetes Secrets, the second create fails because both providers address the same Secret.
PR ownership: This PR introduces the external backend identities and drops the already-available workspace/provider UUID at the new driver boundary.
Requested change: Carry a stable gateway-owned identity (for example, the provider UUID with workspace identity) through store/resolve/delete, derive backend paths and ownership from it, use create-only/CAS semantics for Vault, and add two-workspace collision coverage for both external drivers.
Sibling sites: crates/openshell-driver-vault/src/lib.rs:734, crates/openshell-driver-kubernetes-secrets/src/lib.rs:153, crates/openshell-driver-kubernetes-secrets/src/lib.rs:523, and crates/openshell-server/src/grpc/provider.rs:827.
| } | ||
| } | ||
|
|
||
| let resolved_refs = credentials |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-1806c9be-02
Invariant: Moving a credential from inline storage to a handle must preserve its provider-level expiration semantics.
Prerequisite: A provider has credential_expires_at_ms for a credential stored behind a handle, and the timestamp elapses before sandbox or inference resolution.
Entry point → sink: Sandbox provider resolution or inference authentication → the resolved handle value is inserted based only on the driver's expires_at_ms; all bundled storage drivers return zero.
Base → head: The base inline path omits values expired according to provider.credential_expires_at_ms → the new handle path ignores that persisted expiration and inserts the value.
Impact: Expired credentials continue to be injected into sandboxes or used for inference, while uniqueness validation can simultaneously treat the same credential as inactive and select a different provider value.
Reproducer: Create a handle-backed provider credential with credential_expires_at_ms earlier than the current time, then resolve it for a sandbox or inference. The handle path returns the value because the built-in driver reports expires_at_ms = 0; the base inline path skips it.
PR ownership: This PR adds the handle-resolution path without carrying forward the existing provider-level expiry check.
Requested change: Compute the effective expiry from the provider and driver values using the earliest nonzero timestamp, omit/reject expired values before insertion, propagate the effective expiry downstream, and add inline-versus-handle parity tests.
Sibling sites: crates/openshell-server/src/credentials.rs:382 and crates/openshell-server/src/inference.rs:1146.
| for (key, value) in &minted.additional_credentials { | ||
| creds_to_store.insert(key.clone(), value.clone()); | ||
| } | ||
| let stored = credentials |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-1806c9be-03
Invariant: A rejected refresh must not modify or delete the backend object still referenced by the committed provider record.
Prerequisite: A provider already has an external-driver handle and a refresh reaches attached-sandbox validation or provider CAS failure after storing the minted value.
Entry point → sink: Supported provider refresh → store_provider_credentials reuses and overwrites provider.credential_handles before validation and CAS; error cleanup can then delete the returned, still-committed handle.
Base → head: The base changed a cloned inline provider and validated before committing → the head mutates the live external object first, so a reported validation failure can still change the active value and a CAS failure can leave the provider pointing to a deleted object.
Impact: A failed refresh can silently replace the credential despite returning an error, or permanently break the committed provider by deleting its referenced backend state (CWE-362/CWE-367).
Reproducer: Seed a provider with an external handle containing old-token; make attached-sandbox validation fail; refresh with new-token; then resolve the unchanged provider and observe new-token. Separately force provider CAS failure after store and observe cleanup deleting the original handle still stored in the provider row.
PR ownership: This PR moves credential mutation outside the provider record transaction and reuses the existing effectful backend handle before the existing validation/CAS gates.
Requested change: Stage replacements under new immutable handles, validate, CAS the provider record to the staged handles, and only then delete old handles. Failure cleanup must delete only newly staged objects; add deterministic validation- and CAS-failure tests.
Sibling sites: crates/openshell-server/src/provider_refresh.rs:570, crates/openshell-server/src/provider_refresh.rs:612, and crates/openshell-server/src/credentials.rs:230.
| ) -> CoreResult<Channel> { | ||
| let channel = connect_credential_driver_socket(driver_name, socket_path).await?; | ||
| let mut client = CredentialDriverClient::new(channel.clone()); | ||
| client |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-1806c9be-05
Invariant: A configured external credential-driver timeout must bound both readiness verification and effectful runtime RPCs.
Prerequisite: An operator configures the supported UDS transport and the driver accepts a connection but stops responding because of a defect, overload, or compromised plugin.
Entry point → sink: Gateway startup or provider CRUD/runtime resolution → unbounded GetCapabilities, StoreCredential, DeleteCredential, or ResolveCredentials awaits.
Base → head: The base had no external credential-driver RPC that could block these paths → the head exposes startup_timeout_secs, but a peer that accepts the socket and stalls inside GetCapabilities prevents the deadline loop from running again; runtime RPCs also have no deadline.
Impact: A faulty local driver can hang gateway startup indefinitely or permanently stall credential-dependent provider, sandbox, refresh, and inference operations.
Reproducer: Set startup_timeout_secs = 1 and use a UDS server that accepts the HTTP/2 request but never answers GetCapabilities; startup remains blocked beyond one second. The same server can accept StoreCredential without replying and leave CreateProvider blocked indefinitely.
PR ownership: This PR introduces the UDS lifecycle, the advertised startup timeout, and these unbounded RPC awaits.
Requested change: Apply the remaining startup deadline to connection plus capability negotiation, set bounded deadlines for store/delete/resolve RPCs, return a contextual deadline error, and add a deterministic nonresponding-UDS test.
Sibling sites: crates/openshell-server/src/credentials.rs:1061, crates/openshell-server/src/credentials.rs:1070, and crates/openshell-server/src/credentials.rs:1079.
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