test: expand TLS SNI edge-case coverage#53
Merged
Conversation
Closes #50. Adds 5 pin-tests for SNI edge cases that the existing non-UTF-8 SNI tests (from #49) don't reach. Each new test documents the *current* analyzer behavior so a future refactor can't silently change it. New tests --------- 1. test_sni_extension_with_empty_hostname_list — SNI extension present but the ServerNameList has zero entries. extract_sni returns None, no finding, no count increment, handshake still parses. (Confirmed tls_parser accepts zero-entry lists at parse time.) 2. test_sni_with_empty_hostname_bytes — SNI hostname of b"". Decodes via from_utf8 as Ok(""), counted under the empty-string key, no finding. Degenerate but technically valid wire form. 3. test_valid_utf8_non_ascii_sni_currently_not_flagged — A raw U-label like "café.example" is also a RFC 6066 §3 violation (spec requires A-label Punycode form), but the current analyzer does not flag it. Pins current behavior; will be flipped when issue #51 lands. 4. test_multi_name_sni_list_only_first_entry_counted — SNI extension with 3 ServerName entries. Only the first is counted in sni_counts (matching extract_sni's list.first() behavior). 5. test_non_utf8_sni_finding_fires_when_sni_counts_at_capacity — fills sni_counts to MAX_MAP_ENTRIES (50k unique valid hostnames) via the public API, then sends a non-UTF-8 SNI ClientHello. Verifies that the new non-UTF-8 key is dropped at the cap but the finding still fires. This tests a real invariant: the finding push and the count increment are independent, and a refactor nesting one inside the other would silently break forensic visibility past the cap. Runtime ~660ms in debug builds (acceptable). Builder refactor ---------------- Extracted build_client_hello_with_sni_list from build_client_hello_raw_sni to support zero and multi-entry SNI lists. build_client_hello_raw_sni now delegates to the new helper for a single entry, preserving the existing public API and all prior tests. Out of scope ------------ Issue #51 (flag valid-UTF-8 but non-ASCII SNI) is filed separately and is the natural follow-up that would flip test 3's assertion.
Addresses pr-test-analyzer findings on PR (#50 work): I1 (important) — test_sni_extension_with_empty_hostname_list was exposed to a silent false-positive: if tls_parser ever tightens to reject zero-entry ServerNameList (RFC 6066 §3 specifies server_name_list<1..2^16-1>, which makes empty lists structurally invalid), parse_tls_extensions would fail and our assertions would still pass — but the extract_sni `list.first() == None` branch would no longer be exercised. Added an explicit `assert_eq!(parse_error_count, 0)` to fail loud on that drift. I2 (important) — test_valid_utf8_non_ascii_sni_currently_not_flagged references issue #51 in a comment but the panic message didn't carry a grep-able link. Updated the comment and the assertion's error message to include the full GitHub URL so a maintainer landing #51 without searching the tests sees the cross-reference. S1 — corrected the runtime comment on the capacity test: it actually measures ~650ms in debug builds (not 1-3s). Budgeted ~2s for CI cold caches. S2 — tightened the capacity test's finding delta to filter specifically for non-UTF-8 findings instead of comparing total findings length. A future refactor that adds an unrelated finding on every ClientHello would still pass the old delta test for the wrong reason; filtering by category makes the invariant explicit. S5 — added a doc comment to build_client_hello_with_sni_list noting that passing &[] produces a technically RFC-violating wire form used to exercise the analyzer's defensive `list.first() == None` branch. RFC 6066 §3 ServerNameList bounds and NameType extensibility confirmed via direct RFC fetch before applying.
There was a problem hiding this comment.
Pull request overview
Expands TLS analyzer test coverage for SNI edge cases (empty list, empty hostname, valid UTF-8 non-ASCII, multi-entry lists, and map-capacity behavior) to pin current analyzer behavior and prevent silent regressions.
Changes:
- Refactors the TLS ClientHello test fixture builder to support 0-or-more SNI entries via
build_client_hello_with_sni_list. - Adds four new pin-tests covering empty SNI list, empty hostname bytes, valid UTF-8 non-ASCII SNI, and multi-name lists.
- Adds a boundary pin-test ensuring the non-UTF-8 SNI finding still fires when
sni_countsis atMAX_MAP_ENTRIES.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses Copilot review on PR #53. C1: build_client_hello_with_sni_list used `as u16` casts for length fields, which silently wrap on overflow. If a future test (e.g. the 65,534-byte hostname case requested by issue #52) passed an SNI entry larger than u16::MAX, the builder would produce a malformed ClientHello and the test would pass/fail for the wrong reason. Switched to `u16::try_from(...).expect(...)` for per-entry name length and total ServerNameList length, and `checked_add(2)` for the extension-length calculation. Tests asking for out-of-range inputs now panic with a clear message instead of wrapping. C2: test_sni_with_empty_hostname_bytes described b"" as "degenerate but technically valid wire form". RFC 6066 §3 actually defines `opaque HostName<1..2^16-1>`, so a zero-byte hostname is RFC-violating (minimum length is 1). Updated the comment to say "degenerate, RFC-violating" and clarify that the pin is guarding the analyzer's defensive path — tls_parser accepts the non-spec-compliant form, and we want to document how the analyzer handles it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses Copilot re-review on PR #53. The previous hardening commit only guarded the SNI-internal length fields but missed three other u16 truncation points in the same builder: - ciphers_len = (cipher_ids.len() * 2) as u16 - ext_len = extensions.len() as u16 - hs_len = handshake.len() as u16 All three silently wrap on overflow and would produce malformed ClientHello / TLS record bytes. If a future test passes enough SNI data to push the extensions block past u16::MAX (which the 65,532-byte hostname case in issue #52 easily can), ext_len would wrap and downstream parsing would fail for the wrong reason. Switched each to `u16::try_from(...).expect(...)` with a clear message. Also corrected the comment on the hardening sweep: RFC 6066's ServerNameList carries 3 bytes of ServerName overhead per entry (NameType + u16 length), so a single maximum-length hostname is ~65,532 bytes, not 65,534. Comment accuracy and missed truncation paths both flagged by pr-review-toolkit:code-reviewer / Copilot on the re-review pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
build_client_hello_with_sni_listfrombuild_client_hello_raw_sni, preserving the existing API for prior tests.MAX_MAP_ENTRIESboundary by feeding 50,000 unique ClientHellos through the public API. Runtime ~650ms in debug (budget ~2s for CI). No production code changes — this is test-only.NameType, max-length SNI, trailing bytes) filed as test: add TLS SNI edge-case coverage for non-zero NameType, max length, and trailing bytes #52.New tests
ServerNameListwith zero entries.extract_snireturnsNone, no finding, no count, handshake still parses. Guards against tls_parser drift via an explicitparse_error_count == 0assertion (empty-list wire form is technically RFC-violating perserver_name_list<1..2^16-1>; a future tls_parser strictness tightening would otherwise let this test pass for the wrong reason).b"". Decodes viafrom_utf8asOk(""), counted under the empty-string key, no finding. Degenerate but valid wire form."café.example"(a raw U-label, also a RFC 6066 §3 violation per the A-label requirement). Pins current no-finding behavior; the assertion includes the explicit TLS: flag valid-UTF-8 but non-ASCII SNI as RFC 6066 violation #51 URL so the cross-reference survives grep and will fire loud when that issue lands.extract_sni'slist.first()behavior.sni_countstoMAX_MAP_ENTRIES = 50_000via 50k unique ClientHellos through the public API, then sends a non-UTF-8 SNI ClientHello. Verifies the new non-UTF-8 key is dropped at the cap but the finding still fires. Pins a real invariant: finding emission is independent of count insertion. Delta is filtered bysummary.contains("non-UTF-8 bytes")so the pin stays tight under refactors.Why brute-force for test 5 (not a test helper)
Reaching
MAX_MAP_ENTRIES = 50_000via the public API was chosen over adding a#[cfg(test)] pub(crate)test helper onTlsAnalyzerto expose internal state. The trade-off was validated with Perplexity (rustc community tends to accept slow boundary tests over test-only production surface) and empirically: debug runtime ~650ms, release ~120ms. The constant is hand-duplicated in the test but self-corrects via the length assertion (any change to the cap would loudly fail).RFC validation
Before applying the code-reviewer's fixes, RFC 6066 §3 was fetched directly to confirm the relevant spec claims:
ServerNameList server_name_list<1..2^16-1>— minimum 1 entry (empty lists are RFC-violating wire form, used in test 1 only to exercise the analyzer's defensive branch)enum { host_name(0), (255) } NameType—(255)reserves room for future types (informs the test: add TLS SNI edge-case coverage for non-zero NameType, max length, and trailing bytes #52 follow-up for non-zero NameType testing)Test plan
cargo test --test tls_analyzer_tests— 18/18 pass (~650ms total, test 5 is ~650ms of that)cargo test— full suite greencargo clippy --all-targets -- -D warnings— cleancargo fmt --check— cleanpr-test-analyzer): 1 important finding (I1 silent false-positive), 1 important (I2 panic message link), 3 suggestions — all applied before PR creation