fix: pin downstream TLS version and cipher policy - #76
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. Ready-for-review writer-safe child of #75. Parent is 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. Current
2c4433...is an ADR-only ordinary forward from7a5f5e...(12 additions/8 deletions; no Rust/test/workflow/dependency change). ADR 0014 now 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.Exact-current CI
34558856760, Supply Chain34558856761, and PgErd bounded-origin capacity34558856758are all terminal GREEN. All returned inline review threads are resolved. Exact-current terminal technical COMMENT5174930121re-read the current four-path range after execution and found no additional writer-safe source/test/documentation repair; it is technical evidence only, not independentAPPROVEDgovernance credit.The branch-local implementation/execution gate is therefore closed for independent review and this PR is Ready rather than Draft. The active organization ruleset still requires one approving review, resolved review threads and central required workflows before default-branch integration; no bypass, protected merge, immutable release, canary/shadow, rollback, cutover or legacy-removal credit is claimed.