Skip to content

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

Open
johntmyers wants to merge 32 commits into
refactor/proxy-egress-pipelinefrom
fix/static-credential-endpoint-binding
Open

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

Conversation

@johntmyers

Copy link
Copy Markdown
Collaborator

Summary

Bind static provider credentials to profile-authorized host, port, and path selectors so placeholders cannot be resolved at other policy-allowed endpoints. This is a focused, stacked extension of the proxy refactor in #2373 and preserves dynamic credential behavior.

Related Issue

Design: #2155
Stacked on: #2373

Changes

  • negotiate endpoint-binding capability and send non-secret binding metadata to compatible supervisors
  • install and revoke endpoint-scoped static credentials atomically while preserving dynamic credentials
  • enforce credential scope across CONNECT, forward-proxy, L7 HTTP, and WebSocket paths with fail-closed denial and OCSF reporting
  • document the security boundary and update provider workflow guidance
  • add endpoint-isolation e2e coverage and make existing credential fixtures use valid bound profiles

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

Checklist

@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.

)
.await?;
let provider_environment = super::provider::resolve_provider_environment_with_catalog(
let mut provider_environment = super::provider::resolve_provider_environment_with_catalog(

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 — CWE-367: Build the revision and credential payload from the same immutable provider-record snapshot. This resolver re-fetches providers after revision calculation—and internally fetches them again for validation and dynamic credentials—so concurrent detach/delete/recreate can return revision R from identity A with credentials and binding identity B. The supervisor then admits R into B's identity epoch, allowing an A-issued vR_KEY placeholder to resolve B's secret. Load each provider once and derive revision, static bindings, environment, and dynamic grants from those records; add a replacement-race regression.

@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.

let mut text = String::from_utf8(message_payload)
.map_err(|_| miette!("websocket text message is not valid UTF-8"))?;
let replacements = if let Some(resolver) = options.resolver {
let live_resolver = options

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

Critical — CWE-367/CWE-672: resolver_for_endpoint snapshots credential material, but WebSocket message rewriting carries no credential revision and performs no generation check before write_masked_frame*. A detach, endpoint narrowing, or failed refresh concurrent with inspection/compression can complete before the frame write while this message still sends the revoked value. Return the resolver and revision from one locked state snapshot, carry a CredentialGenerationGuard, and check it immediately before every credential-bearing frame write. Add a blocking-writer regression that resolves, revokes, releases the write, and asserts zero upstream bytes.

for (key, binding) in bindings {
match epochs.get_mut(key) {
Some(epoch) if epoch.identity == binding.credential_identity => {
epoch.revisions.insert(revision);

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

Warning — CWE-400: Every same-identity refresh stores its revision forever, and resolver_for_endpoint clones the complete revision set for every request. A frequently refreshed, long-running sandbox therefore accumulates unbounded memory and makes credential scoping O(number of rotations) per request. Use a stable identity-epoch token or another bounded/O(1) verifier, and add a high-rotation regression that checks retained state and request cost remain bounded.

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.

The memory-growth observation is valid, but I do not consider it a blocker for this PR. The set grows only across authenticated control-plane rotations of the same credential identity; live secret generations are already bounded, and this does not create a fail-open or cross-endpoint exposure. Making verification truly bounded/O(1) requires replacing revision membership with a stable identity-epoch token, which is a broader placeholder-identity redesign rather than a focused endpoint-binding correction. I am intentionally deferring that follow-up instead of adding an arbitrary cap here, since a cap would break long-running placeholders after legitimate rotations.

inner.combined_resolver = None;
inner.non_secret_environment_keys.clear();
inner.static_credential_bindings.clear();
inner.static_credential_identity_epochs.clear();

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

Warning — lifecycle recovery: A transient provider-environment fetch failure invokes this revocation and clears the only record that an existing placeholder revision belonged to the current credential identity. After a successful retry, running processes still hold the old placeholder, but the rebuilt epoch contains only the new revision, so those processes never recover without restart. Either provide a safe recoverable tombstone that still prevents detach/re-attach reuse, or explicitly document and signal the restart requirement. Add a fetch-failure → successful-retry regression using an already-running process.

}

registry.inject_env(&provider, &mut env);
registry.inject_env(provider, &mut env);

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

Warning: Static-key ownership is recorded before plugin configuration is injected, while uniqueness validation covers only credential keys. A plugin-generated config key can collide with another provider’s static credential; attachment order then determines which value is placeholderized and which provider’s binding controls it. Build and classify each provider’s emitted environment independently, reject credential/config collisions before merging, and add order-independent cross-provider collision tests.

The path `/v1/**` matches `/v1` and its descendants. An empty path, `**`, or
`/**` matches every path on the selected host and port. Other path values use
glob matching. OpenShell removes the query string and uses a canonical,
secret-redacted path for this check, so a credential embedded in a request path

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

Warning: Runtime credential binding does not consistently match against the syntax-redacted path described here. CONNECT and forward HTTP pass the canonical path containing placeholder syntax to resolver_for_endpoint; redact_target_for_policy is used for policy selection and telemetry. Either use the same redacted representation for binding enforcement or document the actual selector input. Add path-embedded credential tests covering exact and glob profile selectors.

@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.

normalize_provider_type(&provider.r#type).unwrap_or(provider.r#type.as_str());
let profile =
get_provider_type_profile_for_scope(catalog, profile_id, &provider.profile_workspace);
let profile_endpoints = profile.as_ref().map(|profile| {

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

Critical — built-in provider regression: profile_endpoints becomes Some(vec![]) for the built-in aws and google-cloud profiles: both profiles intentionally declare credentials without endpoints. Those empty bindings are emitted and then rejected by the supervisor, disabling the documented generic AWS SigV4 and GCP metadata-emulator flows even when sandbox policy authorizes the destination. Introduce a least-privilege binding source for policy-scoped profiles without automatically composing broad provider policy, and cover both built-ins end to end.

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: the built-in aws and google-cloud profiles have no profile endpoints, so the new empty binding metadata is rejected. I am not going to restore them by silently bypassing endpoint binding or by inventing broad provider endpoints. Preserving their documented policy-scoped flows requires an explicit policy-derived binding authority with defined lifecycle and authorization semantics, which is a design expansion rather than a focused correction in this PR. This remains a known compatibility blocker requiring an explicit design decision.

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.

The collateral snapshot effect is now isolated in commit 18fbf27: endpointless selected-profile static keys and their metadata are withheld fail-closed, while unrelated endpoint-bound credentials remain active. This intentionally does not claim to restore the AWS/GCP static flows; that still requires the policy-derived binding-authority design described above.

/// 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>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:approval-needed Gator completed review; maintainer approval needed test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant