Skip to content

fix(network): suppress SigV4 signing diagnostics - #4558

Merged
pimlock merged 2 commits into
mainfrom
fix/276-request-validation/alangou
Nov 9, 2026
Merged

pimlock merged 2 commits into
mainfrom
fix/276-request-validation/alangou

Conversation

@alangou

@alangou alangou commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

When supervisor TRACE logging is enabled, AWS SigV4 signing diagnostics can include the temporary session token in the canonical request. Suppress tracing while the synchronous signer runs, then restore the caller's tracing subscriber.

The fix covers buffered requests and headers-only signing, including streaming payloads. A regression test captures TRACE events, verifies that signing diagnostics and the synthetic session token are absent, and checks that tracing still works before and after signing. The AWS provider documentation explains the omitted diagnostics.

Related Issue

Tracked in the maintainer security workflow.

Compatibility

This change addresses the locally reproduced logging exposure. It preserves the existing signing rules, request canonicalization, HTTP response handling, and credential-placement rules already enforced by #4886.

Testing

  • cargo fmt --all -- --check and git diff --check passed.
  • All 19 tests in sigv4.rs passed in an offline harness compiling the source module with the repository's AWS dependency versions.
  • Clippy passed for that module and its tests with the repository's Rust/Clippy lint configuration and -D warnings.
  • The new regression test fails when direct SDK signing is restored in a disposable copy, and passes with tracing suppression enabled.

These are focused module checks. The normal Cargo command cargo test --offline --locked -p openshell-supervisor-network sigv4 -- --test-threads=2 is blocked before compilation because opentelemetry-http is absent from the local dependency cache. Full crate validation remains for CI.

Checklist

  • Conventional Commit with DCO sign-off.
  • Regression test added.
  • Published AWS 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): suppress SigV4 signing diagnostics #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): suppress SigV4 signing diagnostics #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): suppress SigV4 signing diagnostics #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): suppress SigV4 signing diagnostics #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): suppress SigV4 signing diagnostics #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): suppress SigV4 signing diagnostics #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): suppress SigV4 signing diagnostics #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): suppress SigV4 signing diagnostics #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
@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): suppress SigV4 signing diagnostics #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 {"error":"signed_upstream_rejected"}, 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): suppress SigV4 signing diagnostics #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
@alangou

alangou commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

gator-agent

Maintainer Review Nudge

This PR has been in gator:approval-needed for more than 48 business hours with no maintainer approval.

@NVIDIA/openshell-maintainers @NVIDIA/openshell-codeowners, can someone review and either approve, request changes, or close this out?

1 similar comment
@alangou

alangou commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

gator-agent

Maintainer Review Nudge

This PR has been in gator:approval-needed for more than 48 business hours with no maintainer approval.

@NVIDIA/openshell-maintainers @NVIDIA/openshell-codeowners, can someone review and either approve, request changes, or close this out?

@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 22, 2026
@alangou

alangou commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

gator-agent

Blocker Follow-Up Nudge

This PR is still blocked by merge conflicts after more than 48 business hours.

Next action: @alangou, please resolve the conflicts with the default branch and push an updated head so review and required checks can resume.

Gator metadata
  • Head SHA: 736537f7c071b9fbeb81f02953ad1792b2e29069
  • Gator payload: 12
  • Next state: gator:blocked
  • Blocked reason: merge_conflicts

2 similar comments
@alangou

alangou commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

gator-agent

Blocker Follow-Up Nudge

This PR is still blocked by merge conflicts after more than 48 business hours.

Next action: @alangou, please resolve the conflicts with the default branch and push an updated head so review and required checks can resume.

Gator metadata
  • Head SHA: 736537f7c071b9fbeb81f02953ad1792b2e29069
  • Gator payload: 12
  • Next state: gator:blocked
  • Blocked reason: merge_conflicts

@alangou

alangou commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

gator-agent

Blocker Follow-Up Nudge

This PR is still blocked by merge conflicts after more than 48 business hours.

Next action: @alangou, please resolve the conflicts with the default branch and push an updated head so review and required checks can resume.

Gator metadata
  • Head SHA: 736537f7c071b9fbeb81f02953ad1792b2e29069
  • Gator payload: 12
  • Next state: gator:blocked
  • Blocked reason: merge_conflicts

@alangou
alangou force-pushed the fix/276-request-validation/alangou branch from 736537f to 3276466 Compare November 9, 2026 13:42
Signed-off-by: Adrien Langou <alangou@nvidia.com>
@alangou
alangou force-pushed the fix/276-request-validation/alangou branch from 3276466 to 19f5896 Compare November 9, 2026 14:00
@alangou alangou changed the title fix(network): harden signed request handling fix(network): suppress SigV4 signing diagnostics Nov 9, 2026
Signed-off-by: Adrien Langou <alangou@nvidia.com>
@alangou

alangou commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 565794b

@alangou alangou added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:blocked Gator is blocked by process or repository gates labels Nov 9, 2026
@pimlock
pimlock added this pull request to the merge queue Nov 9, 2026
Merged via the queue into main with commit 56d2350 Nov 9, 2026
154 of 155 checks passed
@pimlock
pimlock deleted the fix/276-request-validation/alangou branch November 9, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:in-review Gator is reviewing or awaiting PR review feedback test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants