Repository navigation
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. |
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>
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. |
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
Compatibility
Testing
Validation performed:
cargo fmt --all --checkcargo clippy -p openshell-core -p openshell-supervisor-network -p openshell-supervisor-process --all-targets -- -D warningscargo 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:lintTMPDIR=/var/tmp mise run e2e:dockerFive 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