Repository navigation
fix(network): suppress SigV4 signing diagnostics - #4558
Conversation
|
🌿 Preview your docs: https://nvidia-preview-pr-4558.docs.buildwithfern.com/openshell |
|
/ok to test f456c79 |
|
Label |
PR Review StatusThe 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
|
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 — ACKEDCredential placeholders in signed request headers, paths, or query parameters previously resolved, but now cause a local 403 response.
2. policy — ACKEDAdding any S3 SigV4 route now rejects path-normalizing requests across every L7 route sharing that host and port instead of canonicalizing and forwarding them.
3. api-behavior — ACKEDSigV4 responses previously relayed upstream informational, redirect, error-header, and error-body details, but now suppress them and return a generic local response.
Review details
Advisory review; publication gates are unchanged. |
|
/ok to test a3fa091 |
PR Review StatusThe 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 Blocking findings: None. Carried findings: None. Gator metadata
|
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 — ACKEDSigned requests now reject credential placeholders outside stripped AWS authentication headers instead of resolving them.
2. api-behavior — ACKEDS3 SigV4 endpoints now reject paths requiring slash, dot-segment, parameter, or percent-encoding normalization instead of forwarding their canonicalized form.
3. api-behavior — ACKEDSigned endpoints now replace redirects and 4xx/5xx response details with
Review details
Advisory review; publication gates are unchanged. |
|
/ok to test f9e0114 |
PR Review StatusThe 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
|
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 — ACKEDSigV4 request targets and non-authentication headers previously resolved credential placeholders, but now any such placeholder causes a 403 rejection.
2. api-behavior — ACKEDSigned 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.
Review details
Advisory review; publication gates are unchanged. |
f9e0114 to
736537f
Compare
|
/ok to test 736537f |
PR Review StatusThe 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 Blocking findings: None. Carried findings: None. Gator metadata
|
|
/ok to test 736537f |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. api-behavior — No explicit risk acceptance found
2. api-behavior — No explicit risk acceptance foundSigned requests previously relayed informational, redirect, and 4xx/5xx responses, but now drop informational responses and replace non-2xx details with
Review details
Advisory review; publication gates are unchanged. |
Maintainer Review NudgeThis PR has been in @NVIDIA/openshell-maintainers @NVIDIA/openshell-codeowners, can someone review and either approve, request changes, or close this out? |
1 similar comment
Maintainer Review NudgeThis PR has been in @NVIDIA/openshell-maintainers @NVIDIA/openshell-codeowners, can someone review and either approve, request changes, or close this out? |
Blocker Follow-Up NudgeThis 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
|
2 similar comments
Blocker Follow-Up NudgeThis 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
|
Blocker Follow-Up NudgeThis 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
|
736537f to
3276466
Compare
Signed-off-by: Adrien Langou <alangou@nvidia.com>
3276466 to
19f5896
Compare
Signed-off-by: Adrien Langou <alangou@nvidia.com>
|
/ok to test 565794b |
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 -- --checkandgit diff --checkpassed.sigv4.rspassed in an offline harness compiling the source module with the repository's AWS dependency versions.-D warnings.These are focused module checks. The normal Cargo command
cargo test --offline --locked -p openshell-supervisor-network sigv4 -- --test-threads=2is blocked before compilation becauseopentelemetry-httpis absent from the local dependency cache. Full crate validation remains for CI.Checklist