fix: validate TLS SNI hostname authority - #107
seonghobae merged 13 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:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
seonghobae
left a comment
There was a problem hiding this comment.
Current-exact owner technical review on 5a989c12a7b452a3b6474e34d44f7853e2443f82, COMMENT only. The parent #27@00046c7bc4c48d737a2ef9c243315f55268475b3 validates TLS SNI only for presence/non-empty text, while the delivery adapter passes that string unchanged into Pingora HttpPeer and hostname verification. RFC 6066 §3 limits HostName to an ASCII DNS hostname without a trailing dot and prohibits literal IPv4/IPv6; RFC 9525 §7.4 likewise separates IP-ID from SNI domain-name identity. Test-first 9198413... pins hostile literal-IP, trailing-dot, whitespace, malformed-label and raw-Unicode inputs; 31d3fb9... adds the transport-neutral validator and dedicated InvalidTlsServerName; 8e64d66... pins exact errors plus DNS label/name length boundaries; b27c6ad... and 5a989c1... align the public configuration and test-strategy contracts. Fresh compare is ahead-only from exact parent with four effective paths: src/edge_contract.rs, tests/tls_sni_hostname_contract.rs, API_CONFIG_CONTRACT.md, and TEST_STRATEGY.md. No product auth/business logic, certificate issuance/key custody, Keyverse, Wardnet/EgressWeave, routing authority, workflow/gate, coverage denominator, retry, or dependency behavior is added. This review does not claim hosted GREEN or independent APPROVED governance; exact-head CI/Supply and independent current-range review are still required.
|
@coderabbitai review Please review current exact |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="00046c7bc4c48d737a2ef9c243315f55268475b3"
head="5a989c12a7b452a3b6474e34d44f7853e2443f82"
printf '%s\n' '== Commit availability =='
git cat-file -t "$base"
git cat-file -t "$head"
printf '%s\n' '== Effective changed paths =='
git diff --name-status "$base" "$head"
printf '%s\n' '== Diff stat =='
git diff --stat "$base" "$head"
printf '%s\n' '== Diff =='
git diff --find-renames --find-copies --no-ext-diff --unified=80 "$base" "$head"
printf '%s\n' '== AST outlines of changed Rust files =='
git diff --name-only "$base" "$head" -- '*.rs' | while IFS= read -r f; do
printf '\n-- %s --\n' "$f"
ast-grep outline "$f" --lang rust --items all || true
done
printf '%s\n' '== Relevant SNI validation and peer construction references =='
rg -n -C 4 -P '\b(sni|SNI|HttpPeer::new|UpstreamConfig::validate)\b' -g '*.rs' . || trueLength of output: 81653 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Formatting check =='
rustfmt --check src/edge_contract.rs tests/tls_sni_hostname_contract.rs || true
printf '%s\n' '== Configuration and delivery outlines =='
ast-grep outline src --lang rust --items all --match 'PgErdMigrationConfig|build_peer|build_proxy|from_yaml|validate'
printf '%s\n' '== Calls that construct peers or activate proxy configuration =='
rg -n -C 6 -P '\b(build_peer(?:_from_validated)?|build_proxy|UpstreamConfig::validate|\.validate\(\))\b' \
-g '*.rs' src tests
printf '%s\n' '== Migration configuration validation implementation =='
rg -n -A 160 -B 20 -P 'impl\s+PgErdMigrationConfig|fn\s+from_yaml\s*\(' src/migration_proxy.rs src/*.rs
printf '%s\n' '== Peer-build implementation and its public boundary =='
sed -n '1,125p' src/pingora_delivery.rs
printf '%s\n' '== Cargo formatting and CI configuration =='
fd -a -H '(^rustfmt\.toml$|^\.rustfmt\.toml$|^Cargo\.toml$|^.*\.ya?ml$)' . | while IFS= read -r f; do
case "$f" in
./Cargo.toml|*/.github/*|./rustfmt.toml|./.rustfmt.toml) printf '\n-- %s --\n' "$f"; sed -n '1,180p' "$f";;
esac
doneLength of output: 64952
I confirmed the effective range contains the four specified paths. The SNI validation path reaches both generic and pg-erd peer construction before network authority. Formatting verification is inconclusive because
|
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-exact follow-up on 7c2c375f1ab82d9fc370e23d54ad65fb80b92ae2, COMMENT only. Since the prior owner review, the only movement was documentation normalization: API_CONFIG_CONTRACT.md now changes exactly the TLS-SNI contract paragraph, and TEST_STRATEGY.md changes exactly the paragraph that inventories the new focused test. Fresh per-file patches show no unrelated wording drift. Production/test delta remains the same src/edge_contract.rs + tests/tls_sni_hostname_contract.rs repair. No new actionable owner finding; hosted execution and independent current-range review are still unproven.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-exact owner follow-up on 4bbd8a91dbd94a7d8e71106f81756f1a03b07c4f, COMMENT only. The only delta after 7c2c375... is test strengthening: valid uppercase DNS case-insensitivity, one 63-byte label, and an exact 253-byte textual DNS name are admitted, complementing the existing 64-byte-label and >253-byte negative cases. Production validator and the four-path authority boundary are unchanged. No additional owner finding; this exact must reacquire its own CI/Supply and independent current-range review.
|
@coderabbitai review Please review current exact |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="00046c7bc4c48d737a2ef9c243315f55268475b3"
head="4bbd8a91dbd94a7d8e71106f81756f1a03b07c4f"
printf '%s\n' '== Commit availability =='
git cat-file -t "$base"
git cat-file -t "$head"
printf '%s\n' '== Merge base and range =='
git merge-base "$base" "$head"
git log --oneline --decorate "$base..$head"
printf '%s\n' '== Changed paths =='
git diff --name-status "$base" "$head"
printf '%s\n' '== Range diff =='
git diff --find-renames --find-copies --no-ext-diff --unified=60 "$base" "$head"
printf '%s\n' '== Rust outlines =='
for f in $(git diff --name-only "$base" "$head" -- '*.rs'); do
printf '\n-- %s --\n' "$f"
ast-grep outline "$f" --lang rust --items all || true
done
printf '%s\n' '== Validation, configuration activation, and peer construction call sites =='
rg -n -C 8 -P '\b(InvalidTlsServerName|validate_sni|UpstreamConfig::validate|build_peer(?:_from_validated)?|HttpPeer::new|from_yaml)\b' \
-g '*.rs' src tests || trueLength of output: 122665 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Final commit delta =='
git show --format=fuller --find-renames --find-copies --no-ext-diff --unified=40 \
4bbd8a91dbd94a7d8e71106f81756f1a03b07c4f
printf '%s\n' '== Cargo and Rust toolchain declarations =='
for f in Cargo.toml rust-toolchain rust-toolchain.toml; do
if [ -f "$f" ]; then
printf '\n-- %s --\n' "$f"
cat "$f"
fi
done
printf '%s\n' '== Generic configuration and migration composition implementations =='
sed -n '207,430p' src/edge_contract.rs
sed -n '95,260p' src/migration_admin.rs
sed -n '40,115p' src/gateway_proxy.rs
sed -n '55,115p' src/migration_delivery.rs
printf '%s\n' '== Existing project use of is_some_and =='
rg -n -C 2 -P '\.is_some_and\s*\(' -g '*.rs' src tests || trueLength of output: 30004
I found no source-level defect in the effective four-path range. The final delta from The SNI validator rejects the stated invalid forms before peer construction. Both generic and pg-erd construction paths revalidate configuration before an I did not run formatting, compilation, tests, Clippy, rustdoc, coverage, load, OCI, or supply-chain checks. The inspection environment does not permit repository code execution. You are interacting with an AI system. |
|
@coderabbitai review Please review the exact configured range |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="00046c7bc4c48d737a2ef9c243315f55268475b3"
head="4bbd8a91dbd94a7d8e71106f81756f1a03b07c4f"
printf '%s\n' '== Commit and merge-base =='
git cat-file -t "$base"
git cat-file -t "$head"
git merge-base "$base" "$head"
printf '%s\n' '== Effective paths =='
git diff --name-status "$base" "$head"
printf '%s\n' '== Exact diff =='
git diff --find-renames --find-copies --no-ext-diff --unified=100 "$base" "$head" -- \
src/edge_contract.rs tests/tls_sni_hostname_contract.rs API_CONFIG_CONTRACT.md TEST_STRATEGY.md
printf '%s\n' '== Head outlines =='
ast-grep outline src/edge_contract.rs --lang rust --items all
ast-grep outline tests/tls_sni_hostname_contract.rs --lang rust --items all
printf '%s\n' '== SNI validation, direct validation, and peer construction references at HEAD =='
rg -n -C 8 -P '\b(InvalidTlsServerName|validate_sni|UpstreamConfig::validate|build_peer(?:_from_validated)?|HttpPeer::new|from_yaml)\b' \
-g '*.rs' src tests
printf '%s\n' '== Delivery and configuration activation implementations at HEAD =='
sed -n '1,180p' src/pingora_delivery.rs
sed -n '1,300p' src/edge_contract.rs
sed -n '1,230p' src/migration_proxy.rs
sed -n '1,180p' src/gateway_proxy.rs
sed -n '1,180p' src/migration_delivery.rs
sed -n '1,250p' src/migration_admin.rsLength of output: 178527
I found no source-level defect in the four effective paths.
The focused contract tests use direct programmatic The generic path calls The range preserves the existing transport-neutral authority boundary. It adds no certificate issuance or rotation behavior, DNS resolution, product authentication or business logic, Keyverse, Wardnet, EgressWeave, arbitrary destination selection, or downstream TLS authority. I performed static inspection only. I did not execute formatting, compilation, tests, Clippy, rustdoc, coverage, load, OCI, or supply-chain checks. You are interacting with an AI system. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-exact owner review after parent-first reconciliation. Exact 3612bce0cdb02e746fe36b905aa7299aff27f890 is a two-parent, non-force merge with prior child 4bbd8a91... first and terminal-clean parent #27@84a4df0... second. Fresh compare against the parent is ahead 12 / behind 0 with merge-base exactly 84a4df0..., and the effective child delta is again exactly four paths: src/edge_contract.rs, tests/tls_sni_hostname_contract.rs, API_CONFIG_CONTRACT.md, TEST_STRATEGY.md. The reconciliation preserves #27's hosted Rust 1.98 formatting, including the parent-only upstream-unicast fixture formatting, while retaining only the transport-neutral SNI HostName admission delta. No product auth/business, Keyverse, Wardnet/EgressWeave, certificate issuance, DNS resolution, or routing authority is introduced. COMMENT-only evidence; current-exact hosted execution and independent review are required.
|
@coderabbitai review Please review current exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="84a4df0bb162673f602dac95e9ded7eaa0e23582"
head="3612bce0cdb02e746fe36b905aa7299aff27f890"
printf '%s\n' '== Commit objects and merge base =='
git cat-file -t "$base"
git cat-file -t "$head"
git merge-base "$base" "$head"
git show -s --format='base %H%nparents %P%nsubject %s' "$base"
git show -s --format='head %H%nparents %P%nsubject %s' "$head"
printf '%s\n' '== Requested range changed paths =='
git diff --name-status "$base" "$head"
printf '%s\n' '== Requested range diff stat =='
git diff --stat "$base" "$head"
printf '%s\n' '== Requested range diff =='
git diff --find-renames --find-copies --no-ext-diff --unified=100 "$base" "$head" -- \
src/edge_contract.rs tests/tls_sni_hostname_contract.rs API_CONFIG_CONTRACT.md TEST_STRATEGY.md
printf '%s\n' '== Changed Rust outlines at head =='
ast-grep outline src/edge_contract.rs --lang rust --items all
ast-grep outline tests/tls_sni_hostname_contract.rs --lang rust --items all
printf '%s\n' '== SNI validation and peer construction references at head =='
rg -n -C 8 -P '\b(InvalidTlsServerName|validate_sni|UpstreamConfig::validate|build_peer(?:_from_validated)?|HttpPeer::new|from_yaml)\b' \
-g '*.rs' src tests
printf '%s\n' '== Focused implementations at head =='
sed -n '1,360p' src/edge_contract.rs
sed -n '1,190p' src/pingora_delivery.rs
sed -n '1,280p' src/migration_proxy.rs
sed -n '1,190p' src/gateway_proxy.rs
sed -n '1,190p' src/migration_delivery.rs
sed -n '1,280p' src/migration_admin.rs
printf '%s\n' '== Test and documentation content at head =='
sed -n '1,360p' tests/tls_sni_hostname_contract.rs
sed -n '1,280p' API_CONFIG_CONTRACT.md
sed -n '1,280p' TEST_STRATEGY.mdLength of output: 217156
I found no source-level defect in the four changed paths. The range validates TLS SNI at the transport-neutral The new contract test calls The generic composition path calls The range remains within the Admin Config and upstream TLS identity boundary. It adds no certificate issuance or rotation, DNS resolution, product authentication or business logic, Keyverse identity, Wardnet or EgressWeave authority, arbitrary upstream selection, or downstream TLS termination. I performed static inspection only. I did not run formatting, compilation, tests, Clippy, rustdoc, coverage, traffic, OCI, or supply-chain checks. You are interacting with an AI system. |
|
@coderabbitai review Please review current exact |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="84a4df0bb162673f602dac95e9ded7eaa0e23582"
head="523d825a8678976a3e678cd244918df4da9f691b"
printf '%s\n' '== Commit identity and ancestry =='
git cat-file -t "$base"
git cat-file -t "$head"
git merge-base "$base" "$head"
git show -s --format='base %H%nparents %P%nsubject %s' "$base"
git show -s --format='head %H%nparents %P%nsubject %s' "$head"
printf '%s\n' '== Exact changed paths =='
git diff --name-status "$base" "$head"
printf '%s\n' '== Exact diff stat =='
git diff --stat "$base" "$head"
printf '%s\n' '== Exact four-path diff =='
git diff --find-renames --find-copies --no-ext-diff --unified=100 "$base" "$head" -- \
src/edge_contract.rs tests/tls_sni_hostname_contract.rs API_CONFIG_CONTRACT.md TEST_STRATEGY.md
printf '%s\n' '== Final commit delta =='
git show --format=fuller --find-renames --find-copies --no-ext-diff --unified=80 "$head" -- \
tests/tls_sni_hostname_contract.rs
printf '%s\n' '== Changed Rust outlines =='
ast-grep outline src/edge_contract.rs --lang rust --items all
ast-grep outline tests/tls_sni_hostname_contract.rs --lang rust --items all
printf '%s\n' '== SNI validation and peer construction references =='
rg -n -C 8 -P '\b(InvalidTlsServerName|EmptyTlsServerName|validate_sni|UpstreamConfig::validate|build_peer(?:_from_validated)?|HttpPeer::new|from_yaml)\b' \
-g '*.rs' src tests
printf '%s\n' '== Focused final source =='
sed -n '1,380p' src/edge_contract.rs
sed -n '1,380p' tests/tls_sni_hostname_contract.rs
sed -n '1,210p' src/pingora_delivery.rs
sed -n '1,230p' src/gateway_proxy.rs
sed -n '1,230p' src/migration_proxy.rs
sed -n '1,210p' src/migration_delivery.rs
sed -n '1,300p' src/migration_admin.rsLength of output: 190212
I found no current-exact actionable source-level finding. The range changes exactly these four paths:
The final commit changes only the import layout in
The direct contract test still exercises The generic composition path validates The range remains within the transport-neutral Admin Config and upstream TLS identity boundary. It adds no certificate lifecycle, DNS resolution, product authentication or business logic, Keyverse, Wardnet, EgressWeave, arbitrary upstream selection, or downstream TLS authority. I performed static inspection only. I did not execute formatting, compilation, tests, Clippy, rustdoc, coverage, traffic, OCI, or supply-chain checks. You are interacting with an AI system. |
a2dbb0b
into
fix/network-authority-self-loop-v1
1 participant
Finding
Historical parent
#27@00046c7bc4c48d737a2ef9c243315f55268475b3required TLSsnito be present and non-empty but accepted arbitrary non-empty strings. The delivery adapter passes that value to PingoraHttpPeerfor SNI and hostname verification, so malformed TLS identity must be rejected at the transport-neutral Admin Config boundary.RFC 6066 §3 limits SNI
HostNameto an ASCII DNS hostname without a trailing dot and prohibits literal IPv4/IPv6 addresses. RFC 9525 §7.4 separately distinguishes SNI domain-name identity from IP-ID reference identity.RED → minimal causal repair
919841320e478637f0c966f574105f6ba2712966— test-first RED for IP literals, trailing dot, whitespace, underscore, leading/trailing hyphen, empty DNS label, raw Unicode, and positive DNS/A-label cases.31d3fb9cf089a4504ee89e8e6059b441e493cb3b—InvalidTlsServerNameplus transport-neutral SNI HostName validation before Pingora construction.8e64d66a372f1ff2c87e31b4aa8119d565c3e7ac— exact error classification and negative label/name length boundaries.b27c6ad37e4ada2ca208c06c33ab1b86767440b9/ later normalization — API contract evidence.5a989c12a7b452a3b6474e34d44f7853e2443f82/ later normalization — test-strategy evidence.4bbd8a91dbd94a7d8e71106f81756f1a03b07c4f— uppercase DNS, 63-byte label, and exact 253-byte textual DNS name positive boundaries.Parent-first reconciliation
#27 exact
84a4df0bb162673f602dac95e9ded7eaa0e23582reached terminal SUCCESS CI35047995796and Supply Chain35047995901, with all review threads resolved, and was normally merged as2175507f834081498c46b5693ef4af3ead069fd0into its non-default parent branch.3612bce0cdb02e746fe36b905aa7299aff27f890was an ordinary two-parent/non-force reconciliation: first parent prior child4bbd8a91..., second parent current #2784a4df0.... It adopted parent-only hosted formatting repairs while preserving the SNI child delta. Its effective child authority remained exactly four paths:src/edge_contract.rs,tests/tls_sni_hostname_contract.rs,API_CONFIG_CONTRACT.md, andTEST_STRATEGY.md.Hosted RED → current exact
Predecessor
3612bce...produced terminal SUCCESS Supply Chain35074112853, SUCCESS load-contract and SUCCESS dual-profile OCI in CI35074112834. The same CItestjob received a real Rust 1.98 runner and failed only atVerify formattingbefore compile/lint/rustdoc/coverage. The child-addedtests/tls_sni_hostname_contract.rsused a manually multilineuse cwl_pingora_gateway::edge_contract::{...};import whose complete import fits rustfmt's canonical line width.Current exact
523d825a8678976a3e678cd244918df4da9f691bchanges only that import to the canonical one-line Rust 1.98 rustfmt layout. SNI validation semantics, tests, API contract and authority boundary are unchanged. Predecessor Supply/load/OCI GREEN and predecessor static review remain RCA evidence only; they do not transfer to the new SHA.Current exact review / execution gate
Manual independent CodeRabbit reply
5698268580completed static review for exact range84a4df0bb162673f602dac95e9ded7eaa0e23582..523d825a8678976a3e678cd244918df4da9f691b, found no current-exact actionable source-level finding, and confirmed the effective range remains the four declared paths. There are no inline review threads. Static review is not hosted execution evidence.Fresh current-exact candidates remain CI
35102077473and Supply Chain35102077478; Draft-triggered CI35102072842and Supply35102072847were skipped and are non-promotion evidence. Current exact must independently pass Rust 1.98 formatting, all-target tests, strict Clippy, warning-denied rustdoc, 100% owned-production coverage, load traffic, dual-profile OCI and Supply Chain/source binding before normal integration.Boundary
This child remains transport-neutral Admin Config / upstream TLS identity admission. It does not add certificate issuance or key custody, product authentication/business logic, Keyverse identity, Wardnet/EgressWeave authority, DNS resolution, arbitrary upstream selection, routing authority, or downstream TLS termination.
docs/product-technical-gap-baseline.mdand repository-wide TRACEABILITY remain dedicated #61 authority and are not edited here.No protected merge, immutable release, parity/shadow/canary, rollback, cutover or legacy-removal credit is claimed.