feat(demo/reveng): add real pcap examples, fetch workflow, and automated tshark tests - #52
Conversation
Agent-Logs-Url: https://github.com/protocollab-co/protocollab/sessions/794328ae-007a-423b-84eb-7d6dcf0ec8b7 Co-authored-by: cherninkiy <2933630+cherninkiy@users.noreply.github.com>
Agent-Logs-Url: https://github.com/protocollab-co/protocollab/sessions/794328ae-007a-423b-84eb-7d6dcf0ec8b7 Co-authored-by: cherninkiy <2933630+cherninkiy@users.noreply.github.com>
…y, updated Makefile and README Agent-Logs-Url: https://github.com/protocollab-co/protocollab/sessions/d207c835-b2b6-48b9-af96-6ef77cce9905 Co-authored-by: cherninkiy <2933630+cherninkiy@users.noreply.github.com>
Agent-Logs-Url: https://github.com/protocollab-co/protocollab/sessions/d207c835-b2b6-48b9-af96-6ef77cce9905 Co-authored-by: cherninkiy <2933630+cherninkiy@users.noreply.github.com>
Agent-Logs-Url: https://github.com/protocollab-co/protocollab/sessions/46f1a026-c041-475f-a3d2-0f8a51c420c6 Co-authored-by: cherninkiy <2933630+cherninkiy@users.noreply.github.com>
…ated comment Agent-Logs-Url: https://github.com/protocollab-co/protocollab/sessions/46f1a026-c041-475f-a3d2-0f8a51c420c6 Co-authored-by: cherninkiy <2933630+cherninkiy@users.noreply.github.com>
cherninkiy
left a comment
There was a problem hiding this comment.
Review
Overall the PR is well-structured and covers an important gap: there was no way to validate dissector correctness without running Wireshark manually. The CI-safe skip, stdlib-only make_samples.py, and the --force flag on fetch_samples.sh are all good design choices.
Found a few items to address before merging:
Bugs / correctness
1. Docstring mismatch in make_tls_sni_analysis (make_samples.py)
The function docstring says packet #3 has sni_length=32, but the code uses long_sni_claimed_length = 39, which matches expected.txt and test_dissectors.sh. The docstring is wrong and will mislead anyone trying to regenerate the file manually:
# docstring says:
# #3 sni_length=32 ...
# code does:
long_sni_claimed_length = 39Fix: update the docstring to sni_length=39.
Reliability / portability
2. Stale download URL in fetch_samples.sh
The primary URL for ipv4frags.pcap points to the old MoinMoin Wireshark wiki (wiki.wireshark.org/SampleCaptures?action=AttachFile...), which was retired when Wireshark migrated to GitLab. The first attempt will always fail silently and fall through to the GitHub mirror. Consider swapping the order (use the GitHub mirror as primary) or removing the dead URL.
3. wc -l whitespace trimming in assert_count (test_dissectors.sh)
count=$(tshark ... | wc -l | tr -d ' ')On macOS, wc -l pads with leading spaces; tr -d ' ' works but removes all spaces. More portable:
count=$(tshark ... | wc -l)
count=${count// /}Nits
4. session_key instance is never asserted
session_id.yaml defines both session_key (raw XOR) and session_with_service. Only the latter is tested. A single assert_values for session_key would confirm symmetry independently of port masking.
5. Binary pcap blobs
Downstream reviewers should run make make-samples && git diff --stat to verify blobs are unchanged rather than inspecting the binary diff directly. Worth a note in CONTRIBUTING or the README.
ReviewOverall the PR is well-structured and covers an important gap: there was no way to validate dissector correctness without running Wireshark manually. The CI-safe tshark skip, stdlib-only A few items to address before merging: Bug1. Docstring mismatch in The docstring says packet #3 has # docstring (wrong):
# #3 sni_length=32 sni_name=<random 32-char> → Long/Anomaly (>20)
# code (correct):
long_sni_claimed_length = 39Fix: change the docstring line to Reliability2. Stale primary URL in "https://wiki.wireshark.org/SampleCaptures?action=AttachFile&do=get&target=ipv4frags.pcap"The old MoinMoin Wireshark wiki was retired when the project moved to GitLab. This URL always fails silently and wastes a 30 s timeout before falling through to the GitHub mirror. Recommend swapping the two URLs so the GitHub mirror is primary. 3. On macOS, count=$(tshark ... | wc -l)
count=${count// /} # bash parameter expansion, no extra subprocessNit4.
5. Binary pcap blobs in the diff The committed |
- fix tls_sni_analysis docstring to match claimed SNI length (39) - use GitHub mirror as primary ipv4frags download URL - normalize wc output via bash expansion in test_dissectors.sh - add explicit session_key assertions in session_id tests - update README URLs and add binary pcap review guidance
Extends
demo/revengwith a real-traffic download workflow, a reproducible synthetic sample generator, and a full automated tshark test suite validating all four Lua dissectors.What
Real pcap download workflow (
tools/fetch_samples.sh)ipv4frags.pcap(Wireshark SampleCaptures) →ip_scoped/real_sample.pcap+session_id/real_sample.pcapimap-ssl.pcapng(Lekensteyn/wireshark-notes) →tls_weak_cipher/real_sample.pcapng+tls_sni_analysis/real_sample.pcapngcurlhints if URLs are unreachable; real samples never committed (*.pcapng,real_sample.*added to.gitignore)Synthetic sample regeneration (
tools/make_samples.py)sample.pcapfiles byte-for-byte from spec; idempotent viamake make-samplesAutomated tshark tests (
tools/test_dissectors.sh)tsharkis absent — CI-saferesults/*.luaviagenerate_all.shif missingip_scopedsrc_scope/dst_scopevalues per frame; 8 display-filter counts;src_lan/dst_lanbool filterssession_idsession_with_servicevalues per frame; 2 filter counts; A↔B and C↔D symmetry checkstls_weak_cipherhas_weak_cipherfilter counts (1 weak / 1 safe);is_client_hellocounttls_sni_analysissni_categoryvalues per frame; category filter counts;is_anomalybool filterMakefile targets added:
fetch-samples,make-samples,test: generateMinor fixes:
0x00FFlabel corrected toTLS_FALLBACK_SCSVintls_weak_cipher/expected.txt; SNI packet#3sni_lengthcorrected to39intls_sni_analysis/expected.txtWhy
expected.txtdescribed expected behaviour but required manual Wireshark inspectionsample.pcapfiles from scratchValidation
make_samples.pyproduces byte-identical output to the committedsample.pcapfiles (verified viagit diff— zero binary changes)bash -nsyntax checktest_dissectors.shskips safely whentsharkis absentChecks
Notes
set -eis intentionally absent fromtest_dissectors.sh: tshark exits non-zero when a display filter matches 0 frames, which would abort mid-suite; failures are accumulated via_fail()insteadreal_sample.*) so they cannot accidentally shadow the committed syntheticsample.pcap