diff --git a/docs/source/pcapkit/protocols/internet/hip.rst b/docs/source/pcapkit/protocols/internet/hip.rst index 74e3608070..1ef6482aff 100644 --- a/docs/source/pcapkit/protocols/internet/hip.rst +++ b/docs/source/pcapkit/protocols/internet/hip.rst @@ -96,6 +96,7 @@ Octets Bits Name Description .. automethod:: _read_param_relay_hmac .. automethod:: _make_hip_param + .. automethod:: _make_hip_param_data .. automethod:: _make_param_unassigned .. automethod:: _make_param_esp_info .. automethod:: _make_param_r1_counter diff --git a/pcapkit/protocols/internet/hip.py b/pcapkit/protocols/internet/hip.py index a32f909306..4d98ea7ced 100644 --- a/pcapkit/protocols/internet/hip.py +++ b/pcapkit/protocols/internet/hip.py @@ -95,6 +95,7 @@ from pcapkit.protocols.data.internet.hip import NotificationParameter as Data_NotificationParameter from pcapkit.protocols.data.internet.hip import OverlayIDParameter as Data_OverlayIDParameter from pcapkit.protocols.data.internet.hip import OverlayTTLParameter as Data_OverlayTTLParameter +from pcapkit.protocols.data.internet.hip import Parameter as Data_Parameter from pcapkit.protocols.data.internet.hip import PayloadMICParameter as Data_PayloadMICParameter from pcapkit.protocols.data.internet.hip import PuzzleParameter as Data_PuzzleParameter from pcapkit.protocols.data.internet.hip import R1CounterParameter as Data_R1CounterParameter @@ -221,7 +222,6 @@ from pcapkit.const.hip.suite import Suite as Enum_Suite from pcapkit.const.hip.transport import Transport as Enum_Transport from pcapkit.corekit.protochain import ProtoChain - from pcapkit.protocols.data.internet.hip import Parameter as Data_Parameter from pcapkit.protocols.protocol import ProtocolBase from pcapkit.protocols.schema.internet.hip import Parameter as Schema_Parameter @@ -409,6 +409,9 @@ class HIP(IPv6_Ext[Data_HIP, Schema_HIP], Internet[Data_HIP, Schema_HIP], and the two fixed header bits (:issue:`1313`, :issue:`1314`, :issue:`1323`, :issue:`1328`, :issue:`1365`, :issue:`1417`, :issue:`1459`). + * A ``(code, data)`` pair rebuilds from the parsed data model rather + than from its fields as keyword arguments, which dropped every field + whose constructor keyword is spelled differently (:issue:`2352`). """ @@ -639,7 +642,7 @@ def make(self, controls_reserved: 'int' = 0, shit: 'int' = 0, rhit: 'int' = 0, - parameters: 'Optional[list[Schema_Parameter | tuple[Enum_Parameter, dict[str, Any]] | bytes] | Parameter]' = None, # pylint: disable=line-too-long + parameters: 'Optional[list[Schema_Parameter | tuple[Enum_Parameter, Data_Parameter | dict[str, Any]] | bytes] | Parameter]' = None, # pylint: disable=line-too-long payload: 'bytes | ProtocolBase | Schema' = b'', **kwargs: 'Any') -> 'Schema_HIP': """Make (construct) packet data. @@ -3056,10 +3059,17 @@ def _read_param_relay_hmac(self, schema: 'Schema_RelayHMACParameter', version: ' ) return relay_hmac - def _make_hip_param(self, parameters: 'list[Schema_Parameter | tuple[Enum_Parameter, dict[str, Any]] | bytes] | Parameter', *, + def _make_hip_param(self, parameters: 'list[Schema_Parameter | tuple[Enum_Parameter, Data_Parameter | dict[str, Any]] | bytes] | Parameter', *, # pylint: disable=line-too-long version: 'int') -> 'tuple[list[Schema_Parameter | bytes], int]': """Make HIP parameter. + A ``(code, value)`` pair carries either a mapping of keyword arguments + for the parameter's constructor, or a parsed + :class:`~pcapkit.protocols.data.internet.hip.Parameter` data model, + which goes to the constructor positionally through + :meth:`_make_hip_param_data` rather than expanded as keyword arguments + (:issue:`2352`). + Args: parameters: HIP parameters version: HIP protocol version @@ -3085,7 +3095,8 @@ def _make_hip_param(self, parameters: 'list[Schema_Parameter | tuple[Enum_Parame parameters_list.append(schema_packed) total_length += len(schema_packed) else: - code, args = cast('tuple[Enum_Parameter, dict[str, Any]]', schema) + code, args = cast('tuple[Enum_Parameter, Data_Parameter | dict[str, Any]]', + schema) name = self._lookup_registry(self.__parameter__, code) if isinstance(name, str): meth_name = f'_make_param_{name}' @@ -3094,7 +3105,11 @@ def _make_hip_param(self, parameters: 'list[Schema_Parameter | tuple[Enum_Parame else: meth = name[1] - data = meth(code, version=version, **args) # type: Schema_Parameter + if isinstance(args, Data_Parameter): + data = self._make_hip_param_data(meth, code, args, + version=version) # type: Schema_Parameter + else: + data = meth(code, version=version, **args) data_packed = data.pack() parameters_list.append(data) @@ -3111,17 +3126,43 @@ def _make_hip_param(self, parameters: 'list[Schema_Parameter | tuple[Enum_Parame else: meth = name[1] - data = meth(code, param, version=version) - # NOTE: The trailing padding :meth:`_read_hip_param` kept, if any. - padding = getattr(param, 'padding', None) - if isinstance(padding, bytes) and 'padding' in data.__fields__: - data.padding = padding + data = self._make_hip_param_data(meth, code, param, version=version) data_packed = data.pack() parameters_list.append(data) total_length += len(data_packed) return parameters_list, total_length + def _make_hip_param_data(self, meth: 'ParameterConstructor', code: 'Enum_Parameter', + param: 'Data_Parameter', *, + version: 'int') -> 'Schema_Parameter': + """Make a HIP parameter from the data model its reader produced. + + The parsed parameter goes to ``meth`` **positionally**. Expanding it as + keyword arguments instead would drop every field whose constructor + keyword is spelled differently -- ``HIP_TRANSFORM`` keeps its suites in + ``suite_id`` and :meth:`_make_param_hip_transform` takes them as + ``suites``, so ``**param`` left the parameter empty (:issue:`2352`). + + Args: + meth: Parameter constructor. + code: Parameter code. + param: Parsed parameter data model. + version: HIP protocol version. + + Returns: + HIP parameter schema. + + .. versionadded:: 1.5.0 + + """ + data = meth(code, param, version=version) + # NOTE: The trailing padding :meth:`_read_hip_param` kept, if any. + padding = getattr(param, 'padding', None) + if isinstance(padding, bytes) and 'padding' in data.__fields__: + data.padding = padding + return data + def _make_param_unassigned(self, code: 'Enum_Parameter', param: 'Optional[Data_UnassignedParameter]' = None, *, # pylint: disable=unused-argument version: 'int', contents: 'bytes' = b'', diff --git a/tests/protocols/internet/test_hip_param_data_rebuild_2352_unit.py b/tests/protocols/internet/test_hip_param_data_rebuild_2352_unit.py new file mode 100644 index 0000000000..4472e30632 --- /dev/null +++ b/tests/protocols/internet/test_hip_param_data_rebuild_2352_unit.py @@ -0,0 +1,286 @@ +# -*- coding: utf-8 -*- +"""A HIP ``(code, data)`` pair rebuilds the parameter it was parsed from. + +GitHub issue #2352. ``HIP(parameters=[(code, parsed_data)], ...)`` expanded the +parsed parameter as ``**kwargs`` into its ``_make_param_*`` constructor, so +every field whose constructor keyword is spelled differently never arrived and +the parameter came back with that value at its default -- ``HIP_TRANSFORM`` +keeps its suites in ``suite_id`` and ``_make_param_hip_transform`` takes them as +``suites``, which rebuilt the parameter empty, as ``0241000000000000``. The +constructor now takes the data model positionally, as the +``parameters=parsed.info.parameters`` path already did. + +The mismatch is not confined to ``HIP_TRANSFORM``: +:meth:`HIPParameterDataRebuildTests.cases` carries one non-default value for +each parameter the bug reached, and +:meth:`HIPParameterDataRebuildTests.test_every_renaming_parameter_has_a_case` +compares that list against the constructors, so a parameter that starts +spelling a *top-level* field differently tomorrow cannot land without a +round-trip case here. + +That comparison is only partly mechanical, though, so three cases are listed by +hand rather than caught by it and a fourth parameter needs a second case the +check cannot ask for. What it does mechanically is read each constructor's +keyword-only parameters against its data model's own annotations. Everything +else is by name or by hand: + +* :data:`DERIVED` exempts ``flags`` and ``lifetime`` *by name*, rather than + establishing that the constructor takes a nested model apart into one keyword + per member. ``ROUTE_DST`` and ``ROUTE_VIA`` keep ``symmetric`` and + ``must_follow`` inside a nested ``flags`` model that ``**param`` dropped + wholesale, and that exemption is why they are listed here instead of derived. +* A *nested* collision is invisible to it. ``LOCATOR_SET``'s keyword and field + are both spelled ``locator_set``, so the check passes it over, yet expanding + the parsed parameter handed each nested + :class:`~pcapkit.protocols.data.internet.hip.Locator`'s own ``locator`` key + to ``_make_locator``'s ``locator`` parameter and raised ``AttributeError: + 'IPv6Address' object has no attribute 'locator'``. +* ``HOST_ID`` the check does find -- it renames ``algorithm`` to + ``hi_algorithm`` -- but a case built on an ECDSA-curve host identity hides + the loss, because the constructor re-derives the algorithm from the curve. + Only a raw-``hi`` host identity, carrying no curve to re-derive from, loses + it, so both shapes are here. + +Every case builds its own octets in memory and reads no capture. :class:`HIP` +is imported inside each test, after +:func:`~tests._support.reimport_once_per_class`, for the reason +:mod:`tests.protocols.link.test_ethernet_mac_roundtrip_unit` gives. + +""" +from __future__ import annotations + +import importlib.util +import ipaddress +import re +import unittest + +from tests._support import reimport_once_per_class + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Test tier (#1958): hand-picked round-trip cases, not a sweep. +__tier__ = 'ALL' + +#: Header fields shared by every HIP packet built here. +HIP_BASE = { + 'next': 6, 'packet': 1, 'checksum': b'\x00\x00', + 'controls_anonymous': False, 'shit': 0, 'rhit': 0, 'payload': b'', +} + +#: HIP version each version-bound parameter demands: ``HIP_TRANSFORM`` (577) +#: and ``R1_Counter`` (128) are :rfc:`5201` only, as +#: :data:`examples.generators.options.HIP_VERSION` records. +HIP_VERSION = {128: 1, 577: 1} + +#: Fields a constructor derives rather than accepting, so that a data model +#: holding them is not evidence of a renamed keyword: ``Length`` octets are +#: computed from the value they measure, and a nested flags or lifetime model +#: is taken apart into one keyword per member. Maintained by hand -- +#: :meth:`HIPParameterDataRebuildTests.test_every_renaming_parameter_has_a_case` +#: exempts these names, it does not work out that they are derived. +DERIVED = frozenset({'hi_len', 'di_len', 'pub_len', 'flags', 'lifetime'}) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class HIPParameterDataRebuildTests(unittest.TestCase): + """Parameters whose constructor keyword differs from their data field.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + @staticmethod + def cases() -> 'dict[str, tuple[object, dict[str, object]]]': + """One non-default value per parameter the bug reached. + + Keyed by case name rather than by parameter code, because ``HOST_ID`` + needs two: the ECDSA-curve one, which round-tripped by accident, and + the raw-``hi`` one, which did not. + + A default is useless here: an empty suite list rebuilds as an empty + suite list whether or not the value reached the constructor, which is + exactly why the sweep in + :mod:`tests.protocols.test_option_roundtrip_unit` could not see this. + + """ + from pcapkit.const.hip.ecdsa_curve import ECDSACurve + from pcapkit.const.hip.hi_algorithm import HIAlgorithm + from pcapkit.const.hip.parameter import Parameter + + return { + 'SEQ': (Parameter.SEQ, {'update_id': 0x11223344}), + 'DH_GROUP_LIST': (Parameter.DH_GROUP_LIST, {'groups': [1, 2, 3]}), + 'DIFFIE_HELLMAN': (Parameter.DIFFIE_HELLMAN, + {'group': 3, 'pub_val': 0xdeadbeef}), + 'HIP_TRANSFORM': (Parameter.HIP_TRANSFORM, {'suites': [1, 2]}), + 'HIP_CIPHER': (Parameter.HIP_CIPHER, {'ciphers': [1, 2, 4]}), + 'NAT_TRAVERSAL_MODE': (Parameter.NAT_TRAVERSAL_MODE, {'modes': [1, 2]}), + # The algorithm reaches the rebuild through the curve rather than + # through the renamed ``algorithm`` field, so this case round-trips + # with or without the fix. The raw-``hi`` case below is the one + # that pins the rename. + 'HOST_ID (ECDSA curve)': (Parameter.HOST_ID, + {'hi_algorithm': 7, 'hi_curve': ECDSACurve.NIST_P_256, + 'hi_pub_key': b'\x01\x02\x03\x04', + 'di_type': 1, 'di': b'host'}), + # A raw host identity carries no curve to re-derive the algorithm + # from, so ``algorithm`` -> ``hi_algorithm`` is load-bearing here: + # expanding the parsed parameter dropped it and rebuilt the + # parameter as ``NULL_ENCRYPT`` (1) instead of ``DSA`` (3). + 'HOST_ID (raw hi)': (Parameter.HOST_ID, + {'hi': b'\xaa\xbb\xcc\xdd', 'hi_algorithm': HIAlgorithm.DSA, + 'di_type': 1, 'di': b'host'}), + 'HIT_SUITE_LIST': (Parameter.HIT_SUITE_LIST, {'suites': [1, 2]}), + 'REG_INFO': (Parameter.REG_INFO, + {'min_lifetime': 5, 'max_lifetime': 200, 'reg_info': [1, 2]}), + 'REG_REQUEST': (Parameter.REG_REQUEST, {'lifetime': 42, 'reg_request': [1, 2]}), + 'REG_RESPONSE': (Parameter.REG_RESPONSE, {'lifetime': 42, 'reg_response': [1, 2]}), + 'REG_FAILED': (Parameter.REG_FAILED, {'failure_type': 1, 'reg_failed': [1, 2, 3]}), + 'TRANSPORT_FORMAT_LIST': (Parameter.TRANSPORT_FORMAT_LIST, {'formats': [1, 2]}), + 'ESP_TRANSFORM': (Parameter.ESP_TRANSFORM, {'suites': [1, 2]}), + # ``symmetric`` and ``must_follow`` live in a nested ``flags`` + # model that ``**param`` dropped wholesale, and ``flags`` is in + # :data:`DERIVED`, so these two are listed rather than derived. + 'ROUTE_DST': (Parameter.ROUTE_DST, + {'symmetric': True, 'must_follow': True, + 'hit': [ipaddress.ip_address('::1')]}), + 'HIP_TRANSPORT_MODE': (Parameter.HIP_TRANSPORT_MODE, {'port': 1234, 'modes': [1, 2]}), + 'ROUTE_VIA': (Parameter.ROUTE_VIA, + {'symmetric': True, 'must_follow': False, + 'hit': [ipaddress.ip_address('::2')]}), + # Not a renamed keyword -- ``locator_set`` is spelled the same on + # both sides, which is why + # :meth:`test_every_renaming_parameter_has_a_case` cannot find it. + # The collision is one level down: expanding the parsed parameter + # passed each nested ``Locator``'s ``locator`` key to + # ``_make_locator``'s ``locator`` parameter, which then read + # ``locator.locator`` off an :class:`~ipaddress.IPv6Address`. Both + # locator shapes are here, the bare address and the ``LocatorData`` + # the SPI form produces. + 'LOCATOR_SET': (Parameter.LOCATOR_SET, + {'locator_set': [ + {'traffic': 1, 'type': 0, 'preferred': True, + 'lifetime': 180, 'ip': '2001:db8::1'}, + {'traffic': 2, 'type': 1, 'preferred': False, + 'lifetime': 60, 'ip': '2001:db8::2', + 'spi': 0x11223344}, + ]}), + } + + @staticmethod + def _base(code: 'object') -> 'dict[str, object]': + base = dict(HIP_BASE) + base['version'] = HIP_VERSION.get(int(code), 2) # type: ignore[call-overload] + return base + + def _build(self, code: 'object', value: 'object') -> 'bytes': + from pcapkit.protocols.internet.hip import HIP + + # ``extension=True`` keeps ``HIP.alias`` off ``self._info``, which + # ``__post_init__`` assigns only once ``read`` has returned (#1184). + return HIP(parameters=[(code, value)], extension=True, + **self._base(code)).data # type: ignore[arg-type] + + def test_a_parsed_parameter_rebuilds_byte_for_byte(self) -> None: + """#2352: the pair form reproduces the octets it was parsed from.""" + from pcapkit.protocols.internet.hip import HIP + + for name, (code, kwargs) in self.cases().items(): + with self.subTest(case=name): + octets = self._build(code, kwargs) + parsed = HIP(octets, len(octets), extension=True) + param = parsed.info.parameters[code] # type: ignore[index] + + self.assertEqual(self._build(code, param).hex(), octets.hex()) + + def test_the_pair_form_agrees_with_the_whole_collection(self) -> None: + """#2352: ``[(code, data)]`` and ``parsed.info.parameters`` agree. + + The collection form passed each parameter positionally all along, so it + rebuilt correctly while the pair form did not. Pinning the two against + each other is what keeps them from drifting apart again. + + """ + from pcapkit.protocols.internet.hip import HIP + + for name, (code, kwargs) in self.cases().items(): + with self.subTest(case=name): + octets = self._build(code, kwargs) + parameters = HIP(octets, len(octets), extension=True).info.parameters + + whole = HIP(parameters=parameters, extension=True, + **self._base(code)).data # type: ignore[arg-type] + pairs = HIP(parameters=list(parameters.items(multi=True)), extension=True, + **self._base(code)).data # type: ignore[arg-type] + self.assertEqual(pairs.hex(), whole.hex()) + + def test_hip_transform_keeps_its_suites(self) -> None: + """#2352: the reported case, down to the octets it used to produce.""" + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + + octets = self._build(Parameter.HIP_TRANSFORM, {'suites': [1, 2]}) + self.assertEqual(octets[40:].hex(), '0241000400010002') + + param = HIP(octets, len(octets), extension=True).info.parameters[Parameter.HIP_TRANSFORM] + rebuilt = self._build(Parameter.HIP_TRANSFORM, param) + + self.assertEqual(rebuilt[40:].hex(), '0241000400010002') + # The empty parameter the dropped ``suite_id`` used to produce. + self.assertNotEqual(rebuilt[40:].hex(), '0241000000000000') + + def test_the_keyword_form_still_builds_from_constructor_keywords(self) -> None: + """A mapping is still expanded as keyword arguments, as documented.""" + from pcapkit.const.hip.parameter import Parameter + + self.assertEqual( + self._build(Parameter.SEQ, {'update_id': 0x11223344})[40:].hex(), + '0181000411223344', + ) + self.assertEqual( + self._build(Parameter.HIP_CIPHER, {'ciphers': [1, 2, 4]})[40:].hex(), + '02430006000100020004000000000000', + ) + + def test_every_renaming_parameter_has_a_case(self) -> None: + """Every renaming constructor is covered by :meth:`cases`. + + The renaming set is read off the constructors rather than listed, so + that a parameter which starts spelling a *top-level* field differently + is caught here instead of silently dropping the value again. Two things + are not mechanical, and the cases for them are listed in :meth:`cases` + by hand: :data:`DERIVED` exempts fields by name rather than by + establishing that the constructor derives them, and a *nested* + collision -- ``LOCATOR_SET``'s -- is invisible here, because the + keyword and the field agree at the top level and only the data model + one level down disagrees. + + """ + import inspect + + from pcapkit.protocols.internet import hip as hip_module + from pcapkit.protocols.internet.hip import HIP + from pcapkit.utilities.compat import get_annotations + + registry = HIP.__dict__['__parameter__'] + covered = {registry[code] for code, _ in self.cases().values()} + + renaming = set() + for name in set(registry.values()) | {registry.default_factory()}: + signature = inspect.signature(getattr(HIP, f'_make_param_{name}')) + keywords = {key for key, value in signature.parameters.items() + if value.kind is value.KEYWORD_ONLY} + + annotation = signature.parameters['param'].annotation + match = re.search(r'Data_\w+', annotation) + self.assertIsNotNone(match, f'_make_param_{name} has no data model') + model = getattr(hip_module, match.group()) # type: ignore[union-attr] + + if set(get_annotations(model)) - keywords - DERIVED: + renaming.add(name) + + self.assertEqual(renaming - covered, set()) + + +if __name__ == '__main__': + unittest.main()