Repository navigation
fix(toolkit): key tunnelled TCP by the innermost IP header (#1581) - #1605
Merged
Merged
Conversation
JarryShaw
added
fix
Contributor
|
Coverage: 89.72% (unit tier, Python 3.14,
Per-file detail: the |
Owner
Author
|
Verdict: NEEDS CHANGES at The four-engine claims hold. Across 4in4, 6in4, 4in6, 6in6, 4in4in4 and 4in6in4, default, dpkt, scapy and pypcapfile give identical flow labels, with 158 field comparisons all matching. Plain IPv4 and IPv6, options, fragments, VLAN tags and snaplen cuts are unchanged from
I re-measured items 1 and 2 myself on |
JarryShaw
added
review: needs-changes
TCP reassembly and flow tracing keyed a segment by the frame's first IP layer, which in an IP-in-IP tunnel is the tunnel's: connections through one tunnel sharing a port pair merged into one flow. - pcap: tcp_segment takes the IP layer nearest above TCP; the #1518 raw fall-back applies to that layer, tunnelled or not - dpkt: walk IP-in-IP to the innermost packet (it found no TCP at all), entering protocols 4 and 41 only, as the default engine does - scapy, pyshark: the IP layer nearest above TCP, not the first one - pypcapfile: walk tunnels, decode an innermost IPv4 and read its TCP; the 'tunnel' warning is gone, TCP over IPv6 in a tunnel warns as such - docs: pypcapfile tunnel handling Toolkit tests pass file by file on 3.14 and 3.11; engine agreement passes on both.
JarryShaw
force-pushed
the
fix/tunnelled-tcp-flow-key
branch
from
October 10, 2026 16:14
e05e0c0 to
143d691
Compare
JarryShaw
added
review: pending
Owner
Author
|
Verdict: GOOD TO GO at All three round-1 items are fixed. I re-measured the first two myself.
The review confirmed the rest:
Non-blocking:
|
JarryShaw
added
review: good-to-go
This was referenced Oct 10, 2026
Closed
JarryShaw
removed
the
review: good-to-go
This was referenced Oct 10, 2026
Merged
3 of 13 tasks
JarryShaw
added a commit
that referenced
this pull request
Oct 14, 2026
Third-party engines are best effort (#1687), so the pyshark code that exists only to match the default engine comes out: - revert #1686, #1670 and #1642: the second tshark pass that read a frame's octets, the link type read from the capture file, the 64-layer bound, reading past tshark's 802.1Q limit, and keying a source-routed segment by ip.cur_rt - revert #1613's tshark preferences that turned IP reassembly off, with its FileCapture assertion in test_runtime_engines.py and its sentence in the engines index - keep #1613's partial-TCP-header guard and #1605's keying by the IP header that carries the segment - re-land #1671's crash fix: a TCP layer whose IP layer lacks ip.src or ip.dst is not traced, rather than raising AttributeError - delete the three test modules those reverts added and the agreement checks in test_pyshark_unit.py; the #1611 and #1671 checks assert constants The pyshark test modules, test_runtime_engines.py, test_engine_runtime.py and the project guards pass on 3.11 (tshark 4.6.9) and 3.14.
This was referenced Oct 14, 2026
JarryShaw
added a commit
that referenced
this pull request
Oct 14, 2026
…1721) Third-party engines are best effort (#1687), so the pypcapfile code that exists only to match the default engine comes out: - revert #1682, #1600, #1575 and #1535, and the pypcapfile hunks of #1655, #1608 and #1605: reading loopback and raw-IP link types, the frame layer bound, the VLAN and tunnel walks, keying tunnelled TCP by the innermost header, stepping over AH and extension headers, and checking options with the default engine's parsers - keep the helpers the dpkt and scapy adapters import, unchanged, until their own reverts drop them; their docstrings no longer say pypcapfile asks the default engine's parsers too - re-land #1576, #1577, #1590 and #1591 as guards in _network and _transport, #1513's once-per-capture warning for TCP straight over IPv6, and #1667's packet2dict fix: an undecoded frame is reported as its octets, not as hex text twice as long - keep #1569's record reader and #1690's packet2timestamp - delete the three test modules those reverts added, the pypcapfile timestamp agreement class, and #1682's link-type warning test; the re-landed guards' tests assert constants - drop what the reverts removed from the docs, READ_LINKTYPES included, and say that TCP behind a VLAN tag, in a tunnel or behind an extension header (an IPv6 one, or AH in IPv4) is now left out without a warning The pypcapfile modules, test_new_engine_parity_runtime.py, test_engine_runtime.py, the timestamp module and the project guards pass on 3.11 (pypcapfile 0.12.0) and 3.14.
JarryShaw
added a commit
that referenced
this pull request
Oct 14, 2026
Third-party engines are best effort (#1687), so the scapy code that exists only to match the default engine comes out: - revert #1683, #1663, #1652 and #1631, and the scapy hunks of #1614: the link-type and loopback-family walk, the frame and extension-header bounds, the IPv6 chain walk over the octets, the stops at GRE, VXLAN, PPPoE and the other layers the default engine leaves raw and at ICMP quotes, and the IPv4 header checks - keep #1624's walk (TCP behind AH or an IPv6 extension header in IPv4, and the later-fragment stop), #1605's keying by the carrying IP header and #1589's trailer cut; the walk calls _walk_extension_headers, which the pypcapfile revert keeps - re-land as guards: a zero-length frame is no IPv4 input and has no packet2dict octets (#1676); IPv4 in IPv6 ends at the Payload Length (#1623); a packet built by hand is read off its octets (#1676); ipv6_reassembly takes only its own IPv6 layer's Fragment header, and none in a later IPv4 fragment - delete the four scapy agreement modules and test_scapy_unit's default-engine checks; the guards' tests assert constants - docs: scapy applies the extension-header bound only behind IPv4 and the frame-layer bound nowhere, and its silent gaps are listed where gaps are said to be announced The scapy toolkit and engine modules, the shared modules that run scapy, and the project and tier guards pass on 3.14 (scapy 2.7.0) and 3.11 (scapy 2.8.0).
3 of 13 tasks
JarryShaw
added a commit
that referenced
this pull request
Oct 14, 2026
Third-party engines are best effort (#1687), so the scapy code that exists only to match the default engine comes out: - revert #1683, #1663, #1652 and #1631, and the scapy hunks of #1614: the link-type and loopback-family walk, the frame and extension-header bounds, the IPv6 chain walk over the octets, the stops at GRE, VXLAN, PPPoE and the other layers the default engine leaves raw and at ICMP quotes, and the IPv4 header checks - keep #1624's walk (TCP behind AH or an IPv6 extension header in IPv4, and the later-fragment stop), #1605's keying by the carrying IP header and #1589's trailer cut; the walk calls _walk_extension_headers, which the pypcapfile revert keeps - re-land as guards: a zero-length frame is no IPv4 input and has no packet2dict octets (#1676); IPv4 in IPv6 ends at the Payload Length (#1623); a packet built by hand is read off its octets (#1676); ipv6_reassembly takes only its own IPv6 layer's Fragment header, and none in a later IPv4 fragment - delete the four scapy agreement modules and test_scapy_unit's default-engine checks; the guards' tests assert constants - docs: scapy applies the extension-header bound only behind IPv4 and the frame-layer bound nowhere, and its silent gaps are listed where gaps are said to be announced The scapy toolkit and engine modules, the shared modules that run scapy, and the project and tier guards pass on 3.14 (scapy 2.7.0) and 3.11 (scapy 2.8.0).
JarryShaw
added a commit
that referenced
this pull request
Oct 14, 2026
Third-party engines are best effort (#1687), so the dpkt code that exists only to match the default engine comes out: - revert #1662, #1655, #1624, #1622, #1614 and the dpkt hunks of #1605: the octet walk and its link-type stash, IPv6 in IPv4 and IPv4 in IPv6, refusing MPLS and ISL, the tunnel and extension-header walks, the IPv4 header checks and the layer bounds - keep #1690's timestamps, #1594's parser choice, #1621 and #1653 - re-land as guards: a layer dpkt's own lengths misplace (#1646) is re-serialised, not refused with ProtocolError; a Total Length below the IHL keeps the header whole (#1616), and a header the capture cuts is no input; packet2dict and packet2chain take a str payload and a raw frame - delete the modules #1662 and #1655 added; the 1615/1616 module keeps only what its #1616 tests use, as no scapy test imports it now; dpkt's comparisons with the default engine assert constants or the capture's octets, or are dropped - drop pypcapfile's VLAN_TAGS, VLAN_TAG_LEN and IPV6_EXTENSION_HEADERS, which only dpkt imported, and the 1668 module's _Agreement base, which nothing subclasses now - the docs drop the depth claims and name what dpkt leaves out silently The dpkt modules, the shared modules that run dpkt, tests/foundation/engines and the project guards pass on 3.14 and 3.11 (dpkt 1.9.8, pypcapfile 0.12.0).
JarryShaw
added a commit
that referenced
this pull request
Oct 14, 2026
Third-party engines are best effort (#1687), so the dpkt code that exists only to match the default engine comes out: - revert #1662, #1655, #1624, #1622, #1614 and the dpkt hunks of #1605: the octet walk and its link-type stash, IPv6 in IPv4 and IPv4 in IPv6, refusing MPLS and ISL, the tunnel and extension-header walks, the IPv4 header checks and the layer bounds - keep #1690's timestamps, #1594's parser choice, #1621 and #1653 - re-land as guards: a layer dpkt's own lengths misplace (#1646) is re-serialised, not refused with ProtocolError; a Total Length below the IHL keeps the header whole (#1616), and a header the capture cuts is no input; packet2dict and packet2chain take a str payload and a raw frame - delete the modules #1662 and #1655 added; the 1615/1616 module keeps only what its #1616 tests use, as no scapy test imports it now; dpkt's comparisons with the default engine assert constants or the capture's octets, or are dropped - drop pypcapfile's VLAN_TAGS, VLAN_TAG_LEN and IPV6_EXTENSION_HEADERS, which only dpkt imported, and the 1668 module's _Agreement base, which nothing subclasses now - the docs drop the depth claims and name what dpkt leaves out silently The dpkt modules, the shared modules that run dpkt, tests/foundation/engines and the project guards pass on 3.14 and 3.11 (dpkt 1.9.8, pypcapfile 0.12.0).
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.
Please follow the guide below
You will be asked some questions, please read them carefully and answer honestly
Put an
xinto all the boxes [ ] relevant to your pull request (like that [x])Use Preview tab to see how your pull request will actually look like
Searched for similar pull requests
Followed the coding style (
make pylint,make mypy,make isort)make testpasses, and a test case covers the changeAdded a changelog entry under
docs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visible — N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657What is the purpose of your pull request?
Tick the commit type your subject line carries.
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingrelease— bumps the version or rolls up a distributionchore— anything elseDescription of your pull request and other information
Closes #1581.
Re-measured on
c43e9318c(two connections, same port pair, one tunnel): default keyed all four of 4in4/6in4/4in6/6in6 by the outer addresses and merged them; scapy and pyshark the same except 4in6; dpkt found no TCP; pypcapfile left all four out with its tunnel warning.toolkit/pcap.py):tcp_segmentkeys by the IP layer nearest above TCP. The fix(engines): TCP trace flows disagree between engines on snapped frames and on IPv4 total_length=0 #1518 raw fall-back now applies to that layer, tunnelled or not._innermostwalks the tunnels and decodes an innermost IPv4 packet. The'tunnel'warning is removed. TCP whose innermost layer is IPv6 (6in4, 6in6) warns as TCP over IPv6.tests/toolkit/test_tunnelled_tcp_flow_key_1581_unit.py(default, dpkt, scapy, pyshark) and rewritten pypcapfile tunnel tests. They fail onmain, except three guards that pass there:test_no_segment_where_the_inner_header_carries_nonetest_detection_leaves_the_warnings_filters_and_the_logger_alonetest_protocol_0_is_no_tunnel, which fails against the unrestricted dpkt walk.KNOWN_FAILURESrow cited fix(toolkit): a TCP flow inside an IP-in-IP tunnel is keyed by the tunnel's outer addresses #1581.Not changed here: