Skip to content

fix: pin downstream TLS version and cipher policy - #76

Open
seonghobae wants to merge 24 commits into
test/downstream-tls-h2-config-red-v1from
fix/downstream-tls-explicit-security-profile-v1
Open

fix: pin downstream TLS version and cipher policy#76
seonghobae wants to merge 24 commits into
test/downstream-tls-h2-config-red-v1from
fix/downstream-tls-explicit-security-profile-v1

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Refs #51. Ready-for-review writer-safe child of #75. Parent is exact df70de9cc0c77cfc1dabc51de039ac46af47df08; current child exact is 2c4433b99c539a43bdecc74c7d167b2446dd4fdc, 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 a1a1a5dc7a05c5b14de0cf98d2e1cff8fe80be5a completed Supply Chain 34554449121 and capacity 34554449107; CI 34554449075 passed 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 artifact coverage-a1a1a5dc... (sha256:54afe3d9afc0d66e8a4b4464e8194f8cbf18c65bfc5a07b76cec50b3739de7c3) retained 100% lines/functions and isolated two zero-count regions in src/tls_delivery.rs: the local ? continuation after security-profile application and an assertion-macro branch in the missing-identity test.

Source/test repair exact 7a5f5e04016e93a7fd515517051430d853dd3b45 removed those instrumentation/testability regions without changing TLS/cipher/ALPN semantics: profile propagation uses Result::map, and the missing-material test extracts the error before discriminant comparison. It reached capacity GREEN 34558229725; CI 34558229685 and Supply Chain 34558229694 were 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 from 7a5f5e... (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 Chain 34558856761, and PgErd bounded-origin capacity 34558856758 are all terminal GREEN. All returned inline review threads are resolved. Exact-current terminal technical COMMENT 5174930121 re-read the current four-path range after execution and found no additional writer-safe source/test/documentation repair; it is technical evidence only, not independent APPROVED governance 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.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 249ed7fc-185c-4f64-af80-b4d5d50f8a94

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit that referenced this pull request Sep 10, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit that referenced this pull request Sep 10, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit that referenced this pull request Sep 11, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit that referenced this pull request Sep 11, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit that referenced this pull request Sep 11, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit that referenced this pull request Sep 11, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seonghobae
seonghobae marked this pull request as ready for review September 11, 2026 04:21

Copy link
Copy Markdown
Contributor Author

@coderabbitai approve

Current exact 2c4433b99c539a43bdecc74c7d167b2446dd4fdc is terminal GREEN across CI/Supply Chain/capacity with no open inline review threads. Please review/approve the exact current head only; do not transfer any predecessor review.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown
🧩 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 || true

Length of output: 48814


@seonghobae Approved the exact current head 2c4433b99c539a43bdecc74c7d167b2446dd4fdc.

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 performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant