fix: pin downstream TLS version and cipher policy - #76
seonghobae wants to merge 24 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks 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.
Exact-current writer review on 51ff98f23e177af80bff849e8d30cae4b1a57fec: verified the explicit TLS 1.2–1.3 profile and the ordinary non-force parent restack. TLS 1.2 remains ECDHE+AEAD only, TLS 1.3 remains the selected AES-GCM/ChaCha20-Poly1305 set, and real-wire TLS 1.2/TLS 1.3 plus TLS 1.1 rejection assertions are intact. The current fixture-only delta replaces single-read origin assumptions with the parent's bounded complete-header helper, including the new TLS-version probes; it does not change negotiated-policy semantics. No unresolved review threads are present and I found no further writer-safe source/test/authority defect in this focused exact delta. COMMENT only; exact-current CI/Supply Chain/capacity are nonterminal and this is not independent APPROVED or promotion credit.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current writer review on a5dc77cce607c85442d0bf37dd65522f06ebba5f: the TLS-profile fixture now carries the parent's absolute origin-header deadline repair rather than a resettable per-read inactivity timeout. Forward commit 25f4a777f128c2fa4aaeb6487fc609be6e5025a6 applies the same remaining-budget logic to fallback/no-ALPN/TLS1.2/TLS1.3 origin probes; merge commit a5dc77cce607c85442d0bf37dd65522f06ebba5f adopts parent 96ce09... without force push. TLS 1.2–1.3 policy, cipher assertions and production behavior are unchanged. COMMENT technical evidence only; new exact CI/Supply Chain/capacity are nonterminal and this is not independent APPROVED, merge, release or cutover credit.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current writer review on a0bf55151255beea04fb305e721b5629b7c79b2e: ordinary two-parent merge adopts #75 exact dfb2d9b012e1c3c881796775cdff08067dfd53a1 while preserving this child’s explicit TLS 1.2–1.3/cipher-policy tree. The only parent movement is the focused TLS/H2 TRACEABILITY update for the already-implemented absolute origin-header evidence deadline; no production Rust, TLS policy, fixture semantics, routes/retries or authority boundary changed. COMMENT technical evidence only; fresh exact CI/Supply Chain/capacity are queued and no predecessor GREEN or independent approval transfers.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head technical review after two exact failures were separated. The previous child exact failed cargo fmt --check only in tests/downstream_tls_security_profile_contract.rs; commit 7181219... applies the exact rustfmt output with no semantic change. Parent #75 then moved to b0df584... to repair its independent coverage receipt, so this child incorporated that in-process ALPN coverage test while preserving the TLS 1.2/1.3 and cipher-policy delta, then was non-force restacked by merge commit df7262d.... Compare against current parent shows only the intended four TLS-security-profile paths. Fresh exact-head gates must pass before Ready or merge; this COMMENT is not independent approval.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head follow-up after parent #75's rustfmt-only repair. This child applies the same formatter output to the inherited ALPN coverage test and then non-force restacks onto #75 exact df70de9... via two-parent merge 273832d.... Exact parent→child comparison is ahead-only and still contains exactly the intended four TLS-security-profile paths. Fresh exact gates must pass before Ready or merge; this COMMENT is not independent approval.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current repair review on bc34e6302efd53682b8a207ee73cac3600fe86b5. Prior exact CI 34547472103 failed only at cargo fmt --all -- --check; job 103103105564 showed the remaining diff at src/tls_delivery.rs:156, where Rust 1.98.0 rustfmt joins H2_ALPN and HTTP1_ALPN onto one import line. Compile/test, lint, rustdoc, coverage, and lock stages were skipped; load-contract and OCI jobs succeeded, while Supply Chain and bounded-origin capacity also completed successfully. bc34e630... applies exactly that emitted one-line formatting change with no production semantic change. Parent→child compare against #75 exact df70de9... remains ahead-only with merge base df70de9... and exactly the four intended TLS-profile paths. Fresh exact CI/Supply Chain/capacity are queued. COMMENT technical evidence only; this is not independent APPROVED and Draft/merge/release/cutover credit remains withheld until exact-current gates terminate GREEN.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current repair review on a1a1a5dc7a05c5b14de0cf98d2e1cff8fe80be5a. CI 34550562324 for predecessor bc34e630... passed formatting, compile/test, clippy, rustdoc, the coverage workload, load-contract and OCI runtime, then failed only --fail-under-regions 100. Its exact uploaded LLVM receipt showed all owned production lines/functions at 100%; src/tls_delivery.rs alone had 5 uncovered regions: the four ? fail-fast edges after OpenSSL security-profile setters and the initial TlsSettings::intermediate materialization error edge. Current repair does not weaken or exclude the 100% gate. It preserves setter order and fail-fast semantics through Result::and_then with one final diagnostic mapping, so the real success path exercises every setter without four synthetic untriggerable ? return regions, and adds a focused missing identity-material test for the initial materialization error. TLS 1.2/1.3 bounds, cipher strings, ALPN, certificate custody, product auth/business logic, Keyverse/Wardnet/EgressWeave authority and traffic acceptance are unchanged. Parent→child compare against #75 remains ahead-only with the same four intended paths. Fresh exact CI/Supply Chain/capacity are required before Ready; this COMMENT is technical evidence only, not independent approval.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current repair review on 7a5f5e04016e93a7fd515517051430d853dd3b45. CI 34554449075 on predecessor a1a1a5dc... passed formatting, compile/test, Clippy, warnings-denied rustdoc, coverage workload/export, load-contract and OCI runtime; only final 100% owned-production region enforcement failed. Exact coverage artifact coverage-a1a1a5dc... (sha256:54afe3d9afc0d66e8a4b4464e8194f8cbf18c65bfc5a07b76cec50b3739de7c3) isolated the two remaining zero-count regions to src/tls_delivery.rs: the local ? continuation after apply_downstream_tls_security_profile and an assert!(matches!(...)) macro branch in the missing-identity test. Current repair preserves the exact TLS 1.2/1.3, cipher and ALPN policy: security-profile error propagation is expressed as Result::map, and the materialization test extracts the error then compares its discriminant. No coverage exclusion, threshold reduction, routing/product/security authority expansion, or supplier change was introduced. Parent remains exact #75 df70de9...; #77 has been non-force restacked onto this exact head with only its H2 multiplexing file as effective child delta. Fresh exact CI/Supply Chain/capacity are still nonterminal. COMMENT technical evidence only, not independent APPROVED.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current documentation/evidence review on 2c4433b99c539a43bdecc74c7d167b2446dd4fdc. Fresh compare from source/test repair exact 7a5f5e04016e93a7fd515517051430d853dd3b45 is ahead-only by one commit and changes only ADR 0014 (12 additions/8 deletions); no Rust, traffic fixture, workflow or dependency content changed. The ADR was stale relative to the actual coverage RCA and live stack, so it now records #75 exact GREEN, predecessor #76 a1a1a5dc... coverage artifact digest and two uncovered regions, immediate repair 7a5f5e..., current supplier boundaries, PR #77 fixture lineage, and current IETF RFC 10015 / 9846 / 9852 references while remaining Proposed rather than prematurely Accepted. The ADR explicitly states that this documentation commit creates a new exact head and cannot transfer predecessor GREEN. Fresh current-head CI 34558856760, Supply Chain 34558856761, and capacity 34558856758 are nonterminal. COMMENT technical evidence only, not independent APPROVED.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current terminal review on 2c4433b99c539a43bdecc74c7d167b2446dd4fdc. Fresh execution is now terminal GREEN across CI 34558856760, Supply Chain 34558856761, and PgErd bounded-origin capacity 34558856758. Re-read of the current four-path parent→child range confirms the explicit TLS 1.2 floor/TLS 1.3 ceiling, ECDHE+AEAD TLS 1.2 allowlist, selected TLS 1.3 AEAD suites, real-wire TLS 1.2/TLS 1.3 acceptance, TLS 1.1 rejection, and Proposed ADR 0014 remain aligned with the gateway-only authority boundary. No unresolved review threads are present and no additional writer-safe source/test/documentation repair was found on this exact head. COMMENT technical evidence only; this is not independent APPROVED governance and does not authorize protected merge, release or cutover.
|
@coderabbitai approve Current exact |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Checked-out commit =='
git rev-parse HEAD
git status --short
echo '== PR metadata =='
gh pr view 76 --repo ContextualWisdomLab/pingora-gateway \
--json number,state,isDraft,headRefOid,baseRefOid,mergeable,reviewDecision,reviews,statusCheckRollup \
--jq '{number,state,isDraft,headRefOid,baseRefOid,mergeable,reviewDecision,reviews:[.reviews[]|{author:.author.login,state}],checks:[.statusCheckRollup[]|{name,conclusion,status}]}'
echo '== Changed files and stat against PR base =='
git diff --stat df70de9cc0c77cfc1dabc51de039ac46af47df08..HEAD
git diff --name-status df70de9cc0c77cfc1dabc51de039ac46af47df08..HEAD
echo '== Full PR diff =='
git diff --find-renames --find-copies --unified=80 df70de9cc0c77cfc1dabc51de039ac46af47df08..HEAD
echo '== Open review threads =='
gh api graphql -F owner='ContextualWisdomLab' -F name='pingora-gateway' -F number=76 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
comments(first: 20) { nodes { author { login } body } }
}
}
}
}
}' --jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path,line,comments:[.comments.nodes[]|{author:.author.login,body}]}]'Length of output: 57921 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Source outline =='
ast-grep outline src/tls_delivery.rs --items all
echo '== TLS adapter =='
cat -n src/tls_delivery.rs
echo '== Integration-test outline =='
ast-grep outline tests/downstream_tls_http1_fallback_wire.rs --items all
echo '== Integration test =='
cat -n tests/downstream_tls_http1_fallback_wire.rs
echo '== Structural contract test =='
cat -n tests/downstream_tls_security_profile_contract.rs
echo '== Production listener construction and TLS settings use =='
rg -n -C 6 --type rust '(build_downstream_tls_settings\s*\(|add_tls_with_settings\s*\()' src tests
echo '== Dependency versions relevant to Pingora TLS API =='
rg -n -C 3 '^\s*(pingora|openssl)\s*=' Cargo.toml Cargo.lock 2>/dev/null || trueLength of output: 48814
I reviewed the current PR range only. I found no blocking issue. The current head is mergeable, has no unresolved review threads, and reports successful completed required checks. ✅ Action performedComments resolved. Approval is disabled; enable |
Refs #51. Writer-safe child of historical #75 exact
df70de9cc0c77cfc1dabc51de039ac46af47df08; current child exact is2c4433b99c539a43bdecc74c7d167b2446dd4fdc, reached only by ordinary forward commits/non-force stack maintenance.This PR owns the explicit downstream TLS security profile: TLS 1.2 minimum/TLS 1.3 maximum, code-owned TLS 1.2 ECDHE+AEAD suites and selected TLS 1.3 AES-GCM/ChaCha20-Poly1305 suites, with generic production-root TLS 1.2/TLS 1.3 round trips and TLS 1.1 rejection. It does not introduce arbitrary operator cipher strings. Certificate lifecycle/private-key custody, product auth/business logic, Keyverse/Wardnet/EgressWeave authority, routes/retries, supplier source, deployment authority, H3/QUIC and the broad product-gap-baseline writer remain outside scope.
Coverage root: predecessor exact
a1a1a5dc7a05c5b14de0cf98d2e1cff8fe80be5acompleted Supply Chain34554449121and capacity34554449107; CI34554449075passed formatting, locked compile/test, Clippy, warnings-denied rustdoc, coverage workload/export, load-contract and OCI runtime, then failed only final 100% owned-production region enforcement. Exact artifactcoverage-a1a1a5dc...(sha256:54afe3d9afc0d66e8a4b4464e8194f8cbf18c65bfc5a07b76cec50b3739de7c3) retained 100% lines/functions and isolated two zero-count regions insrc/tls_delivery.rs: the local?continuation after security-profile application and an assertion-macro branch in the missing-identity test.Source/test repair exact
7a5f5e04016e93a7fd515517051430d853dd3b45removed those instrumentation/testability regions without changing TLS/cipher/ALPN semantics: profile propagation usesResult::map, and the missing-material test extracts the error before discriminant comparison. It reached capacity GREEN34558229725; CI34558229685and Supply Chain34558229694were cancelled only after the ordinary-forward head update below, so their nonterminal work is not current-head evidence and no GREEN is transferred.Fresh review then found ADR 0014 stale relative to the live coverage RCA and current stack. Exact
2c4433...is an ADR-only ordinary forward from7a5f5e...(12 additions/8 deletions; no Rust/test/workflow/dependency change). ADR 0014 records the actual coverage evidence, current supplier boundaries, #77 fixture lineage and current RFC 10015 / 9846 / 9852 references while remaining Proposed. It explicitly states that documentation movement creates a new exact head and cannot inherit predecessor GREEN.Parent→child PR scope remains four intended paths: ADR 0014,
src/tls_delivery.rs, HTTP/1 fallback/version wire acceptance, and the structural security-profile contract. #77 is ordinarily/non-force stacked on this exact parent and retains only its H2 multiplexing test as effective child delta.Historical exact CI
34558856760, Supply Chain34558856761, and PgErd bounded-origin capacity34558856758are all terminal GREEN. All returned inline review threads are resolved. Exact-current-at-the-time technical COMMENT5174930121found no additional writer-safe source/test/documentation repair; it remains technical evidence only, not independentAPPROVEDgovernance credit. These receipts are regression/characterization evidence for the historical #75 ancestry only.Parent-first lifecycle repair — 2026-09-19
Live PR metadata is correctly Draft. Earlier wording that called this lane Ready is superseded because parent #75 is itself Draft on stale #73 ancestry. #73 is Draft at historical
625cae4f156366bc39d6782161a4a5f336d58624and must wait for current #70df5d2f05fc5fbdd94bbfb487283bf2a6d73a55bfto inherit current foundation listener-readiness through ordinary #44/#47 release-line reconciliation and reacquire exact evidence. #73 then reconciles, followed by #75, before this child can move.Current order is
foundation promotion prerequisites -> #44/#47 release-line reconciliation -> #70 fresh exact -> #73 reconciliation/fresh exact -> #75 reconciliation/fresh exact -> #76 ordinary/non-force reconciliation preserving only this TLS security-profile delta -> fresh exact CI/Supply/capacity/current-range review -> independent governance -> normal integration -> #77 and later descendants reconcile in order.Do not copy #75/#73/foundation repairs into this branch and do not transfer the historical GREEN above across the future ancestry movement. No self-approval, administrator bypass, force update, destructive rebase, gate weakening, protected merge, immutable release, canary/shadow, rollback, cutover or legacy-removal credit is claimed.