scapy: load the layer registry, so the engine actually dissects

pcapkit/foundation/engines/scapy.py

@@ -9,6 +9,33 @@
 
 .. _Scapy: https://scapy.net
 
+.. note::
+
+   Constructing this engine imports :mod:`scapy.all`, which is what populates
+   `Scapy`_'s layer registries -- see :meth:`Scapy.__init__` for why anything
+   narrower silently returns undissected frames.
+
+   One side effect is worth knowing about in advance: :mod:`scapy.all` loads
+   :mod:`scapy.layers.dcerpc`, which reaches `Scapy`_'s TLS layer and there
+   triggers a ``CryptographyDeprecationWarning`` from :mod:`cryptography` about
+   finite-field Diffie-Hellman. It is emitted by any complete registry load, not
+   by :mod:`scapy.all` in particular, and it concerns a key-exchange code path
+   :mod:`pcapkit` never executes -- but it subclasses :exc:`UserWarning`, not
+   :exc:`DeprecationWarning`, so Python's default filters show it.
+
+   :mod:`pcapkit` deliberately does not filter it away. The warning is `Scapy`_'s
+   to emit and the consumer's to silence, on the same footing as every other
+   category (see :mod:`pcapkit.utilities.warnings`); hiding a third-party
+   deprecation notice from inside a constructor would suppress the only advance
+   warning that a future :mod:`cryptography` release breaks `Scapy`_'s TLS layer.
+   To silence it, filter it as usual::
+
+       import warnings
+
+       from cryptography.utils import CryptographyDeprecationWarning
+
+       warnings.filterwarnings('ignore', category=CryptographyDeprecationWarning)
+
 """
 from typing import TYPE_CHECKING, cast
 
@@ -39,10 +66,10 @@ class Scapy(Engine['ScapyPacket']):
 
     """
     if TYPE_CHECKING:
-        import scapy.sendrecv
+        import scapy.all
 
         #: Engine extraction package.
-        _expkg: 'scapy.sendrecv'
+        _expkg: 'scapy.all'
         #: Engine extraction temporary storage.
         _extmp: 'Iterator[ScapyPacket]'
 
@@ -61,7 +88,41 @@ class Scapy(Engine['ScapyPacket']):
     ##########################################################################
 
     def __init__(self, extractor: 'Extractor') -> 'None':
-        from scapy import sendrecv as scapy  # isort:skip
+        """Initialise the engine.
+
+        Args:
+            extractor: :class:`~pcapkit.foundation.extraction.Extractor` instance.
+
+        """
+        # NOTE: :mod:`scapy.all`, not :mod:`scapy.sendrecv`, and the difference is
+        # load-bearing rather than cosmetic. Scapy dispatches on two registries that
+        # exist only as *import side effects* of its layer modules: ``conf.l2types``,
+        # mapping a capture's link type onto a link-layer class, and the
+        # ``bind_layers`` payload table. ``scapy/__init__.py`` fills in neither, so
+        # importing just the sniffing submodule leaves both empty --
+        # :class:`~scapy.utils.PcapReader` then cannot map even link type 1 (plain
+        # Ethernet), writes ``unknown LL type [1]/[0x1]`` to stderr and returns every
+        # frame as one opaque :class:`~scapy.packet.Raw` layer. Nothing raises, so the
+        # engine used to deliver no dissection whatsoever and announce it only on
+        # stderr (#406).
+        #
+        # Naming the layer modules individually is not a cheaper way to the same
+        # place: it repairs the link layer and leaves the payload table short, so
+        # ``dhcp.pcapng`` still comes back as ``Ethernet / IP / UDP / Raw`` rather
+        # than ``... / UDP / BOOTP / DHCP options``. Enumerating them here would also
+        # go stale silently -- a link or payload type added by a later scapy would
+        # regress to ``Raw`` with no error -- whereas :mod:`scapy.all` defers to
+        # ``conf.load_layers``, which scapy itself keeps current. The cost is the
+        # layer loading, not the façade: importing :mod:`scapy.layers.all` alone
+        # measures the same, so there is no complete-but-cheaper option to prefer.
+        #
+        # This stays inside ``__init__`` rather than moving to module scope so that
+        # ``import pcapkit`` does not pay for it; only callers who actually select
+        # this engine do. And it is bound to :attr:`_expkg`, the attribute
+        # :meth:`run` calls ``sniff`` on, rather than left as a bare side-effecting
+        # import -- an import whose value is used cannot be dropped later as unused,
+        # which is how this class of bug comes back.
+        from scapy import all as scapy  # isort:skip
 
         self._expkg = scapy
         self._extmp = cast('Iterator[ScapyPacket]', None)
@@ -75,9 +136,11 @@ def __init__(self, extractor: 'Extractor') -> 'None':
     def run(self) -> 'None':
         """Call :func:`scapy.sendrecv.sniff` to extract PCAP files.
 
-        This method assigns :attr:`self._expkg <Scapy._expkg>`
-        as :mod:`scapy.sendrecv` and :attr:`self._extmp <Scapy._extmp>`
-        as an iterator from :func:`scapy.sendrecv.sniff`.
+        This method assigns :attr:`self._extmp <Scapy._extmp>` as an iterator
+        from :func:`scapy.sendrecv.sniff`, reached through
+        :attr:`self._expkg <Scapy._expkg>` -- which :meth:`__init__` binds to
+        :mod:`scapy.all`, since that is the import that populates the layer
+        registries ``sniff`` needs to dissect anything.
 
         Warns:
             AttributeWarning: If :attr:`self.extractor._exlyr <pcapkit.foundation.extraction.Extractor._exlyr>`

pcapkit/toolkit/scapy.py

@@ -15,6 +15,24 @@
 
    This module requires installed `Scapy`_ engine.
 
+.. note::
+
+   Several functions here import a `Scapy`_ layer module lazily, inside the
+   function body -- :func:`ipv6_reassembly` needs
+   :class:`scapy.layers.inet6.IPv6ExtHdrFragment`, for instance. Those imports are
+   for the *classes* they name and nothing more. They must not be mistaken for how
+   `Scapy`_'s layer registries get populated, even though they do populate them as
+   a side effect, because by the time any of these functions runs the engine has
+   already called ``sniff`` and every frame has already been dissected -- or not.
+
+   That distinction is what made #406 hard to see. Reaching
+   :func:`ipv6_reassembly` repaired ``conf.l2types`` mid-run, one call too late to
+   affect the frames being reassembled, so whether a process dissected correctly
+   depended on what had imported `Scapy`_ earlier. Populating the registries before
+   ``sniff`` is
+   :class:`~pcapkit.foundation.engines.scapy.Scapy`'s job, and it does it by
+   importing :mod:`scapy.all` in its constructor.
+
 """
 import ipaddress
 import time

tests/foundation/engines/test_scapy_engine.py

@@ -0,0 +1,157 @@
+"""Unit tests for :class:`pcapkit.foundation.engines.scapy.Scapy`'s import contract.
+
+`Scapy`_ builds its dissection tables as *import side effects* of its layer
+modules. Two matter here, and neither is populated by ``scapy/__init__.py``:
+
+* ``conf.l2types`` maps a capture's link type onto a link-layer class. Without it
+  :class:`~scapy.utils.PcapReader` cannot tell what it is reading, writes
+  ``unknown LL type [1]/[0x1]`` to stderr and returns each frame as one opaque
+  :class:`~scapy.packet.Raw` layer.
+* the ``bind_layers`` table maps each layer onto its payload. Without it a frame
+  dissects down as far as the loaded modules reach and then stops at ``Raw``.
+
+So this engine's *import statement* is load-bearing in a way an engine's usually
+is not, and #406 is what happens when it is wrong: the engine imported only
+:mod:`scapy.sendrecv`, and every frame of every capture came back as ``Raw`` --
+no exception, no :mod:`pcapkit` warning, and a stderr line from `Scapy`_ that a
+library consumer never sees. ``follow_tcp_stream`` quietly reported no streams.
+
+**Why these tests use a subprocess.** The tables live on `Scapy`_'s process-wide
+``conf`` singleton, and once any module has populated them they stay populated: no
+public API resets them, and ``sys.modules`` surgery does not un-run a
+``bind_layers`` call. So an in-process assertion here would pass whenever
+something earlier in the run had imported a `Scapy`_ layer -- which is exactly
+what happened before #406 was found. :mod:`tests.toolkit.test_scapy_unit` imports
+:mod:`scapy.layers.l2`, :mod:`~scapy.layers.inet` and :mod:`~scapy.layers.inet6`
+to build its fixtures, so ``pytest tests/toolkit tests/interface/test_misc.py``
+saw correct dissection while ``pytest tests/interface/test_misc.py`` alone did
+not. A fresh interpreter is the only place the engine's own import can be held
+responsible for the outcome, so that is where it is asserted.
+
+The end-to-end behaviour these guard is asserted through the public interface in
+:meth:`tests.interface.test_misc.FollowTCPStreamTests.test_scapy_engine_matches_the_default_engine`;
+this module pins the cause rather than the symptom.
+
+.. _Scapy: https://scapy.net
+
+"""
+from __future__ import annotations
+
+import importlib.util
+import json
+import pathlib
+import subprocess
+import sys
+import unittest
+
+from tests._support import sample_path
+
+#: Repository root, which the child interpreter needs on :data:`sys.path` to
+#: import the :mod:`pcapkit` under test rather than an installed copy.
+ROOT = pathlib.Path(__file__).resolve().parents[3]
+
+RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper')
+HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS)
+HAS_SCAPY = importlib.util.find_spec('scapy') is not None
+
+#: Marker the child prefixes its one JSON line with. `Scapy`_ writes to stderr on
+#: an unmapped link type and :mod:`pcapkit` warns on EOF, so the result is picked
+#: out by prefix rather than by assuming a clean stream.
+RESULT = '@@RESULT@@'
+
+#: Run in a fresh interpreter, with nothing imported that could have populated
+#: `Scapy`_'s tables beforehand. Reports what the engine produced *and* what the
+#: link-type table looked like afterwards, so a failure distinguishes "the engine
+#: did not import enough" from "the capture is not what the test thinks".
+CHILD = f'''
+import json, sys, warnings
+
+sys.path.insert(0, {str(ROOT)!r})
+warnings.simplefilter('ignore')
+
+import pcapkit
+
+extractor = pcapkit.extract(fin=sys.argv[1], engine='scapy', store=True, nofile=True)
+
+# Imported only now, and from ``scapy.config`` rather than ``scapy.all``, so that
+# reading the table cannot be what filled it in.
+from scapy.config import conf
+
+print({RESULT!r} + json.dumps({{
+    'frames': [type(frame).__name__ for frame in extractor.frame],
+    'chain': extractor.frame[0].summary(),
+    'has_tcp': any(frame.haslayer('TCP') for frame in extractor.frame),
+    'l2types': len(conf.l2types.num2layer),
+    'dlt1': getattr(conf.l2types.num2layer.get(1), '__name__', None),
+}}))
+'''
+
+
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
+@unittest.skipUnless(HAS_SCAPY, 'scapy not installed')
+class ScapyLayerRegistryTests(unittest.TestCase):
+    """What the Scapy engine dissects in an interpreter it did not warm up."""
+
+    def dissect(self, capture: str) -> dict:
+        """Extract ``capture`` with the Scapy engine in a fresh interpreter.
+
+        Args:
+            capture: Absolute path to the capture to read.
+
+        Returns:
+            The child's decoded report -- frame class names, frame 0's protocol
+            chain, whether any frame holds a TCP layer, and the size and DLT-1
+            entry of `Scapy`_'s link-type table.
+
+        """
+        completed = subprocess.run(
+            [sys.executable, '-c', CHILD, capture],
+            capture_output=True, text=True, timeout=300, check=False,
+        )
+        self.assertEqual(
+            completed.returncode, 0,
+            f'child interpreter failed\nstdout:\n{completed.stdout}\n'
+            f'stderr:\n{completed.stderr}',
+        )
+        for line in completed.stdout.splitlines():
+            if line.startswith(RESULT):
+                return json.loads(line[len(RESULT):])
+        self.fail(f'child printed no result line\nstdout:\n{completed.stdout}\n'
+                  f'stderr:\n{completed.stderr}')
+
+    def test_engine_populates_the_link_type_table_by_itself(self) -> None:
+        # The engine's own import has to be sufficient. Asserted on the table
+        # rather than on the frames so that a regression names its cause: an empty
+        # ``l2types`` is the missing import, nothing else.
+        report = self.dissect(sample_path('in.pcap'))
+
+        self.assertGreater(
+            report['l2types'], 0,
+            "scapy's conf.l2types was empty after the engine ran, so the engine "
+            'imported no layer module -- see #406: it must import scapy.all',
+        )
+        self.assertEqual(
+            report['dlt1'], 'Ether',
+            'link type 1 (Ethernet) did not map to scapy.layers.l2.Ether, so '
+            'PcapReader cannot dissect even an ordinary Ethernet capture',
+        )
+
+    def test_frames_are_dissected_not_returned_as_raw(self) -> None:
+        # in.pcap is six Ethernet frames: two ICMPv6 neighbour-discovery, three
+        # TCP, one UDP. Under #406 all six came back as ``Raw``.
+        report = self.dissect(sample_path('in.pcap'))
+
+        self.assertNotIn(
+            'Raw', report['frames'],
+            f"scapy returned undissected Raw frames from in.pcap: {report['frames']} "
+            '-- the layer registry was not populated; see #406',
+        )
+        self.assertEqual(report['frames'], ['Ether'] * 6)
+        self.assertTrue(report['chain'].startswith('Ether / IPv6 / ICMPv6ND_NS'),
+                        f"unexpected chain for frame 0: {report['chain']}")
+        self.assertTrue(report['has_tcp'],
+                        'no TCP layer found, though in.pcap holds three TCP frames')
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/integration/test_engine_parity.py

@@ -111,10 +111,20 @@ def test_scapy_engine_returns_the_same_link_layer_bytes(self) -> None:
         self.assertEqual(len(native.frame), len(foreign.frame))
         for number, (mine, theirs) in enumerate(zip(native.frame, foreign.frame), start=1):
             with self.subTest(frame=number):
-                # scapy does not know this capture's link type and hands back one
-                # opaque ``Raw`` layer holding the whole 60 octet frame, padded to
-                # the Ethernet minimum. Its first fourteen octets are the Ethernet
-                # header that the default engine parsed into a layer of its own.
+                # Re-serialising scapy's frame reproduces the whole 60 octet frame,
+                # padded to the Ethernet minimum, and its first fourteen octets are
+                # the Ethernet header the default engine parsed into a layer of its
+                # own. That is what makes this a bytes-level parity check: both
+                # engines account for every captured octet, whatever they call the
+                # layers they split it into.
+                #
+                # #406: this comment used to say scapy "does not know this capture's
+                # link type and hands back one opaque ``Raw`` layer". That was the
+                # missing-layer-registry bug, not a property of the capture --
+                # arp.pcap is link type 1 and scapy now dissects it as
+                # ``Ethernet / ARP / Padding``. The assertions below are unchanged
+                # and still pass, because ``bytes()`` of a dissected frame is the
+                # same octet string as ``bytes()`` of an undissected one.
                 captured = bytes(theirs)
                 self.assertEqual(len(captured), 60)
                 self.assertEqual(captured[:14], bytes(mine['Ethernet'].packet.header))

tests/integration/test_engine_runtime.py

@@ -43,14 +43,26 @@ def test_dpkt_engine_returns_dpkt_packets_and_toolkit_chain(self) -> None:
 
     @unittest.skipUnless(HAS_SCAPY, 'scapy not installed')
     def test_scapy_engine_returns_scapy_packets(self) -> None:
+        # #406: this asserted ``Raw`` -- the engine imported only ``scapy.sendrecv``,
+        # so scapy's layer registries were empty and every frame came back
+        # undissected. ``Ether`` is what the capture actually holds: frame 1 of
+        # in.pcap is an Ethernet frame carrying an ICMPv6 neighbour solicitation over
+        # IPv6, which is exactly what the two tests above independently report for the
+        # same frame -- 'Ethernet:IPv6:IPv6_ICMP' from the default engine and
+        # 'Ethernet:IP6:ICMP6' from DPKT. The three chains are three libraries'
+        # spellings of one frame, so scapy agreeing here is parity, not a new claim.
         from pcapkit.interface import extract
+        from pcapkit.toolkit.scapy import packet2chain
 
         extractor = extract(fin=sample_path('in.pcap'), fout='/tmp/out', format='tree', store=True, nofile=True, engine='scapy')
         self.addCleanup(close_extractor, extractor)
 
         frame = extractor.frame[0]
         self.assertEqual(extractor.length, 6)
-        self.assertEqual(type(frame).__name__, 'Raw')
+        self.assertEqual(type(frame).__name__, 'Ether')
+        self.assertEqual(packet2chain(frame),
+                         'Ethernet:IPv6:ICMPv6 Neighbor Discovery - Neighbor Solicitation:'
+                         'ICMPv6 Neighbor Discovery Option - Source Link-Layer Address')
         self.assertGreater(len(bytes(frame)), 0)
 
     @unittest.skipUnless(HAS_PYSHARK, 'pyshark not installed')