fix: deny HTTP/1 Upgrade in immutable Pingora peers - #33
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
There was a problem hiding this comment.
Owner technical review on exact head df2a9bef4e99678259d1666c0262ae1cae9c4ff1.
Reviewed the full final-#31 → current-#33 range after ordinary/non-force succession. Merge base is exactly final #31 8230f8d1311adccc133b7f6ed86df441533e486c with zero behind debt. The runtime delta is bounded to pingora_delivery: immutable peers switch from HttpUpstreamRequestPolicy::standard() to deny_upgrades() while keeping existing TLS/ALPN/timeouts/trust handling. Pinned supplier source confirms default() uses H1UpgradePolicy::WebSocketOnly with standard stripping fields and deny_upgrades() changes only h1_upgrade to Deny via ..Self::default().
tests/peer_protocol_policy.rs exercises public build_peer() and locks exact peer policy; helper/test rustdoc describes the defense-in-depth constraint rather than restating code. Existing real-listener #29 acceptance remains the wire-visible 501/no-origin-contact evidence, so this peer-level contract does not manufacture WebSocket parity. ADR 0009 remains Proposed and the current ARCHITECTURE/SECURITY/TEST_STRATEGY/TRD/CHANGELOG/gap-baseline projection is consistent with the source boundary. Product auth/business routing, Wardnet/EgressWeave policy and Keyverse identity are not introduced.
No actionable source, test, architecture or documentation finding remains in this exact-range technical review. This COMMENT is technical evidence only, not an independent human APPROVED governance decision. Exact df2a9bef... CI/Supply Chain are still pending and predecessor #31 receipts do not transfer.
Fixes #32.
Finding
The existing HTTP/1 protocol-transition admission guard returns 501 before application admission or origin contact, but the immutable Pingora peer below that guard inherited
HttpUpstreamRequestPolicy::standard(). At pinned supplier revision09696b51bc59315353d96686355861604d0bb48c, that standard policy retains WebSocket upgrade capability. A later callback/composition refactor could therefore widen the protocol surface without changing the transport-neutral CWL contract.Fresh supplier evidence on 2026-09-09 still shows protected
cloudflare/pingora/mainat09696b51bc59315353d96686355861604d0bb48c, issuecloudflare/pingora#946open, and proposed fixcloudflare/pingora#947open/unmerged. Those are supplier-owner evidence only; this PR does not consume a mutable PR head.RED → causal fix
Historical RED
b0719a893c0136083efc12918c6e906a28f39319requires publicbuild_peer()to produceHttpUpstreamRequestPolicy::deny_upgrades(). Historical source fix0109031b97ce1816a7936987a4d63330a2d19cbademonstrated the minimal semantic change.Current source keeps that exact repair on final #31:
pingora_delivery::build_peer_from_validatedusesdeny_upgrades()and documents the invariant.tests/peer_protocol_policy.rsindependently locks the immutable peer option, with constraint-oriented rustdoc so the test/documentation coverage gate is not weakened. Standard hop-by-hop andConnection-nomination sanitization remain; only the supplier-default HTTP/1 upgrade capability is denied.No WebSocket implementation, product auth/business routing, retry/idempotency policy, Wardnet/EgressWeave verdict, or Keyverse identity authority is added.
Ordinary/non-force succession
Historical #33 head
d6eaa9f9249a77551e24014d72adc3940e6a4404was based on an obsolete #31 snapshot. Final #31 is8230f8d1311adccc133b7f6ed86df441533e486c, with terminal CI34251472731, Supply Chain34251472751, and an exact-head owner technical COMMENT reporting no actionable finding.Ordinary two-parent commit
b98646582698ea99f94f62910c217d6ee4bbcdb9preserves historical #33 as first parent and final #31 as second parent. Its resolution tree is final #31 plus only the valid peer-policy source/test/ADR delta; no force-push, destructive rebase, stale-parent source, or predecessor receipt was replayed. Subsequent documentation-only commits project that same decision into ARCHITECTURE, SECURITY, TEST_STRATEGY, TRD, CHANGELOG anddocs/product-technical-gap-baseline.mdwithout changing runtime semantics.Final exact head is
df2a9bef4e99678259d1666c0262ae1cae9c4ff1. Fresh compare against final #31 has merge base exactly8230f8d...and zero behind debt. Effective scope is the peer-policy source/test/Proposed ADR plus code-current documentation.Exact-head closure
The unchanged final head has terminal evidence:
34254199047: terminal success on exactdf2a9bef...; formatting, compile/tests, strict Clippy, warnings-denied public rustdoc, 100% owned-production coverage, lock verification, generic/routed Rust-origin load, and dual-profile OCI runtime all closed.34254199064: terminal success on exactdf2a9bef...after dependency audit, candidate builds, SBOM/image scans and exact-source evidence binding.df2a9bef....5144705431found no actionable source, test, architecture or documentation finding. It is technical evidence only, not an independent humanAPPROVEDgovernance decision.No final #31 receipt was transferred; #33 independently re-executed on its own unchanged final head. ADR 0009 remains Proposed until protected governance/integration rather than creating a status-only self-staling descendant. HTTP/1 Upgrade is intentionally unsupported; WebSocket/Extended CONNECT/H2/H3/QUIC parity remains separate versioned work.
#37 may now ordinarily/non-force succeed from this exact tree, preserving its distinct pg-erd upstream TLS delta and independently reacquiring hosted/review evidence.
Keep Draft. No self-approval, force-push/destructive rebase, protected merge, immutable release, canary, cutover, rollback or legacy-removal credit is claimed.