Curation pass: fix real-world TLS 1.2 decryption + clippy + honest status#1
Merged
Conversation
Newer clippy (1.94) tightened several lints that were clean at the Feb authoring time: needless_range_loop, useless_conversion, useless vec!, and needless borrow. All mechanical; no behavior change. Restores a clean `cargo clippy -- -D warnings` on current stable, matching what CI enforces.
netgrep set the session's TLS version from the ClientHello's supported_versions extension. But that extension is the client's *offer* — every modern client (curl, browsers) advertises TLS 1.3 there even when the server negotiates down to 1.2. netgrep then locked the connection to 1.3, looked up (empty) 1.3 traffic secrets for a session whose keylog held a 1.2 CLIENT_RANDOM master secret, and silently decrypted nothing. Effect: essentially every real-world TLS 1.2 session failed to decrypt, since real 1.2 sessions almost always come from clients that also offer 1.3. The existing synthetic 1.2 test missed it because its hand-built ClientHello carried no supported_versions extension. Fix: the negotiated version is the server's choice. Only the ServerHello sets it (legacy version field, upgraded to 1.3 iff its supported_versions extension says so). The ClientHello no longer touches the negotiated version. Verified end-to-end against live OpenSSL captures: a real TLS 1.2 ECDHE-RSA-AES128-GCM session (client offering 1.3) now decrypts; TLS 1.3 unaffected. Adds a regression unit test for the negotiation.
The existing TLS tests drive the crypto with synthetic known-answer vectors. These run the built binary against real TLS 1.2 and 1.3 handshakes captured on loopback, decrypt them with the session SSLKEYLOGFILE, and grep a marker that exists only inside the encrypted payload — exercising the whole pipeline end to end. A negative control asserts the marker is invisible without the keylog, so the tests prove decryption rather than cleartext matching. Fixtures are throwaway localhost sessions to a self-signed cert (committed keylogs protect nothing) and are regenerable via tests/fixtures/generate.sh.
Curation so a reader isn't misled: states plainly that this is an AI-assisted portfolio demo that never got its intended production use, what's verified against live traffic vs known-answer vectors, and that it's unmaintained (issues/PRs welcome, no support promised).
Two P2 findings from the cli-review-panel, both in the new test code: - codex: generate.sh leaked the privileged tcpdump (and openssl) if a capture step failed under set -e — the EXIT trap only removed the cert. Now every spawned PID and temp file is tracked and torn down on any exit. Also adds --retry-connrefused for server readiness and a fail-loud check so a bad capture never becomes a committed fixture. - claude: the negative-control e2e test asserted process exit success, coupling it to netgrep's incidental exit-0-on-no-match. Now it asserts netgrep actually processed the capture (its summary line), so the test can't silently pass on a crash and doesn't break if netgrep ever adopts grep's nonzero-on-no-match convention. Fixtures regenerated with the hardened script; shellcheck clean; 378 tests pass.
Round-2 cli-review-panel P3s on generate.sh, all real, all in the new test code: - SSLKEYLOGFILE opens in append mode, so re-running accumulated stale secrets from prior runs (the committed keylogs had carried two sessions). Truncate the keylog before each capture. - A run that aborted between the tcpdump write and the chown left a root-owned pcap in the tree. Always chown (even on failure) so the next non-sudo run isn't tripped by it. - curl's exit status was discarded, so a port already held by a foreign service produced a silently-bad fixture. Capture the rc and fail loud. Regenerated fixtures now carry a single session each. shellcheck clean, 378 tests pass. The round-2 P2 (removing the ClientHello 1.3-offer detection could drop 1.3 detection on a ServerHello-less capture) is a verified non-issue: try_derive_keys gates on cipher_suite and should_derive, both set only in the ServerHello arm, so a client-only capture never derived keys or decrypted before or after this change.
Round-3 cli-review-panel findings, all on the fixture generator (the shipped product code was clean across all three rounds): - Verify our openssl s_server actually bound the port (kill -0) before curl runs. Two services can't share a port, so a squatter would make s_server fail to bind; without this, curl -k could talk to the foreign service and capture someone else's session (codex + claude). - Assert the keylog is non-empty. A curl whose TLS backend ignores SSLKEYLOGFILE exits 0 with a good pcap and an empty keylog — a silently-useless fixture. Fail loud instead (claude). - Drop reaped PIDs from the cleanup arrays after wait, so the EXIT trap can't signal a reused PID on a later capture (codex). Remaining round-3 findings are dispositioned P3 polish: the version- None-without-ServerHello note (verified moot — decryption already requires the ServerHello's cipher_suite + should_derive) and two negative-control test niceties (1.2-only coverage, no exit assert). shellcheck clean; fixtures regenerated single-session; 378 tests pass.
Round-4 review: - Parameterize the negative control over BOTH fixtures (was 1.2-only). Without a 1.3 control, a regenerated 1.3 capture that leaked its marker into an unencrypted record would let decrypts_real_tls13 pass for the wrong reason. - generate.sh: a failure path (notably the new s_server liveness check, which returns before the chown) could leave a root-owned pcap in the now-tracked fixtures dir — contradicting the 'always chown' comment. Replace it with trap-based cleanup: each capture registers its artifacts, cleared on success, so any abort removes the partial (root-owned) pcap + stale keys. Also warn if tcpdump never reported listening, instead of a misleading 'port busy' later. Dispositioned (not changed): ServerHello legacy-version trusted verbatim (theoretical — every modern stack writes 0x0303) and a possible unused import (clippy -D warnings verified clean; tls_parser is a glob import). shellcheck clean; 378 tests pass.
Reviewer's GuideFixes TLS session version negotiation to use the ServerHello (enabling real-world TLS 1.2 decryption), adds real capture-based TLS e2e tests and fixture generator script, tightens tests around version negotiation, updates README status/provenance, and applies small clippy-driven cleanups. Sequence diagram for TLS session version negotiation in parse_handshakesequenceDiagram
actor Client
actor Server
participant Netgrep
Client->>Netgrep: parse_handshake(ch_body, c_ip, 12345, None)
Netgrep-->>Client: result.version = None
Server->>Netgrep: parse_handshake(sh_body, s_ip, 443, after_ch.version)
Netgrep-->>Server: result.version = TlsVersion_Tls12
Flow diagram for determining negotiated TLS version from ServerHelloflowchart TD
A[ServerHello received] --> B[Set session_version = sh.version]
B --> C{supported_versions extension contains TlsVersion_Tls13}
C -->|yes| D[Set session_version = TlsVersion_Tls13]
C -->|no| E[Keep session_version as legacy TLS 1.2]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds live keylog refresh and late-secret TLS decryption handling, reproducible TLS 1.2/1.3 capture fixtures, end-to-end tests, corrected ServerHello-based version negotiation, and supporting test, formatting, and documentation updates. ChangesTLS decryption verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant netgrep
participant KeyLog
participant TLSDecryption
participant PCAPFixtures
TestSuite->>netgrep: Run with PCAP and keylog path
netgrep->>PCAPFixtures: Read captured TLS packets
netgrep->>KeyLog: Load or refresh TLS secrets
KeyLog-->>TLSDecryption: Provide available secrets
TLSDecryption->>TLSDecryption: Derive missing direction keys
TLSDecryption-->>netgrep: Return decrypted payload
netgrep-->>TestSuite: Return output and processing status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="README.md" line_range="30" />
<code_context>
+Where it stands after a July 2026 cleanup pass:
+
+- Builds clean and passes its full test suite (~380 tests) on current stable Rust, with a clean `clippy -D warnings`.
+- Core paths are verified against live traffic, not only unit vectors: DNS parsing, TCP reassembly, and TLS decryption of real TLS 1.2 and 1.3 AES-GCM sessions. The end-to-end decryption is reproducible from checked-in captures (`tests/tls_e2e.rs`, regenerate with `tests/fixtures/generate.sh`).
+- Other cipher suites (ChaCha20-Poly1305, AES-256, RSA key exchange) pass known-answer vectors but haven't been run against captured sessions yet.
+- Not actively maintained. Issues and PRs are welcome, no support promised.
</code_context>
<issue_to_address>
**suggestion (typo):** “unit vectors” here likely should be “test vectors” to match common terminology.
Here “test vectors” is the usual term for predefined input/output pairs used to validate crypto/protocol behavior. Unless you literally mean mathematical unit vectors, please rename accordingly.
```suggestion
- Core paths are verified against live traffic, not only test vectors: DNS parsing, TCP reassembly, and TLS decryption of real TLS 1.2 and 1.3 AES-GCM sessions. The end-to-end decryption is reproducible from checked-in captures (`tests/tls_e2e.rs`, regenerate with `tests/fixtures/generate.sh`).
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Where it stands after a July 2026 cleanup pass: | ||
|
|
||
| - Builds clean and passes its full test suite (~380 tests) on current stable Rust, with a clean `clippy -D warnings`. | ||
| - Core paths are verified against live traffic, not only unit vectors: DNS parsing, TCP reassembly, and TLS decryption of real TLS 1.2 and 1.3 AES-GCM sessions. The end-to-end decryption is reproducible from checked-in captures (`tests/tls_e2e.rs`, regenerate with `tests/fixtures/generate.sh`). |
There was a problem hiding this comment.
suggestion (typo): “unit vectors” here likely should be “test vectors” to match common terminology.
Here “test vectors” is the usual term for predefined input/output pairs used to validate crypto/protocol behavior. Unless you literally mean mathematical unit vectors, please rename accordingly.
Suggested change
| - Core paths are verified against live traffic, not only unit vectors: DNS parsing, TCP reassembly, and TLS decryption of real TLS 1.2 and 1.3 AES-GCM sessions. The end-to-end decryption is reproducible from checked-in captures (`tests/tls_e2e.rs`, regenerate with `tests/fixtures/generate.sh`). | |
| - Core paths are verified against live traffic, not only test vectors: DNS parsing, TCP reassembly, and TLS decryption of real TLS 1.2 and 1.3 AES-GCM sessions. The end-to-end decryption is reproducible from checked-in captures (`tests/tls_e2e.rs`, regenerate with `tests/fixtures/generate.sh`). |
Post-override review caught one genuinely real item: netgrep falls back to the SSLKEYLOGFILE env var when --keylog is absent, so the negative control would false-fail for any developer with that variable exported (plausible right after running the fixture generator). Strip it in the shared command builder so the tests depend only on the explicit flag. Also note in generate.sh that it captures on Linux's 'lo' (macOS is 'lo0'). Other post-override findings dispositioned: dead-import (clippy -D warnings verified clean), version-requires-ServerHello (moot — already gated on ServerHello-only cipher_suite+should_derive), port-collision retry and other generator niceties (fails loud already). 378 tests.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
The keylog was read once at startup, which broke the headline live use case (`netgrep --keylog sslkeylog.txt "Set-Cookie"`): SSLKEYLOGFILE is written by the client *during* the sessions being captured, so a new session's secrets land after that read and were never seen. A missing keylog at startup also hard-crashed the whole capture. Both went undetected because pcap-replay testing always has a complete keylog before netgrep reads it. - KeyLog gains a source path + size and a refresh() that re-reads the (append-only) keylog when it grows, merging in newly-written secrets and zeroizing the superseded maps. - from_file tolerates a missing keylog: start empty, warn, and pick it up via refresh() once it appears, instead of aborting the capture. - The derive paths refresh on a secret miss, and decrypt_record re-attempts derivation when a connection still has no keys, so a secret that lands after the ServerHello is caught on a later record. Verified live off the wire (not just tests): netgrep capturing on lo with the keylog created/appended mid-session now decrypts real live TLS 1.2 and 1.3 sessions (previously exit 1 / os error 2, or a silent 0 matches). pcap replay unregressed. Adds unit tests for missing-file tolerance and refresh-picks-up-appended-secrets; updates two tests that asserted the old crash-on-missing contract. 380 tests pass.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tls/keylog.rs">
<violation number="1" location="src/tls/keylog.rs:122">
P2: Live decryption misses secrets after a keylog is replaced or rotated to the same byte length because this size-only guard returns before reading the new contents. The method documents rotation support, so tracking file identity/metadata in addition to size (or re-reading on misses) would keep replacement files detectable.</violation>
</file>
<file name="src/tls/mod.rs">
<violation number="1" location="src/tls/mod.rs:636">
P2: Unmatched high-volume TLS sessions now perform a keylog filesystem stat for every encrypted record. The `no_keys` retry needs throttling or a cached refresh check so a missing secret does not turn packet processing into per-record filesystem work.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // Not there (yet) or unreadable — keep what we have. | ||
| Err(_) => return false, | ||
| }; | ||
| if size == self.last_size { |
There was a problem hiding this comment.
P2: Live decryption misses secrets after a keylog is replaced or rotated to the same byte length because this size-only guard returns before reading the new contents. The method documents rotation support, so tracking file identity/metadata in addition to size (or re-reading on misses) would keep replacement files detectable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tls/keylog.rs, line 122:
<comment>Live decryption misses secrets after a keylog is replaced or rotated to the same byte length because this size-only guard returns before reading the new contents. The method documents rotation support, so tracking file identity/metadata in addition to size (or re-reading on misses) would keep replacement files detectable.</comment>
<file context>
@@ -65,11 +93,50 @@ impl KeyLog {
+ // Not there (yet) or unreadable — keep what we have.
+ Err(_) => return false,
+ };
+ if size == self.last_size {
+ return false;
+ }
</file context>
| .get(key) | ||
| .map(|c| c.client_keys.is_none() && c.server_keys.is_none()) | ||
| .unwrap_or(false); | ||
| if no_keys { |
There was a problem hiding this comment.
P2: Unmatched high-volume TLS sessions now perform a keylog filesystem stat for every encrypted record. The no_keys retry needs throttling or a cached refresh check so a missing secret does not turn packet processing into per-record filesystem work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tls/mod.rs, line 636:
<comment>Unmatched high-volume TLS sessions now perform a keylog filesystem stat for every encrypted record. The `no_keys` retry needs throttling or a cached refresh check so a missing secret does not turn packet processing into per-record filesystem work.</comment>
<file context>
@@ -613,6 +623,20 @@ impl TlsDecryptor {
+ .get(key)
+ .map(|c| c.client_keys.is_none() && c.server_keys.is_none())
+ .unwrap_or(false);
+ if no_keys {
+ self.try_derive_keys(key);
+ }
</file context>
Adversarial review of the live-keylog feature found a critical AEAD sequence desync. decrypt_record re-attempts key derivation for a keyless connection when a late keylog secret appears. But a freshly derived DirectionKeys starts its sequence counter at 0, which is only correct if no earlier ciphertext record was skipped. If application data record #1 was skipped (secret not yet visible) and the secret lands before record #2, deriving at record #2 sets seq=0 while the peer is already at seq=1 — so record #2 and every record after it fail to decrypt, seq never advances (it only advances on success), and the connection becomes silently, permanently undecryptable. This is the exact 'silent 0 matches' symptom the feature set out to remove, for both TLS 1.2 and 1.3. Live tests missed it because the secret happened to arrive by record #1 (seq 0 aligned). Fix: gate mid-stream derivation on app_data_started — only derive before any application-data record has flowed for the connection, so a fresh DirectionKeys at seq 0 stays aligned. Once app data has passed without keys, decline and warn once rather than build a broken decryptor. Conservative by construction: it only declines cases that previously corrupted; the timely-secret path (verified live for 1.2 and 1.3) is unchanged. Also harden refresh(): only re-read on growth, so a keylog rotation/truncation can't swap in a strict subset and drop secrets we still hold (SSLKEYLOGFILE is append-only). Zeroization and the size-cap TOCTOU were reviewed clean. 380 tests pass; live 1.2/1.3 decryption + pcap replay reverified.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Follow-up verification found the prior gate wrong for TLS 1.3. Every encrypted 1.3 record after ServerHello carries the ApplicationData wire type (EncryptedExtensions, Certificate, Finished, real app data all share type 23 — the true type is inside the ciphertext). So keying app_data_started on the wire ApplicationData type flipped it on the *handshake* records and then blocked the real app-key derivation. Real stacks write the handshake-traffic secret before the app-traffic secret, so live 1.3 would decline decryption in the common case (it only worked when the client wrote both secrets before netgrep processed the first record — the timing my tests happened to hit). Fix: determine app-epoch correctly. TLS 1.2 keeps the wire type (no disguise). TLS 1.3 uses the per-direction handshake-complete flag as it stood *before* this record — only records after the Finished are app-epoch (the Finished flips hs_complete but is itself handshake). So handshake records no longer close the derivation window, while a skipped post-handshake app record still does (preserving the desync guard). Conservative and non-corrupting either way. Live 1.2/1.3 decryption + replay reverified; 380 tests pass. The handshake-secret-before-app-secret timing race is analysis-verified (as the issue was found), not runtime-reproduced — it's a live race the CLI can't trigger deterministically.
… keys Third review round found the epoch gate correct but an adjacent pre-existing bug still defeating live 1.3: derive_tls13_keys only refreshed the keylog when NO entry existed for the client_random. The handshake-traffic secrets create a partial entry, so once they were read, contains_key stayed true forever and the later-appended application-traffic secrets were never picked up — the connection stayed undecryptable. - Refresh until BOTH application-traffic secrets are present, not just until an entry exists. - try_derive_keys no longer bails on client_keys alone; TLS 1.3 client/server traffic secrets can land separately, so it retries until both directions are keyed. - derive_tls13_keys only fills a direction that has no keys yet (and skips handshake keys once that direction's handshake is complete), so re-entry as late secrets arrive never resets an already- decrypting direction's AEAD sequence. - decrypt_record's re-derive gate fires when EITHER direction is missing keys, and its diagnostic softened to reflect that decryption can be partial. 380 tests pass; clippy/fmt clean. Deterministic staged-secret test still to come.
Unit-test the data-layer behavior the refresh-gating fix depends on: handshake-traffic secrets written first (partial entry), app-traffic secrets appended later for the same client_random, and refresh() must merge the app secrets into the existing entry rather than treat it as complete. Directly covers the case whose presence-only gate defeated live 1.3 decryption.
Fourth review round: the desync gate used a connection-wide app_data_started, so the first direction to decrypt app data slammed the derivation window shut on the other. With TLS 1.3 client/server traffic secrets able to arrive separately, that stranded a direction whose secret lagged — it would decline and warn even though the is_none guards would have safely filled just that direction. An availability gap (partial decryption), not corruption. Split into client_app_data_started / server_app_data_started: - drain_buffer flags only the record's own direction as app-epoch. - decrypt_record's re-derive gate keys on THIS record's direction: derive if that direction lacks keys and hasn't started, else warn. - derive_tls13_keys guards each direction's app-key derivation on both is_none and that direction's !app_data_started, so a fresh key at sequence 0 is only installed as that direction's first app-epoch record. - try_derive_keys stops only once each direction is keyed or locked. Live 1.2/1.3 + replay reverified; 381 tests pass; clippy/fmt clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tls/keylog.rs`:
- Around line 105-143: Update refresh() to validate the KeyLog returned by
Self::from_file(&path) before swapping its maps: if fresh.last_size is smaller
than the previously observed self.last_size, treat the refresh as unsuccessful
and retain the existing secrets and last_size. Only perform the swaps and update
last_size when the fresh read is at least as large as the expected size,
preserving the existing no-op behavior for rotation, truncation, or
disappearance.
In `@src/tls/mod.rs`:
- Around line 56-66: Replace the connection-wide app_data_started guard with
per-direction state in TlsConnection, such as client_app_data_started and
server_app_data_started. In the app-epoch handling around drain_buffer and the
derivation gate in decrypt_record, set and check only the flag for the record’s
direction so TLS 1.3 can still derive keys for the incomplete direction;
preserve connection-wide blocking for TLS 1.2, whose derivation overwrites both
directions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac4a93f4-3d22-413b-8207-8357768a2598
📒 Files selected for processing (4)
README.mdsrc/tls/keylog.rssrc/tls/mod.rstests/stress.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The per-direction split (previous commit) guarded derive_tls13_keys but left derive_tls12_keys installing both directions' keys unconditionally. Since decrypt_record's re-derive gate is now per-direction, a TLS 1.2 session with asymmetric app-data timing (server speaks first, master secret lands late) could reach derive_tls12_keys to fill the client direction and, in the same call, install a fresh server key at sequence 0 after the server direction already skipped an app record — desyncing it. Fails closed (no corrupted plaintext) but permanently, and with no warning since server_keys is then Some. Guard each direction's install with the same is_none() && !<dir>_app_data_started check as the 1.3 path. Both TLS versions now gate derivation identically. Live 1.2 + replay reverified; 381 tests. This completes the per-direction fix across both sibling paths — the 1.3 and 1.2 halves of one issue.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
- README: 'unit vectors' -> 'test vectors' (the standard term for predefined input/output pairs; avoids confusion with the math sense). - handshake.rs: fix the parse_handshake current_version doc, which still described the removed ClientHello version-detection behavior. It now correctly documents that current_version only guards against a retransmitted ServerHello clobbering the negotiated version. Both flagged by cubic/CodeRabbit on the PR; no code behavior change.
CI floats `stable`, now 1.97.1, which added lints my local 1.94.1 didn't flag — the same toolchain-drift class as the earlier clippy commit, just a newer stable. Both in pre-existing code: - protocol/http.rs: collapsible_match — fold the inner method check into a match guard (`b'T' if check_methods(..) =>`); a non-matching guard falls through to the catch-all, same behavior as before. - tui/event.rs: useless_borrows_in_formatting — drop the redundant `&` on a format! arg. Verified with rustup stable (1.97.1) clippy --all-targets, test (381), and fmt, matching what CI runs.
GitHub-side review (cubic + CodeRabbit) surfaced real edge cases the local review chain missed: - TLS 1.2 Finished consumes the AEAD sequence (cubic P2): post-CCS the encrypted Finished (a Handshake record) is sequence 0 and app data follows, but app_epoch only tracked ApplicationData — so a skipped Finished then a late key could install at sequence 0 after it and desync. TLS 1.2 now treats any cipher-active record as sequence- consuming (cipher_active), matching how its single post-CCS cipher actually works. TLS 1.3 is unchanged (separate handshake/app epochs). - refresh() could wipe secrets (CodeRabbit major): the file can be unlinked/truncated between metadata() and from_file's read, and from_file tolerates a missing file by returning an empty log — swapping that in dropped every secret. Only accept a re-read at least as large as what we hold. - refresh() I/O storm on a bad keylog (cubic P2): an oversized/invalid file returned Err and left last_size unchanged, so it was re-read in full on every decrypt miss. Cache the size to stop retrying until it grows. - warning wording (cubic P3): a missing secret was reported as one that 'arrived late'; now says no usable key was available (missing or too late). (cubic's TLS 1.2 both-directions P1 was already fixed in 683b609.) Verified on stable 1.97: clippy --all-targets, 381 tests, fmt, and live 1.2/1.3 + replay all still decrypt.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/tls/keylog.rs (1)
132-155: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCompare against the observed size, not the prior size.
If metadata sees 1,000 bytes,
last_sizeis 500, and a replacement race makesfrom_file()read 600 bytes, this condition accepts a non-superset and replaces existing secrets. Require the fresh read to be at least thesizeobserved before re-reading.Proposed fix
- Ok(mut fresh) if fresh.last_size >= self.last_size => { + Ok(mut fresh) if fresh.last_size >= size => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tls/keylog.rs` around lines 132 - 155, Update the acceptance guard in the keylog re-read logic to require fresh.last_size to be at least the observed size value, not self.last_size. Keep the existing secret-map swaps and last_size update for accepted reads, while continuing to reject smaller reads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/tls/keylog.rs`:
- Around line 132-155: Update the acceptance guard in the keylog re-read logic
to require fresh.last_size to be at least the observed size value, not
self.last_size. Keep the existing secret-map swaps and last_size update for
accepted reads, while continuing to reject smaller reads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3fc2ff2-364a-4f72-b6a5-aa63a0b7ffa1
📒 Files selected for processing (6)
README.mdsrc/protocol/http.rssrc/tls/handshake.rssrc/tls/keylog.rssrc/tls/mod.rssrc/tui/event.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- src/tls/handshake.rs
- src/tls/mod.rs
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…file Follow-up to the refresh() I/O fix (cubic P2): caching last_size on *any* read error meant a transient failure locked out refreshes until the file grew. Cache only when the file is genuinely over MAX_KEYLOG_SIZE (the real per-record I/O risk that won't self-resolve); leave transient or malformed-file errors un-cached so a later good read can recover. Stable 1.97: clippy, 381 tests, fmt clean.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tls/keylog.rs (1)
53-77: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the keylog read before the size check.
std::fs::readloads the entire file into memory beforeMAX_KEYLOG_SIZEruns, so an oversized keylog can still exhaust the capture process. Read at mostMAX_KEYLOG_SIZE + 1bytes, keep the currentNotFoundhandling, and reject larger input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tls/keylog.rs` around lines 53 - 77, Update the keylog loading flow in the visible read block to avoid using std::fs::read for unbounded input: open the path and read at most MAX_KEYLOG_SIZE + 1 bytes, preserving the existing NotFound return behavior and other read-error context. Use the bounded byte count to reject input larger than MAX_KEYLOG_SIZE before parsing or storing it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tls/keylog.rs`:
- Around line 150-159: The oversized-file suppression logic in the keylog
refresh path must use explicit state rather than encoding suppression in
last_size. Update the relevant keylog state and refresh logic so persistent
oversized append-only files are not fully reread on growth, while shrinking or
returning within MAX_KEYLOG_SIZE clears suppression and allows valid contents to
refresh; add regression tests covering both transitions.
---
Outside diff comments:
In `@src/tls/keylog.rs`:
- Around line 53-77: Update the keylog loading flow in the visible read block to
avoid using std::fs::read for unbounded input: open the path and read at most
MAX_KEYLOG_SIZE + 1 bytes, preserving the existing NotFound return behavior and
other read-error context. Use the bounded byte count to reject input larger than
MAX_KEYLOG_SIZE before parsing or storing it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+150
to
+159
| // Suppress future full re-reads only for a persistently oversized | ||
| // file — the real per-record I/O risk, and a condition that won't | ||
| // fix itself below this size. A transient read error or a malformed | ||
| // (non-UTF-8) file is left un-cached, so a later good read can still | ||
| // recover rather than being locked out until the file grows. | ||
| Err(_) => { | ||
| if size > Self::MAX_KEYLOG_SIZE { | ||
| self.last_size = size; | ||
| } | ||
| false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use separate state for oversized-file suppression.
Assigning last_size = size on an oversized error causes a persistent append-only oversized file to be fully re-read on every subsequent growth. Conversely, after rotation or truncation to a valid smaller file, size <= last_size prevents refresh from retrying, so newly written secrets are missed. Track an explicit oversized state, clear it when the source shrinks or returns within the limit, and add regression tests for both cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tls/keylog.rs` around lines 150 - 159, The oversized-file suppression
logic in the keylog refresh path must use explicit state rather than encoding
suppression in last_size. Update the relevant keylog state and refresh logic so
persistent oversized append-only files are not fully reread on growth, while
shrinking or returning within MAX_KEYLOG_SIZE clears suppression and allows
valid contents to refresh; add regression tests covering both transitions.
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tls/keylog.rs">
<violation number="1" location="src/tls/keylog.rs:156">
P2: A malformed UTF-8 keylog below 50 MiB now causes a full file read on every TLS record that misses keys because this branch leaves `last_size` unchanged. A retry state with backoff or file-change tracking would preserve recovery from transient errors without creating unbounded per-record I/O.</violation>
<violation number="2" location="src/tls/keylog.rs:156">
P2: Setting `self.last_size = size` for an oversized file creates a lockout after log rotation: if the keylog file is later rotated or truncated to a smaller (valid) file, the `size <= self.last_size` guard in the refresh path will prevent reading the new content until it grows past the stale cached size. Additionally, a persistently growing oversized file will still trigger a full re-read on every size increase since each growth passes the `size > last_size` check.
Consider tracking the oversized state as a separate boolean flag (e.g., `is_oversized: bool`) that is cleared when the file shrinks below `MAX_KEYLOG_SIZE`, so rotation to a valid file is correctly detected and retried.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // (non-UTF-8) file is left un-cached, so a later good read can still | ||
| // recover rather than being locked out until the file grows. | ||
| Err(_) => { | ||
| if size > Self::MAX_KEYLOG_SIZE { |
There was a problem hiding this comment.
P2: A malformed UTF-8 keylog below 50 MiB now causes a full file read on every TLS record that misses keys because this branch leaves last_size unchanged. A retry state with backoff or file-change tracking would preserve recovery from transient errors without creating unbounded per-record I/O.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tls/keylog.rs, line 156:
<comment>A malformed UTF-8 keylog below 50 MiB now causes a full file read on every TLS record that misses keys because this branch leaves `last_size` unchanged. A retry state with backoff or file-change tracking would preserve recovery from transient errors without creating unbounded per-record I/O.</comment>
<file context>
@@ -147,11 +147,15 @@ impl KeyLog {
+ // recover rather than being locked out until the file grows.
Err(_) => {
- self.last_size = size;
+ if size > Self::MAX_KEYLOG_SIZE {
+ self.last_size = size;
+ }
</file context>
| // (non-UTF-8) file is left un-cached, so a later good read can still | ||
| // recover rather than being locked out until the file grows. | ||
| Err(_) => { | ||
| if size > Self::MAX_KEYLOG_SIZE { |
There was a problem hiding this comment.
P2: Setting self.last_size = size for an oversized file creates a lockout after log rotation: if the keylog file is later rotated or truncated to a smaller (valid) file, the size <= self.last_size guard in the refresh path will prevent reading the new content until it grows past the stale cached size. Additionally, a persistently growing oversized file will still trigger a full re-read on every size increase since each growth passes the size > last_size check.
Consider tracking the oversized state as a separate boolean flag (e.g., is_oversized: bool) that is cleared when the file shrinks below MAX_KEYLOG_SIZE, so rotation to a valid file is correctly detected and retried.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tls/keylog.rs, line 156:
<comment>Setting `self.last_size = size` for an oversized file creates a lockout after log rotation: if the keylog file is later rotated or truncated to a smaller (valid) file, the `size <= self.last_size` guard in the refresh path will prevent reading the new content until it grows past the stale cached size. Additionally, a persistently growing oversized file will still trigger a full re-read on every size increase since each growth passes the `size > last_size` check.
Consider tracking the oversized state as a separate boolean flag (e.g., `is_oversized: bool`) that is cleared when the file shrinks below `MAX_KEYLOG_SIZE`, so rotation to a valid file is correctly detected and retried.</comment>
<file context>
@@ -147,11 +147,15 @@ impl KeyLog {
+ // recover rather than being locked out until the file grows.
Err(_) => {
- self.last_size = size;
+ if size > Self::MAX_KEYLOG_SIZE {
+ self.last_size = size;
+ }
</file context>
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
1 participant
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
A curation + fix-up pass on netgrep after ~4 months cold, so it holds up if someone reads or runs it. The audit turned into a real bug hunt.
The headline: netgrep could not decrypt real-world TLS 1.2 traffic at all. It read the negotiated TLS version from the ClientHello's
supported_versions(the client's offer), but every modern client advertises 1.3 there even when the server negotiates down to 1.2. netgrep locked onto 1.3, looked up empty 1.3 secrets for a session whose keylog held a 1.2 master secret, and silently decrypted nothing. Since essentially all real 1.2 sessions come from clients that also offer 1.3, the "TLS 1.2 decryption — tested" claim held for hand-built vectors and broke on the wire. The synthetic 1.2 test missed it because its ClientHello carried nosupported_versionsextension.Changes
tests/fixtures/generate.sh), with a negative control proving the marker is genuinely encrypted.clippy -D warningson current stable.Verification
cargo build --release,cargo fmt --check,cargo clippy --all-targets -- -D warningsall clean on rustc 1.94.1.cargo test: 378 pass (was 374; +1 regression unit test, +3 e2e).0 matches→1 match; TLS 1.3 unaffected.Test plan
cargo test --test tls_e2edecrypts both fixtures and confirms the negative control.sudo -v && tests/fixtures/generate.sh.Summary by Sourcery
Fix TLS session version negotiation and add real-world TLS decryption coverage while refreshing docs and lint compliance.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by cubic
Fix TLS 1.2 decryption by negotiating the version from ServerHello and harden live TLS decryption with safe keylog refresh, per-direction key installs, and correct epoch/sequence handling. Keep the code clippy-clean on stable Rust 1.97.
Bug Fixes
SSLKEYLOGFILEon growth and merge later TLS 1.3 application secrets into partial entries; tolerate a missing file at startup; only accept re-reads at least as large as the current cache; suppress future re-reads only for a persistently oversized file (transient read errors no longer lock out refreshes).Chores
cargo clippy -- -D warningson stable Rust 1.97 with minor HTTP/TUI formatting fixes.Written for commit 69ecae5. Summary will update on new commits.
Summary by CodeRabbit