Skip to content

fix(network): harden signed request handling - #4558

Open
alangou wants to merge 5 commits into
mainfrom
fix/276-request-validation/alangou
Open

alangou wants to merge 5 commits into
mainfrom
fix/276-request-validation/alangou

Conversation

@alangou

@alangou alangou commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Harden proxy-generated SigV4 traffic so signing credentials remain limited to the trusted signer and upstream diagnostics cannot return signed request material to workloads. Preserve the existing request-path canonicalization contract while ensuring S3 signs exactly the canonical target sent upstream.

Related Issue

Tracked in the maintainer security workflow.

Changes

  • Separate workload-controlled request validation from trusted SigV4 credential resolution across CONNECT and forward-proxy paths.
  • Apply S3-specific signing rules after path canonicalization so route selection, policy evaluation, signing, and forwarding use the same request target.
  • Replace non-success responses to signed requests with local diagnostics and suppress signer canonical-request traces from workload log delivery.
  • Add regression coverage for request placement, response framing, path canonicalization, resolver composition, and log filtering.
  • Document the signed-request restrictions and response behavior.

Compatibility

  • SigV4 request fields no longer resolve workload-selected credential placeholders. This is intentionally fail-closed because preserving arbitrary placeholder placement would expose resolved credentials to the signed service. Workloads that need generic placeholder injection must use a separate non-SigV4 endpoint.
  • Existing path canonicalization remains unchanged for S3 and non-S3 routes. S3 object-key spellings containing repeated slashes, dot segments, path parameters, or decoded unreserved bytes are therefore addressed in canonical form; OpenShell signs and forwards that same canonical form.
  • Signed requests relay only final 2xx upstream responses. OpenShell preserves upstream 4xx/5xx status codes but replaces response details; it suppresses redirects, informational responses, and malformed responses. Clients must not depend on opaque AWS error bodies, headers, or redirect locations through a signed endpoint.

Testing

  • Checks appropriate to the affected code and behavior pass
  • Unit tests added/updated
  • E2E tests added/updated

Validation performed:

  • cargo fmt --all --check
  • cargo clippy -p openshell-core -p openshell-supervisor-network -p openshell-supervisor-process --all-targets -- -D warnings
  • cargo test -p openshell-core (559 passed)
  • cargo test -p openshell-supervisor-network (1,579 passed, 2 ignored; integration tests passed)
  • cargo test -p openshell-supervisor-process aws_sigv4_events_are_never_pushed_at_trace_level (1 passed)
  • cargo test -p openshell-supervisor-process (126 passed)
  • mise run markdown:lint
  • TMPDIR=/var/tmp mise run e2e:docker

Five optional LocalStack tests remained ignored because LocalStack was not running. The Docker E2E lane passed for the core security patch; the compatibility follow-up passed the complete network crate suite and independent code, security, and test reviews.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (not applicable; published AWS behavior documentation updated)

@copy-pr-bot

copy-pr-bot Bot commented Oct 14, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown

@alangou alangou added test:e2e Requires end-to-end coverage gator:in-review Gator is reviewing or awaiting PR review feedback labels Oct 14, 2026
@alangou

alangou commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test f456c79

@github-actions

Copy link
Copy Markdown

Label test:e2e applied for f456c79. 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, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@alangou

alangou commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

gator-agent

PR Review Status

The initial code review found no blocking issues in the signed-request hardening. The published AWS documentation covers the credential restrictions, S3 path requirements, and response filtering.

Thanks @alangou for authorizing tests on the current head. Branch Checks is queued, Helm Lint and Trivy Changes passed, and the E2E workflow has progressed through PR metadata into its build stage. The label-help rerun hint is already satisfied by this fresh dispatch with E2E enabled; no rerun is needed.

Blocking findings: None.

Carried findings: None.

Gator metadata
  • Validation: Maintainer-authored, focused network credential and signing hardening.
  • Docs: docs/how-it-works/providers/aws.mdx updated for the direct behavior changes.
  • Checks: Current-head Branch Checks queued; Helm Lint, Trivy Changes, DCO, and vouch passed.
  • E2E: test:e2e present; current-head Branch E2E Checks run 38616902551 is in progress, with metadata complete and version stage running.
  • Head SHA: f456c79a07961a03857346594a0ac2cc50096d4a
  • Base SHA: 8b774e2a6a5986d73efa273ba027ffdbc0ad5281
  • Merge base SHA: 6e69360d0e1536d891f4abfbb60c1d90e50188c9
  • Patch ID: 801d689c7e776591321105bea20027c598e44fe6
  • Gator payload: 12
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@alangou alangou added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Oct 14, 2026
@github-actions

Copy link
Copy Markdown

Compatibility review (advisory)

Unaddressed breaking changes

Existing SigV4 consumers can now receive local 403/502 responses instead of credential substitution or upstream diagnostics. S3 signing also makes path normalization failures authority-wide, including neighboring non-S3 routes.

1. api-behavior — ACKED

Credential placeholders in signed request headers, paths, or query parameters previously resolved, but now cause a local 403 response.

  • Impact: Applications using provider placeholders outside stripped AWS authentication headers on SigV4 endpoints can no longer send those requests.
  • Action: Preserve placeholder substitution by default on this non-breaking train, or gate the prohibition behind an explicit opt-in policy setting and document how affected fields must be supplied.
  • PR: fix(network): harden signed request handling #4558

2. policy — ACKED

Adding any S3 SigV4 route now rejects path-normalizing requests across every L7 route sharing that host and port instead of canonicalizing and forwarding them.

  • Impact: Existing S3 object-key requests and unrelated neighboring routes fail when paths contain repeated slashes, dot segments, path parameters, or decodable bytes.
  • Action: Keep existing policies working by scoping byte-exact handling to the selected S3 route or making strict authority-wide rejection opt-in until a breaking train.
  • PR: fix(network): harden signed request handling #4558

3. api-behavior — ACKED

SigV4 responses previously relayed upstream informational, redirect, error-header, and error-body details, but now suppress them and return a generic local response.

  • Impact: AWS clients lose service error codes, diagnostic bodies, redirect locations, and retry metadata, while redirects and malformed responses change to HTTP 502.
  • Action: Retain the established upstream response contract by default, or introduce an opt-in hardened response mode with migration guidance for AWS SDK error and redirect handling.
  • PR: fix(network): harden signed request handling #4558
Review details
  • Execution: complete
  • Candidate: PR #4558 (5b77671806997e986ebb09d0e2537c9e79b77ddd)
  • Baseline: 8b774e2a6a5986d73efa273ba027ffdbc0ad5281 (8b774e2a6a5986d73efa273ba027ffdbc0ad5281)
  • Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

@alangou

alangou commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test a3fa091

@alangou

alangou commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

gator-agent

PR Review Status

The follow-up review found no blocking issues in the S3 route-scoping change. I checked canonical route selection and byte-exact validation in both CONNECT and forward-proxy paths, the new regression coverage, and the updated AWS documentation.

CI authorization now covers the current head. Branch Checks, Helm Lint, and Branch E2E Checks are running; E2E has completed PR metadata and queued its version stage. The earlier label-help rerun instruction applied to the previous head; this fresh dispatch includes the existing test:e2e label.

Blocking findings: None.

Carried findings: None.

Gator metadata
  • Validation: Maintainer-authored, focused network credential and signing hardening.
  • Docs: docs/how-it-works/providers/aws.mdx updated for selected-route S3 restrictions.
  • Checks: Current-head Branch Checks and Helm Lint running; Trivy Changes, DCO, and PR Issue Acceptance passed. No merge conflict.
  • E2E: test:e2e present; current-head Branch E2E Checks run 38624375822 running, metadata passed and version queued.
  • Head SHA: a3fa091d360ad411962b94fb1fe9b5fb58a0df98
  • Base SHA: 8b774e2a6a5986d73efa273ba027ffdbc0ad5281
  • Merge base SHA: 6e69360d0e1536d891f4abfbb60c1d90e50188c9
  • Patch ID: 4b1f3acb9ec181fadd8ec2b67a8f296df16ab7da
  • Gator payload: 12
  • Review mode: follow_up
  • Previous reviewed SHA: f456c79a07961a03857346594a0ac2cc50096d4a
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@github-actions

Copy link
Copy Markdown

Compatibility review (advisory)

Unaddressed breaking changes

Existing SigV4 consumers can now receive 403/400 rejections for previously accepted requests and lose AWS redirects and structured error responses. PR #4558 explicitly acknowledges these changes, but this train does not allow Stable behavior breaks.

1. api-behavior — ACKED

Signed requests now reject credential placeholders outside stripped AWS authentication headers instead of resolving them.

  • Impact: Existing SigV4 requests with placeholders in custom headers, paths, or query values fail with HTTP 403.
  • Action: Preserve placeholder resolution on existing signed endpoints for this train, or move the restriction to a breaking train with guidance to use a separate non-SigV4 endpoint.
  • PR: fix(network): harden signed request handling #4558

2. api-behavior — ACKED

S3 SigV4 endpoints now reject paths requiring slash, dot-segment, parameter, or percent-encoding normalization instead of forwarding their canonicalized form.

  • Impact: Existing S3 requests using non-canonical path spellings fail before reaching AWS.
  • Action: Preserve canonicalization for existing S3 policies on this train, or move rejection to a breaking train with guidance to emit canonical object paths.
  • PR: fix(network): harden signed request handling #4558

3. api-behavior — ACKED

Signed endpoints now replace redirects and 4xx/5xx response details with signed_upstream_rejected, map redirects to 502, and drop informational responses.

  • Impact: AWS clients lose redirect handling and structured service errors, so retries, region correction, and error parsing can fail.
  • Action: Preserve upstream response semantics on this train, or move suppression to a breaking train with actionable guidance for clients that consume AWS redirects and error payloads.
  • PR: fix(network): harden signed request handling #4558
Review details
  • Execution: complete
  • Candidate: PR #4558 (8a1db8d0c3fea8e995f451ebbf12dd16ee3dc825)
  • Baseline: 8b774e2a6a5986d73efa273ba027ffdbc0ad5281 (8b774e2a6a5986d73efa273ba027ffdbc0ad5281)
  • Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

@alangou alangou added gator:approval-needed Gator completed review; maintainer approval needed and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Oct 14, 2026
@alangou

alangou commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test f9e0114

@alangou

alangou commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

gator-agent

PR Review Status

The follow-up review found no blocking issues in the change restoring S3 path canonicalization. I checked that route selection, policy evaluation, signing, and forwarding use the same canonical target in both CONNECT and forward-proxy paths, along with the regression coverage and updated AWS documentation.

Tests are authorized for the current head. Branch Checks, Helm Lint, and Branch E2E Checks are queued on the refreshed test mirror, so this PR is returning to pipeline monitoring.

Blocking findings: None.

Carried findings: None.

Gator metadata
  • Validation: Maintainer-authored, focused network credential and signing hardening.
  • Docs: docs/how-it-works/providers/aws.mdx explains canonicalization before signing and forwarding.
  • Checks: Current-head Branch Checks run 38635821239 and Helm Lint run 38635820805 queued; Trivy Changes, DCO, and PR Issue Acceptance passed. No merge conflict.
  • E2E: test:e2e present; current-head Branch E2E Checks run 38635821926 queued with the existing label. No workflow rerun needed for this fresh dispatch.
  • Head SHA: f9e0114f5b661d40fe9d65bf66c49b827b8fe064
  • Base SHA: 8b774e2a6a5986d73efa273ba027ffdbc0ad5281
  • Merge base SHA: 6e69360d0e1536d891f4abfbb60c1d90e50188c9
  • Patch ID: 78961beb08ab698cc882e4cd39a86ad05ff99dbd
  • Gator payload: 12
  • Review mode: follow_up
  • Previous reviewed SHA: a3fa091d360ad411962b94fb1fe9b5fb58a0df98
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@alangou alangou added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:approval-needed Gator completed review; maintainer approval needed labels Oct 14, 2026
@github-actions

Copy link
Copy Markdown

Compatibility review (advisory)

Unaddressed breaking changes

SigV4 endpoints now reject request-field credential placeholders and hide all non-2xx upstream response details, breaking existing workloads and AWS SDK error handling. The PR explicitly acknowledges both changes, but this train does not allow Stable breaks and the documentation lacks complete migration guidance.

1. api-behavior — ACKED

SigV4 request targets and non-authentication headers previously resolved credential placeholders, but now any such placeholder causes a 403 rejection.

  • Impact: Workloads combining SigV4 signing with placeholder injection in a path or custom header can no longer send the request.
  • Action: Defer this change to a break-allowing train and direct affected workloads to a separate non-SigV4 endpoint for generic placeholder injection.
  • PR: fix(network): harden signed request handling #4558

2. api-behavior — ACKED

Signed requests previously relayed informational, redirect, and 4xx/5xx response headers and bodies, but now only final 2xx responses remain intact while other responses become generic local errors.

  • Impact: AWS SDKs and applications that parse AWS error bodies, inspect response headers, follow redirects, or consume informational responses lose that behavior.
  • Action: Defer this default change to a break-allowing train and document an actionable alternative for clients that require AWS errors or redirects, such as a separate non-SigV4 endpoint.
  • PR: fix(network): harden signed request handling #4558
Review details
  • Execution: complete
  • Candidate: PR #4558 (ac00c3982423993fb85ff5105b651681b95af6b0)
  • Baseline: 8b774e2a6a5986d73efa273ba027ffdbc0ad5281 (8b774e2a6a5986d73efa273ba027ffdbc0ad5281)
  • Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

@alangou alangou added gator:blocked Gator is blocked by process or repository gates gator:approval-needed Gator completed review; maintainer approval needed and removed gator:watch-pipeline Gator is monitoring PR CI/CD status gator:blocked Gator is blocked by process or repository gates labels Oct 14, 2026
@alangou alangou added gator:blocked Gator is blocked by process or repository gates and removed gator:approval-needed Gator completed review; maintainer approval needed labels Oct 15, 2026
Signed-off-by: Adrien Langou <alangou@nvidia.com>
Signed-off-by: Adrien Langou <alangou@nvidia.com>
Signed-off-by: Adrien Langou <alangou@nvidia.com>
Signed-off-by: Adrien Langou <alangou@nvidia.com>
Signed-off-by: Adrien Langou <alangou@nvidia.com>
@alangou
alangou force-pushed the fix/276-request-validation/alangou branch from f9e0114 to 736537f Compare October 16, 2026 13:35
@alangou

alangou commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 736537f

@alangou

alangou commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

gator-agent

PR Review Status

The bounded Critical-only review found no new Critical defects in the rebase adaptations. The author-only range comparison shows an updated proxy import and test-constructor adjustment; no carried findings or maintainer convergence decision remain.

Tests are authorized for this head. Branch Checks, Helm Lint, and Branch E2E Checks are queued on the refreshed mirror with test:e2e already present, so pipeline monitoring can resume.

Blocking findings: None.

Carried findings: None.

Gator metadata
  • Validation: Maintainer-authored, focused signed-request hardening.
  • Docs: docs/how-it-works/providers/aws.mdx documents placeholder restrictions, canonicalization, and signed-response filtering.
  • Checks: Current-head Branch Checks run 38909486754 and Helm Lint run 38909485975 queued; Trivy Changes, DCO, and PR Issue Acceptance passed. No merge conflict.
  • E2E: test:e2e present; current-head Branch E2E Checks run 38909486734 queued by fresh dispatch. No rerun required.
  • Head SHA: 736537f7c071b9fbeb81f02953ad1792b2e29069
  • Base SHA: a4c848adbddd0b4732fd70ddcf8a563a5b1e9317
  • Merge base SHA: a4c848adbddd0b4732fd70ddcf8a563a5b1e9317
  • Patch ID: e88dd37b15167fd03054e3125501a28a1b035b6e
  • Gator payload: 12
  • Review mode: critical_only
  • Previous reviewed SHA: f9e0114f5b661d40fe9d65bf66c49b827b8fe064
  • Review budget exhausted: yes
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@alangou alangou added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:blocked Gator is blocked by process or repository gates labels Oct 16, 2026
@alangou

alangou commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 736537f

@github-actions

Copy link
Copy Markdown

Compatibility review (advisory)

New or updated breaking changes; see the full report for all findings.

1. api-behavior — No explicit risk acceptance found

credential_signing: sigv4, sigv4:body, and sigv4:no_body previously resolved OpenShell placeholders in request targets and non-authentication headers, but now reject them with HTTP 403.

  • Impact: Existing workloads combining SigV4 signing with provider placeholders in custom headers, paths, or query values fail before contacting the upstream service.
  • Action: Preserve placeholder resolution by default or gate the prohibition behind a new opt-in policy setting; otherwise defer it to a minor train with guidance to use a separate non-SigV4 endpoint.
  • PR: fix(network): harden signed request handling #4558

2. api-behavior — No explicit risk acceptance found

Signed requests previously relayed informational, redirect, and 4xx/5xx responses, but now drop informational responses and replace non-2xx details with {&quot;error&quot;:&quot;signed_upstream_rejected&quot;}, mapping redirects to HTTP 502.

  • Impact: AWS SDKs and applications lose redirect handling, service error bodies, response headers, retry metadata, and informational responses.
  • Action: Preserve upstream response relay by default or gate suppression behind an opt-in policy setting; otherwise defer it to a minor train and document a viable path for clients requiring AWS errors and redirects.
  • PR: fix(network): harden signed request handling #4558
Review details
  • Execution: complete
  • Candidate: PR #4558 (5e25650f7b0aa6111ae39bbd0a37137341f24b93)
  • Baseline: a4c848adbddd0b4732fd70ddcf8a563a5b1e9317 (a4c848adbddd0b4732fd70ddcf8a563a5b1e9317)
  • Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

@alangou alangou added gator:approval-needed Gator completed review; maintainer approval needed and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Oct 16, 2026

This branch has not been deployed

No deployments
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