From a6d746c6b32ec6544edea8b806149b33c64b298d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 12:06:37 -0400 Subject: [PATCH] fix(tcp): set MPTCP.subtype on construction, fix MP_CAPABLE's length/rkey (#566, #567) TCP(options=[(Enum_Option.Multipath_TCP, {...})]) raised AttributeError: '' object has no attribute 'subtype' for every Multipath TCP subtype but MP_JOIN, and _make_mptcp_capable packed MP_CAPABLE with the wrong length either way. - MPTCP.subtype was declared only under typing.TYPE_CHECKING, so it was an annotation, never a field; the only code that ever set it was _MPTCP.post_process, which runs on a real byte-level unpack, not on the in-memory schema TCP's convenience constructor builds and reads straight back through _read_mptcp_*. Fixed on the construction path (TCP._make_mode_mp, the single dispatcher every _make_mptcp_* maker returns through) rather than by adding a third real field alongside kind/length: subtype is already packed as 4 bits of each subtype's own test bitfield, and a second field for the same bits would either double-pack them or need a "derive, don't pack" field kind this library's corekit.fields does not have. Recorded as a comment on MPTCP itself, since this is the third and last TYPE_CHECKING-only attribute that class had (#566). - _make_mptcp_capable wrote length=20 if rkey is None else 32, where RFC 8684 section 3.1 gives 12 and 20 -- both branches wrong, and the no-key branch writing the other case's value. MPTCPCapable.rkey's own condition (pkt['length'] != 32) independently dropped the receiver's key for exactly the length the maker used to mean "key present", so a 12-octet, key-absent MP_CAPABLE could not be built at all. Fixed together, to 12/20 and pkt['length'] == 20. A third site sharing the same wrong constants, _read_mptcp_capable's length guard and its rkey=... if length == 32 else None, is fixed alongside them -- only reachable once #566 let construction get that far (#567). - Added tests/protocols/transport/test_tcp_mptcp_subtype_unit.py and test_tcp_mptcp_capable_length_unit.py: subtype round-trips through the public TCP() constructor for every buildable subtype, and byte-exact packed assertions for both RFC 8684 MP_CAPABLE forms via the maker, via hand-built octets parsed independently of any maker, and via TCP() end to end. Each confirmed to fail on the pre-fix code. - Re-pointed tests/protocols/test_option_roundtrip_unit.py: six of the seven tcp-mptcp/* EXPECTED_FAILURES entries #541 left pointing at "no attribute 'subtype'" now read 'OK' and are deleted. MP_FASTCLOSE does not: fixing subtype gets it past that AttributeError and into a second, independent defect (its maker, schema and parser disagree on its own length) that #566/#567 do not touch, filed as #576. - test_tcp_udp_unit.py's test_tcp_mptcp_readers_cover_subtype_and_error_branches hand-marked MP_CAPABLE schemas at the pre-#567 lengths (20/32/12) as if that were correct behaviour, which #567 makes wrong; updated to 12/20, with the invalid-length case moved to 32. - Also found, filed separately, not fixed here (#576): the same shape of length-arithmetic defect in MP_FASTCLOSE, MP_JOIN SYN/ACK, MP_JOIN ACK, REMOVE_ADDR, MP_PRIO and DSS. Stacked on #565 (branched from its head, 9001e4cf1); merge after it. Build/test: tests/protocols/transport and tests/protocols pass in full (528 passed, 1412 subtests); the full tier passes in full as well (1281 passed, 17 skipped, 2850 subtests). --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 23 ++ pcapkit/protocols/schema/transport/tcp.py | 24 +- pcapkit/protocols/transport/tcp.py | 27 +- tests/protocols/test_option_roundtrip_unit.py | 71 ++-- .../test_tcp_mptcp_capable_length_unit.py | 304 ++++++++++++++++++ .../transport/test_tcp_mptcp_subtype_unit.py | 235 ++++++++++++++ .../protocols/transport/test_tcp_udp_unit.py | 13 +- 8 files changed, 644 insertions(+), 54 deletions(-) create mode 100644 tests/protocols/transport/test_tcp_mptcp_capable_length_unit.py create mode 100644 tests/protocols/transport/test_tcp_mptcp_subtype_unit.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b7de9a4ce..721b795c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ The largest release since 1.0, and the first recorded here as it happened rather - **Fixed** -- `TCP._make_mptcp_addaddr` could not build an `ADD_ADDR` option end to end: its `kind=`/`length=` arguments were rejected with `UnknownFieldWarning` and silently dropped, and `.pack()` then raised `KeyError: 'length'` from `port`'s own condition, `pkt['length'] in (10, 22)`. The cause was one layer up -- `MPTCP`, the base class every Multipath TCP subtype schema inherits, declared `kind` and `length` only under `typing.TYPE_CHECKING` rather than as real fields, unlike `Option`, which every non-Multipath TCP option schema inherits instead. That silently dropped `kind=`/`length=` for every `_make_mptcp_*` constructor, not only `ADD_ADDR`'s, so `MPTCP` now declares both for real, the same way `Option` already did (#541). The same missing fields broke parsing too: with no `kind`/`length` fields ahead of it, a Multipath TCP subtype schema's own leading field read the `kind` octet itself rather than the octet meant for it, an off-by-two in field alignment rather than a wire-format change -- a correct sender's octets were always right, only this library's reading of them was shifted. Spec-correct `ADD_ADDR` and `MP_PRIO` options failed to parse with `FieldError: TCP: [OptNo 30] 3 invalid IP version` and `KeyError: 'length'` respectively; both parse correctly now. - **Fixed** -- which exception a malformed TCP SACK option raised depended on unrelated process state: a clean interpreter raised `ProtocolError` as documented, but a process that had already popped `pcapkit.corekit.fields.misc` from `sys.modules` -- which the `#439` ABC-cache regression tests do in every case's `setUp`/`tearDown` -- raised `FieldValueError` instead, from a different layer entirely, before the documented check was even reached (#525). The cause was `ListField.unpack` resolving `SchemaField` through a function-local import re-run on every call; a module popped and reimported mid-process comes back as a second, distinct class, so `isinstance` against it silently misclassified the field and billed each item by its declared length instead of by what it actually consumed. **Any caller relying on the previously-observed** `FieldValueError` **for this case now gets** `ProtocolError` **instead, deterministically**, matching the method's own docstring. Fixed by importing at module level instead. - **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during this release and never filed until now: HIP's `_make_param_encrypted` passed `cipher=` to a schema with no such field, so the value was silently dropped and an AES-cipher `ENCRYPTED` parameter built through `make` packed without its IV; and IPv6-Route's `RPL.post_process`, which runs on every `Schema.pack` and not only after a parse, assumed `self.addresses` was still the concatenated `bytes` a parse leaves it as, and raised slicing the `list[bytes]` a `make`-built multi-address header actually holds there (#556). +- **Fixed** -- two more defects #541 exposed rather than caused, both since it let construction reach code that had never run before. `MPTCP.subtype` was still `typing.TYPE_CHECKING`-only, an annotation rather than a field, so `TCP(options=[(Enum_Option.Multipath_TCP, ...)])` raised `AttributeError: ... has no attribute 'subtype'` for every subtype but `MP_JOIN`: the convenience constructor builds a schema in memory and reads it straight back through `_read_mptcp_*` with no byte round trip, so `_MPTCP.post_process` -- the only code that ever set `subtype` -- never ran. Fixed on the construction path (`TCP._make_mode_mp`) rather than by adding a third real field the way `kind`/`length` got in #541: unlike those two, `subtype` is already packed as 4 bits of each subtype's own `test` bitfield, and a second, independent field for the same bits would either double-encode them or need a "derive, don't pack" field kind this library does not have (#566). Separately, `_make_mptcp_capable` wrote `length=20 if rkey is None else 32` where [RFC 8684](https://datatracker.ietf.org/doc/html/rfc8684) section 3.1 gives 12 and 20, and `MPTCPCapable.rkey`'s own condition (`pkt['length'] != 32`) dropped the receiver's key for exactly the length the maker used to mean "key present" -- so a spec-correct, key-absent MP_CAPABLE could not be built at all, and a key-present one silently lost its key on the wire. Both, and the matching guard in `_read_mptcp_capable`, now agree on 12/20. **This changes MP_CAPABLE's packed output**: a 20-octet, key-present option built or parsed under the old code becomes 12 octets with no key, or 20 octets with the key actually present, depending on which the caller meant (#567). Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index fd0a9fb59..23344a4a7 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -389,6 +389,29 @@ pull requests between #326 and #509. still the concatenated ``bytes`` a parse leaves it as, and raised slicing the ``list[bytes]`` a ``make``-built multi-address header actually holds there (#556). +* **Fixed** -- two more defects #541 exposed rather than caused, both since it let + construction reach code that had never run before. ``MPTCP.subtype`` was still + ``typing.TYPE_CHECKING``-only, an annotation rather than a field, so + ``TCP(options=[(Enum_Option.Multipath_TCP, ...)])`` raised + ``AttributeError: ... has no attribute 'subtype'`` for every subtype but + ``MP_JOIN``: the convenience constructor builds a schema in memory and reads it + straight back through ``_read_mptcp_*`` with no byte round trip, so + ``_MPTCP.post_process`` -- the only code that ever set ``subtype`` -- never ran. + Fixed on the construction path (``TCP._make_mode_mp``) rather than by adding a + third real field the way ``kind``/``length`` got in #541: unlike those two, + ``subtype`` is already packed as 4 bits of each subtype's own ``test`` + bitfield, and a second, independent field for the same bits would either + double-encode them or need a "derive, don't pack" field kind this library does + not have (#566). Separately, ``_make_mptcp_capable`` wrote + ``length=20 if rkey is None else 32`` where :rfc:`8684` section 3.1 gives 12 and + 20, and ``MPTCPCapable.rkey``'s own condition (``pkt['length'] != 32``) dropped + the receiver's key for exactly the length the maker used to mean "key present" + -- so a spec-correct, key-absent MP_CAPABLE could not be built at all, and a + key-present one silently lost its key on the wire. Both, and the matching guard + in ``_read_mptcp_capable``, now agree on 12/20. **This changes MP_CAPABLE's + packed output**: a 20-octet, key-present option built or parsed under the old + code becomes 12 octets with no key, or 20 octets with the key actually present, + depending on which the caller meant (#567). Preceded by ``1.5.0a1`` (2026-09-15), ``1.5.0b1`` and ``1.5.0b2`` (both 2026-09-18) and ``1.5.0b3`` (2026-09-19), all published as prereleases and so diff --git a/pcapkit/protocols/schema/transport/tcp.py b/pcapkit/protocols/schema/transport/tcp.py index 739d339de..0e65953de 100644 --- a/pcapkit/protocols/schema/transport/tcp.py +++ b/pcapkit/protocols/schema/transport/tcp.py @@ -645,6 +645,23 @@ class MPTCP(EnumSchema[Enum_MPTCPOption]): #: MPTCP length. length: 'int' = UInt8Field() + # NOTE: ``subtype`` stays an annotation rather than becoming a third real + # field alongside ``kind``/``length`` above -- deliberately, and the + # difference from those two is why. ``kind`` and ``length`` are each the + # *sole* source of their own octet: nothing else in the schema packs them, + # so declaring them as real fields was the only way to get them onto the + # wire at all. ``subtype`` is not like that: every concrete subclass + # already encodes it as 4 bits of its own ``test`` :class:`BitField` (e.g. + # ``MPTCPCapable.test['subtype']``), which is what actually gets packed. + # A real ``Field`` for ``subtype`` on top of that would either pack the + # same 4 bits twice under two names, or need a "derive, don't pack" kind + # of field that this library's :mod:`~pcapkit.corekit.fields` does not + # have. So this attribute is populated by the *construction* path instead + # -- :meth:`~pcapkit.protocols.transport.tcp.TCP._make_mode_mp` sets it + # right after building the subtype-specific schema, mirroring exactly what + # :meth:`_MPTCP.post_process` already does for real unpacking. C.f. #566, + # the third and last ``TYPE_CHECKING``-only attribute this class had; the + # other two (``kind``, ``length``) were fixed in #541. if TYPE_CHECKING: #: MPTCP subtype. subtype: 'Enum_MPTCPOption' @@ -684,9 +701,14 @@ class MPTCPCapable(MPTCP, code=Enum_MPTCPOption.MP_CAPABLE): #: Option sender's key. skey: 'int' = UInt64Field() #: Option receiver's key. + #: + #: :rfc:`8684` section 3.1 gives MP_CAPABLE as 12 octets without this key + #: and 20 octets with it, so the field is present only for the latter -- + #: not, as it read until #567, for every length *except* 32, which is not + #: an MP_CAPABLE length either RFC form uses. rkey: 'int' = ConditionalField( UInt64Field(), - lambda pkt: pkt['length'] != 32, + lambda pkt: pkt['length'] == 20, ) if TYPE_CHECKING: diff --git a/pcapkit/protocols/transport/tcp.py b/pcapkit/protocols/transport/tcp.py index 222546ad2..a17182d7b 100644 --- a/pcapkit/protocols/transport/tcp.py +++ b/pcapkit/protocols/transport/tcp.py @@ -1483,10 +1483,15 @@ def _read_mptcp_capable(self, schema: 'Schema_MPTCPCapable', *, options: 'Option Parsed option data. Raises: - ProtocolError: If length is **NOT** ``20`` or ``32``. + ProtocolError: If length is **NOT** ``12`` or ``20``. """ - if schema.length not in (20, 32): + # NOTE: :rfc:`8684` section 3.1 gives MP_CAPABLE as 12 octets without + # the receiver's key and 20 octets with it -- this guard, and the + # ``rkey=`` below, read ``(20, 32)``/``32`` until #567, which is what + # made a spec-correct 12-octet MP_CAPABLE unparseable and read a + # spec-correct 20-octet one (with the key) as having none. + if schema.length not in (12, 20): raise ProtocolError(f'{self.alias}: [OptNo {schema.kind}] invalid format') data = Data_MPTCPCapable( @@ -1500,7 +1505,7 @@ def _read_mptcp_capable(self, schema: 'Schema_MPTCPCapable', *, options: 'Option hsa=bool(schema.flags['hsa']), ), skey=schema.skey, - rkey=schema.rkey if schema.length == 32 else None, + rkey=schema.rkey if schema.length == 20 else None, ) return data @@ -2601,6 +2606,15 @@ def _make_mode_mp(self, code: 'Enum_Option', opt: 'Optional[Data_MPTCP]' = None, meth = name[1] schema = meth(subtype_val, opt, **kwargs) + # NOTE: ``Schema_MPTCP.subtype`` is not a packable field (see the + # comment on :class:`~pcapkit.protocols.schema.transport.tcp.MPTCP`), + # so nothing above set it -- ``subtype`` only ever went into ``test``, + # the bitfield each concrete maker actually packs. Real unpacking gets + # it from :meth:`~pcapkit.protocols.schema.transport.tcp._MPTCP.post_process`; + # this is that same assignment for the construction path, so a schema + # built via ``TCP(options=[(Enum_Option.Multipath_TCP, ...)])`` has + # ``.subtype`` set exactly as one built by parsing bytes does. C.f. #566. + schema.subtype = subtype_val return schema def _make_mptcp_unknown(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTCPUnknown]' = None, *, @@ -2666,7 +2680,12 @@ def _make_mptcp_capable(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_M return Schema_MPTCPCapable( kind=cast('Enum_Option', Enum_Option.Multipath_TCP), - length=20 if rkey is None else 32, + # NOTE: :rfc:`8684` section 3.1 gives MP_CAPABLE as 12 octets + # without the receiver's key and 20 octets with it. This read + # ``20 if rkey is None else 32`` until #567 -- both branches + # wrong, and the no-key branch writing the value that RFC 8684 + # assigns to the *other* case. + length=12 if rkey is None else 20, test={ 'subtype': subtype.value, 'version': version, diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index 7c94b09dd..9fb255081 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -146,53 +146,32 @@ class Gap(NamedTuple): 'negative below 40 kbps, and the option it emits at rate=0 does not ' 'parse back'), - # #541 declared real ``kind``/``length`` fields on ``MPTCP``, which is what - # gets these seven far enough to construct and pack -- they used to fail - # here with ``KeyError: 'length'`` (four of them) or - # ``AttributeError: ... no attribute 'kind'`` (three), and #541's fix closed - # both. The next field the construction path never sets is ``subtype``: - # ``MPTCP.subtype`` is still ``TYPE_CHECKING``-only, an annotation rather - # than a field, and the only thing that ever sets it is - # ``_MPTCP.post_process``, which runs on a real byte-level unpack -- not on - # the schema a ``_make_mptcp_*`` maker returns in memory, which is what - # ``TCP``'s convenience constructor (``TCP(options=[(code, kwargs)])``) - # reads straight back through ``_read_mptcp_*`` with no round trip in - # between. Filed as #566, which also has the fix for the pattern itself - # (whether ``subtype`` should become a real field the way ``kind``/ - # ``length`` did, or be set some other way) rather than a per-case patch - # here. - 'tcp-mptcp/MP_CAPABLE': Gap( - 'CONSTRUCT', "'MPTCPCapable' object has no attribute 'subtype'", - 'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is ' - 'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566); and ' - "separately, pcapkit/protocols/transport/tcp.py:2669's " - '`length=20 if rkey is None else 32` is RFC 8684 section 3.1\'s 12/20 ' - 'swapped, so once #566 is fixed this case would still pack the wrong ' - 'length rather than round-trip (#567)'), - 'tcp-mptcp/ADD_ADDR': Gap( - 'CONSTRUCT', "'MPTCPAddAddress' object has no attribute 'subtype'", - 'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is ' - 'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'), - 'tcp-mptcp/REMOVE_ADDR': Gap( - 'CONSTRUCT', "'MPTCPRemoveAddress' object has no attribute 'subtype'", - 'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is ' - 'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'), - 'tcp-mptcp/MP_PRIO': Gap( - 'CONSTRUCT', "'MPTCPPriority' object has no attribute 'subtype'", - 'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is ' - 'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'), - 'tcp-mptcp/DSS': Gap( - 'CONSTRUCT', "'MPTCPDSS' object has no attribute 'subtype'", - 'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is ' - 'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'), - 'tcp-mptcp/MP_FAIL': Gap( - 'CONSTRUCT', "'MPTCPFallback' object has no attribute 'subtype'", - 'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is ' - 'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'), + # #541 declared real ``kind``/``length`` fields on ``MPTCP``, which got six + # of these seven far enough to construct and pack, and #566/#567 closed + # the rest of the chain: #566 gave ``MPTCP.subtype`` the same treatment + # (it was ``TYPE_CHECKING``-only, an annotation rather than a field, and + # the only thing that ever set it was ``_MPTCP.post_process``, which runs + # on a real byte-level unpack -- not on the schema a ``_make_mptcp_*`` + # maker returns in memory, which is what ``TCP``'s convenience + # constructor, ``TCP(options=[(code, kwargs)])``, reads straight back + # through ``_read_mptcp_*`` with no round trip in between); #567 fixed + # MP_CAPABLE's own ``length``/``rkey`` arithmetic on top of that. Six of + # the seven (MP_CAPABLE, ADD_ADDR, REMOVE_ADDR, MP_PRIO, DSS, MP_FAIL) now + # read ``'OK'`` and so have no entry below any more. + # + # MP_FASTCLOSE does not: fixing ``subtype`` got it *past* the + # ``AttributeError`` this table used to record and into a second, + # unrelated defect that #566/#567 do not touch and #576 tracks -- see that + # entry for the detail. 'tcp-mptcp/MP_FASTCLOSE': Gap( - 'CONSTRUCT', "'MPTCPFastclose' object has no attribute 'subtype'", - 'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is ' - 'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'), + 'CONSTRUCT', 'TCP: [OptNo 30] invalid format', + 'pcapkit/protocols/transport/tcp.py:1893 -- _read_mptcp_fastclose ' + 'requires schema.length == 16, which agrees with neither the maker ' + '(_make_mptcp_fastclose at pcapkit/protocols/transport/tcp.py:3054, ' + 'length=12, which is the RFC 8684 section 3.7 value) nor the schema ' + '(MPTCPFastclose.test at ' + 'pcapkit/protocols/schema/transport/tcp.py:907, which packs an ' + '11-octet option against the declared 12) (#576)'), # ``_make_mptcp_join`` branches on ``self._flags``, which only the parse # path ever sets, so the constructor cannot be called at all. diff --git a/tests/protocols/transport/test_tcp_mptcp_capable_length_unit.py b/tests/protocols/transport/test_tcp_mptcp_capable_length_unit.py new file mode 100644 index 000000000..fb624bf7d --- /dev/null +++ b/tests/protocols/transport/test_tcp_mptcp_capable_length_unit.py @@ -0,0 +1,304 @@ +# -*- coding: utf-8 -*- +"""MP_CAPABLE's length and its receiver's-key predicate agreed on neither RFC 8684 form. + +GitHub issue #567, found while fixing #541 and widened during #565's cross-review. + +Two independent defects, and both had to move together +-------------------------------------------------------- + +:rfc:`8684` section 3.1 gives ``MP_CAPABLE`` as **12** octets without the receiver's key and +**20** octets with it. + +``TCP._make_mptcp_capable`` (``pcapkit/protocols/transport/tcp.py``) wrote +``length=20 if rkey is None else 32`` -- both branches wrong, and the no-key branch writing +the value RFC 8684 assigns to the *other* case. + +Independently, ``MPTCPCapable.rkey`` (``pcapkit/protocols/schema/transport/tcp.py``) was + +.. code-block:: python + + rkey: 'int' = ConditionalField( + UInt64Field(), + lambda pkt: pkt['length'] != 32, + ) + +-- the receiver's key field packed for every length *except* 32, i.e. dropped for exactly the +length the (also wrong) maker used to signal "key present". Fixing only the maker would have +made a 20-octet, key-present option fail this predicate (``20 != 32`` is true, so the field +would misfire as present regardless -- but a *correct* 12-octet, key-absent option satisfies +``12 != 32`` too, so the key field would still pack, appending 8 phantom octets). Fixing only +the predicate without the maker would leave both of the maker's wrong lengths (20, 32) on the +wire. Neither half alone closes the gap; both are fixed in the same change here, to +``length=12 if rkey is None else 20`` and ``lambda pkt: pkt['length'] == 20`` respectively. + +A third site shared the same wrong constants and would otherwise have re-broken this the +moment #566 let construction reach it: ``TCP._read_mptcp_capable`` rejected anything but +``schema.length in (20, 32)``, and read ``rkey`` only ``if schema.length == 32``. Fixed +alongside the other two, to ``(12, 20)`` and ``== 20``. + +Filed measurement, pre-fix (all three sites still at the old constants): a maker call with +``rkey=None`` packed a *20*-octet option (the value RFC 8684 assigns to the with-key form) +carrying no key octets, and a maker call with an explicit ``rkey`` packed a *32*-octet option +whose last 12 octets are entirely unaccounted for by the RFC 8684 figure. + +Coverage +-------- + +Byte-exact packed assertions for both RFC-defined forms -- 12 octets without the receiver's +key, 20 with it -- rather than asserting either component (the maker's arithmetic, or the +schema's predicate) in isolation, per the pattern #565 established for ``ADD_ADDR``: this +module also splices hand-built, spec-correct octets into a full segment via +``Schema_TCP(options=..., ...)``, bypassing the makers entirely, so the *parse* direction is +pinned independently of whatever the *pack* direction happens to produce. + +""" +from __future__ import annotations + +import unittest + +#: A spec-correct MP_CAPABLE without the receiver's key, RFC 8684 figure 4: ``Kind`` ``0x1e``, +#: ``Length`` ``12``, ``Subtype`` ``0`` (MP_CAPABLE)/``Version`` ``1`` packed into ``0x01``, +#: flag octet ``0x81``, then the sender's key, 8 octets of ``0xAA``. Hand-built, not made, so a +#: symmetric pack/parse bug could not cancel itself out here -- the same octets +#: :data:`tests.protocols.transport.test_tcp_mptcp_length_unit.MP_CAPABLE_OPTION` uses for the +#: header-offset regression, extended here to a full parse. +MP_CAPABLE_NO_KEY_SPEC_OCTETS = bytes([0x1E, 0x0C, 0x01, 0x81]) + b'\xAA' * 8 + +#: The same option, with the receiver's key present: RFC 8684 figure 4's other conditional row +#: (there is one MP_CAPABLE figure, not two -- both length forms come from it), ``Length`` +#: ``20``, the sender's key unchanged, and 8 octets of ``0xBB`` for the receiver's key. +MP_CAPABLE_WITH_KEY_SPEC_OCTETS = bytes([0x1E, 0x14, 0x01, 0x81]) + b'\xAA' * 8 + b'\xBB' * 8 + + +def build_tcp_segment(option_octets: 'bytes') -> 'bytes': + """Pack a whole TCP segment carrying ``option_octets`` verbatim as its only option. + + Goes through :class:`~pcapkit.protocols.schema.transport.tcp.TCP` (the schema, not the + protocol) directly, supplying ``option_octets`` to the ``options`` field as raw bytes, + which :class:`~pcapkit.corekit.fields.collections.OptionField` packs unchanged -- so + nothing here calls a ``_make_mptcp_*`` maker. + + Args: + option_octets: The whole option, header octets included, already padded to a multiple + of 4 octets (both spec octets constants above are). + + Returns: + The packed TCP segment. + + Note: + ``Schema_TCP`` is imported here, inside the function, not at module level -- see + :func:`tests.protocols.transport.test_tcp_mptcp_subtype_unit.build_mptcp_option`'s + own docstring for why: a name bound to ``pcapkit`` at collection time can go stale + once another test module in this suite pops ``pcapkit``'s submodules out of + ``sys.modules`` and a later import re-creates them. + + """ + from pcapkit.protocols.schema.transport.tcp import TCP as Schema_TCP + + if len(option_octets) % 4: + raise ValueError('option_octets must already be a multiple of 4 octets long') + + schema = Schema_TCP( + srcport=50000, dstport=80, seq=1, ack=0, + offset={'offset': 5 + len(option_octets) // 4, 'ns': 0}, + flags={'cwr': 0, 'ece': 0, 'urg': 0, 'ack': 0, 'psh': 0, 'rst': 0, 'syn': 1, 'fin': 0}, + window=8192, checksum=b'\x00\x00', urgent=0, + options=option_octets, + payload=b'', + ) + return schema.pack() + + +def make_capable(**kwargs: 'object') -> 'bytes': + """Build an MP_CAPABLE option through the maker under test and pack it. + + Args: + **kwargs: forwarded to :meth:`TCP._make_mptcp_capable + `. + + Returns: + The packed option bytes. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.protocols.transport.tcp import TCP + + tcp = TCP.__new__(TCP) + schema = tcp._make_mptcp_capable(Enum_MPTCPOption.MP_CAPABLE, **kwargs) # pylint: disable=protected-access + return schema.pack() + + +class TCPMPTCPCapableMakerUnitTests(unittest.TestCase): + """``_make_mptcp_capable`` packs the RFC 8684 length for each form.""" + + def test_no_key_packs_twelve_octets(self) -> None: + """``rkey=None`` packs 12 octets: RFC 8684 figure 4, the key-absent form. + + Pre-fix this packed **20** octets (the with-key value) and carried no key octets at + all -- the maker's own ``length=20 if rkey is None else 32`` read backwards relative + to RFC 8684. + + """ + packed = make_capable(skey=0x0102030405060708, rkey=None) + + self.assertEqual(len(packed), 12) + self.assertEqual(packed[1], 12, 'the packed length octet must match the wire length') + self.assertEqual(packed, bytes([0x1E, 0x0C, 0x00, 0x00]) + bytes.fromhex('0102030405060708')) + + def test_with_key_packs_twenty_octets(self) -> None: + """An explicit ``rkey`` packs 20 octets: RFC 8684 figure 4, the key-present form. + + Pre-fix this packed **32** octets, the last 12 of which are unaccounted for by RFC + 8684 -- the maker wrote ``length=32``, and pre-#567's ``rkey`` predicate + (``pkt['length'] != 32``) then *dropped* the key for this exact length, so those 12 + extra octets were not even the key it claimed to carry. + + """ + packed = make_capable(skey=0x0102030405060708, rkey=0x1112131415161718) + + self.assertEqual(len(packed), 20) + self.assertEqual(packed[1], 20, 'the packed length octet must match the wire length') + self.assertEqual( + packed, + bytes([0x1E, 0x14, 0x00, 0x00]) + + bytes.fromhex('0102030405060708') + + bytes.fromhex('1112131415161718'), + ) + + def test_default_rkey_of_zero_is_still_the_with_key_form(self) -> None: + """``rkey=0`` is a *present* key of value zero, not an absent one. + + ``_make_mptcp_capable``'s own default is ``rkey=0``, not ``None`` -- this is the case + :func:`examples.generators.options.options` actually exercises for MP_CAPABLE, and it + must produce the 20-octet form with an all-zero receiver's key on the wire, not the + 12-octet form. + + """ + packed = make_capable(skey=0x0102030405060708) + + self.assertEqual(len(packed), 20) + self.assertEqual(packed[-8:], bytes(8)) + + +class TCPMPTCPCapableParseUnitTests(unittest.TestCase): + """Spec-correct MP_CAPABLE octets parse for both RFC 8684 forms. + + Neither case here is built through a maker; both are spliced, as raw bytes, into a real + TCP segment and parsed back through :class:`TCP` proper -- so a pack-side bug and a + parse-side bug could not cancel each other out and hide behind a passing round trip. + + """ + + def test_no_key_spec_octets_parse_correctly(self) -> None: + """RFC 8684 figure 4's 12-octet form parses, with ``rkey`` absent. + + Pre-#566/#567 this raised ``ProtocolError: TCP: [OptNo 30] invalid format`` from + ``_read_mptcp_capable``'s ``schema.length not in (20, 32)`` guard -- 12 satisfied + neither of the two (both wrong) accepted values. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + raw = build_tcp_segment(MP_CAPABLE_NO_KEY_SPEC_OCTETS) + tcp = TCP(raw, len(raw)) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.kind, Enum_Option.Multipath_TCP) + self.assertEqual(data.length, 12) + self.assertEqual(data.subtype, Enum_MPTCPOption.MP_CAPABLE) + self.assertEqual(data.version, 1) + self.assertEqual(data.skey, 0xAAAAAAAAAAAAAAAA) + self.assertIsNone(data.rkey) + + def test_with_key_spec_octets_parse_correctly(self) -> None: + """RFC 8684 figure 4's 20-octet form parses, with ``rkey`` present. + + Pre-#566/#567, ``schema.length not in (20, 32)`` did accept 20 -- but + ``rkey=schema.rkey if schema.length == 32 else None`` then discarded it anyway, since + 20 is not 32, so the receiver's key silently vanished rather than being reported. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + raw = build_tcp_segment(MP_CAPABLE_WITH_KEY_SPEC_OCTETS) + tcp = TCP(raw, len(raw)) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.kind, Enum_Option.Multipath_TCP) + self.assertEqual(data.length, 20) + self.assertEqual(data.subtype, Enum_MPTCPOption.MP_CAPABLE) + self.assertEqual(data.version, 1) + self.assertEqual(data.skey, 0xAAAAAAAAAAAAAAAA) + self.assertEqual(data.rkey, 0xBBBBBBBBBBBBBBBB) + + +class TCPMPTCPCapablePublicConstructorUnitTests(unittest.TestCase): + """Both RFC 8684 forms round-trip through the public ``TCP`` convenience constructor.""" + + #: Header fields shared by every constructed TCP segment in this class, matching + #: :data:`examples.generators.options.TCP_BASE`. + TCP_BASE = { + 'srcport': 50000, 'dstport': 80, 'seq': 1, 'ack': 0, + 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack_flag': False, + 'psh': False, 'rst': False, 'syn': True, 'fin': False, + 'window': 8192, 'checksum': b'\x00\x00', 'urgent_pointer': 0, + 'payload': b'', + } + + def test_no_key_round_trip_is_twelve_octets(self) -> None: + """``rkey=None`` through ``TCP()`` packs a 12-octet option and reports no ``rkey``.""" + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + tcp = TCP( + options=[(Enum_Option.Multipath_TCP, { + 'subtype': Enum_MPTCPOption.MP_CAPABLE, + 'skey': 0x0102030405060708, + 'rkey': None, + })], + **self.TCP_BASE, # type: ignore[arg-type] + ) + data = tcp.info.options[Enum_Option.Multipath_TCP] + packed = bytes(tcp) + + self.assertEqual(data.length, 12) + self.assertIsNone(data.rkey) + # NOTE: the option is the only thing in the segment's payload-free options list, so it + # is the last 12 octets of the packed segment -- the fixed 20-octet TCP header plus + # this option is the whole packet. + self.assertEqual(packed[-12:], bytes([0x1E, 0x0C, 0x00, 0x00]) + bytes.fromhex('0102030405060708')) + + def test_with_key_round_trip_is_twenty_octets(self) -> None: + """An explicit ``rkey`` through ``TCP()`` packs a 20-octet option and reports it back.""" + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + tcp = TCP( + options=[(Enum_Option.Multipath_TCP, { + 'subtype': Enum_MPTCPOption.MP_CAPABLE, + 'skey': 0x0102030405060708, + 'rkey': 0x1112131415161718, + })], + **self.TCP_BASE, # type: ignore[arg-type] + ) + data = tcp.info.options[Enum_Option.Multipath_TCP] + packed = bytes(tcp) + + self.assertEqual(data.length, 20) + self.assertEqual(data.rkey, 0x1112131415161718) + self.assertEqual( + packed[-20:], + bytes([0x1E, 0x14, 0x00, 0x00]) + + bytes.fromhex('0102030405060708') + + bytes.fromhex('1112131415161718'), + ) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py b/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py new file mode 100644 index 000000000..47d40d8ef --- /dev/null +++ b/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py @@ -0,0 +1,235 @@ +# -*- coding: utf-8 -*- +"""``MPTCP.subtype`` is set for a schema built in memory, not only one parsed from bytes. + +GitHub issue #566, found while fixing #541 -- the same defect the ``EXPECTED_FAILURES`` +table in :mod:`tests.protocols.test_option_roundtrip_unit` recorded for +``tcp-mptcp/MP_CAPABLE``, ``tcp-mptcp/ADD_ADDR``, ``tcp-mptcp/REMOVE_ADDR``, +``tcp-mptcp/MP_PRIO``, ``tcp-mptcp/DSS`` and ``tcp-mptcp/MP_FAIL`` until this fix, each with +a fragment of the form ``"'MPTCPCapable' object has no attribute 'subtype'"``. + +Root cause +---------- + +:class:`~pcapkit.protocols.schema.transport.tcp.MPTCP`, the base class every Multipath TCP +subtype schema inherits, declared ``subtype`` only under :data:`~typing.TYPE_CHECKING`: + +.. code-block:: python + + if TYPE_CHECKING: + #: MPTCP subtype. + subtype: 'Enum_MPTCPOption' + +so it was an annotation for a type checker, never a real attribute any construction path +set. The only code that ever assigned ``schema.subtype`` was +:meth:`_MPTCP.post_process `, +which runs once per option while parsing real bytes off the wire. Building an option through +:class:`~pcapkit.protocols.transport.tcp.TCP`'s convenience constructor -- +``TCP(options=[(Enum_Option.Multipath_TCP, {'subtype': ..., **kwargs})], ...)`` -- never +unpacks anything: the dispatcher (``TCP._make_mode_mp``) calls the matching ``_make_mptcp_*`` +maker to build the schema in memory and then hands that same schema straight to the matching +``_read_mptcp_*`` to build the :class:`~pcapkit.protocols.data.transport.tcp.MPTCP` data +model -- and every one of those eleven ``_read_mptcp_*`` methods reads ``schema.subtype``. +Filed measurement, pre-fix: ``TCP(options=[(Enum_Option.Multipath_TCP, {'subtype': +Enum_MPTCPOption.MP_FAIL, 'dsn': 7})], ...)`` raised +``AttributeError: 'MPTCPFallback' object has no attribute 'subtype'``. + +Why a real field was not the fix +--------------------------------- + +#541 fixed the identical shape of problem for ``kind``/``length`` by declaring them as real +fields on ``MPTCP``, and the obvious next step is to do the same for ``subtype``. That would +be wrong here, and the difference is worth recording since this is the third +``TYPE_CHECKING``-only attribute this one class has had (``kind``, ``length`` in #541, +``subtype`` here) and a reader deserves to know the pattern was considered as a whole rather +than patched a third time by reflex. + +``kind`` and ``length`` are each the *sole* source of their own octet -- nothing else in any +subtype schema packs them, so a real field was the only way to get them onto the wire at all. +``subtype`` is not like that: every concrete subclass already encodes it as 4 bits of its own +``test`` :class:`~pcapkit.corekit.fields.strings.BitField` (e.g. +``MPTCPCapable.test['subtype']``), which is what the wire format actually carries. Declaring +a second, independent ``Field`` for the same 4 bits would either double-pack them under two +names, or need a "derive this from another field, do not pack it yourself" kind of field -- +which nothing in :mod:`pcapkit.corekit.fields` provides (every ``FieldBase`` subclass either +contributes its own bytes or selects among nested schemas; none merely mirrors a sibling +field's decoded value into a second attribute). So ``MPTCP.subtype`` stays an annotation, and +the fix is on the *construction* path instead: :meth:`TCP._make_mode_mp +`, the single dispatcher every +``_make_mptcp_*`` maker returns through, now sets ``schema.subtype = subtype_val`` right after +building the schema -- the same assignment :meth:`_MPTCP.post_process` already makes for a +real unpack, just made unconditionally. + +That leaves no other ``TYPE_CHECKING``-only *data* attribute on ``MPTCP`` itself: the class +body has exactly ``kind``, ``length`` (both real fields since #541) and ``subtype`` (fixed +here by the construction path). The ``if TYPE_CHECKING: def __init__(...): ...`` blocks inside +the concrete subclasses below it are ordinary typed-signature stubs for the ``__init__`` that +:class:`~pcapkit.protocols.schema.schema.Schema` synthesises at runtime, not latent fields, so +they are not instances of this pattern. + +Coverage +-------- + +Every case here goes through the *public* ``TCP`` convenience constructor, not +``_make_mptcp_*`` directly, because that in-memory, no-byte-round-trip path is exactly what +exposed the defect. ``MP_JOIN`` is left out: it fails independently with +``AttributeError: 'TCP' object has no attribute '_flags'`` (``_make_mptcp_join`` reads +``self._flags``, which only the parse path ever sets), which is not this issue. ``MP_FASTCLOSE`` +is also left out, for a reason this fix does not touch: fixing ``subtype`` gets its +construction *past* the ``AttributeError`` this module would otherwise still see and into a +second, independent defect (its declared length agrees with neither its own maker nor its own +parser) tracked as GitHub issue #576. + +""" +from __future__ import annotations + +import unittest + +#: Header fields shared by every constructed TCP segment in this module, matching +#: :data:`examples.generators.options.TCP_BASE` so these cases build through exactly the +#: keyword shape the fixture generator uses. +TCP_BASE = { + 'srcport': 50000, 'dstport': 80, 'seq': 1, 'ack': 0, + 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack_flag': False, + 'psh': False, 'rst': False, 'syn': True, 'fin': False, + 'window': 8192, 'checksum': b'\x00\x00', 'urgent_pointer': 0, + 'payload': b'', +} + + +def build_mptcp_option(subtype: 'object', **kwargs: 'object') -> 'object': + """Build a whole TCP segment carrying one Multipath TCP option, through the public API. + + Args: + subtype: MPTCP subtype to construct. + **kwargs: forwarded to the matching ``_make_mptcp_*`` maker as the option's own + arguments. + + Returns: + The constructed :class:`~pcapkit.protocols.transport.tcp.TCP` protocol instance. + + Note: + ``pcapkit.protocols.transport.tcp`` and ``pcapkit.const.tcp.option`` are imported + here, inside the function, rather than at module level. Several test modules + elsewhere in this suite (see e.g. + :func:`tests._support.purge_modules`) pop ``pcapkit``'s submodules out of + ``sys.modules`` and let a later import re-create them, which produces a *new* + ``Schema`` class object -- so a name bound at collection time (module level) can + end up pointing at a schema class built from a stale ``Schema`` base, while + :meth:`~pcapkit.corekit.fields.collections.OptionField.pack`'s own delayed + ``from pcapkit.protocols.schema.schema import Schema`` resolves fresh against + whatever is current when it actually runs. The two then fail + ``isinstance(item, Schema)`` against each other despite both being named + ``Schema``. Importing here instead, at call time, keeps this function on + whichever ``pcapkit`` the rest of the suite is currently using -- the same + precaution :meth:`tests.protocols.transport.test_tcp_udp_unit.TCPUDPUnitTests.test_tcp_mptcp_readers_cover_subtype_and_error_branches` + already takes for the identical reason. + + """ + from pcapkit.const.tcp.option import Option as Enum_Option + from pcapkit.protocols.transport.tcp import TCP + + args = dict(kwargs) + args['subtype'] = subtype + return TCP(options=[(Enum_Option.Multipath_TCP, args)], **TCP_BASE) # type: ignore[arg-type] + + +class TCPMPTCPSubtypeUnitTests(unittest.TestCase): + """Every constructible MPTCP subtype round-trips ``.subtype`` through ``TCP()``. + + Pre-fix, every one of these raised ``AttributeError: '' object has no + attribute 'subtype'`` -- construction never got as far as returning a + :class:`~pcapkit.protocols.transport.tcp.TCP` instance at all, so there is nothing to + assert on the resulting data model until the fix lands. + + """ + + def test_mp_capable_subtype_round_trips(self) -> None: + """``MP_CAPABLE`` built through ``TCP()`` reports its own subtype.""" + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + + tcp = build_mptcp_option(Enum_MPTCPOption.MP_CAPABLE, skey=0x0102030405060708) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.MP_CAPABLE) + + def test_add_addr_subtype_round_trips(self) -> None: + """``ADD_ADDR`` built through ``TCP()`` reports its own subtype.""" + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + + tcp = build_mptcp_option(Enum_MPTCPOption.ADD_ADDR, addr_id=1, addr='192.0.2.1') + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.ADD_ADDR) + + def test_remove_addr_subtype_round_trips(self) -> None: + """``REMOVE_ADDR`` built through ``TCP()`` reports its own subtype.""" + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + + tcp = build_mptcp_option(Enum_MPTCPOption.REMOVE_ADDR, addr_id=[1]) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.REMOVE_ADDR) + + def test_mp_prio_subtype_round_trips(self) -> None: + """``MP_PRIO`` built through ``TCP()`` reports its own subtype.""" + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + + tcp = build_mptcp_option(Enum_MPTCPOption.MP_PRIO, addr_id=1) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.MP_PRIO) + + def test_dss_subtype_round_trips(self) -> None: + """``DSS`` built through ``TCP()`` reports its own subtype. + + ``ack`` alone (no ``dsn``) keeps clear of the unrelated length-arithmetic defect + tracked as #576, which is not what this test is about. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + + tcp = build_mptcp_option(Enum_MPTCPOption.DSS, ack=100) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.DSS) + + def test_mp_fail_subtype_round_trips(self) -> None: + """``MP_FAIL`` built through ``TCP()`` reports its own subtype.""" + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.const.tcp.option import Option as Enum_Option + + tcp = build_mptcp_option(Enum_MPTCPOption.MP_FAIL, dsn=7) + data = tcp.info.options[Enum_Option.Multipath_TCP] + + self.assertEqual(data.subtype, Enum_MPTCPOption.MP_FAIL) + + def test_mp_fastclose_still_fails_but_no_longer_on_subtype(self) -> None: + """``MP_FASTCLOSE`` no longer raises the ``subtype`` ``AttributeError`` this fixes. + + It still cannot be built through ``TCP()`` -- #576, not this issue -- so this pins + that the *remaining* failure is the length mismatch, not a regression back to the + defect this module is otherwise about. + + """ + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaises(ProtocolError) as ctx: + build_mptcp_option(Enum_MPTCPOption.MP_FASTCLOSE, key=9) + + # NOTE: bare 'invalid format' occurs at some 205 sites across 13 modules (see + # Gap.fragment's docstring in test_option_roundtrip_unit.py), so it does not pin + # anything on its own. The alias plus the bracketed option-number code this message + # actually carries -- 'TCP: [OptNo 30] invalid format' -- narrows it to the one option + # that can print 30 (Multipath_TCP). + self.assertIn('TCP: [OptNo 30] invalid format', str(ctx.exception)) + self.assertNotIn('subtype', str(ctx.exception)) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 4971856d4..5a47fd5eb 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -901,10 +901,17 @@ def mark(schema, length: int, subtype: MPTCPOption): ) self.assertEqual(proto._read_mode_mp(unknown, options=options).data, b'\x0a\x01') + # NOTE: :rfc:`8684` section 3.1 gives MP_CAPABLE as 12 octets without the + # receiver's key and 20 octets with it (#567); these three cases used + # 20/32/12 respectively, the pre-#567 (wrong) split, and so were + # exercising -- and pinning -- the very defect #567 fixes rather than + # correct behaviour. Updated to 12/20, with the third case's length + # moved to 32, which is not an MP_CAPABLE length under either the old + # or the new split, so it stays a genuine rejection. capable_no_receiver = mark( MPTCPCapable(test={'subtype': MPTCPOption.MP_CAPABLE.value, 'version': 0}, flags={'req': 1, 'ext': 0, 'hsa': 1}, skey=1, rkey=2), - 20, + 12, MPTCPOption.MP_CAPABLE, ) capable = proto._read_mode_mp(capable_no_receiver, options=options) @@ -915,7 +922,7 @@ def mark(schema, length: int, subtype: MPTCPOption): capable_with_receiver = mark( MPTCPCapable(test={'subtype': MPTCPOption.MP_CAPABLE.value, 'version': 0}, flags={'req': 0, 'ext': 1, 'hsa': 0}, skey=1, rkey=2), - 32, + 20, MPTCPOption.MP_CAPABLE, ) self.assertEqual(proto._read_mode_mp(capable_with_receiver, options=options).rkey, 2) @@ -923,7 +930,7 @@ def mark(schema, length: int, subtype: MPTCPOption): proto._read_mode_mp(mark( MPTCPCapable(test={'subtype': MPTCPOption.MP_CAPABLE.value, 'version': 0}, flags={'req': 0, 'ext': 0, 'hsa': 0}, skey=1, rkey=None), - 12, + 32, MPTCPOption.MP_CAPABLE, ), options=options)