diff --git a/docs/source/pcapkit/protocols/internet/hip.rst b/docs/source/pcapkit/protocols/internet/hip.rst index 731f851ed..29a4eac7a 100644 --- a/docs/source/pcapkit/protocols/internet/hip.rst +++ b/docs/source/pcapkit/protocols/internet/hip.rst @@ -431,6 +431,10 @@ Auxiliary Functions .. autofunction:: pcapkit.protocols.schema.internet.hip.encrypted_data_len .. autofunction:: pcapkit.protocols.schema.internet.hip.parameter_total_len .. autofunction:: pcapkit.protocols.schema.internet.hip.parameter_padding_len +.. autofunction:: pcapkit.protocols.schema.internet.hip.locator_set_len_callback +.. autofunction:: pcapkit.protocols.schema.internet.hip.locator_set_padding_len + +.. autodata:: pcapkit.protocols.schema.internet.hip.LOCATOR_SET_LEN Data Models ----------- diff --git a/examples/generators/options.py b/examples/generators/options.py index 82d95403b..3ca92bfe7 100644 --- a/examples/generators/options.py +++ b/examples/generators/options.py @@ -980,47 +980,63 @@ def _mh_extension_build(code: 'Any', kwargs: 'dict[str, Any]') -> 'Any': #: statement available that the padding was what made a lone parameter #: unrepresentable -- 41 codes that could not survive alone now can. #: -#: It is still *two*, though, because the four codes that fail at one copy fail -#: for reasons that have nothing to do with padding. Three of them pack a number -#: of contents octets that disagrees with the ``len`` they declare, so their -#: record is not 8-aligned however the padding is computed, and a pair cancels -#: that misalignment exactly as it used to cancel the padding error: +#: What still fails at one copy fails for reasons that have nothing to do with +#: padding. Two of the three pack a number of contents octets that disagrees with +#: the ``len`` they declare, so their record is not 8-aligned however the padding +#: is computed; the third never needed a pair at all: #: -#: * ``R1_COUNTER`` (129) and ``R1_Counter`` (128) both declare ``len=12``, -#: which :rfc:`7401` Section 5.2.3 agrees with, but pack 12 octets in total -#: rather than 16 -- because ``counter`` is a -#: :class:`~pcapkit.corekit.fields.numbers.UInt32Field` where the RFC -#: specifies "R1 generation counter, 8 bytes", a 64-bit unsigned integer. Four -#: octets short, not filed anywhere yet, and found while measuring #651. -#: -#: ``R1_COUNTER`` is the one code that round-tripped at one copy *before* #651 -#: and does not after, and the reason is worth keeping: at ``len=12`` the old -#: contents-aligning rule appended exactly four surplus octets, which happened -#: to fill this parameter's four-octet shortfall and bring the record to 16. -#: Two defects cancelling, again. Correcting the padding removes the -#: compensation and leaves the shortfall visible, which is the right outcome -#: and not a regression in anything but this table's tally. -#: -#: Once the width is fixed, ``R1_Counter`` will fail on its own second defect -#: instead: code 128 parses as an ``UnassignedParameter``, because the schema -#: registry is keyed on ``code=`` and ``R1CounterParameter`` declares only 129. -#: That is what ``hip-parameter/R1_Counter`` in the expected-failure table -#: records, and it is why this code fails at two copies as well as at one. #: * ``HOST_ID`` declares ``len=8`` and packs 18. Recorded as #: ``hip-parameter/HOST_ID`` in that table. #: * ``HIP_TRANSFORM`` is HIPv1-only -- ``_read_param_hip_transform`` raises for #: any other version -- while this table builds it at version 2. Nothing to do #: with lengths at all, and it is the one whose recorded ``defect`` string in #: that table names the header arithmetic rather than this. +#: * ``R1_Counter`` (128) parses as an ``UnassignedParameter``: ``_read_param_*`` +#: and ``_make_param_*`` are found by enumeration member name so both exist for +#: code 128, but the *schema* registry is keyed on the ``code=`` of the class +#: statement and ``R1CounterParameter`` declares only 129. Recorded as +#: ``hip-parameter/R1_Counter``, filed as #690, and it fails at two copies as +#: well as at one, so the pair never routed around it. Note that #672 widened +#: this parameter's ``counter`` to the eight octets :rfc:`7401` Section 5.2.3 +#: requires and deliberately left this alone, so the entry is unchanged by it. +#: +#: Two codes that used to be on that list are not on it any more, and the +#: measurement is the point of saying so rather than quietly shortening the list. +#: ``R1_COUNTER`` (129) packed 12 octets against the correct ``len=12`` where +#: :rfc:`7401` Section 5.2.3's 8-octet counter makes the record 16; and +#: ``LOCATOR_SET`` (193) declared its ``Length`` in 4-octet units where the RFC +#: counts bytes, which at this table's default empty locator set produced a +#: four-octet record. Both were fixed by #672 and #679, and at one copy the +#: former went from ``CONSTRUCT`` (``ProtocolError: HIPv2: invalid format``) to +#: ``OK`` while the latter went from ``MISMATCH`` to ``OK``. Measured over this +#: table's 49 HIP codes on ``f0999858e`` and on the fix: +#: +#: ====================== ======== ========== +#: tree one copy two copies +#: ====================== ======== ========== +#: ``f0999858e`` (before) 44 OK 46 OK +#: #672 + #679 (after) 46 OK 46 OK +#: ====================== ======== ========== +#: +#: The 44 is one fewer than the 45 the #651 row above records at one copy, and +#: the missing code is ``LOCATOR_SET``. #651 had corrected its padding along with +#: the other 45 parameters, which made its empty record eight octets and let it +#: round-trip alone; #664 then narrowed that correction back out of this one +#: parameter, deliberately, so the four-octet record returned and with it the +#: ``MISMATCH``. The row is not a regression in #664 -- it is the accidental +#: conformance #679 documents, showing up in this table rather than on the wire. +#: +#: So **the two settings now agree**, on the same three codes above, and the +#: reason this constant is still two is no longer that one copy fails. It is that +#: changing it halves every ``hip-parameter`` frame in +#: :file:`options-internet.pcap` -- which is the fixture the RFC-only +#: conformance walk reads -- and that is a change about this table's test data +#: rather than about either defect. #689 tracks making the drop, so that the +#: before-and-after figures quoted for #672 and #679 stay comparable in the +#: meantime. #: -#: Dropping to one copy would therefore trade this module's padding workaround -#: for two freshly exposed expected-failure entries (``R1_COUNTER`` and a -#: changed status on ``R1_Counter``) and the loss of ``R1_COUNTER`` from the -#: round-tripping set -- a change about *those* defects rather than about this -#: one, and one that belongs with their fixes. The -#: single-parameter case is not lost in the meantime: it is asserted directly, -#: and now positively, by -#: ``test_a_hip_packet_carrying_one_parameter_round_trips`` in +#: The single-parameter case is not lost while it waits: it is asserted directly, +#: and positively, by ``test_a_hip_packet_carrying_one_parameter_round_trips`` in #: :mod:`tests.protocols.test_option_roundtrip_unit`. HIP_COPIES = 2 @@ -1040,6 +1056,24 @@ def _hip_overrides() -> 'dict[Any, dict[str, Any]]': Parameter.SOLUTION: {'lifetime': 1}, # ``hi_curve=None`` falls through to an explicit raise. Parameter.HOST_ID: {'hi_curve': ECDSACurve.NIST_P_256}, + # These two are not here because the default fails -- ``counter=0`` + # constructs and parses perfectly well. They are here because a + # zero-valued field cannot discriminate a width defect from a correct + # one, and this parameter's width defect (#672) hid behind exactly that + # for as long as it did. The RFC-only walk over + # ``options-internet.pcap`` -- which takes its stride solely from + # :rfc:`7401` Section 5.2.1's ``Total Length = 11 + Length - (Length + + # 3) % 8`` -- read ``Length = 12``, advanced 16 over a 12-octet record, + # landed four octets inside the second copy, and found a phantom + # ``Type = 0, Length = 0`` record there whose "padding" was four zero + # octets. Every octet it mis-read was zero, so its zero-padding check + # passed and it reported no violation. Measured: patching one counter to + # ``aabbccdd`` turned that silence into ``type 0 padding not zeroed: + # aabbccdd``. With a non-zero counter the fixture can no longer conceal + # a mis-stride here, whether or not the width is right -- which is the + # reason to keep this override now that #672 has corrected the width. + Parameter.R1_COUNTER: {'counter': 0xaabbccdd}, + Parameter.R1_Counter: {'counter': 0xaabbccdd}, } diff --git a/pcapkit/protocols/internet/hip.py b/pcapkit/protocols/internet/hip.py index c45ef7222..77738eda7 100644 --- a/pcapkit/protocols/internet/hip.py +++ b/pcapkit/protocols/internet/hip.py @@ -989,13 +989,12 @@ def _read_locator(locator: 'Schema_Locator') -> 'Data_LocatorData | IPv6Address' locator_set = Data_LocatorSetParameter( type=schema.type, critical=bool(schema.type & 0b1), - # NOTE: The one reported record length in this module left on the - # pre-#651 expression, to match the one padding site left on it -- - # see ``LocatorSetParameter.padding`` in - # :mod:`pcapkit.protocols.schema.internet.hip` for why touching - # either alone makes a conformant parameter non-conformant. #679 - # fixes both together, and this line moves with them. - length=4 + schema.len + (8 - schema.len % 8) % 8, + # NOTE: This was the one reported record length in this module left + # on the pre-#651 expression, held back to match the one padding + # site left on it. #679 moves both, together with the ``Length`` + # unit they both read -- see ``LocatorSetParameter.padding`` in + # :mod:`pcapkit.protocols.schema.internet.hip`. + length=parameter_total_len(schema.len), locator_set=tuple(_locs), ) @@ -3142,7 +3141,20 @@ def _make_locator(locator: 'Optional[Data_Locator]' = None, *, return Schema_LocatorSetParameter( type=code, - len=sum(locator['len'] for locator in locators), + # NOTE: ``Locator.len`` is ``Locator Length``, which + # :rfc:`8046#section-4` gives "in 4-octet units" and which counts + # only the ``Locator`` field -- so a locator record is the eight + # fixed octets (traffic type, locator type, locator length, + # reserved-and-flags, lifetime) plus ``Locator Length`` * 4. This + # parameter's ``len`` is :rfc:`7401` Section 5.2.1's ``Length``, + # "length of the Contents, in bytes", so it is the sum of those + # record sizes. It was ``sum(locator['len'])`` until #679: ``4n`` + # where the contents are ``24n`` octets for plain IPv6 locators, + # which both mis-declared the record on the wire and starved the + # reader's ``ListField`` of the octets it needed -- see + # ``LocatorSetParameter.locators`` in + # :mod:`pcapkit.protocols.schema.internet.hip`. + len=sum(8 + locator['len'] * 4 for locator in locators), locators=locators, ) diff --git a/pcapkit/protocols/schema/internet/hip.py b/pcapkit/protocols/schema/internet/hip.py index b6ff252a4..31679edd4 100644 --- a/pcapkit/protocols/schema/internet/hip.py +++ b/pcapkit/protocols/schema/internet/hip.py @@ -26,7 +26,7 @@ from pcapkit.corekit.fields.ipaddress import IPv6AddressField from pcapkit.corekit.fields.misc import ConditionalField, PayloadField, SchemaField, SwitchField from pcapkit.corekit.fields.numbers import (EnumField, NumberField, UInt8Field, UInt16Field, - UInt32Field) + UInt32Field, UInt64Field) from pcapkit.corekit.fields.strings import BitField, BytesField, PaddingField from pcapkit.protocols.schema.schema import EnumSchema, Schema, schema_final from pcapkit.utilities.exceptions import FieldValueError @@ -452,6 +452,72 @@ def parameter_padding_len(pkt: 'dict[str, Any]') -> 'int': return parameter_total_len(length) - 4 - length +#: Packet-context key under which :class:`LocatorSetParameter` keeps its own +#: ``Length`` for the benefit of :func:`locator_set_padding_len`. +#: +#: It exists because ``LOCATOR_SET`` is the one parameter in this module whose +#: contents are a :class:`~pcapkit.corekit.fields.collections.ListField` of +#: nested schemas, and a nested schema packed through one **shares the +#: enclosing packet context**: :meth:`Schema.pack +#: ` opens with +#: ``packet.update(self.__dict__)``, and :class:`Locator` declares a ``len`` of +#: its own. So by the time ``padding`` is evaluated -- after the list, since +#: fields are packed in declaration order -- ``pkt['len']`` is the *last +#: locator's* ``len``, not the parameter's. That is 4 for any IPv6 locator +#: whatever the locator count, which is how the pre-#679 padding expression came +#: to append exactly four octets to every ``LOCATOR_SET`` regardless of size. +#: +#: The shadowing is specific to the packing path. :meth:`Schema.unpack +#: ` hands each field +#: ``packet.copy()``, so nested writes do not propagate back and ``pkt['len']`` +#: stays the parameter's there. Snapshotting under a key no nested schema +#: declares is what makes one callback correct on both paths rather than +#: accidentally correct on one. +LOCATOR_SET_LEN = '__locator_set_len__' + + +def locator_set_len_callback(field: 'ListField', # pylint: disable=unused-argument + pkt: 'dict[str, Any]') -> 'None': + """Snapshot ``LOCATOR_SET``'s own ``Length`` before its locators shadow it. + + Installed as the ``callback`` of :attr:`LocatorSetParameter.locators`, which + :meth:`ListField.__call__ + ` runs when the field + is resolved against the packet -- i.e. before any nested :class:`Locator` has + been packed into it. See :data:`LOCATOR_SET_LEN` for why the snapshot is + needed at all. + + Args: + field: The field being resolved. Unused; the signature is + :class:`~pcapkit.corekit.fields.collections.ListField`'s. + pkt: Parameter unpacked schema, modified in place. + + """ + pkt[LOCATOR_SET_LEN] = pkt['len'] + + +def locator_set_padding_len(pkt: 'dict[str, Any]') -> 'int': + """Return the number of padding octets a ``LOCATOR_SET`` parameter needs. + + The same arithmetic as :func:`parameter_padding_len` -- it defers to it + rather than repeating it -- but read off the snapshot + :func:`locator_set_len_callback` took, for the reason :data:`LOCATOR_SET_LEN` + gives. + + Args: + pkt: Parameter unpacked schema. + + Returns: + Padding length in octets, between 0 and 7 inclusive. + + Raises: + FieldValueError: If the parameter's ``Length`` on the wire is + negative; see :func:`parameter_total_len`. + + """ + return parameter_padding_len({'len': pkt[LOCATOR_SET_LEN]}) + + class Parameter(EnumSchema[Enum_Parameter]): """Base schema for HIP parameters.""" @@ -503,7 +569,21 @@ class R1CounterParameter(Parameter, code=Enum_Parameter.R1_COUNTER): #: Reserved. reserved: 'bytes' = PaddingField(length=4) #: R1 counter. - counter: 'int' = UInt32Field() + #: + #: Eight octets, not four. :rfc:`7401#section-5.2.3` labels the field "R1 + #: generation counter, 8 bytes" in its diagram and then says outright that + #: the parameter "contains a 64-bit unsigned integer in network byte + #: order", so the width is stated twice and inferred from neither. + #: :rfc:`5201#section-5.2.3` gives the same 4 + 8 layout, so there is no + #: version under which four octets is right, and both codes that reach this + #: class -- ``R1_Counter`` (128, HIPv1) and ``R1_COUNTER`` (129) -- are + #: affected. It was a :class:`~pcapkit.corekit.fields.numbers.UInt32Field` + #: until #672: the parameter declared the correct ``len=12`` and packed 12 + #: octets in total where :rfc:`7401` Section 5.2.1's arithmetic makes the + #: record 16, leaving it four short at ``4 (mod 8)``. Measured before the + #: fix, at ``counter=1``: ``00 80 00 0c 00 00 00 00 00 00 00 01`` for code + #: 128 and the same twelve octets under ``00 81`` for code 129. + counter: 'int' = UInt64Field() #: Padding. padding: 'bytes' = PaddingField(length=parameter_padding_len) @@ -544,51 +624,64 @@ class LocatorSetParameter(Parameter, code=Enum_Parameter.LOCATOR_SET): """Header schema for HIP ``LOCATOR_SET`` parameters.""" #: List of locators. + #: + #: The ``length`` callback is the parameter's own ``Length``, and that is + #: correct on the unpacking path for the reason :data:`LOCATOR_SET_LEN` + #: gives -- nothing has shadowed ``len`` yet when this field is resolved, + #: since ``type`` and ``len`` are the only fields ahead of it. What was + #: wrong until #679 is the *quantity* it was being handed: + #: :meth:`~pcapkit.protocols.internet.hip.HIP._make_param_locator_set` wrote + #: ``Length`` as ``sum(Locator.len)``, in the 4-octet units + #: :rfc:`8046#section-4` gives ``Locator Length``, where :rfc:`7401` + #: Section 5.2.1's ``Length`` is a byte count. :meth:`Schema.unpack + #: ` reads exactly + #: ``field.length`` octets off the stream and hands only those to + #: :meth:`ListField.unpack + #: `, so a set of *n* + #: plain IPv6 locators offered ``4n`` octets of a ``24n``-octet contents: + #: measured on ``f0999858e``, n = 2 and n = 5 both parsed **one** truncated + #: locator and left the rest of the record unconsumed, with + #: :exc:`~pcapkit.utilities.warnings.SchemaWarning` for the negative + #: remainder and a repack that did not match the octets read. With + #: ``Length`` a byte count the budget is the contents, each locator bills + #: the 8 + ``Locator Length`` * 4 octets it actually consumed, and the + #: count comes out exact. locators: 'list[Locator]' = ListField( length=lambda pkt: pkt['len'], item_type=SchemaField(schema=Locator), + callback=locator_set_len_callback, ) #: Padding. #: - #: **This is the one parameter in this module that does not use** - #: :func:`parameter_padding_len`, **and the exclusion is deliberate. Do not - #: "finish" #651 by changing this line on its own: doing so takes a - #: conformant parameter to four octets short.** Two defects here cancel each - #: other exactly, and #679 tracks fixing them together: - #: - #: 1. This callback does not receive the parameter's ``len`` at all. ``ListField`` - #: packs each nested :class:`Locator` into the shared packet context, whose - #: own ``len`` key overwrites the parameter's, and ``padding`` is evaluated - #: after the list -- so the value seen is the last locator's ``len``, which - #: is 4 for any IPv6 locator. Measured by building this schema directly with - #: a parameter ``len`` of 9 over a single locator of ``len`` 4: the record - #: pads by the amount for 4, not the 3 that 9 would give. - #: 2. :meth:`~pcapkit.protocols.internet.hip.HIP._make_param_locator_set` sets - #: this parameter's ``len`` to ``sum(Locator.len)``, and ``Locator.len`` - #: counts 4-octet units where :rfc:`7401` Section 5.2.1's ``Length`` is a - #: byte count -- so it writes ``4n`` where the contents are ``24n`` octets. + #: Not :func:`parameter_padding_len`, which every other parameter in this + #: module uses, but :func:`locator_set_padding_len` -- the same arithmetic + #: read off a snapshot of this parameter's ``Length`` rather than off + #: ``pkt['len']`` directly. :data:`LOCATOR_SET_LEN` documents why the direct + #: read does not work here: the nested :class:`Locator` schemas share this + #: parameter's packet context while they pack, and their own ``len`` + #: overwrites it before ``padding`` is reached. #: - #: Because the shadowed value is always 4 for a plain IPv6 locator, the old - #: expression always appends 4, giving ``4 + 24n + 4 = 24n + 8``; and because - #: ``24n`` is a multiple of 8, the RFC total for a byte-count ``Length`` of - #: ``24n`` is ``11 + 24n - 3``, the same ``24n + 8``. Measured at n = 1, 2, 5 as - #: 32, 56 and 128 octets, on this tree and on the tree before #651 alike. So the - #: wire output is right today, by two wrongs, and correcting only the padding - #: would leave ``24n + 4``. + #: This is the site #651 deliberately left alone and #664 documented as an + #: exclusion, because two defects in this parameter cancelled at the shape + #: its tests sampled and correcting either alone made the wire output worse. + #: The shadowed ``len`` is 4 for any IPv6 locator, so the old expression + #: appended exactly four octets whatever the locator count; and the wrong + #: ``Length`` unit above made the declared ``Length`` ``4n`` where the + #: contents were ``24n``. ``4 + 24n + 4`` is ``24n + 8``, and since ``24n`` + #: is a multiple of eight the RFC total for a byte-count ``Length`` of + #: ``24n`` is ``11 + 24n - 3``, the same ``24n + 8``. Hence 32, 56 and 128 + #: octets at n = 1, 2, 5 -- conformant, by two wrongs. #: - #: That cancellation holds for a set of **plain IPv6 locators only**, and the - #: reason to say so here is that it would be easy to read the paragraph above as - #: a guarantee about this parameter in general. It is not. A locator carrying an - #: SPI is 28 octets rather than 24, and an empty set has no locator to shadow - #: ``len`` at all, so measured on both trees: the empty set packs 4 octets where - #: :rfc:`7401` Section 5.2.1 wants 8; one SPI locator packs 35; two pack 63; and - #: a mixed plain-and-SPI pair packs 59 or 60 depending on order -- none of them a - #: multiple of eight. Those shapes are non-conformant *before and after* #651, - #: byte-identically, which is exactly why leaving this line alone is the safe - #: choice rather than the correct one: it ships nothing different. #679 owns - #: making them right, and has to account for all of these shapes, not just the - #: homogeneous one. - padding: 'bytes' = PaddingField(length=lambda pkt: (8 - (pkt['len'] % 8)) % 8) + #: That cancellation was never general, which is why the pair had to move + #: together rather than one at a time. It needs every locator to be 24 + #: octets and there to be at least one, and measured on ``f0999858e`` the + #: shapes that break it were non-conformant before this fix: an empty set + #: packed 4 octets where :rfc:`7401` Section 5.2.1 wants 8, one SPI-bearing + #: locator packed 35, two packed 63, and a mixed plain-and-SPI pair packed + #: 59 or 60 depending on order. Afterwards all seven shapes are the RFC's + #: own total: 8, 32, 56, 128, 32, 64, 56 and 56 respectively, with the three + #: plain figures unchanged. + padding: 'bytes' = PaddingField(length=locator_set_padding_len) if TYPE_CHECKING: def __init__(self, type: 'Enum_Parameter', len: 'int', locators: 'list[Locator]') -> 'None': ... diff --git a/tests/corekit/test_fields_ipaddress.py b/tests/corekit/test_fields_ipaddress.py index 8d49f3224..affb2b592 100644 --- a/tests/corekit/test_fields_ipaddress.py +++ b/tests/corekit/test_fields_ipaddress.py @@ -550,57 +550,61 @@ def test_switch_backed_address_makers_reject_a_bool(self) -> None: # the same two sites still take every legitimate value they took before. # - # These two literals are unchanged by #651, and that is worth a note rather - # than being left to look like an oversight: #651 corrected HIP parameter - # padding to :rfc:`7401` Section 5.2.1's - # ``Total Length = 11 + Length - (Length + 3) % 8`` at 45 of the 46 - # parameters, and ``LOCATOR_SET`` is the one deliberately left alone. So - # these records are byte-for-byte what ``main`` emits -- verified against - # ``b34f132f6`` rather than assumed -- and they are *also* what the RFC - # asks for. + # Both literals moved with #679, and in a way worth stating precisely + # because it is not the direction a reader of #651 would guess: the + # records are still **32 octets**, and only the two ``Length`` octets + # changed, from ``0004`` to ``0018``. Measured with ``.pack()`` on both + # trees rather than reasoned about -- ``f0999858e`` emits + # ``00c10004...00000000`` and the fix emits ``00c10018...00000000``, 32 + # octets each. Note the argument order: the first argument is the + # produced octets and the second is the literal. # - # The reason they are already right is that two defects in this parameter - # cancel each other exactly, which is why correcting the padding here alone - # would have broken it. ``LocatorSetParameter.padding``'s callback never - # receives the parameter's ``len``: ``ListField`` packs each nested - # ``Locator`` into the shared packet context, whose own ``len`` overwrites - # the parameter's, and ``padding`` is evaluated after the list -- so it sees - # the last locator's ``len``, which is 4 for any IPv6 locator. Meanwhile - # ``_make_param_locator_set`` writes the parameter's ``len`` as - # ``sum(Locator.len)``, in 4-octet units, where the RFC's ``Length`` is a - # byte count: ``4n`` where the contents are ``24n`` octets. + # ``LOCATOR_SET`` was the one parameter #651 and #664 left on the old + # padding expression, because two defects in it cancelled. Its ``len`` + # was ``sum(Locator.len)`` -- 4-octet units, where :rfc:`7401` + # Section 5.2.1's ``Length`` is "length of the Contents, in bytes" -- so + # it declared ``4n`` for a ``24n``-octet contents; and its padding + # callback read the nested ``Locator.len`` instead of the parameter's, + # because ``ListField`` packs each ``Locator`` into the shared packet + # context and ``padding`` is evaluated after the list. That shadowed + # value is 4 for any IPv6 locator, so the old expression appended exactly + # four octets at every count: ``4 + 24n + 4 = 24n + 8``, which is also + # the RFC total for a byte-count ``Length`` of ``24n``. Hence 32 octets + # here before and after, by two wrongs then and by the arithmetic now. # - # Always-4 padding gives ``4 + 24n + 4 = 24n + 8``; and because ``24n`` is a - # multiple of 8, the RFC total for a byte-count ``Length`` of ``24n`` is - # ``11 + 24n - 3``, the same ``24n + 8``. Measured at n = 1, 2, 5 on both - # trees: 32, 56 and 128 octets, equal to the RFC total in every case. #679 - # tracks fixing the pair together; see ``LocatorSetParameter.padding`` for - # why neither half moves on its own. + # #679 fixed the pair together. ``Length`` is now the byte count 24, the + # padding is ``rfc_total(24) - 4 - 24`` = 4, and the total is the same 32 + # -- while the shapes the cancellation never covered stopped being + # wrong: an empty set went from 4 octets to the 8 the RFC requires, one + # SPI-bearing locator from 35 to 32, two from 63 to 64, and a mixed pair + # from 59 or 60 to 56. Those live in + # :mod:`tests.protocols.internet.test_hip_locator_set_length_unit`, which + # sweeps shapes rather than counts for exactly that reason. # # They pin exact octets rather than a length or a prefix, deliberately. # Exact octets are the whole subject of #651, and pcapkit round-trips its # own output whatever the padding rule says -- writer and reader shared the # error -- so a comparison that tolerated trailing bytes would have gone on # passing through the defect and through the fix alike. For the same reason - # these stay 32 octets: a shorter pin here would silently bless the - # four-octet shortfall that narrowing #651 exists to avoid. + # these stay 32 octets: a shorter pin here would silently bless a + # four-octet shortfall. self.assertEqual( hip._make_param_locator_set( # type: ignore[arg-type] Parameter.LOCATOR_SET, version=2, locator_set=[{'ip': '2001:db8::1'}]).pack().hex(), - '00c10004000004000000000020010db800000000000000000000000100000000') + '00c10018000004000000000020010db800000000000000000000000100000000') self.assertEqual( tcp._make_mptcp_addaddr( # type: ignore[arg-type] MPTCPOption.ADD_ADDR, addr_id=1, addr='192.0.2.1').address, ipaddress.IPv4Address('192.0.2.1')) # int(True) is 1, and 1 is ::1 for an IPv6-only locator -- the escape # hatch works and still widens to the family the wire format fixes. - # Four trailing octets shorter since #651, as above. + # Same 32 octets, same two ``Length`` octets moved, as above. self.assertEqual( hip._make_param_locator_set( # type: ignore[arg-type] Parameter.LOCATOR_SET, version=2, locator_set=[{'ip': int(True)}]).pack().hex(), - '00c1000400000400000000000000000000000000000000000000000100000000') + '00c1001800000400000000000000000000000000000000000000000100000000') def test_ipv4_interface_post_process_rejects_a_non_contiguous_netmask(self) -> None: """``IPv4InterfaceField.post_process`` builds diff --git a/tests/protocols/internet/test_hip_locator_set_length_unit.py b/tests/protocols/internet/test_hip_locator_set_length_unit.py new file mode 100644 index 000000000..8c47d09d6 --- /dev/null +++ b/tests/protocols/internet/test_hip_locator_set_length_unit.py @@ -0,0 +1,368 @@ +# -*- coding: utf-8 -*- +"""HIP ``LOCATOR_SET``, whose ``Length`` unit and padding source were both wrong. + +GitHub issue #679. Two pre-existing defects, neither fixable alone: + +1. ``LocatorSetParameter.padding`` read the **nested** ``Locator.len`` rather than + the parameter's. A :class:`~pcapkit.corekit.fields.collections.ListField` packs + each nested schema into the enclosing packet context -- + :meth:`Schema.pack ` opens with + ``packet.update(self.__dict__)`` -- and :class:`Locator` declares a ``len`` of + its own, so by the time ``padding`` was evaluated ``pkt['len']`` was the last + locator's. That is 4 for any IPv6 locator whatever the count, so the old + expression appended exactly four octets to every ``LOCATOR_SET``. +2. The parameter's ``len`` was ``sum(Locator.len)``, in the 4-octet units + :rfc:`8046#section-4` gives ``Locator Length``, where :rfc:`7401` + Section 5.2.1's ``Length`` is "length of the Contents, in bytes". So contents of + 24, 48 and 120 octets declared ``Length`` 4, 8 and 20 -- ``4n`` where the + contents are ``24n``. + +Why they had to move together +----------------------------- + +Always-4 padding gives ``4 + 24n + 4 = 24n + 8``; and because ``24n`` is a +multiple of eight, the RFC total for a byte-count ``Length`` of ``24n`` is +``11 + 24n - 3``, the same ``24n + 8``. So the two errors cancelled and the +parameter was conformant -- 32, 56 and 128 octets at n = 1, 2, 5 -- which is why +#651 and #664 deliberately excluded this one site: correcting the padding alone +would have left ``24n + 4``, four octets short at every count. + +**The cancellation was never general, and that is the whole reason this module +sweeps shapes rather than counts.** It needs every locator to be 24 octets and +there to be at least one of them. Measured on ``f0999858e``, the shapes where it +does not hold were non-conformant before this fix: + +=================== ============== ====== ========================= +shape declared ``len`` packed RFC total for the contents +=================== ============== ====== ========================= +empty, n = 0 0 **4** 8 +plain IPv6, n = 1 4 32 32 +plain IPv6, n = 2 8 56 56 +plain IPv6, n = 5 20 128 128 +SPI, n = 1 5 **35** 32 +SPI, n = 2 10 **63** 64 +plain then SPI 9 **59** 56 +SPI then plain 9 **60** 56 +=================== ============== ====== ========================= + +Four of the seven were not even multiples of eight. A sweep over n = 1..5 of +plain locators passes today by cancellation and would pass a half-fix too, so it +discriminates nothing; the empty set, the SPI-bearing variant and a mixed set in +both orders are the shapes that do. + +The reader consumed the same quantity +------------------------------------- + +This is why #679 was judged a different class of change from #664's +one-expression padding fix rather than a second instance of it. +``LocatorSetParameter.locators`` is declared +``ListField(length=lambda pkt: pkt['len'])``, and :meth:`Schema.unpack +` reads exactly ``field.length`` +octets off the stream and hands only those to the list. With ``Length`` at ``4n`` +a set of *n* plain locators was offered ``4n`` octets of a ``24n``-octet +contents, so -- measured on ``f0999858e`` -- n = 2 and n = 5 both parsed **one** +truncated locator, warned ``SchemaWarning: packet length < 0``, left the +remainder of the record unconsumed, and repacked to something other than what was +read. In a whole packet that remainder was read as a second, fabricated +parameter: a one-copy ``LOCATOR_SET`` frame parsed as *two* parameters. The +expression was right and the quantity was wrong, so fixing the unit repairs the +reader rather than breaking it -- which +:meth:`test_every_shape_parses_back_to_the_locators_it_was_built_from` and +:meth:`test_a_lone_locator_set_parameter_survives_a_hip_packet` assert directly. +""" +from __future__ import annotations + +import importlib.util +import unittest +import warnings + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Header fields shared by every HIP packet built here, as in +#: :data:`examples.generators.options.HIP_BASE`. +HIP_BASE = { + 'next': 6, 'packet': 1, 'version': 2, 'checksum': b'\x00\x00', + 'controls_anonymous': False, 'shit': 0, 'rhit': 0, 'payload': b'', +} + +#: A plain IPv6 locator: ``Locator Length`` 4, so 8 + 16 = 24 octets. +PLAIN = {'ip': '2001:db8::1'} +#: An SPI-bearing locator: ``Locator Length`` 5, so 8 + 20 = 28 octets. +#: +#: ``type=1`` is not redundant. ``_make_param_locator_set`` sets +#: ``length = 5`` when ``spi`` is given but leaves ``type`` at its default 0, and +#: :func:`~pcapkit.protocols.schema.internet.hip.locator_value_selector` accepts +#: only ``type == 0, len == 4`` or ``type == 1, len == 5`` -- so ``spi=`` without +#: an explicit ``type=1`` raises ``FieldValueError: invalid locator type or +#: length``. That is a separate rough edge in the maker, noted rather than fixed +#: here, and it is why an earlier attempt to measure these shapes could not reach +#: the SPI path at all. +SPI = {'ip': '2001:db8::2', 'spi': 0xdeadbeef, 'type': 1} + +#: Every shape, with the octets each locator record occupies. Ordered so that the +#: three the old cancellation covered come first and the four it did not follow. +SHAPES = ( + ('empty', [], 0), + ('plain n=1', [PLAIN], 24), + ('plain n=2', [PLAIN, PLAIN], 48), + ('plain n=5', [PLAIN] * 5, 120), + ('SPI n=1', [SPI], 28), + ('SPI n=2', [SPI, SPI], 56), + ('mixed plain then SPI', [PLAIN, SPI], 52), + ('mixed SPI then plain', [SPI, PLAIN], 52), +) + + +def rfc_total(length: 'int') -> 'int': + """:rfc:`7401#section-5.2.1`'s ``Total Length = 11 + Length - (Length + 3) % 8``. + + Written out here rather than imported from + :func:`pcapkit.protocols.schema.internet.hip.parameter_total_len`, so the + packed octets are compared against the RFC rather than against the library's + own reading of it. + + Args: + length: The parameter's ``Length`` field, i.e. its contents in octets. + + Returns: + The whole record's length in octets, including ``Type``, ``Length`` and + padding. + + """ + return 11 + length - (length + 3) % 8 + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class HIPLocatorSetLengthTests(unittest.TestCase): + """``LOCATOR_SET``'s ``Length`` unit and padding source, over seven shapes.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def _make(self, locators: 'list[dict]') -> 'tuple': + """Build one ``LOCATOR_SET`` schema and pack it. + + Args: + locators: Locator keyword mappings, as ``_make_param_locator_set`` + takes them. + + Returns: + The schema and its packed octets. + + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + + hip = object.__new__(HIP) + schema = hip._make_param_locator_set( # type: ignore[arg-type] + Parameter.LOCATOR_SET, version=2, + locator_set=[dict(locator) for locator in locators]) + return schema, schema.pack() + + def test_every_shape_packs_the_rfc_total(self) -> None: + """Each shape's record is ``11 + Length - (Length + 3) % 8`` octets. + + Shapes, not counts. Sampling counts within the homogeneous plain-IPv6 + shape is exactly how the cancellation stayed hidden: n = 1, 2 and 5 all + passed on ``f0999858e`` and all would pass a fix to only one of the two + defects. The empty set, the SPI variant and the two mixed orders are what + distinguish the three states -- broken, half-fixed and right. + """ + for name, locators, contents in SHAPES: + with self.subTest(shape=name): + schema, raw = self._make(locators) + + # ``Length`` is the contents in bytes, not in 4-octet units. + self.assertEqual( + schema.len, contents, + msg=f'{name}: Length must be a byte count; RFC 8046 section ' + f'4 gives Locator Length in 4-octet units, RFC 7401 ' + f'section 5.2.1 gives Length in bytes') + # And the record is the total that ``Length`` implies. + self.assertEqual( + len(raw), rfc_total(schema.len), + msg=f'{name}: {len(raw)} octets declared Length ' + f'{schema.len}, which requires {rfc_total(schema.len)}') + self.assertEqual( + len(raw) % 8, 0, + msg=f'{name}: RFC 7401 section 5.2.1 requires a multiple of ' + f'eight; got {len(raw)}') + # The contents really are where ``Length`` says, so the total is + # not reached by over-padding a short contents. + self.assertEqual(len(raw), 4 + contents + (len(raw) - 4 - contents)) + self.assertEqual(raw[2:4], contents.to_bytes(2, 'big')) + + def test_the_three_plain_shapes_keep_the_octet_counts_main_emitted(self) -> None: + """n = 1, 2, 5 still pack 32, 56 and 128 octets. + + The one part of this parameter that was already right, and the reason + #651 and #664 left it alone. Those three are the shapes where the two + defects cancelled exactly, so they are also the three that a correct fix + must not move -- if they change, something has gone wrong even though + every other shape improved. Only the two ``Length`` octets differ from + ``f0999858e``: ``0004`` became ``0018`` at n = 1, and so on. + """ + for locators, total, length in (([PLAIN], 32, 24), + ([PLAIN, PLAIN], 56, 48), + ([PLAIN] * 5, 128, 120)): + with self.subTest(n=len(locators)): + schema, raw = self._make(locators) + self.assertEqual(len(raw), total) + self.assertEqual(len(raw), rfc_total(length)) + self.assertEqual(schema.len, length) + + def test_padding_comes_from_the_parameters_length_not_the_locators(self) -> None: + """The padding callback reads the parameter's ``Length``. + + This is the discriminating assertion for the *first* defect on its own, + and it is built by hand rather than through the maker precisely so that + the two lengths disagree. A parameter ``len`` of 9 over a single locator + whose own ``len`` is 4 needs ``rfc_total(9) - 4 - 9`` = 3 padding octets; + the shadowed nested value would give the 4 that ``len = 4`` implies. + Measured on ``f0999858e``: 32 octets, i.e. 4 of padding. The declared + ``Length`` deliberately does not match the 24 octets the locator actually + occupies, so the record total here is not meaningful -- the padding count + is what is under test. + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.schema.internet.hip import (Locator, LocatorSetParameter, + locator_set_padding_len) + + locator = Locator(traffic=0, type=0, len=4, flags={'preferred': False}, + lifetime=0, value=b'\x20\x01\x0d\xb8' + b'\x00' * 11 + b'\x01') + schema = LocatorSetParameter(type=Parameter.LOCATOR_SET, len=9, + locators=[locator]) + raw = schema.pack() + + self.assertEqual(rfc_total(9) - 4 - 9, 3) + self.assertEqual( + len(raw), 4 + 24 + 3, + msg=f'{len(raw)} octets: 4 header + 24 of locator + padding. 3 is the ' + f'count for the parameter Length of 9; 4 would be the count for ' + f'the nested locator len of 4, which is what shadowing gives') + + # And the callback in isolation, on both the key it reads and the value + # that key is a snapshot of. + from pcapkit.protocols.schema.internet.hip import LOCATOR_SET_LEN + self.assertEqual(locator_set_padding_len({LOCATOR_SET_LEN: 9}), 3) + self.assertEqual(locator_set_padding_len({LOCATOR_SET_LEN: 0}), 4) + self.assertEqual(locator_set_padding_len({LOCATOR_SET_LEN: 24}), 4) + self.assertEqual(locator_set_padding_len({LOCATOR_SET_LEN: 28}), 0) + + def test_the_snapshot_survives_the_nested_locators(self) -> None: + """The packet context keeps the parameter's ``Length`` under its own key. + + The mechanism, asserted rather than inferred: after a pack, the shared + context's ``len`` has been overwritten by the last nested locator's while + :data:`~pcapkit.protocols.schema.internet.hip.LOCATOR_SET_LEN` still holds + the parameter's. Without this the two would be indistinguishable at the + one shape where the numbers happen to agree. + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + from pcapkit.protocols.schema.internet.hip import LOCATOR_SET_LEN + + hip = object.__new__(HIP) + schema = hip._make_param_locator_set( # type: ignore[arg-type] + Parameter.LOCATOR_SET, version=2, locator_set=[dict(SPI)]) + context = {} # type: dict + schema.pack(context) + + self.assertEqual(context[LOCATOR_SET_LEN], 28) + self.assertEqual( + context['len'], 5, + msg='the nested Locator.len is expected to shadow the parameter here; ' + 'if it no longer does, the snapshot has become unnecessary rather ' + 'than wrong') + + def test_every_shape_parses_back_to_the_locators_it_was_built_from(self) -> None: + """The reader consumes the whole contents and repacks identically. + + ``ListField(length=lambda pkt: pkt['len'])`` is handed ``Length`` octets + and no more, so this is the half of #679 that a padding fix could not + have reached. Measured on ``f0999858e``, every non-empty shape parsed + exactly **one** locator regardless of how many it was built with, warned + ``SchemaWarning: packet length < 0``, and repacked to something other + than what it read. + + The residual warnings are asserted to be absent *at the parameter level* + only for the empty shape. Every nested :class:`Locator` still raises six + of them, one per field, because ``SchemaField`` seeds a nested schema's + ``__length__`` at the ``-1`` placeholder + :meth:`Schema.pack ` + documents; that is pre-existing, unrelated to ``Length`` units, and + unchanged by this fix -- what changed is that it now fires once per + locator actually parsed rather than once per truncated first locator. + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + from pcapkit.protocols.schema.internet.hip import LocatorSetParameter + + hip = object.__new__(HIP) + for name, locators, contents in SHAPES: + with self.subTest(shape=name): + schema, raw = self._make(locators) + + back = LocatorSetParameter.unpack(raw, len(raw)) # type: ignore[arg-type] + self.assertEqual(back.len, contents) + self.assertEqual( + len(back.locators), len(locators), + msg=f'{name}: built {len(locators)} locator(s), parsed ' + f'{len(back.locators)}') + self.assertEqual( + back.pack(), raw, + msg=f'{name}: repacking what was read did not reproduce it') + + # And the data model reports the whole record, not a derivative + # of the wrong unit. On ``f0999858e`` this read 12 for the + # 32-octet n = 1 record. + data = hip._read_param_locator_set( # type: ignore[arg-type] + back, version=2, options=None) # type: ignore[arg-type] + self.assertEqual(data.length, len(raw)) + self.assertEqual(data.length, rfc_total(contents)) + self.assertEqual(len(data.locator_set), len(locators)) + + def test_a_lone_locator_set_parameter_survives_a_hip_packet(self) -> None: + """One copy in a real packet builds, parses, and yields one parameter. + + The end-to-end consequence of the ``Length`` unit, and the sharpest of + these assertions because it catches the silent case rather than the loud + one. Measured on ``f0999858e``: the empty shape raised, the SPI and mixed + shapes raised ``ProtocolError: HIPv2: invalid format`` -- and the three + plain shapes *succeeded* while parsing **two** parameters, the second + fabricated out of the 20n octets the reader never consumed, with a + reported ``length`` of 12 for a 32-octet record. + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + + for name, locators, contents in SHAPES: + with self.subTest(shape=name): + built = HIP( + parameters=[(Parameter.LOCATOR_SET, + {'locator_set': [dict(x) for x in locators]})], + extension=True, **HIP_BASE) + octets = built.data + + with warnings.catch_warnings(): + # The per-locator ``__length__`` warnings described above are + # pre-existing noise on a now-correct parse; the assertions + # below are what this case is about. + warnings.simplefilter('ignore') + parsed = HIP(octets, len(octets), extension=True) + + params = parsed.info.parameters + self.assertEqual( + len(list(params)), 1, + msg=f'{name}: unconsumed contents get read as a second, ' + f'fabricated parameter') + param = params[Parameter.LOCATOR_SET] + self.assertEqual(param.length, rfc_total(contents)) + self.assertEqual(len(param.locator_set), len(locators)) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/internet/test_hip_r1_counter_width_unit.py b/tests/protocols/internet/test_hip_r1_counter_width_unit.py new file mode 100644 index 000000000..33057b9c8 --- /dev/null +++ b/tests/protocols/internet/test_hip_r1_counter_width_unit.py @@ -0,0 +1,236 @@ +# -*- coding: utf-8 -*- +"""The HIP ``R1_COUNTER`` generation counter is eight octets, not four. + +GitHub issue #672. :rfc:`7401#section-5.2.3` states the width twice -- its +diagram labels the field "R1 generation counter, 8 bytes", and the prose below it +says the parameter "contains a 64-bit unsigned integer in network byte order" -- +so nothing here is inferred from a figure. +:rfc:`5201#section-5.2.3` gives the same ``Reserved, 4 bytes`` plus 8-octet +counter layout, which is why both codes that reach +:class:`~pcapkit.protocols.schema.internet.hip.R1CounterParameter` are affected: +``R1_Counter`` (128, HIPv1) and ``R1_COUNTER`` (129). + +``counter`` was a :class:`~pcapkit.corekit.fields.numbers.UInt32Field`. The +parameter therefore *declared* the correct ``len=12`` -- 4 octets of ``Reserved`` +plus 8 of counter, which is the ``Length 12`` the RFC states -- and *packed* only +eight of those twelve, for a record of 12 octets where :rfc:`7401` +Section 5.2.1's ``Total Length = 11 + Length - (Length + 3) % 8`` makes it 16. +Four short, at ``4 (mod 8)``, in both directions: a conformant receiver reading +``Length = 12`` consumes 16 octets and takes four octets of the next parameter as +this one's tail, and a real peer's 16-octet ``R1_COUNTER`` leaves its last four +octets to be read as the following parameter's ``Type``. + +Why the existing suite could not see it +--------------------------------------- + +Three things hid it at once, and the third is the reason this module asserts +against the RFC's arithmetic rather than against a round trip. + +1. ``HIP_COPIES = 2`` in :mod:`examples.generators.options` puts two copies of + each parameter in a frame, and two four-octet shortfalls sum to eight, so the + record area stays 8-aligned and ``HIP.make``'s ``total_length // 8 + 4`` + comes out exact. +2. Before #651 the padding aligned the parameter *contents* rather than the + record, which at ``Length = 12`` appended exactly the four surplus octets this + parameter was missing. Correcting the padding did not break ``R1_COUNTER``; it + stopped compensating for it. +3. The round trip cannot see it at all, because pcapkit's reader consumes exactly + the twelve octets its writer wrote. Only a comparison against the RFC's own + stride -- or a real peer -- can tell the difference, which is what + :meth:`test_r1_counter_record_is_the_rfc_total_at_both_codes` does, with the + formula spelled out locally rather than taken from the library. + +The fixture could not see it either, for a fourth reason +-------------------------------------------------------- + +``R1_COUNTER`` had no entry in ``_hip_overrides()``, so the generator built it +with ``counter = 0``. An RFC-only walk over :file:`options-internet.pcap` read +``Length = 12``, advanced 16 over a 12-octet record, landed four octets inside the +second copy, and found a phantom ``Type = 0, Length = 0`` record in that copy's +own zeroed contents -- whose "padding" was four zero octets, so its zero-padding +check passed and the walk ended tidily on the area boundary. Measured on +``f0999858e``: patching one counter to ``aabbccdd`` turns that silence into +``frame 101: type 0 padding not zeroed: aabbccdd``. A zero-valued field cannot +discriminate a width defect from a correct one, so +:meth:`test_the_generator_gives_r1_counter_a_non_zero_counter` pins the override +that stops the fixture hiding the next one. +""" +from __future__ import annotations + +import importlib.util +import unittest + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Header fields shared by every HIP packet built here. The same set +#: :data:`examples.generators.options.HIP_BASE` uses, so a failure here is not a +#: failure to drive the constructor. +HIP_BASE = { + 'next': 6, 'packet': 1, 'checksum': b'\x00\x00', + 'controls_anonymous': False, 'shit': 0, 'rhit': 0, 'payload': b'', +} + + +def rfc_total(length: 'int') -> 'int': + """:rfc:`7401#section-5.2.1`'s ``Total Length = 11 + Length - (Length + 3) % 8``. + + Written out here rather than imported from + :func:`pcapkit.protocols.schema.internet.hip.parameter_total_len`, so that + this module compares the packed octets against the RFC and not against the + library's own reading of it. + + Args: + length: The parameter's ``Length`` field, i.e. its contents in octets. + + Returns: + The whole record's length in octets, including ``Type``, ``Length`` and + padding. + + """ + return 11 + length - (length + 3) % 8 + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class HIPR1CounterWidthTests(unittest.TestCase): + """``R1_COUNTER``'s counter width, against :rfc:`7401#section-5.2.3`.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_r1_counter_record_is_the_rfc_total_at_both_codes(self) -> None: + """Both codes pack the 16 octets ``Length = 12`` requires. + + The exact octets are pinned, not just the length, because the length + alone would pass for a record padded to 16 with a four-octet counter as + readily as for one carrying the eight-octet counter the RFC specifies -- + and the padding is what the pre-#651 rule appended. So the assertion + names where the eight octets sit: four of ``Reserved`` after the + type-and-length header, then the counter, then no padding at all, since + ``rfc_total(12) - 4 - 12`` is zero. + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + + hip = object.__new__(HIP) + for code, version, prefix in ((Parameter.R1_Counter, 1, '0080'), + (Parameter.R1_COUNTER, 2, '0081')): + with self.subTest(code=int(code), name=code.name): + schema = hip._make_param_r1_counter( # type: ignore[arg-type] + code, version=version, counter=1) + raw = schema.pack() + + # The declared ``Length`` was never wrong -- it is the packed + # contents that disagreed with it. + self.assertEqual(schema.len, 12) + self.assertEqual(len(raw), rfc_total(12)) + self.assertEqual(len(raw), 16) + self.assertEqual(len(raw) % 8, 0) + # Twelve octets before #672, four of them the counter: + # ``00 8x 00 0c 00 00 00 00 00 00 00 01``. + self.assertEqual( + raw.hex(), prefix + '000c' + '00000000' + '0000000000000001') + # No padding: ``Length = 12`` plus the four header octets is + # already a multiple of eight. + self.assertEqual(rfc_total(12) - 4 - 12, 0) + + def test_the_counter_carries_a_64_bit_value(self) -> None: + """A counter above ``2**32`` survives the round trip. + + This is the assertion that cannot be satisfied by padding. :rfc:`7401` + Section 5.2.3 calls the field "a 64-bit unsigned integer", so the + interesting values are the ones a 32-bit field cannot hold at all: a + :class:`~pcapkit.corekit.fields.numbers.UInt32Field` either raises + packing ``2**32`` or silently keeps the low half, and both outcomes lose + the generation counter a peer sent. + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + from pcapkit.protocols.schema.internet.hip import R1CounterParameter + + hip = object.__new__(HIP) + for counter in (0, 1, 0xffffffff, 0x1_0000_0000, 0x0123456789abcdef, + 0xffffffffffffffff): + with self.subTest(counter=counter): + schema = hip._make_param_r1_counter( # type: ignore[arg-type] + Parameter.R1_COUNTER, version=2, counter=counter) + raw = schema.pack() + self.assertEqual(len(raw), 16) + # Network byte order, over the whole eight octets. + self.assertEqual(raw[8:16], counter.to_bytes(8, 'big')) + back = R1CounterParameter.unpack(raw, len(raw)) # type: ignore[arg-type] + self.assertEqual(back.counter, counter) + self.assertEqual(back.pack(), raw) + + def test_a_lone_r1_counter_parameter_survives_a_hip_packet(self) -> None: + """One copy in a real packet builds, parses, and gives the counter back. + + ``HIP.make`` derives the header's ``len`` as ``total_length // 8 + 4``, + and ``_read_hip_param`` compares the recovered length exactly, so a + parameter that is not a multiple of eight on its own cannot survive + alone -- the floor division drops the remainder. Measured on + ``f0999858e`` this raised ``ProtocolError: HIPv2: invalid format`` at + both codes; the two-copy pairing in + :mod:`examples.generators.options` is what concealed that. + + Code 128 is deliberately not exercised here. It fails for an unrelated + reason that #672 does not touch -- the schema registry is keyed on the + ``code=`` of the class statement and ``R1CounterParameter`` declares only + 129, so 128 parses as an ``UnassignedParameter`` -- which is #690, and + which ``hip-parameter/R1_Counter`` in + :mod:`tests.protocols.test_option_roundtrip_unit` records. + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + + counter = 0x0123456789abcdef + built = HIP(parameters=[(Parameter.R1_COUNTER, {'counter': counter})], + extension=True, version=2, **HIP_BASE) + octets = built.data + parsed = HIP(octets, len(octets), extension=True) + + params = parsed.info.parameters + self.assertEqual(len(list(params)), 1, + msg='a 12-octet record leaves four octets to be read as ' + 'a second, fabricated parameter') + param = params[Parameter.R1_COUNTER] + self.assertEqual(param.counter, counter) + # The reported length is the whole record, ``rfc_total(12)``. + self.assertEqual(param.length, rfc_total(12)) + self.assertEqual(param.length, 16) + + def test_the_generator_gives_r1_counter_a_non_zero_counter(self) -> None: + """``_hip_overrides()`` must not leave this counter at zero. + + Not a style preference. The RFC-only walk over + :file:`options-internet.pcap` reported no violation on this parameter for + as long as the counter was zero, because every octet it mis-read while + striding 16 over a 12-octet record was itself zero -- so its + zero-padding check passed on a phantom record it had fabricated out of + the second copy. Measured on ``f0999858e``: a non-zero counter takes that + walk from 1 violation to 2, the new one being ``frame 101: type 0 padding + not zeroed: aabbccdd``. + + The width is fixed now, so the walk finds nothing here either way. This + pins the override anyway, because a fixture that cannot discriminate a + wrong width from a right one is what let this defect survive as long as + it did -- the same trap #608 fell into with the ``SOLUTION`` field. + """ + from pcapkit.const.hip.parameter import Parameter + + from tests.protocols.test_option_roundtrip_unit import _load_generator + + overrides = _load_generator()._hip_overrides() + for code in (Parameter.R1_Counter, Parameter.R1_COUNTER): + with self.subTest(code=int(code), name=code.name): + self.assertIn(code, overrides) + self.assertNotEqual( + overrides[code].get('counter', 0), 0, + msg='a zero-valued counter cannot discriminate a width ' + 'defect from a correct one') + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/internet/test_hip_unit.py b/tests/protocols/internet/test_hip_unit.py index 4d896b5b1..63a6652fc 100644 --- a/tests/protocols/internet/test_hip_unit.py +++ b/tests/protocols/internet/test_hip_unit.py @@ -3078,12 +3078,15 @@ def pre_651_total(length: int) -> int: # Which parameters this governs, stated rather than implied: every padding # site in ``schema/internet/hip.py`` and every reported record length in # ``internet/hip.py`` routes through these two helpers -- 45 of the 46 - # parameter schemas and 48 of the 49 reported lengths -- with exactly one - # exclusion, ``LOCATOR_SET``, kept on the pre-#651 expression on purpose. - # See ``LocatorSetParameter.padding`` for why, and #679 for the fix. The - # counts are asserted directly in - # :meth:`test_hip_padding_helpers_cover_every_parameter_but_locator_set`, - # so the exclusion cannot silently grow to two. + # parameter schemas and 48 of the 49 reported lengths. The forty-sixth is + # ``LOCATOR_SET``, which since #679 uses + # :func:`~pcapkit.protocols.schema.internet.hip.locator_set_padding_len` + # instead: the same arithmetic, read off a snapshot of the parameter's + # own ``Length`` rather than off ``pkt['len']``, which its nested + # locators shadow while they pack. The counts and the identity of that + # one callback are asserted directly in + # :meth:`test_hip_padding_sites_are_the_two_known_callbacks_and_nothing_else`, + # so a third expression cannot appear unnoticed. # # The sweep above covers all eight residues, which is enough for any # formula periodic in ``Length % 8`` -- but not for one that is not. @@ -3119,27 +3122,43 @@ def pre_651_total(length: int) -> int: with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): hip_schema.parameter_padding_len({'len': -1}) - def test_hip_padding_helpers_cover_every_parameter_but_locator_set(self) -> None: - """#651/#679: the exclusion is exactly one parameter, and it is ``LOCATOR_SET``. + def test_hip_padding_sites_are_the_two_known_callbacks_and_nothing_else(self) -> None: + """#651/#664/#679: two padding callbacks cover all 46, and no third exists. #651 routed every HIP padding site through :func:`~pcapkit.protocols.schema.internet.hip.parameter_padding_len`, with - one deliberate exception: ``LOCATOR_SET``, whose own two defects cancel so - exactly that correcting its padding alone would take a conformant parameter - to four octets short. #679 fixes that pair together. - - A deliberate exception needs a guard, or it grows. Two failure modes this - catches, and nothing else does: - - * **The exclusion spreading.** A later change that reverts a second - parameter to the old expression -- to make some other pinned literal - pass, say -- would be indistinguishable from this one by inspection. - * **The exclusion evaporating.** Someone "finishing" #651 by pointing - ``LocatorSetParameter.padding`` at the helper would make that parameter - emit ``24n + 4`` octets where the RFC wants ``24n + 8``, and no existing - assertion would fail: the corekit field test's literals are what - ``main`` emits, so they would go red, but from a file whose connection to - HIP padding is not obvious from its name. + one deliberate exception. ``LOCATOR_SET`` carried two defects that + cancelled at the shape its tests sampled -- a ``Length`` in 4-octet units + where :rfc:`7401` Section 5.2.1 counts bytes, and a padding callback + reading the nested ``Locator.len`` instead of the parameter's -- so + correcting either alone made the wire output worse, and #664 documented + the exclusion rather than leaving it to look accidental. + + **#679 fixed the pair, so the exclusion is no longer a hold-back.** + ``LocatorSetParameter.padding`` now uses + :func:`~pcapkit.protocols.schema.internet.hip.locator_set_padding_len`, + which defers to ``parameter_padding_len`` for the arithmetic and differs + from it only in *where it reads the length from*: the + :data:`~pcapkit.protocols.schema.internet.hip.LOCATOR_SET_LEN` snapshot + that :func:`~pcapkit.protocols.schema.internet.hip.locator_set_len_callback` + takes before the nested locators overwrite ``pkt['len']``. + + So the shape this guards has changed, and the reason to keep guarding it + has not. Three failure modes, none of which anything else catches: + + * **A third expression appearing.** A later change that gives some + parameter an inline lambda -- to make a pinned literal pass, say -- + would be indistinguishable from the two legitimate callbacks by + inspection. ``bespoke`` below must stay empty. + * **The ``LOCATOR_SET`` callback regressing to a raw ``pkt['len']`` + read.** That is the pre-#664 state, and it silently re-introduces + defect 1: the value read would be the last locator's 4, so the padding + would be four octets at every locator count again. + * **``LOCATOR_SET`` being "tidied" onto ``parameter_padding_len`` + directly.** That looks like a simplification and is not one: on the + packing path the nested locators have already shadowed ``len`` by then, + so it would read 4 for an IPv6 locator and 5 for an SPI-bearing one + rather than the parameter's byte count. This reads the declared field objects rather than the module source, so it is about what the schemas *do*, not about how they are written. @@ -3149,7 +3168,8 @@ def test_hip_padding_helpers_cover_every_parameter_but_locator_set(self) -> None from pcapkit.protocols.schema.internet import hip as hip_schema on_helper = [] # type: list[str] - excluded = [] # type: list[str] + on_locator_set = [] # type: list[str] + bespoke = [] # type: list[str] unpadded = [] # type: list[str] for name in dir(hip_schema): obj = getattr(hip_schema, name) @@ -3164,15 +3184,24 @@ def test_hip_padding_helpers_cover_every_parameter_but_locator_set(self) -> None callback = getattr(field, '_length_callback', None) if callback is hip_schema.parameter_padding_len: on_helper.append(name) + elif callback is hip_schema.locator_set_padding_len: + on_locator_set.append(name) else: - excluded.append(name) + bespoke.append(name) self.assertEqual( - excluded, ['LocatorSetParameter'], - 'exactly one HIP parameter schema may sit outside ' - 'parameter_padding_len, and it is LocatorSetParameter (see #679). ' - 'If this list grew, the narrowing of #651 has leaked; if it emptied, ' - 'LOCATOR_SET now emits four octets too few.' + bespoke, [], + 'every HIP parameter schema must pad through parameter_padding_len ' + 'or, for LOCATOR_SET alone, locator_set_padding_len. A name here is ' + 'a third padding expression, which is what #651 existed to remove.' + ) + self.assertEqual( + on_locator_set, ['LocatorSetParameter'], + 'exactly one HIP parameter schema reads its padding length from the ' + 'LOCATOR_SET_LEN snapshot, and it is LocatorSetParameter (see #679). ' + 'If this emptied, LOCATOR_SET is reading a len its nested locators ' + 'have already shadowed; if it grew, some other parameter has been ' + 'given a snapshot it has no nested schemas to need.' ) # Three schemas declare no padding field at all, which is correct rather @@ -3187,13 +3216,22 @@ def test_hip_padding_helpers_cover_every_parameter_but_locator_set(self) -> None ['RegFromParameter', 'RelayFromParameter', 'RelayToParameter']) self.assertEqual(len(on_helper), 45) - self.assertEqual(len(on_helper) + len(excluded) + len(unpadded), 49) + self.assertEqual( + len(on_helper) + len(on_locator_set) + len(bespoke) + len(unpadded), 49) - # and the excluded one is the schema registered for LOCATOR_SET, not some - # similarly-named class that merely sorts next to it + # and the one on the snapshot is the schema registered for LOCATOR_SET, + # not some similarly-named class that merely sorts next to it self.assertIs(hip_schema.Parameter.registry[Parameter.LOCATOR_SET], hip_schema.LocatorSetParameter) + # the snapshot callback is installed where it has to be -- on the + # ``locators`` ListField, which resolves before any nested Locator has + # packed -- rather than merely existing in the module + self.assertIs( + getattr(hip_schema.LocatorSetParameter.__fields__['locators'], + '_callback', None), + hip_schema.locator_set_len_callback) + def test_hip_parameter_records_are_eight_octet_aligned_on_the_wire(self) -> None: """#651: the octets a real parameter packs, not just the arithmetic.