feat(providers): AWS STS AssumeRole refresh strategy and aws-s3 profile

[russellb]

Summary

Add gateway-owned AWS STS AssumeRole as a credential refresh strategy, ship aws and aws-s3 provider profiles, and extend policy validation to support single-label * wildcards in middle DNS labels.

Combined with the SigV4 proxy re-signing from #1638 (now merged), sandboxes can access S3 and other AWS services without ever seeing real credentials — the gateway mints short-lived STS credentials and the proxy re-signs requests on the fly.

Related Issue

Refs #1576

Changes

STS credential refresh (openshell-server)

• New AwsStsAssumeRole variant in ProviderCredentialRefreshStrategy (proto field AWS_STS_ASSUME_ROLE = 6).
• provider_refresh.rs implements the refresh loop: calls sts:AssumeRole using the gateway's ambient AWS credentials (or explicit long-lived keys from refresh material), stores the resulting AccessKeyId, SecretAccessKey, and SessionToken on the provider, and schedules re-rotation before expiry.
• ConfigureProviderRefresh and RotateProviderCredential RPCs gated behind the providers_v2_enabled setting.
• Validates that configuring STS refresh for one provider won't collide with credential keys already held by another provider attached to the same sandbox.

Provider profiles (openshell-providers)

AWS provider profiles follow the same pattern as the existing aws-bedrock profile: a generic base plus service-specific variants.

• providers/aws.yaml — base AWS profile with STS refresh material (role_arn, session_name, external_id, aws_region, optional long-lived keys) but no endpoints or binaries allowlist. Intended for AWS services that don't yet have a dedicated profile — the user attaches their own policy to supply endpoints. Same role google-cloud plays relative to google-vertex-ai.
• providers/aws-s3.yaml — S3-specific profile that adds pre-configured endpoints covering regional (*.s3.*.amazonaws.com), global (*.s3.amazonaws.com with signing_region: us-east-1), and dualstack variants. All endpoints use credential_signing: sigv4 and signing_service: s3. Includes a binaries allowlist for Python, curl, and the AWS CLI.

Policy: single-label * wildcard in middle DNS labels (openshell-policy)

• validate_host_wildcard now permits * as a complete middle label (e.g. *.s3.*.amazonaws.com) in addition to leading-label and intra-label positions. Matches exactly one DNS label via Rego's glob.match with . separator, consistent with TLS wildcard semantics.
• OPA endpoint matching and L7 config lookup updated: host_matches_wildcard_middle rule and endpoint_config_for_middle_wildcard in Rego.
• Validation rejects ** in any position and partial wildcards in middle labels.

TUI (openshell-tui)

• Added AwsStsAssumeRole match arm to the refresh strategy label in the provider settings view.

Documentation

• docs/sandboxes/manage-providers.mdx: STS refresh setup instructions
• examples/aws-s3-sts.md: end-to-end manual test guide (create IAM role, configure provider, test S3 PUT/GET/LIST from sandbox)

Testing

• cargo test -p openshell-providers — 65 passed (STS serde roundtrip, aws/aws-s3 profile parsing, endpoint validation)
• cargo test -p openshell-policy — 91 passed (wildcard DNS validation, middle-label wildcards, credential_signing validation)
• cargo test -p openshell-server — all passed (STS configure v2 gate, success path, credential key collision)
• cargo test -p openshell-supervisor-network — all passed (wildcard host matching, OPA endpoint config)
• cargo test -p openshell-tui — 24 passed
• End-to-end manual test: Podman sandbox → boto3 → S3 PUT/GET/LIST with STS-minted credentials and proxy-side SigV4 re-signing against real AWS (all passed)

Checklist

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

[copy-pr-bot]

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.

[github-actions]

This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity.

[russellb]

This is still waiting on #1638 to go in first.

[russellb]

@pimlock willing to take a look at this one since you reviewed #1638 ?

[pimlock]

/ok to test cb10f83

[pimlock]

Hi @russellb, I did a first pass, but I run out of time to get through this fully. I needed to check this out locally and run this to understand how it worked (wasn't as familiar with the cred refresh).

I added a few comments.
The biggest item I had was around how the implicit extra env vars that are managed by the refresh, but not defined as such in the provider profile.
I did investigation around this and options. I would need to spend more time figuring out what makes sense, but I will need to come back to it on Thursday. In the meantime, I can share raw notes from agent, but I haven't checked these very deeply yet: https://gist.github.com/pimlock/6a99b3d9c4e19c6efc3b0d2da59b6b63

[russellb]

@pimlock thank you very much for the review! I know it can be very time-consuming. I will dig into the comments tomorrow.

[pimlock]

/ok to test cbe9b43

[pimlock]

@russellb Thanks for addressing the feedback! I'm running a bit behind, so I'll do a final review tomorrow and follow up.

[russellb]

@russellb Thanks for addressing the feedback! I'm running a bit behind, so I'll do a final review tomorrow and follow up.

No problem!

[pimlock]

/ok to test 2c6dfb8

[github-actions]

Label test:e2e applied for 2c6dfb8. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

[pimlock]

gator-agent

PR Review Status

Validation: This PR is project-valid because it implements the linked provider-v2 feature request #1576 and completes the AWS credential path enabled by merged PR #1638.

Head SHA: 2c6dfb8022272983f8ff1223bcd94cd6f1b94f8e

Thanks @pimlock — I checked the runtime-credential resolution, duplicated mintability logic, declarative multi-output mapping, and collision-reservation concerns from your July 7 review. Thanks @russellb — the July 8 update does address those original concerns with the shared mintability check and pinned additional_outputs mapping. The independent code review found additional blockers:

• Critical — CWE-918 (SSRF): crates/openshell-server/src/provider_refresh.rs:603-608 accepts undocumented sts_endpoint_url material and lets a caller direct an AWS-signed STS request to an arbitrary loopback HTTP service. Remove the production material override; inject test endpoints only through test-only code, and reject this material key at the configure boundary.
• Blocking — CWE-20: crates/openshell-server/src/grpc/provider.rs:1500-1532,2103-2115 permits STS configuration without requiring a matching profile refresh declaration or canonical primary env key, while provider_refresh.rs:651-670 falls back to hardcoded sibling keys. Resolve and validate one canonical profile binding (strategy, AWS_ACCESS_KEY_ID, and both declared outputs), store the canonical primary key, and remove the fallback.
• Blocking — CWE-20: provider_refresh.rs:586-598 silently ignores a partial explicit AWS source-credential pair and falls back to the gateway's ambient identity. Require aws_access_key_id and aws_secret_access_key to be both present or both absent.
• Blocking: The v2 gate is enforced only during configuration (grpc/provider.rs:2019-2030); manual and worker rotations can keep minting STS credentials after providers_v2_enabled is disabled. Enforce the gate in the shared refresh execution path.
• Blocking: Deleting an STS refresh clears only the primary expiry (grpc/provider.rs:2274-2300). Clear refresh-owned expiry metadata for the primary and every pinned additional output in one provider update, while preserving independently changed expiries.
• Blocking docs/security — CWE-200: docs/sandboxes/manage-providers.mdx:209-215 expands the long-lived AWS secret into process arguments. Use --secret-material-env aws_secret_access_key=AWS_SECRET_ACCESS_KEY instead.

Please add regression coverage for the endpoint override rejection, partial source credentials, profile/strategy/canonical-key validation, v2 disable-after-config rotation, and additional-output expiry cleanup. No local tests or builds were run as part of this code-only review.

Docs: The feature is otherwise covered in existing Fern-managed pages and needs no navigation change, but the docs gate fails until the secret-handling example is corrected.

CI: test:e2e is applied and the required E2E gate is currently pending; author changes are required before pipeline status can advance the PR.

Next state: gator:in-review

[russellb]

Addressed each finding in its own commit on top of 2c6dfb8. Summary below; commit SHAs referenced per item.

Critical — CWE-918 (SSRF): provider_refresh.rs:603-608 accepts undocumented sts_endpoint_url material and lets a caller direct an AWS-signed STS request to an arbitrary loopback HTTP service. Remove the production material override; inject test endpoints only through test-only code, and reject this material key at the configure boundary.

Fixed in e7a978c. The endpoint override is now a test_sts_endpoint_override helper that is #[cfg(test)]-only — the production build returns None and never reads material, so the STS endpoint is always region-resolved. handle_configure_provider_refresh rejects the sts_endpoint_url material key outright, so it can never reach a stored refresh state outside tests.

Blocking — CWE-20: grpc/provider.rs:1500-1532,2103-2115 permits STS configuration without requiring a matching profile refresh declaration or canonical primary env key, while provider_refresh.rs:651-670 falls back to hardcoded sibling keys. Resolve and validate one canonical profile binding (strategy, AWS_ACCESS_KEY_ID, and both declared outputs), store the canonical primary key, and remove the fallback.

Fixed in 916e9d2. Added validate_strategy_profile_binding, called at configure: for strategies that pin canonical env keys, it requires (1) a profile refresh declaration whose strategy matches, (2) the canonical primary env key (AWS_ACCESS_KEY_ID), and (3) every required strategy output resolved to its canonical env key from the profile's additional_outputs. The canonical primary key is what's stored as the refresh credential key. The mint fallback to hardcoded AWS_SECRET_ACCESS_KEY/AWS_SESSION_TOKEN is removed — a missing resolved mapping now fails closed instead of guessing.

Blocking — CWE-20: provider_refresh.rs:586-598 silently ignores a partial explicit AWS source-credential pair and falls back to the gateway's ambient identity. Require aws_access_key_id and aws_secret_access_key to be both present or both absent.

Fixed in 29970bb. The mint path now treats the pair as all-or-nothing and errors on a partial pair instead of falling through to the ambient identity; configure rejects the partial pair early with the same message.

Blocking: The v2 gate is enforced only during configuration (grpc/provider.rs:2019-2030); manual and worker rotations can keep minting STS credentials after providers_v2_enabled is disabled. Enforce the gate in the shared refresh execution path.

Fixed in a726dfc. The gate now runs in refresh_provider_credential — the shared path for both the worker sweep and manual rotation — so disabling providers_v2_enabled halts further mints. The rejection is recorded on the refresh state for observability.

Blocking: Deleting an STS refresh clears only the primary expiry (grpc/provider.rs:2274-2300). Clear refresh-owned expiry metadata for the primary and every pinned additional output in one provider update, while preserving independently changed expiries.

Fixed in 6799dcc. handle_delete_provider_refresh now clears the expiry for the primary and every pinned additional output whose stored expiry still matches the refresh's, in a single provider update. Expiries that were changed independently (no longer matching the refresh) are left untouched.

Blocking docs/security — CWE-200: docs/sandboxes/manage-providers.mdx:209-215 expands the long-lived AWS secret into process arguments. Use --secret-material-env aws_secret_access_key=AWS_SECRET_ACCESS_KEY instead.

Fixed in e8495fd. The off-AWS example now uses --secret-material-env aws_secret_access_key=AWS_SECRET_ACCESS_KEY so the CLI reads the secret from its environment instead of the process table.

Please add regression coverage for the endpoint override rejection, partial source credentials, profile/strategy/canonical-key validation, v2 disable-after-config rotation, and additional-output expiry cleanup.

Regression tests land with each corresponding fix rather than as a separate commit: endpoint-override rejection and partial-source-credential rejection (configure + mint), profile/strategy/canonical-key validation (no-profile and non-canonical-key cases), v2 disable-after-config rotation, and additional-output expiry cleanup (with an independently-changed expiry preserved).

Verification: openshell-server (912) and openshell-providers (79) unit tests pass; clippy clean with -D warnings. Also ran the documented AWS STS scenario manually against a live Podman-backed gateway (real CLI → real gateway) — 12/12 checks: the documented happy path configures and reaches the real STS call, and all five negative/gate cases behave as intended.

One note for the CI gate: the test:e2e lanes are Docker/Podman-backed and I couldn't run the automated suite in this environment (the Docker CLI here is a Podman compat shim whose docker info health check fails), so the required E2E gate still needs a CI run.

[pimlock]

gator-agent

Re-check After Author Update

Thanks @russellb. I reviewed current head e8495fdf0b81e49853fa6a4533095bacdec5f298 after your July 14 response explaining how each finding was addressed.

What I checked: the full current diff, the six fix commits, the STS configure/mint/delete paths, regression coverage, middle-label wildcard consumers, Fern docs, and the current CI state. The endpoint-override, canonical profile binding, partial source-credential, providers-v2 gate, and process-argument secret findings are resolved. Multi-key expiry cleanup is present, but its preservation guarantee is not concurrency-safe.

Disposition: partially resolved; author changes are still required.

Remaining items:

• Blocking — CWE-362: crates/openshell-server/src/grpc/provider.rs:2358-2392 decides which expiries to clear from the provider snapshot fetched at line 2335, then update_provider_record fetches and merges into a potentially newer provider. A concurrent rotation or provider update can change an expiry between those operations, after which the unconditional zero removes the new value. Move the equality check and removal into the provider CAS closure, and add a regression test that changes an output expiry after the handler's initial read but before its write.
• Warning — CWE-693: crates/openshell-prover/src/credentials.rs:182-208 still supports only first-label wildcard matching, while this PR adds runtime support for whole middle-label wildcards such as *.s3.*.amazonaws.com. Update the prover to mirror the runtime's label-aware semantics and cover matching plus non-matching label counts so it does not under-report credential-associated policy risk.
• Docs/example: examples/aws-s3-sts.md:166 installs boto3 inside a sandbox whose aws-s3 profile does not allow PyPI; preinstall boto3 or attach the pypi profile. The cleanup should also delete s3-curl.

No local tests or builds were run as part of this code-only review.

Docs: the Fern-managed pages are updated and need no navigation change; the remaining example corrections are listed above.

CI: test:e2e remains appropriate. Required Branch Checks, Helm Lint, and E2E are waiting for the copy-PR mirror on this head, but author changes are required before pipeline monitoring should advance.

Next state: gator:in-review

[russellb]

Addressed the three remaining items from the re-check, each in its own commit on top of e8495fd.

Blocking — CWE-362: crates/openshell-server/src/grpc/provider.rs:2358-2392 decides which expiries to clear from the provider snapshot fetched at line 2335, then update_provider_record fetches and merges into a potentially newer provider. A concurrent rotation or provider update can change an expiry between those operations, after which the unconditional zero removes the new value. Move the equality check and removal into the provider CAS closure, and add a regression test that changes an output expiry after the handler's initial read but before its write.

Fixed in 557221f. The equality check and removal now live in a pure helper (clear_refresh_owned_expiries) invoked inside the update_message_cas closure, so the decision runs against the CAS-locked current provider rather than the snapshot read earlier in the handler — a concurrently changed expiry is preserved instead of clobbered. Regression test clear_refresh_owned_expiries_preserves_concurrently_changed_expiry exercises the closure logic against a provider whose output expiry was changed after the refresh's value (as a concurrent rotation would leave it): the still-owned keys are cleared and the changed one is preserved. (A fully interleaved mid-update_message_cas write isn't reproducible without a test seam, so the regression targets the closure's decision function directly — that decision is now the only thing that clears anything.)

Warning — CWE-693: crates/openshell-prover/src/credentials.rs:182-208 still supports only first-label wildcard matching, while this PR adds runtime support for whole middle-label wildcards such as *.s3.*.amazonaws.com. Update the prover to mirror the runtime's label-aware semantics and cover matching plus non-matching label counts so it does not under-report credential-associated policy risk.

Fixed in 623512e. host_pattern_covers now matches label-for-label via a labels_match helper: a * (or a partial * glob) matches exactly one label in any position, literal labels compare literally, and a leading ** still matches one or more labels. Added host_patterns_overlap_matches_whole_middle_label_wildcard covering middle-label matches (*.s3.*.amazonaws.com, s3.*.amazonaws.com) and non-matching label counts (too few / too many labels, literal mismatch). Updated the prior test that asserted middle-label wildcards did not overlap, since that encoded the under-reporting behavior.

Docs/example: examples/aws-s3-sts.md:166 installs boto3 inside a sandbox whose aws-s3 profile does not allow PyPI; preinstall boto3 or attach the pypi profile. The cleanup should also delete s3-curl.

Fixed in 6c58d2a. The boto3 walkthrough now creates a pypi provider and attaches it (--provider s3-test --provider pypi) so pip can reach PyPI, with a note about the custom-image alternative. Cleanup now deletes the s3-curl sandbox and the pypi provider.

Verification: openshell-server (913) and openshell-prover (26) unit tests pass; clippy clean with -D warnings on both. The test:e2e gate still needs a CI run against this head.

[pimlock]

gator-agent

Re-check After Author Update

Thanks @russellb. I reviewed current head 623512efc2e50e133e63b9b96e4c4f28d8d97dd9 after your July 14 response explaining the CAS cleanup, prover wildcard, and AWS S3 example updates.

What I checked: the full current diff, the focused commits since e8495fdf, refresh configure/mint/delete concurrency, the middle-label wildcard implementation and tests, the Fern docs, the example cleanup, and the current CI state. The three items from the prior re-check are resolved: expiry ownership is now decided inside the provider CAS closure, prover matching covers exact-one-label middle wildcards, and the boto3 example now has PyPI egress with complete cleanup.

Disposition: partially resolved; one blocking correctness/security issue remains.

Remaining items:

• Blocking — CWE-362: crates/openshell-server/src/provider_refresh.rs:300-308,341-386 reads refresh state, awaits the STS request, writes credentials, and then persists that stale state. Meanwhile, crates/openshell-server/src/grpc/provider.rs:2369-2405 can delete the refresh and return success. If deletion completes while STS is in flight, the rotation can subsequently mint credentials and recreate the deleted refresh, including stored source-credential material. Add a store-backed generation/lease or tombstone protocol shared by rotate/configure/delete so the minted result atomically verifies that its generation still exists before modifying the provider and cannot recreate deleted state. Please add a deterministic regression test that pauses mock STS, deletes the refresh, releases STS, and verifies that both refresh state and provider credentials remain unchanged.
• Non-blocking warning — CWE-362: collision validation at crates/openshell-server/src/grpc/provider.rs:2167-2186 is separate from persistence at :2235-2266. Concurrent configuration of two providers attached to the same sandbox can let both reserve the same primary or additional key, leaving conflicting unusable refreshes even though later mint validation prevents injection. Please make reservation atomic or serialize configuration by affected sandbox/key and add a concurrent-configure regression test.

No local tests, builds, clippy, pre-commit, or E2E were run as part of this code-only review.

Docs: the user-facing behavior is covered by existing Fern pages under docs/; those pages are already in navigation, so no docs/index.yml change is needed.

CI: test:e2e remains appropriate. Required Branch Checks, Helm Lint, E2E, and DCO are pending on this new head; author changes are required before pipeline monitoring can advance.

Next state: gator:in-review

[russellb]

Addressed both concurrency items from the latest re-check (head 623512e), each in its own commit.

Blocking — CWE-362: crates/openshell-server/src/provider_refresh.rs:300-308,341-386 reads refresh state, awaits the STS request, writes credentials, and then persists that stale state. Meanwhile, crates/openshell-server/src/grpc/provider.rs:2369-2405 can delete the refresh and return success. If deletion completes while STS is in flight, the rotation can subsequently mint credentials and recreate the deleted refresh, including stored source-credential material. Add a store-backed generation/lease or tombstone protocol shared by rotate/configure/delete so the minted result atomically verifies that its generation still exists before modifying the provider and cannot recreate deleted state. Please add a deterministic regression test that pauses mock STS, deletes the refresh, releases STS, and verifies that both refresh state and provider credentials remain unchanged.

Fixed in f462775. refresh_provider_credential now captures the refresh's generation (metadata.resource_version) at the start, and every terminal persist goes through persist_refresh_state_if_current, which writes with WriteCondition::MatchResourceVersion. That is an UPDATE-only write: it never inserts, so a refresh deleted (or superseded) while the mint request is in flight is detected and cannot be recreated, and its stored source-credential material is not resurrected. After minting, the rotation re-checks that the refresh still exists before writing credentials into the provider, so a removed refresh is not re-minted; on a detected deletion it returns aborted without touching the provider.

The generation acts as the store-backed check you described (a delete bumps the row out of existence, so the version-matched write fails closed). I did not introduce a separate lease/tombstone object — the existing per-row resource_version already gives the compare-and-swap needed to make the persist atomic with respect to delete, and keeping the mechanism in one primitive avoids a second source of truth to keep in sync across rotate/configure/delete. Happy to switch to an explicit tombstone if you'd prefer the stronger audit trail.

Deterministic regression test rotation_does_not_resurrect_refresh_deleted_mid_flight: a gated wiremock STS responder blocks until released; the test waits until the rotation is inside the STS call, deletes the refresh, releases STS, then asserts the rotation aborts, the refresh state is not recreated, and no AWS credentials were written to the provider.

Residual window I want to be transparent about: a delete that lands between the post-mint existence re-check and the provider credential write can still leave the freshly minted credential values in the provider. That is consistent with delete semantics — handle_delete_provider_refresh clears refresh-owned expiries but intentionally does not purge credential values — and the security-critical invariant (never recreate the deleted refresh state / its source material) holds unconditionally via the version-matched persist.

Non-blocking warning — CWE-362: collision validation at crates/openshell-server/src/grpc/provider.rs:2167-2186 is separate from persistence at :2235-2266. Concurrent configuration of two providers attached to the same sandbox can let both reserve the same primary or additional key, leaving conflicting unusable refreshes even though later mint validation prevents injection. Please make reservation atomic or serialize configuration by affected sandbox/key and add a concurrent-configure regression test.

Fixed in bf68e0b. handle_configure_provider_refresh now holds the process-wide sandbox sync guard across the validate-and-persist sequence — the same guard sandbox create/attach and profile changes already take. Because the collision validation consults each attached provider's active env keys (which include configured-but-unminted refresh reservations), serializing configuration means the second configure observes the first's persisted reservation and is rejected instead of double-reserving. Regression test concurrent_configure_aws_sts_reserves_key_exactly_once runs two configures for providers on the same sandbox concurrently and asserts exactly one succeeds and the other fails with a collision.

Scope note: this guard serializes within a gateway process, matching the existing concurrency model for sandbox-affecting mutations. It is not cross-replica; a store-backed reservation would be needed for multi-writer HA, which would be a broader change than this profile-scoped PR.

Verification: openshell-server (915) and openshell-prover (26) unit tests pass; clippy clean with -D warnings. The test:e2e gate still needs a CI run against this head.

[pimlock]

gator-agent

Re-check After Author Update

Thanks @russellb. I reviewed current head bf68e0b29482ff983b162138ebe0c9a0a35403ac after your July 14 response describing the resource-version guard for in-flight refresh deletion and the sandbox sync guard for concurrent key reservation.

What I checked: the full current diff, the two commits since 623512ef, refresh configure/mint/delete ordering, the shared-store HA contract, regression coverage, AWS source credential handling, Fern docs, and current CI state. The update prevents a deleted refresh row from being reinserted and serializes same-process configures, but it does not make the provider credential write atomic with refresh-generation ownership or serialize reservations across gateway replicas.

Disposition: partially resolved; author changes are still required.

Remaining items:

• Blocking — CWE-362: crates/openshell-server/src/provider_refresh.rs:383-458 checks only that a refresh with the same name exists, writes minted values to the provider, and only afterward performs the version-matched refresh-state write. A refresh deleted and recreated during mint, or a losing concurrent rotation, can therefore write credentials minted from a stale generation before returning Aborted. Make refresh-generation ownership and the provider credential update one atomic datastore operation: require the refresh row to retain expected_version, and if that condition loses, leave both the refresh state and provider unchanged. Add deterministic delete/reconfigure-between-check-and-write and concurrent-losing-rotation tests.
• Blocking — CWE-362/CWE-269: crates/openshell-server/src/grpc/provider.rs:2161-2274 holds sandbox_sync_guard() across validation and persistence, but that guard is process-local. OpenShell explicitly supports multiple gateway replicas sharing Postgres, so two replicas can still reserve the same sandbox credential keys. Use a shared-store lock/transaction or durable atomic reservations keyed by sandbox and env key across configure, sandbox attach/create, provider/profile mutation, and release. Cover this with two independently guarded server states sharing one store.
• Warning: provider_refresh.rs:704-719 accepts explicit access/secret keys but always passes no AWS session token. Temporary source credentials from SSO or a prior AssumeRole will fail. Add optional secret aws_session_token, validate it requires the access/secret pair, pass it to the AWS SDK, and document it.
• Warning: provider_refresh.rs:73-79 classifies a missing conditional-write target by matching database error text for "not found". Replace this backend-dependent string match with a typed condition-not-met/not-found persistence result.

No local tests, builds, clippy, pre-commit, or E2E were run as part of this code-only review.

Docs: the user-facing behavior is covered in existing Fern pages; no docs/index.yml navigation change is needed.

CI: DCO is green. Required Branch Checks, Helm Lint, and E2E are waiting for the copy-PR mirror on this head. test:e2e remains appropriate, but review changes are required before pipeline monitoring can advance.

Next state: gator:in-review

[pimlock]

gator-agent

PR Review Status

Validation: This PR is project-valid because it implements the linked provider-v2 request #1576 and completes the AWS credential path enabled by merged PR #1638.

Head SHA: b0365a117f3872c17d7b9f967c1977b7d1d63981

Thanks @russellb. I checked the three commits since the prior bf68e0b2 disposition, including typed absent-row conflicts, generation-gated credential writes, and temporary AWS source-session-token support. The typed conflict and session-token changes resolve those prior warnings, but the independent code review found two concurrency blockers:

• Blocking — CWE-362: crates/openshell-server/src/provider_refresh.rs:419-435 claims the refresh generation and writes provider credentials in two separate datastore operations. A delete or reconfiguration can commit between them, allowing the old generation to write credentials; a crash in that window leaves refresh state marked successful while provider credentials remain stale. Commit the refresh-version check, refresh-state update, and provider credential update in one SQLite/Postgres transaction, and add deterministic delete/recreate and reconfigure tests for the post-verification window.
• Blocking — CWE-362/CWE-269: credential-key reservation still relies on the process-local sandbox_sync_guard() (crates/openshell-server/src/grpc/provider.rs:2178; crates/openshell-server/src/compute/mod.rs:332) and ordinary provider updates at grpc/provider.rs:1959-1976 do not take it. Multiple gateway replicas sharing Postgres, or a provider update racing configuration, can therefore reserve colliding keys. Use durable reservation rows or datastore-scoped locking/transactions across every relevant mutation, then test two independently guarded server states sharing Postgres plus provider-update-versus-configure.

Non-blocking follow-ups:

• docs/sandboxes/providers-v2.mdx:314 should list optional aws_session_token and its complete-pair requirement.
• Add direct SQLite and Postgres coverage for Conflict { current_resource_version: None } on an absent-row conditional write.

No local tests, builds, clippy, pre-commit, or E2E were run as part of this code-only review.

Docs: the direct UX is covered in existing Fern pages and needs no navigation change, subject to the reference correction above.

CI: test:e2e remains appropriate. Branch Checks, Helm Lint, and E2E are currently waiting for the new-head mirror, but author changes are required before pipeline monitoring can advance.

Next state: gator:in-review

[russellb]

Addressed the two warnings and the first blocking item (head b0365a1). On the second blocking item I'm pushing back — reasoning below.

Blocking — CWE-362: crates/openshell-server/src/provider_refresh.rs:383-458 checks only that a refresh with the same name exists, writes minted values to the provider, and only afterward performs the version-matched refresh-state write. A refresh deleted and recreated during mint, or a losing concurrent rotation, can therefore write credentials minted from a stale generation before returning Aborted. Make refresh-generation ownership and the provider credential update one atomic datastore operation: require the refresh row to retain expected_version, and if that condition loses, leave both the refresh state and provider unchanged. Add deterministic delete/reconfigure-between-check-and-write and concurrent-losing-rotation tests.

Fixed in a5a89f0. The rotation now claims the generation before touching the provider: it performs the version-matched refresh-state write (put_if with MatchResourceVersion(expected_version)) first, and only writes credentials into the provider if that claim succeeds. On a lost claim — deleted, recreated, or superseded by a concurrent rotation — it returns Aborted and leaves both the refresh state and the provider unchanged, so no stale-generation credentials are written and a deleted refresh is never resurrected.

On literal single-operation atomicity across the two objects: the store exposes only per-object CAS — there is no multi-object transaction API — so a single atomic datastore op spanning the refresh row and the provider row isn't available. The claim-then-write ordering is the optimistic-concurrency equivalent: generation ownership is the gate, and the loser mutates nothing. (I also switched the persist to consume a typed conditional-write result rather than matching error strings — see the CWE-693 item below.)

Deterministic tests: rotation_superseded_mid_flight_discards_credentials (a concurrent write bumps the generation while mock STS is paused; the losing rotation aborts, writes no credentials, and leaves the winner's state intact) and the existing rotation_does_not_resurrect_refresh_deleted_mid_flight (delete while STS is paused; no resurrection, no credentials written).

Blocking — CWE-362/CWE-269: crates/openshell-server/src/grpc/provider.rs:2161-2274 holds sandbox_sync_guard() across validation and persistence, but that guard is process-local. OpenShell explicitly supports multiple gateway replicas sharing Postgres, so two replicas can still reserve the same sandbox credential keys...

You're right that the gateway is active-active (I checked: replicaCount > 1 on shared Postgres, a plain Deployment, no leader election; even single-replica runs briefly overlap two pods during a RollingUpdate), so the cross-replica race is real. I'm pushing back on treating it as a blocker for this PR, for two reasons:

1. It's pre-existing and subsystem-wide, not introduced here. sandbox_sync_guard() is the established concurrency model for every sandbox-mutating operation — create_sandbox, provider attach/detach, and provider-profile create/update/delete all rely on the same process-local guard (e.g. grpc/sandbox.rs:140,322,422, grpc/policy.rs:1863,2087, grpc/provider.rs:1264,1347,1447). Refresh configuration just follows that pattern. None of these are cross-replica safe today.
2. A correct fix is cross-cutting and disproportionate to bolt onto an STS-refresh PR: durable store-backed reservations (or Postgres advisory locks) would have to span configure and attach/create and profile mutation and release. Adding it to configure alone wouldn't actually close the hole, since a concurrent attach on another replica re-creates the collision.

The guard added in this PR is still a strict improvement — it closes the same-process race (the common case, including single-replica rollouts). I'd propose the cross-replica reservation be tracked as its own follow-up rather than gating this feature. Happy to file that issue / open an RFC if you agree with the scoping.

Warning: provider_refresh.rs:704-719 accepts explicit access/secret keys but always passes no AWS session token...

Fixed in b0365a1. The explicit source-credential path now accepts an optional secret aws_session_token and passes it to the AWS SDK, so temporary credentials from SSO or a prior AssumeRole work. It requires the access/secret pair, enforced at both the configure boundary and the mint path. Declared as optional secret material on the aws and aws-s3 profiles and documented in the off-AWS example.

Warning: provider_refresh.rs:73-79 classifies a missing conditional-write target by matching database error text for "not found". Replace this backend-dependent string match with a typed condition-not-met/not-found persistence result.

Fixed in 31c339e. put_if with MatchResourceVersion now reports an absent row as a typed Conflict { current_resource_version: None } (a version mismatch carries the current version), in both the SQLite and Postgres backends. persist_refresh_state_if_current matches the typed Conflict and no longer inspects error strings.

Verification: openshell-server (919) and openshell-prover (26) unit tests pass; clippy clean with -D warnings. The test:e2e gate still needs a CI run against this head.

[pimlock]

/ok to test b0365a1

[pimlock]

Hi @russellb, thanks for addressing all the comments. I think this is ready

For the context, the gator-agent that I used to help reviewing this is now part of the repo: https://github.com/NVIDIA/OpenShell/blob/main/.agents/skills/launch-openshell-gator/SKILL.md

I agree on postponing this item:

Blocking — CWE-362/CWE-269: crates/openshell-server/src/grpc/provider.rs:2161-2274 holds sandbox_sync_guard() across validation and persistence, but that guard is process-local. OpenShell explicitly supports multiple gateway replicas sharing Postgres, so two replicas can still reserve the same sandbox credential keys...

IMO this is now ready, if you could please resolve conflicts, I will approve and we can get this merged.

[russellb]

Conflicts are resolved, but I'm running through the full example scenario to make sure it's good, as the resolved conflicts were not trivial edits.

[russellb]

Conflicts are resolved, but I'm running through the full example scenario to make sure it's good, as the resolved conflicts were not trivial edits.

Testing complete. Still functional.

[pimlock]

/ok to test e3ad817

[russellb]

@pimlock do you have more visibility into what failed in CI? or the ability to retrigger it if it's an environment thing?

[pimlock]

@pimlock do you have more visibility into what failed in CI? or the ability to retrigger it if it's an environment thing?

@russellb Looks like a flake, I will create an issue, saw it pop up on main as well. I restarted it and it will hopefully pass this time.

EDIT: issue for the flake: #2326