feat(proxy): bind static credentials to provider endpoints

[johntmyers]

Summary

Bind static provider credentials to the provider identity and authorized host, port, and path where they may be used. A placeholder for one provider can no longer resolve merely because the destination is otherwise allowed by network policy.

This is a focused extension of the proxy refactor merged in #2373. Dynamic credential grants keep their existing behavior.

Related Issue

Design: #2155
Refactor foundation: #2373 (merged)

User-facing behavior

Profiles that define endpoints

No additional sandbox policy authoring is required. Each static credential environment key is automatically bound to the endpoints declared by its provider profile. The placeholder resolves only when the request matches both:

• the effective network policy; and
• the profile-derived credential boundary, including host, port, and canonical path.

Allowing another destination in network policy does not make that provider's credential usable there.

Endpointless profiles

A sandbox policy must explicitly bind each authorized endpoint to the concrete attached provider instance:

network_policies:
  aws_s3:
    endpoints:
      - host: s3.us-west-2.amazonaws.com
        port: 443
        protocol: rest
        access: full
        credential_binding:
          provider: work-aws
        credential_signing: sigv4
        signing_service: s3
        signing_region: us-west-2

credential_binding.provider names the attached provider instance (work-aws), not its profile type (aws). This keeps identity explicit when a sandbox attaches multiple providers of the same type. The binding chooses which provider may supply credentials; signing and rewrite fields continue to describe how the proxy applies them.

Explicit bindings are sandbox-scoped. They are rejected in gateway-global policy and are unnecessary—and rejected—for profiles that already define endpoints.

Fail-closed behavior

• An endpointless profile with no policy binding contributes its non-secret configuration, but OpenShell withholds its static credential keys and binding metadata.
• A placeholder used outside its bound host, port, or path is denied before credentials reach the upstream. The denial is reported through OCSF.
• Policy submission fails with FAILED_PRECONDITION when a binding names an unattached provider, a provider without a selected profile, or an endpoint-bearing profile. The complete update is rejected without partial activation.
• Detaching a provider that an active sandbox policy references is rejected.
• Credential refresh and rotation retain the provider identity and binding. Expired, revoked, stale, or incomplete generations fail closed.
• CONNECT, forward-proxy, inspected HTTP, and WebSocket paths enforce the same identity boundary.

Upgrade behavior

A gateway sends static credentials only to supervisors that advertise endpoint-binding support. Restart or recreate older running sandboxes after upgrading the gateway and supervisor, and rotate credentials if an older sandbox may previously have received real values.

Changes

• negotiate endpoint-binding capability and send non-secret binding metadata to compatible supervisors
• derive static credential bindings from profile endpoints or explicit sandbox policy bindings for endpointless profiles
• install, rotate, and revoke provider-scoped static credentials atomically while preserving dynamic credential behavior
• enforce provider identity and endpoint scope across CONNECT, forward-proxy, L7 HTTP, and WebSocket paths
• reject invalid binding lifecycle changes before persistence or partial activation
• emit fail-closed denial rationale through OCSF without logging credential material
• document provider setup, policy authoring, AWS SigV4 usage, failure behavior, and upgrade guidance
• add endpoint-isolation and lifecycle regression coverage

Testing

• mise run pre-commit passes
• mise run test passes
• Unit tests added/updated
• mise run e2e:rust passes
• mise run e2e:python passes (87 passed)
• mise run e2e:mcp passes (3 scenarios)
• E2E tests added/updated
• Remote Docker, Podman, Kubernetes, and VM E2E matrices pass

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated
• User-facing provider and policy docs updated

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[github-actions]

🌿 Preview your docs: https://nvidia-preview-pr-2510.docs.buildwithfern.com/openshell

[johntmyers]

/ok to test 6d36f25

[github-actions]

Label test:e2e applied for 6d36f25. 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.

[johntmyers]

/ok to test e2b271a

[johntmyers]

/ok to test 6231d64

[johntmyers]

/ok to test 94491fe

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid security hardening authored by a maintainer, following RFC #2155 and stacked on proxy refactor #2373.
Head SHA: 94491febb429bc4bff1d801abdef2386239d96d8

Review findings:

• Four blocking authorization/lifecycle flaws are anchored inline.
• One high-severity dynamic-credential lifecycle flaw and one medium-severity adapter-parity flaw are anchored inline.
• General medium finding: forward-proxy telemetry still receives the original request path. Without L7 this can expose query strings, placeholder syntax, and environment-key names, contrary to the documented OCSF boundary. Construct a syntax-only redacted target before all forward telemetry, omit queries, and add allowed/denied/malformed-request log assertions.

Docs: Fern coverage is extensive and the affected pages are auto-indexed, so no navigation update is needed. The current text promises canonical path matching, immediate detach revocation, preserved dynamic credentials, and adapter-parity reporting; those claims are not accurate until the findings are resolved.

Checks: Required branch, Helm, DCO, and E2E gates are green. The required test:e2e label and current-head /ok to test are already present.

Next state: gator:in-review pending author changes.

[johntmyers]

Addressed the general forward-telemetry finding in 59b7c13. Allowed and denied events now receive a query-free, syntax-redacted canonical path, while malformed targets use a fixed sentinel. Regression coverage serializes allowed, denied, and malformed OCSF events and asserts that query values, placeholder syntax, and environment-key names are absent.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid security hardening authored by a maintainer, following RFC #2155 and stacked on proxy refactor #2373.
Head SHA: 59b7c13f00bbe696c71df6100d1ce281aa2a9f71

Thanks @johntmyers. I checked your current-head remediation notes against the full diff. The canonical target boundary, live-state authority, provider identity, dynamic-snapshot preservation, and typed body/SigV4 denial changes resolve six prior findings. The forward-telemetry finding remains incomplete for no-path absolute URIs, and the independent review found one additional WebSocket fail-closed edge plus two documentation inaccuracies.

Review findings:

• Four actionable findings are anchored inline.
• No unanchored general findings.

Docs: Fern pages and related skills were updated, and docs/index.yml navigation does not need a change. Two statements still need correction as noted inline.

Checks: Code-only review; no local tests were run. The required test:e2e label is already present. Pipeline progression remains gated on author changes.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: 44ba6839f994dfe5855cfe9c2cc17cb3b3176a4b

Thanks @johntmyers. I checked your current-head replies against the full diff. The absolute-URI telemetry fix, fail-closed WebSocket relay selection, gateway dynamic-snapshot wording, and HTTP-versus-WebSocket failure documentation resolve all four findings from the prior head.

Review findings:

• Critical — CWE-284: Provider environment revisioning hashes the catalog's effective profile, while credential binding resolution selects a profile using provider.profile_workspace. When a workspace override and platform fallback share an ID, narrowing the platform profile can leave provider_env_revision unchanged and let a supervisor retain an obsolete broader binding. Hash the exact scoped profile selected by get_provider_type_profile_for_scope(), including its scope, and add a platform-scoped-provider regression beneath a same-ID workspace override. Relevant code: crates/openshell-server/src/grpc/policy.rs:1611 and crates/openshell-server/src/provider_profile_sources.rs:437,470.
• Two actionable warnings are anchored inline.
• Warning — agent guidance: The sync-agent-infra maintenance map routes provider and direct-inference behavior changes to debug-inference, but that skill still diagnoses direct external denials only as missing policy/provider attachment. Add credential_endpoint_mismatch guidance that checks profile endpoints and warns against widening sandbox policy unless the profile intentionally authorizes the recipient.

Docs: Fern pages are updated and already navigated, so no navigation change is needed. One architecture statement remains inaccurate as noted inline.

Checks: Code-only review; no local tests were run. test:e2e is already applied and the current E2E run is pending. No additional /ok to test action is needed in this cycle.

Next state: gator:in-review pending author changes.

[johntmyers]

Addressed the two unanchored findings from review 4802466107:

• b92f7696 hashes the exact provider profile selected by profile_workspace, including its scope and the platform fallback beneath a same-ID workspace override. Regressions cover broad-to-narrow platform endpoint changes and verify workspace-scoped providers remain keyed to the workspace override.
• 42a454bb adds credential_endpoint_mismatch diagnosis to debug-inference, including profile endpoint inspection and an explicit warning not to widen sandbox policy as a workaround. The sync-agent-infra maintenance map and companion openshell-cli guidance were checked.

Validation: 1,116 gateway tests and 1,074 network tests passed; pre-commit passed.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: 42a454bb4143e672fa57f3daa0e0ee34eab11131

Thanks @johntmyers. I checked your current-head remediation notes against the full diff. The exact scoped-profile revision hash and its fallback/override regressions resolve the prior critical finding. The new credential_endpoint_mismatch guidance correctly warns against widening policy, but its inspection command is incomplete. The independent review also found two remaining single-route adapter-parity gaps after the request-admission ordering fix.

Review findings:

• Three actionable warnings are anchored inline.
• No unanchored general findings.

Docs: Fern coverage is extensive and the affected pages are already navigated, so no docs/index.yml change is needed. The documented HTTP 403 and OCSF behavior remains inaccurate for single-config JSON-RPC/MCP and GraphQL until the two relay findings are resolved.

Checks: Code-only review; no local tests were run. test:e2e is already applied and the current-head E2E run is pending. No additional /ok to test action is needed in this cycle.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: 9c56260580350b65c0775f4ad4b83a64a82fd830

Thanks @johntmyers. I checked the current-head remediation against the full diff. The complete profile-inspection guidance and the typed single-route JSON-RPC/MCP and GraphQL denial paths resolve all three findings from the prior head. The independent review found two remaining credential-binding lifecycle issues.

Review findings:

• Two actionable findings are anchored inline.
• No unanchored general findings.

Docs: Fern coverage is extensive and the affected pages are already included by folder navigation, so no docs/index.yml change is needed. The rotation guarantee in docs/sandboxes/providers-v2.mdx remains inaccurate until the retained-generation finding is resolved or the guarantee is narrowed.

Checks: Code-only review; no local tests were run. test:e2e is already applied and the current-head E2E check is pending. The workflow is active, so no additional /ok to test action is needed in this cycle.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: a06b27236061a6cb1fe5a82ac4ab1144eb2bd72d

Thanks @johntmyers. I checked your current-head remediation against the full diff. The CONNECT authority guard is now applied before credential scoping across the advertised L7 flows, and the nine-rotation regression exercises the intended same-identity fallback. The independent review found that the epoch implementation treats digest-derived revisions as ordered, so it can still expose a replacement identity's credential; it also found three remaining identity/authority edge cases.

Review findings:

• One critical and three actionable warnings are anchored inline.
• No unanchored general findings.

Docs: Fern and architecture coverage are updated and already navigated, so no docs/index.yml change is needed. The documented rotation and replacement guarantees remain inaccurate until the identity-epoch findings are resolved.

Checks: Code-only review; no local tests were run. test:e2e is already applied and the current-head Branch Checks/E2E pipelines are active, so no additional /ok to test action is needed in this cycle.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: 3f5be52b4dfd7688dc165352f31bd7db7c1ffd85

Thanks @johntmyers. I checked your current-head fixes for opaque revision membership, identityless placeholder rejection, authority-less HTTP/1.0 handling, and effective-port matching against the full diff; those changes resolve the prior-head findings. The independent review found two remaining credential-lifecycle issues.

Review findings:

• One high-severity TOCTOU finding is anchored inline.
• Medium — credential rotation lifecycle (crates/openshell-server/src/grpc/policy.rs:1605): Provider-environment revision hashing still uses millisecond updated_at_ms, which can remain unchanged across consecutive writes even though both persistence backends increment resource_version. With unchanged key names, profile, and expiry, a same-millisecond credential rotation can therefore leave the same revision and the supervisor will not fetch the new value. Hash record.resource_version and add a no-sleep consecutive-write regression. This line is outside the current diff hunk, so the finding is retained in the summary rather than anchored inline.

Docs: Fern and architecture coverage are comprehensive and already navigated. The atomic lifecycle guarantee remains inaccurate until the TOCTOU finding is resolved.

Checks: Code-only review; no local tests were run. test:e2e is already applied. Branch Checks, Helm, and DCO are green; the current E2E run completed red in the Python E2E job. The workflow ran for this head, so no additional /ok to test action is needed in this cycle.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: e897bcf034085b17629d8f847340c339abe4770b

Thanks @johntmyers. I checked the current-head remediation against the full diff. Deriving the revision and payload from one immutable provider-record snapshot resolves the prior TOCTOU finding, and hashing authoritative resource_version with a no-sleep regression resolves the same-millisecond rotation finding. The independent review found three remaining security-sensitive gaps plus two actionable warnings.

Review findings:

• Three critical findings and two warnings are anchored inline.
• No unanchored general findings.

Docs: Static-binding behavior is documented comprehensively and the affected pages are already in folder navigation. The new request_authority_mismatch response still needs troubleshooting guidance for explicit non-default authority ports.

Checks: Code-only review; no local tests were run. test:e2e is already applied. Branch Checks and E2E are active for this head, so no additional /ok to test action is needed in this cycle.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: feec81dd8d67be710b6a5ef6a56ab116aa3c809f

Thanks @johntmyers. I checked the current-head remediation against the full stacked-base diff. Post-admission credential reacquisition and pre-write guards resolve the prior CONNECT/L7 and forward-proxy races; explicit non-secret classification closes the GCP environment exposure; lifecycle logs now describe dynamic-grant preservation accurately; and the new authority-mismatch guidance covers the user-visible failure. The independent review found one remaining WebSocket revocation race and four actionable warnings.

Review findings:

• One critical finding and four warnings are anchored inline.
• No unanchored general findings.

Docs: The affected Fern pages are already included by folder navigation, so no docs/index.yml change is needed. The binding-path description remains inaccurate until it matches the runtime representation, and failed-refresh recovery/restart behavior should be documented with the lifecycle fix.

Checks: Code-only review; no local tests were run. test:e2e is already applied. Branch Checks and E2E are active for this head, so no additional /ok to test action is needed in this cycle.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: 8f656f412351c9126349274034bcf5616499c2d7

Thanks @johntmyers. I checked the current-head fixes against the full diff. The WebSocket pre-write generation guard, identity tombstones after failed refresh, cross-provider collision validation, and canonical secret-redacted binding path resolve four prior findings. I also checked your note that an arbitrary revision-set cap would break long-running placeholders: that is correct, but the set still grows and is cloned per request, so the bounded identity-epoch redesign remains necessary.

Review findings:

• One critical, three high-severity, and two medium-severity findings are anchored inline.
• General medium finding: endpoint-binding guidance now contradicts the documented endpointless aws and google-cloud provider flows, and the CLI skill demonstrates generic immediately before stating profileless credentials fail closed. Resolve the provider design, update the AWS and Google Cloud pages, and replace the stale skill example.
• Missing coverage: add end-to-end regressions for built-in AWS/GCP usability, GCP metadata-issued placeholder resolution, atomic resolver/revision acquisition in CONNECT and forward paths, and origin-form targets containing ://.

Docs: The endpoint-binding explanation and 403 diagnostics are otherwise clear, and no navigation change is needed. The provider-page and CLI-skill contradictions must be corrected.

Checks: Code-only review; no local tests were run. test:e2e is already present. Branch Checks are green; the E2E aggregate remains pending with one failed child job, so the current-head workflows are active and no additional /ok to test is needed.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

High — GCP metadata credential becomes unusable: Bound credentials correctly reject identityless placeholders here, but ProviderCredentialState::gcp_token_response() still returns placeholder_for_env_key(...), and the metadata emulator serves that revisionless value to the SDK. The endpoint-scoped resolver will therefore always reject the token it receives. Return and validate the current revision-scoped child-environment placeholder, and add a metadata-to-proxy regression test.

[johntmyers]

Confirmed as a consequence of the endpointless built-in GCP design: metadata currently returns an identityless token that a bound resolver rejects. Changing only the token to be revision-scoped would still authorize no Google API endpoint because google-cloud declares none. I am not applying that partial fix; this must be resolved together with the policy-derived binding authority design noted in the built-in provider thread.

[johntmyers]

Rechecked on the current head: this remains valid but has no safe standalone fix. Revision-scoping the metadata token would still leave it with no authorized Google API endpoint. It remains coupled to the endpointless-provider binding-authority design rather than receiving a partial token-only patch.

[johntmyers]

Regarding Gator’s unanchored documentation finding: the inconsistency is confirmed, but a docs-only edit would either preserve contradictory guidance or document a newly introduced AWS/GCP regression as intended behavior. I am deferring those documentation changes with the policy-derived binding-authority design identified in the built-in provider threads; the PR should not claim those flows are preserved until that design decision is resolved.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: dc7d26aa6014f1564714d75b79649f23583a2459

Thanks @johntmyers. I checked your current-head response and fixes against the full stacked-base diff. Atomic resolver/revision acquisition resolves the prior CONNECT/L7 and forward-proxy ABA findings, and parsed-scheme classification resolves the origin-form :// finding.

Your response confirms and defers the endpointless AWS/GCP compatibility break, the GCP metadata placeholder mismatch, and the unbounded revision-membership design. The independent review confirms those remain blocking and also found an IPv6 authority regression.

Review findings:

• Critical — endpointless built-in providers invalidate the static credential snapshot: aws and google-cloud declare credentials but no endpoints, so the server emits empty bindings that bound-state validation rejects. The sandbox fallback then clears all static credentials, including otherwise valid credentials from unrelated attached providers. Preserve the supported built-in flows with an approved binding source, safely omit only unsupported keys without invalidating unrelated providers, or gate rollout until compatibility is preserved.
• Warning — GCP metadata emits an unusable placeholder: ProviderCredentialState::gcp_token_response() returns an identityless placeholder that the bound resolver rejects, and the metadata emulator returns it to GCP SDKs. Return a current revision-scoped placeholder from the same atomic credential snapshot, or define a trusted metadata-specific resolution contract, with metadata-to-proxy coverage.
• One CWE-400 revision-state finding and one IPv6 CONNECT authority finding are anchored inline.

Docs: Blocking. The Fern pages were updated and no navigation change is needed, but the endpoint-binding guidance remains inconsistent with the broken built-in AWS/GCP and metadata flows. Finalize the docs after preserving those workflows.

Missing coverage: real built-in AWS/GCP bound-state behavior, metadata-handler-to-proxy GCP resolution, bracketed IPv6 CONNECT authority matching, and a bounded-state invariant across many same-identity rotations.

Checks: Code-only review; no local tests were run. test:e2e is already applied and current-head Branch Checks/E2E workflows are active, so no additional /ok to test is needed.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: da06ea5260f37035219f92ddba273d6c161f62c2

Thanks @johntmyers. I checked your current-head responses and fixes against the full stacked-base diff. Normalizing both authority operands resolves the bracketed IPv6 finding, and withholding endpointless-profile keys individually resolves the collateral revocation of unrelated endpoint-bound credentials.

Your responses explicitly defer the built-in AWS/GCP compatibility break, GCP metadata placeholder contract, and unbounded revision membership. The independent review confirms those remain actionable; it also confirms that the architecture and Fern lifecycle text still describe snapshot-wide revocation even though the new implementation withholds endpointless keys independently.

Review findings:

• One blocking compatibility finding and two warnings are anchored inline.
• Blocker — documentation contract: architecture/gateway.md:403-411 and docs/sandboxes/providers-v2.mdx:132-138 say an incomplete endpointless provider revokes all static credentials, but crates/openshell-server/src/grpc/provider.rs:643-653 now withholds those keys independently. Meanwhile, docs/providers/aws-sigv4.mdx:10-15,38-42,145-157 and docs/providers/google-cloud.mdx:10-15,117-130,189-193 still present the now-disabled built-in flows as operational. This spans changed and unchanged files, so it is retained in the summary. Align the lifecycle text and either preserve those workflows or document their unavailability and migration path.

Docs: Blocking. The affected pages are already navigated, so no docs/index.yml update is needed.

Missing coverage: built-in AWS SigV4 compatibility, GCP metadata response through authorized outbound rewrite, and a bounded-state invariant across many same-identity rotations.

Checks: Code-only review; no local tests were run. test:e2e is already applied and the current-head Branch Checks are green while E2E is active, so no /ok to test action is needed.

Next state: gator:in-review pending author changes.

[johntmyers]

gator-agent

PR Review Status

Validation: Project-valid credential security hardening authored by a maintainer, following design #2155 and stacked on proxy refactor #2373.
Head SHA: b42e2a6fff9a446fb91383a6c0532a37d5c800b2
Base SHA: c42268ba0a71644ca38ac243c15eb294741f5df7
Merge base SHA: c42268ba0a71644ca38ac243c15eb294741f5df7
Patch ID: e1b39c871f67f4eb96b81fb4f51838737169954d
Gator payload: 2
Review mode: human_checkpoint
Previous reviewed SHA: 619afefda9433ff5f52ec939292283b053109e20

Thanks @johntmyers. I reconciled your prior replies about the two latest invalidated findings against the authoritative feedback ledger and the current rebase:

• The author-series range-diff maps all 32 commits exactly, so this head introduces no author-only delta. The aggregate patch ID changed because the base moved.
• gator-inline-3679554729, the built-in AWS/GCP compatibility family, remains addressed by the policy-derived binding implementation preserved in the rebased series.
• gator-inline-3679554737, the identity-epoch redesign, remains waived as non-blocking follow-up debt. This rebase does not invalidate that disposition.

Blocking findings:

• No blocking findings remain. The bounded human-checkpoint review found no newly introduced Critical defect.

Carried findings:

• None.

Docs: Fern and architecture guidance remain complete; the rebase introduces no author-only documentation change.

Checks: OpenShell / Branch Checks and OpenShell / E2E are pending. OpenShell / Helm Lint, OpenShell / GPU E2E, and DCO are green.

E2E: test:e2e is already applied and the current-head workflow is active, so no additional /ok to test action is needed.

Next state: gator:watch-pipeline

[rhuss]

The glob::Pattern::matches() call here uses default MatchOptions where require_literal_separator is false, so * matches across / path separators. The test at line 35 confirms this is intentional: matches("/v1/*", "/v1/chat/messages") returns true.

This makes * and /** functionally equivalent for credential binding, which reduces the pattern language's expressiveness. Operators familiar with HTTP routing conventions (where * matches a single path segment) may be surprised.

Have you considered using glob::MatchOptions { require_literal_separator: true, ..Default::default() } so that * matches a single path component and ** matches across separators? That would give profile authors two distinct tools:

• /v1/* matches /v1/chat but not /v1/chat/completions (single segment)
• /v1/** matches /v1/chat, /v1/chat/completions, /v1/admin/secrets (all descendants)

This is more flexible for fine-grained credential binding, for example binding to /v1/*/completions without inadvertently covering /v1/admin/keys. It also aligns with the semantics already used in the network policy rules path globs, which would keep operator mental models consistent.

If the current catch-all behavior for * is preferred, a comment here explaining the deliberate choice would help future readers.

[rhuss]

See also https://github.com/NVIDIA/OpenShell/pull/2510/changes#diff-218f32b3113bf219a24d7d6726fd7401d9593bac7bb4a449845661dfb3fa2e23R35

[rhuss]

scoped_context_for_request passes request.target directly to resolver_for_endpoint_with_revision without path canonicalization. The forward proxy path (proxy.rs:3646) correctly canonicalizes via canonicalize_request_target before credential resolution, but this relay path (CONNECT tunnel) does not.

A request with path /v1/../v2/completions matches a binding pattern of /v1/** (because "/v1/../v2/completions".starts_with("/v1/") is true), but the upstream server normalizes to /v2/completions. This could resolve Provider A's credentials for a request that actually reaches Provider B's API surface.

Practical risk is lower than it sounds (most HTTP libraries normalize paths before sending, and L7 policy must also admit the request), but canonicalizing here for consistency with the forward proxy path would be defense in depth.

[rhuss]

HostPattern::new(&endpoint.host) is compiled from scratch on every HTTP request for every endpoint binding, inside the read-lock critical section. HostPattern::new() allocates a lowercased String, splits by DNS labels, compiles a glob::Pattern per label, and builds a Vec.

With N bindings having M endpoints each, this is N*M pattern compilations per request. For a sandbox routing through 3-5 providers, that is 3-15 pattern compilations per HTTP request on the hot path.

These patterns are static between credential refreshes. Pre-compiling them at install time (in from_bound_environment / install_bound_environment) and storing them alongside the binding would eliminate repeated allocation.

[rhuss]

No sure though if performance is an issue here anyway (and storing them statical wouldn't make things more complex) [H]

[rhuss]

This emit_credential_endpoint_mismatch function is duplicated nearly identically in websocket.rs (line 136) and a third variant exists in relay.rs (via build_credential_endpoint_mismatch_finding). All three emit the same OCSF finding (openshell.provider_credential.endpoint_mismatch). The websocket.rs version differs only by adding ("protocol", "websocket") to evidence pairs.

When the finding schema evolves, all three must be updated in lockstep. Consider extracting a shared builder that accepts an optional protocol tag.

[rhuss]

This validation function has 7 distinct rejection branches (duplicate non-secret keys, credential-vs-non-secret overlap, unclassified credential keys, missing env keys, empty credential identity, invalid endpoint host/port) but only the "no authorized endpoints" branch is exercised by existing tests. These checks are the defense against malformed provider metadata from the gateway. Consider adding targeted unit tests for each rejection branch to prevent regressions.

[rhuss]

All existing tests for resolver_for_endpoint use a single API_KEY binding. A test with two credential keys bound to different endpoints (e.g., KEY_A to a.example.com, KEY_B to b.example.com) that verifies KEY_B does NOT resolve at endpoint A would directly cover the multi-provider credential isolation property, which is the core security goal of this PR.

[rhuss]

This E2E test proves cross-host credential denial (credential used at provider B's host is denied), but both the allowed and denied requests use /allowed/check which matches the profile's /allowed/** pattern. There is no test proving that a request to a non-matching path (e.g., /other/check) at the SAME host is also denied.

Adding a third curl request that hits the bound host but a non-matching path would cover the path-based endpoint binding boundary end-to-end, complementing the host isolation already tested here.

[rhuss]

When install_bound_environment fails validation here, it calls revoke_static_provider_environment_inner which acquires the write lock independently. If concurrent callers existed, there would be a brief window where the failing thread's revocation could wipe state that a concurrent thread is about to overwrite.

In practice the refresh loop serializes these calls, so this is safe today. A brief doc comment noting the single-writer assumption would guard against future refactors that might introduce concurrency.

[rhuss]

Code duplication across relay paths

Two patterns are duplicated across the L7 relay files that may drift over time:

1. graphql_log_summary is fully duplicated between relay.rs (line 1817) and websocket.rs (line 1134). Both format GraphQL operation info identically. Consider extracting to a shared module.

2. engine_type_for_protocol is defined as a function at relay.rs:418 but an inline match config.protocol at relay.rs:713 reproduces the same mapping. A protocol added to one site but not the other would silently produce the wrong engine type in OCSF events. Consider calling engine_type_for_protocol(config.protocol) at line 713.

[rhuss]

Additional observations (informational, not blocking)

Beyond the inline comments above, here are lower-priority observations from a multi-perspective review. None of these are blocking, just flagging for your awareness.

Minor findings:

Area	Observation
Hot path allocations	resolver_for_endpoint_with_revision allocates a HashSet<String> and clones epoch.revisions (HashSet<u64>) per request inside the read lock. Proportional to binding count (typically 1-5), so tolerable, but the epoch.revisions clone grows over time with same-identity refreshes. (provider_credentials.rs:251-278)
Unbounded epoch revisions	StaticCredentialIdentityEpoch.revisions grows by one entry per credential refresh for stable-identity providers and is never pruned. ~288 entries/day with 5-minute refreshes. Only the most recent MAX_RETAINED_CREDENTIAL_GENERATIONS = 8 can actually resolve, so older entries serve no purpose. (provider_credentials.rs:637-658)
WebSocket generation guard	The WebSocket credential relay creates a CredentialGenerationGuard per text message but does not check it against the current provider revision before upstream write, unlike the REST path which does. The window is very small (microseconds per rotation). (websocket.rs:586-596)
GCP resolution on old path	install_environment (the unbound credential path) clears non_secret_environment_keys, which means child_env_with_gcp_resolved will never resolve GCP static config vars through this path. If any production code still uses install_environment for GCP providers, this would silently break config resolution. (provider_credentials.rs:423-462)
IPv6 normalization	Authority matching normalizes hostnames via normalized_endpoint_host (strips brackets, trailing dots), but credential binding uses HostPattern::new() directly. Inconsistency is fail-closed (safe direction) but could cause unexpected credential unavailability for IPv6 endpoints. (relay.rs:146-156)
Capability downgrade	When a supervisor doesn't set supports_static_credential_bindings = true, credentials may be sent without binding metadata, making them resolve for all endpoints. A gateway-side log warning when credentials are sent without binding support would help operators detect downgrades.
#[cfg(test)] behavior divergence	request_default_port in relay.rs:82-97 has a #[cfg(test)] fallback that returns Some(ctx.port) while production returns None. Test-only behavior changes could mask bugs triggered only when the port is absent.
decision_str duplication	The (allowed, enforcement, is_upgrade) to string mapping is repeated 4 times in relay.rs. Extract to a function to prevent drift.

Notable (design observations, not bugs):

• MAX_RETAINED_CREDENTIAL_GENERATIONS = 8 has no documented rationale. The choice affects the window during which in-flight requests with old placeholders can still resolve. A brief comment explaining the tradeoff would help.
• known_static_credential_keys is monotonically extended via .extend() and never pruned. Bounded in practice (fewer than 20 distinct env var names), but stale key names remain after provider replacement.
• All RwLock acquisitions use .expect("poisoned"). If any thread panics while holding a write lock, all subsequent operations panic, cascading to the entire sandbox proxy. This is a deliberate fail-fast design, just noting it.

[rhuss]

@johntmyers All my comments above are from my own review agents, they look plausible to me (i've added manual comments tagged with [H] where I'm unsure)

Funny new discovery during the review round today :-)

> does our review also checked whether the provided PR matches its declared goal ?

...

No, our review didn't check goal alignment. The existing comments focused on implementation quality (glob
  semantics, missing canonicalization, hot-path allocation, code duplication). Here's the goal-alignment analysis:

  All 9 declared goals are DELIVERED. The enforcement covers all four claimed paths (CONNECT, forward-proxy, L7 HTTP,
  WebSocket) with consistent typed-error handling and OCSF emission.

Undeclared changes worth calling out in the PR description:

1. Absolute-form authority validation (l7/rest.rs): New security hardening that validates HTTP request-target authority matches the Host header. Substantive new behavior not mentioned in the PR body.

2. Origin-form embedded URL fix (l7/path.rs): canonicalize_request_target now distinguishes absolute-form URIs from origin-form targets containing ://. This is a pre-existing bug fix, not part of the binding feature.

3. Revision hash version bump from v1 to v3 (grpc/policy.rs): Now hashes resource_version instead of updated_at_ms. Existing supervisor caches see a forced revision change on upgrade even without credential changes.

4. Provider profile scope-aware hashing (provider_profile_sources.rs): Renamed hash_profile_revision to hash_type_profile_revision_for_scope, changing multi-workspace behavior. Not mentioned.

5. Forward-proxy URI fragment rejection (proxy.rs): parse_proxy_uri now rejects URIs with # fragments. Minor but undeclared.

None of these contradict the PR's goals, but items 1-3 are substantive enough to warrant a mention in the description.

  None of these contradict the goals, but items 1-3 are substantive enough that they should be called out in the PR
  description. Want me to draft a review comment about the undeclared changes?

I think that's an interesting way to ask for possible compatibility challenges :-)

[rhuss]

That actually check the same behaviour as for **. I would expect to match this only when there is one remaining path part, not two. See also my other comment.