Skip to content

feat(proxy): bind static credentials to provider endpoints - #2510

Open
johntmyers wants to merge 59 commits into
mainfrom
fix/static-credential-endpoint-binding
Open

feat(proxy): bind static credentials to provider endpoints#2510
johntmyers wants to merge 59 commits into
mainfrom
fix/static-credential-endpoint-binding

Conversation

@johntmyers

@johntmyers johntmyers commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

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, stacked extension of the proxy refactor in #2373. Dynamic credential grants keep their existing behavior.

Related Issue

Design: #2155
Stacked on: #2373

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

copy-pr-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

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

Copy link
Copy Markdown

@johntmyers johntmyers added the test:e2e Requires end-to-end coverage label Jul 28, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator Author

/ok to test 6d36f25

@github-actions

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Collaborator Author

/ok to test e2b271a

@johntmyers

Copy link
Copy Markdown
Collaborator Author

/ok to test 6231d64

@johntmyers

Copy link
Copy Markdown
Collaborator Author

/ok to test 94491fe

@johntmyers
johntmyers marked this pull request as ready for review July 28, 2026 16:08

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-supervisor-network/src/proxy.rs Outdated
Comment thread crates/openshell-supervisor-network/src/proxy.rs Outdated
Comment thread crates/openshell-supervisor-network/src/l7/websocket.rs Outdated
Comment thread crates/openshell-core/src/provider_credentials.rs Outdated
Comment thread crates/openshell-server/src/grpc/policy.rs Outdated
Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated
@johntmyers johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jul 28, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator Author

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 johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-supervisor-network/src/proxy.rs
Comment thread crates/openshell-supervisor-network/src/l7/relay.rs
Comment thread architecture/gateway.md Outdated
Comment thread docs/sandboxes/providers-v2.mdx Outdated

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated
Comment thread architecture/sandbox.md Outdated
@johntmyers

Copy link
Copy Markdown
Collaborator Author

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 johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated
Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated
Comment thread .agents/skills/debug-inference/SKILL.md Outdated

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated
Comment thread crates/openshell-core/src/secrets.rs Outdated

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-core/src/secrets.rs Outdated
Comment thread crates/openshell-core/src/secrets.rs
Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated
Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-server/src/grpc/policy.rs Outdated

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-sandbox/src/lib.rs
Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated
Comment thread crates/openshell-supervisor-network/src/proxy.rs Outdated
Comment thread crates/openshell-sandbox/src/lib.rs Outdated
Comment thread crates/openshell-supervisor-network/src/l7/relay.rs

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-supervisor-network/src/l7/websocket.rs Outdated
Comment thread crates/openshell-core/src/provider_credentials.rs
Comment thread crates/openshell-core/src/provider_credentials.rs Outdated
Comment thread crates/openshell-server/src/grpc/provider.rs Outdated
Comment thread docs/sandboxes/providers-v2.mdx

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-server/src/grpc/provider.rs
/// Returns `None` if the placeholder is unknown or the resolved value
/// contains prohibited control characters (CRLF, null byte).
pub fn resolve_placeholder(&self, value: &str) -> Option<&str> {
if placeholder_env_key(value).is_some_and(|key| self.identity_bound_env_keys.contains(key))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated
Comment thread crates/openshell-supervisor-network/src/proxy.rs Outdated
Comment thread crates/openshell-supervisor-network/src/l7/rest.rs Outdated
Comment thread crates/openshell-core/src/provider_credentials.rs
@johntmyers

Copy link
Copy Markdown
Collaborator Author

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 johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-core/src/provider_credentials.rs
Comment thread crates/openshell-supervisor-network/src/l7/relay.rs Outdated

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Validation: 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.

Comment thread crates/openshell-server/src/grpc/provider.rs Outdated
Comment thread crates/openshell-core/src/secrets.rs
Comment thread crates/openshell-core/src/provider_credentials.rs
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Jul 30, 2026
@johntmyers
johntmyers force-pushed the refactor/proxy-egress-pipeline branch from f7b8219 to 04c16b0 Compare July 30, 2026 23:19
@johntmyers
johntmyers force-pushed the fix/static-credential-endpoint-binding branch from 619afef to 2035963 Compare July 30, 2026 23:20
@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:blocked Gator is blocked by process or repository gates labels Jul 30, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator Author

gator-agent

Maintainer Approval Needed

Gator validation and PR monitoring are complete for the current head.

Head SHA: 2035963b44d436c024e4ed252b83657b30da43af
Base SHA: 04c16b08142f9de132b86bc01f59baf7c41ea1c3
Merge base SHA: 04c16b08142f9de132b86bc01f59baf7c41ea1c3
Patch ID: 0e7b0f6f3a9657e798b8c62a2f98b7bdb06a74a7
Gator payload: 2
Review mode: already_reviewed
Previous reviewed SHA: 619afefda9433ff5f52ec939292283b053109e20

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

Review: The authoritative feedback ledger identifies this head as a rebase-equivalent patch. Its patch ID exactly matches the previously reviewed patch, so Gator did not run another reviewer or generate another finding-bearing round. No open blocking findings remain.

Thanks @johntmyers. I rechecked your replies on the two latest invalidated findings against the durable ledger and the unchanged effective patch:

  • gator-inline-3679554729, the built-in AWS/GCP compatibility family, remains addressed by the policy-derived binding implementation preserved in this patch.
  • gator-inline-3679554737, the identity-epoch redesign, remains waived as non-blocking follow-up debt. The equivalent rebase does not invalidate that disposition.

Docs: Fern and architecture guidance remain complete, with existing navigation sufficient.

Checks: OpenShell / Branch Checks, OpenShell / Helm Lint, OpenShell / E2E, DCO, and the required CI gate are green.

E2E: test:e2e is already applied and passed for this head. No additional /ok to test action is needed.

Human maintainer approval is now required.

@johntmyers johntmyers added gator:approval-needed Gator completed review; maintainer approval needed gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:watch-pipeline Gator is monitoring PR CI/CD status gator:approval-needed Gator completed review; maintainer approval needed labels Jul 30, 2026
Base automatically changed from refactor/proxy-egress-pipeline to main July 31, 2026 21:10
@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates and removed gator:approval-needed Gator completed review; maintainer approval needed labels Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:blocked Gator is blocked by process or repository gates test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant