Step 3: Ingest and normalization (#17) - #31
Conversation
`normalize(capture, workdir)` produces the one normalized pcap every downstream stage reads (spec §2.4), following spec §8's order of operations exactly: - format sniffed by magic bytes, never by extension — all four pcap magics, pcapng, and gzip, whose payload is sniffed again after decompression because gzip says nothing about what it wraps; - validation by flabel's own record-header walk, since no tool in the dependency set reports a truncation *offset*; driven by seeks, so cost scales with packet count rather than capture size; - truncated pcap proceeds as `partial` with the offset recorded; the incomplete tail record is dropped, because Zeek 8.0.9 exits 1 on a short final record and "proceed as partial" is otherwise impossible; - truncated pcapng is a hard failure carrying the `editcap` repair command — a partial block cannot be converted safely; - pcapng converts with `editcap -F pcap`; a capture mixing link types keeps the dominant type by packet count (ties to the lowest link type, for reproducibility) and records `discarded_link_types` / `discarded_packets` as `partial`; - every transformation recorded in `normalization`; - a failure leaves no output directory (spec §13). `tests/fixtures/make_awkward.py` extends `make_canary.py` with the awkward inputs — truncated pcap and pcapng, multi-datalink pcapng, bad header, byte-order and precision variants, gzipped versions. Nothing is committed as a binary: tests generate fixtures into `tmp_path`, and the generator reproduces the committed `benign.pcap` byte for byte. 42 tests. `editcap` and `capinfos` run for real, with `capinfos` used as an independent oracle for packet counts and encapsulation; no network. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ark selection (#17) Review of PR #31. Seven findings, all fixed in the three owned files. 1. Atomicity hole. `_sha256` and `stat` ran after the try/except, so a capture that became unreadable in that window left `normalized.pcap` behind — the partial output spec §13 forbids. Both moved inside, along with building the result. Cleanup also now removes every directory `mkdir(parents=True)` created, not just the leaf. 2. Error messages named a deleted file. After decompression the file being read is a temp that cleanup unlinks, so a truncated `.pcapng.gz` told the operator to run `editcap` on a path that no longer existed. Messages now carry a `subject` (the operator's file, marked "(decompressed)") and the repair command names the original — `editcap` reads gzipped captures directly, verified. Test added. 3. Corrupt pcapng escaped as `struct.error`, not a `FlabelError`. Block bodies are now length-checked against each type's mandatory fields before unpacking, and the walk has an outer `struct.error` guard. Tests: bad `total_length`, oversized packet block, trailer mismatch, undefined interface, block too short for its own fields, short pcap file header, and a big-endian pcapng (the `">"` branch had no fixture). 4. The 16 MiB bound was applied to every block type, so a capture with a large decryption secrets block (Wireshark embeds TLS key logs) was called corrupt. Renamed `MAX_PACKET_BYTES` and applied to packet blocks only; bodies are peeked at and skipped rather than read, so a 17 MiB block is not pulled into memory. Test uses a real 17 MiB DSB. 5. Range-based selection replaced with tshark. Ranges only stayed small while link types arrived contiguously; a real `dumpcap -i eth0 -i lo` capture interleaves per packet. Now `tshark -Y 'frame.interface_id==N || ...' -F pcapng` then `editcap -F pcap -T <encap>`. Note the reviewer's one-shot `tshark -F pcap` does not work: tshark picks the output encapsulation from the interface blocks, exactly as editcap does, so it refuses a per-packet-encap file even with a filter — hence two steps and the surviving `-T`. Filter is a disjunction because several interfaces can share the dominant link type; interleaved fixture added, and it asserts the kept *frames*, not just the count. tshark-dependent tests skip with a clear reason while it is absent from the CI container. 6. Fabricated `ToolFailure` in `_verify_converted` had `argv=()` and `exit_code=0`. Now carries the real argv and `None`; `_run_editcap` generalised to `_run_tool`, and `EditcapError` renamed `ConversionError` since tshark can raise it too. 7. `make_awkward.py` no longer defaults its outdir — the documented command could write capture blobs into the repo, and `.gitignore` does not cover `*.gz`. Missing argument now exits 2 with usage. 176 passed, 8 skipped (was 162/8); 4 skip additionally where tshark is absent. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review findings addressed in
New fixtures: interleaved multi-datalink (3 interfaces, 2 of them Ethernet, alternating), big-endian pcapng, short pcap header, bad block length, trailer mismatch, undefined interface, short block body, large decryption secrets block, gzipped truncated pcapng. Requested notes for the spec §10 amendment — what the code actually does:
Also unchanged and still open for step 8: spec §10's run block has one |
Closes #17. PLAN.md step 3, built test-first against
docs/spec.md§8 "Ingest".What this builds
src/flabel/ingest.py—normalize(capture: Path, workdir: Path) -> NormalizedCapture, in spec §8's order and no other:.pcap.gzholding a pcapng is ordinary.packets_readandtruncated_at_offset. Driven by seeks and header reads, so cost scales with packet count, not capture size.CaptureError, no output.input_status = "partial"with the offset recorded.editcaprepair command.editcap -F pcap. Mixed link types: dominant type by packet count is kept, the rest are discarded intodiscarded_link_types/discarded_packetswithinput_status = "partial".normalization.tests/fixtures/make_awkward.pyextendsmake_canary.py: truncated pcap (mid-payload and mid-record-header), truncated pcapng, multi-datalink pcapng in four variants, bad header, corrupt gzip, the four pcap byte-order/precision encodings, and gzipped versions of each. Nothing is committed as a binary — tests generate intotmp_path, andwrite_plain_pcapreproduces the committedbenign.pcapbyte for byte, which is asserted.Decisions that go beyond the plan's wording
Each is a consequence of what the tools actually do, verified rather than assumed.
1. A truncated pcap's incomplete tail record is dropped from the normalized copy. The plan says "proceed,
input_status = partial". That is impossible if the bytes pass through untouched: on the pinned toolchain, Zeek 8.0.9 exits 1 withfatal error: failed to read a packet ... truncated dump file, and Suricata 8.0.6 logspcap: error code -1 truncated dump file. Passing it through would convert a loss condition the spec says to report into a tool failure that ends the run.packets_readandtruncated_at_offsetstate exactly what was dropped and where, so the trim removes bytes, not information. A test assertscapinfosrejects the input and accepts the output.2. Multiple link types are detected from our own walk, not from
editcap's complaint. Spec §8 step 7 says "if it reports multiple link types".editcapreports it as an English error string (The capture file being read can't be written as a "pcap" file.), and branching on parsed tool prose is fragile across versions. The offset-accurate walk already knows every interface's link type and packet count, so the decision is made from data.3. Discarding link types requires
editcap -T <encap>. Verified on Wireshark 4.6.7:editcappicks the output encapsulation from the interface description blocks, not from the packets that survive-rselection — so a pcapng that ever declared two link types still refuses to become a pcap even after the minority packets are dropped, whether the selection happens in one step or two. The kept packets are selected as 1-based ranges (-r) and the encapsulation is asserted with-T. Since-Tis a claim about the kept packets, it is only ever passed a name for the type those packets actually carry:LINK_TYPESmaps libpcap link types toeditcap's own names, and a dominant type absent from that table is a hardCaptureErrorwith instructions rather than a mislabelled capture. Tested withUSER0(147).4. Ties in "dominant by packet count" break to the lowest link type. A tie has no meaningful winner, so the rule is chosen to be stable: Goal 2 needs two runs over one capture to keep the same packets, which dict order would not guarantee. Asserted by comparing two runs, not by inspecting the rule.
5.
editcap's output is re-walked and its packet count checked. Exit 0 is not evidence that the requested packets were written, and a silently short conversion would surface downstream as flows that never existed rather than as an ingest failure.6.
EditcapError(ToolError)carries theToolFailure. Spec §11 wants a tool failure intool_failures[]and a hard failure, butNormalizedCapturehas no field for one (anormalizethat fails returns nothing at all), andmodels.py/errors.pyare read-only in this step. So the record travels on the exception:cli.pyreads.failure, and anything catching plainToolErrorstill gets exit 1. Flagged for step 9 — see open questions.Open questions, not guessed at
sha256/bytes_totaldescribe the input as handed to us, so a compressed capture hashes as the compressed file, consistent withcapture_formatreportingpcap.gz. MeanwhileNormalizedCapture.pathis the derived file. Spec §10's run block has a singleinput.path, so step 8 must decide whether that key carriesoriginal_pathorpath— one of the two facts is otherwise unrecorded inlabels.json. My reading:original_path, sincesha256,bytesandformatall describe the original.truncated_at_offsetis an offset into the uncompressed capture. A gzip member has no record boundaries, so nothing else would be actionable. Worth one clause indocs/spec.md§10 or §11 if the field is documented for consumers.tests/fixtures/README.mddoes not mentionmake_awkward.pyand still describes only the two canaries. Not edited (read-only in this step); it should gain a short section on the generated awkward fixtures.NormalizedCaptureandToolFailureas landed in step 2 cover everything here.How it was tested
uv run pytest -q→ 162 passed, 8 skipped (baseline before this branch: 120 passed, 8 skipped; no change to the skip set).uv run pytest -q --require-tool-testsgreen, so the tool layer genuinely executed.uv run ruff format . && uv run ruff check .clean.42 new tests.
editcapandcapinfosrun for real (14 markedrequires_tools), withcapinfosused as an independent oracle for packet counts and encapsulation — checking flabel's walk against flabel's own reader would prove only that the code agrees with itself. No test touches the network, and one asserts that at runtime by makingsocket.socketraise for the duration of anormalize.Coverage of the plan's required outcomes:
partialwith the correct offsetCaptureError, no outputeditcap; workdir does not existCaptureError, no outputcapinfosconfirms count and encapsulation; dominance re-tested with the majority flipped; tie-break re-tested across two runsworkdirholding onlynormalized.pcapPATH(spec §11's fault injection) and a real non-zeroeditcapexitAlso verified by hand, outside the suite: Zeek 8.0.9 reads both the trimmed truncated output and the split multi-datalink output cleanly (
conn.logwritten, no errors).🤖 Generated with Claude Code