fix(hip): pad HIP parameters to the record length RFC 7401 gives, not the contents (#651) - #664
Conversation
JarryShaw
added
fix
749fb9a to
32b6b65
Compare
…t values (#653, #654, #655) `_make_param_puzzle` and `_make_param_solution` derived three wire-format quantities from the payload value rather than taking them from the data model. All three derivations were wrong, in the same two functions, and they are fixed together because the width resolution is one expression that cannot be written twice. * The field width came from `int.bit_length()` and nothing else, so every leading zero octet was dropped on re-serialisation. A SOLUTION read with `Length = 20` rebuilt as `Length = 6`, a PUZZLE read with `Length = 12` as `Length = 5` -- silently, since the integers survive and nothing raises. Both data models now carry `rhash_len`, the field's on-wire width in bits, and both builders prefer it. This only became reachable end to end once #608 was fixed (#629); before that the undersized rebuild tripped the reader's parity guard first and failed loudly. (#653) * SOLUTION's second contents octet is `Reserved`, "zero when sent, ignored when received" (RFC 7401 5.2.5, RFC 5201 5.2.5 identically) -- not a PUZZLE `Lifetime`, which only RFC 7401 5.2.4 defines. pcapkit wrote `0x20`, `0x21`, `0x25` or `0x2b` there and could not write the mandated zero at all: a conformant SOLUTION with `Reserved = 0x00` parsed to `timedelta(0)` and then escaped a bare `ValueError` from `math.log2(0.0)`. `Data_SolutionParameter` and `SolutionParameter` now carry `reserved` verbatim, so the conformant zero round-trips and a received non-zero octet is reproduced rather than re-derived. (#654) * Neither builder read its own `version` keyword, so `version=1` and `version=2` computed identical lengths at every bit width. RFC 5201 5.2.4 and 5.2.5 state both fields as literally 8 bytes and `Length` as literally 12 and 20, and the readers enforce exactly that -- so under HIPv1 each builder accepted only a `bit_length()` of 57..64 and built, for everything else, a parameter this library's own reader rejects. Width now comes from the version under HIPv1. (#655) The remaining `math.log2` sites, both in PUZZLE where a `Lifetime` really lives, now raise `ProtocolError` rather than letting `ValueError` escape. `ProtocolError(BaseError, ValueError)` is what the readers already raise for a malformed PUZZLE or SOLUTION, and deriving from the builtin it replaces keeps any caller written around today's bare `ValueError` working; `EnumError` is `(BaseError, TypeError)` and would silently stop being caught. The same guard covers the upper end, because `UInt8Field` wraps rather than raising -- measured, `300` packs as `0x2c` -- so an out-of-range lifetime would otherwise be written as some other valid-looking duration. A plain `float` lifetime used to escape an `AttributeError` from the `isinstance(lifetime, int)` branch; the test keyed on `timedelta` instead found that. `_make_param_solution` no longer accepts `lifetime=`. `reserved=` and `rhash_len=` are both `None`-sentinelled, so an explicit value overrides what a parsed parameter carries: `Data_SolutionParameter` is immutable, so without that a caller holding a parsed parameter had no way to write the conformant zero over a peer's non-conformant `Reserved`. The plain data fields still let `param` win, as they did before. `rhash_len=` is also the only way a from-scratch HIPv2 build can state a width, which genuinely varies with the Responder's HIT Suite (RFC 7401 2.3, 5.2.10). Five new tests, each verified to fail on 0c7f2b7 for the defect's own reason and pass here: `12 != 5` and friends for #655, `expected a positive input` and `'reserved' not found` for #654, `no attribute 'rhash_len'` for #653. The widths exercised are 1, 9, 15, 17, 57 and 65 bits, where the candidate formulas disagree, with the byte-aligned widths kept as controls -- at a multiple of 8 the correct `2 * ceil(b / 8)` coincides with #608's `ceil(b / 4)`, which is why every byte-aligned fixture passed through that defect unharmed. The byte-exact assertions compare the parameter without its trailing padding, and then against the re-packed source schema rather than against a literal, so they pin the `Length` field and the payload octets without encoding a padding rule that #651/#664 is concurrently changing. Verified against a `git merge-tree` of this branch and #664: both library files auto-merge with no conflict, and all five new tests pass against the merged library. Two cases are deliberately accepted rather than rejected, and now say so in `_make_puzzle_field_width`'s docstring: `rhash_len == 0`, which is what the derived path yields for the default `random=0` and what a `Length = 4` parameter parses back to; and a `version` other than 1, which is treated as HIPv2 exactly as both readers' `version == 1` guards do. Coverage holds at 100% statement and branch on all three changed modules, with statements 1385 -> 1414 and branches 316 -> 338; 30 tests / 434 subtests -> 35 / 455. Scoped unit tier green: 291 passed, 1191 subtests. No EXPECTED_FAILURES entry moved -- still 45, with the same four HIP entries. No changelog entry on this branch: that is consolidated in #657.
Cross-review: GOOD TO GOIndependent cross-review of Verdict: GOOD TO GO, with four follow-ups, none of them blocking. All four What it confirmed independently
What it disputed, and what changed as a result
Two corrections in the other directionReported for the record, because a cross-review is only worth having if it is
Could not verifyNo real third-party HIP capture was found. The Wireshark SampleCaptures wiki On the
|
The bullet #664 would otherwise have carried, kept here so that #664 touches only `pcapkit/protocols/internet/hip.py`, `pcapkit/protocols/schema/internet/hip.py`, `tests/protocols/internet/test_hip_unit.py`, `tests/protocols/test_option_roundtrip_unit.py`, `examples/generators/options.py` and `docs/source/pcapkit/protocols/internet/hip.rst`. One bullet, because it is one root cause in 95 places: every HIP padding site aligned the parameter's *contents* to eight octets rather than the record, ignoring the four-octet type-and-length header, so every parameter pcapkit wrote was `4 (mod 8)` for every possible `Length`. The bullet says in bold that both the emitted octets and the data model's reported `length` change, and carries a migration sentence: a `SEQ` parameter's `length` is 8 where it was 12, so code comparing stored output byte for byte or asserting on `Data_*Parameter.length` sees different values. It also records what was deliberately *not* changed, since both look like part of the same defect and are not: `HIP.make`'s `len = total_length // 8 + 4`, which RFC 7401 section 5.1.3 shows is correct and merely needed 8-aligned parameters; and `HIP_COPIES`, which stays at two for `R1_COUNTER`'s four-octet `counter` against section 5.2.3's eight -- a separate, still-unfiled defect this one had been masking. The `EncryptedParameter.data` length callback is named as fixed in the same change because the two four-octet errors cancelled at four of the eight residues of `Length`, so correcting the padding alone would have regressed it. 41 lines added to the entry file; `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited. `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests. Committed from a detached HEAD on 2ce3687 and pushed to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree at a stale f846523 and could not be taken here. Note 2ce3687, not the 69a6e13 I was given: the branch had already moved on with #665's entry.
Interaction with #665 — not order-independent, and the reason is in #665's fixturesRecorded here rather than only in a handoff note, because whoever merges these two
But the two are coupled on correctness, in both directions, and the coupling is The cause, verified directly against #665's diff: its hand-written wire literals wire = bytes.fromhex('0141' '0014' '01' '20' '6f70'
'0000000000000001' '0000000000000001' '00000000')
self.assertEqual(len(wire), 28)That is a Four literals need the trailing The coupling runs the other way too, and one line of this PR is on the hook. Recommended order: this PR first, then rebase #665 and make both edits as part One thing that is not an interaction, checked because it looked like one: HIPv1 |
Correction: #664 and #665 are order-independent. My earlier comment was wrong.Retracting the load-bearing claim of What I got wrong, and howI did not build or run the merge before posting that comment. I verified the The conclusion is false, because #665's author already made those tests # Compared without the trailing padding, and then against the re-packed
# source schema rather than against the literal. Both are deliberate: the
# padding rule is itself in flight (#651/#664) ...
self.assertEqual(bytes(rebuilt)[:4 + rebuilt.len], wire[:4 + unpacked.len])
self.assertEqual(bytes(rebuilt), bytes(unpacked))The first assertion slices the padding off; the second compares two schemas The run I should have done firstTwo disposable clones under git clone --shared --no-checkout <worktree> repo
git branch -f pr664 32b6b65a4 && git branch -f pr665 ebc929cd2
git checkout pr665 && git merge --no-ff --no-edit pr664 # and the reverseBoth library files auto-merge; one conflict, Provenance asserted in-process before any other import, with Exit codes read from a file, not off a pipeline:
What survives from the original note
Why the earlier finding was ever trueNot stale by sha on my side — |
d0d8b8c to
819dbbd
Compare
819dbbd to
9f17491
Compare
…ly fixes #664 was narrowed per the owner's decision in #679: `LOCATOR_SET` is excluded and keeps the pre-#651 padding expression, so the change corrects 45 of the 46 HIP parameters rather than all of them. This entry claimed all of them, which would have shipped a false statement about wire output in the release notes. Three claims narrowed, in place rather than as a second bullet: - the opening, from "every HIP parameter" to 45 of 46, naming the exclusion; - the site counts, from "the 46 callbacks and the 49 record lengths ... instead of 95 times" to 93 of the 95 sites, 45 of 46 and 48 of 49; - the migration sentence, which said every other parameter's `length` moves likewise -- now the other 44, with `LOCATOR_SET` called out as unchanged in both its octets and its data model. Added the reason for the exclusion, because a reader who meets it in the code otherwise cannot tell it from an oversight: two defects in that parameter cancel exactly -- the padding callback never receives the parameter's `len`, since the nested `Locator` schemas share a packet context whose own `len` shadows it, and the parameter's `len` is in 4-octet units where the RFC's `Length` is a byte count. Always-4 padding gives `4 + 24n + 4`, and because `24n` is a multiple of 8 the RFC total for a byte-count `Length` of `24n` is the same `24n + 8`. Measured at n = 1, 2, 5 as 32, 56 and 128 octets on `b34f132f6` and on #664's head alike, so correcting only the padding would have taken a conformant parameter to four octets short. #679 carries the pair. 32 lines changed in the entry file; `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited. `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests. Committed from a detached HEAD on aef0fb9 and pushed fast-forward to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree and could not be taken here.
…t values (#653, #654, #655) (#665) `_make_param_puzzle` and `_make_param_solution` derived three wire-format quantities from the payload value rather than taking them from the data model. All three derivations were wrong, in the same two functions, and they are fixed together because the width resolution is one expression that cannot be written twice. * The field width came from `int.bit_length()` and nothing else, so every leading zero octet was dropped on re-serialisation. A SOLUTION read with `Length = 20` rebuilt as `Length = 6`, a PUZZLE read with `Length = 12` as `Length = 5` -- silently, since the integers survive and nothing raises. Both data models now carry `rhash_len`, the field's on-wire width in bits, and both builders prefer it. This only became reachable end to end once #608 was fixed (#629); before that the undersized rebuild tripped the reader's parity guard first and failed loudly. (#653) * SOLUTION's second contents octet is `Reserved`, "zero when sent, ignored when received" (RFC 7401 5.2.5, RFC 5201 5.2.5 identically) -- not a PUZZLE `Lifetime`, which only RFC 7401 5.2.4 defines. pcapkit wrote `0x20`, `0x21`, `0x25` or `0x2b` there and could not write the mandated zero at all: a conformant SOLUTION with `Reserved = 0x00` parsed to `timedelta(0)` and then escaped a bare `ValueError` from `math.log2(0.0)`. `Data_SolutionParameter` and `SolutionParameter` now carry `reserved` verbatim, so the conformant zero round-trips and a received non-zero octet is reproduced rather than re-derived. (#654) * Neither builder read its own `version` keyword, so `version=1` and `version=2` computed identical lengths at every bit width. RFC 5201 5.2.4 and 5.2.5 state both fields as literally 8 bytes and `Length` as literally 12 and 20, and the readers enforce exactly that -- so under HIPv1 each builder accepted only a `bit_length()` of 57..64 and built, for everything else, a parameter this library's own reader rejects. Width now comes from the version under HIPv1. (#655) The remaining `math.log2` sites, both in PUZZLE where a `Lifetime` really lives, now raise `ProtocolError` rather than letting `ValueError` escape. `ProtocolError(BaseError, ValueError)` is what the readers already raise for a malformed PUZZLE or SOLUTION, and deriving from the builtin it replaces keeps any caller written around today's bare `ValueError` working; `EnumError` is `(BaseError, TypeError)` and would silently stop being caught. The same guard covers the upper end, because `UInt8Field` wraps rather than raising -- measured, `300` packs as `0x2c` -- so an out-of-range lifetime would otherwise be written as some other valid-looking duration. A plain `float` lifetime used to escape an `AttributeError` from the `isinstance(lifetime, int)` branch; the test keyed on `timedelta` instead found that. `_make_param_solution` no longer accepts `lifetime=`. `reserved=` and `rhash_len=` are both `None`-sentinelled, so an explicit value overrides what a parsed parameter carries: `Data_SolutionParameter` is immutable, so without that a caller holding a parsed parameter had no way to write the conformant zero over a peer's non-conformant `Reserved`. The plain data fields still let `param` win, as they did before. `rhash_len=` is also the only way a from-scratch HIPv2 build can state a width, which genuinely varies with the Responder's HIT Suite (RFC 7401 2.3, 5.2.10). Five new tests, each verified to fail on 0c7f2b7 for the defect's own reason and pass here: `12 != 5` and friends for #655, `expected a positive input` and `'reserved' not found` for #654, `no attribute 'rhash_len'` for #653. The widths exercised are 1, 9, 15, 17, 57 and 65 bits, where the candidate formulas disagree, with the byte-aligned widths kept as controls -- at a multiple of 8 the correct `2 * ceil(b / 8)` coincides with #608's `ceil(b / 4)`, which is why every byte-aligned fixture passed through that defect unharmed. The byte-exact assertions compare the parameter without its trailing padding, and then against the re-packed source schema rather than against a literal, so they pin the `Length` field and the payload octets without encoding a padding rule that #651/#664 is concurrently changing. Verified against a `git merge-tree` of this branch and #664: both library files auto-merge with no conflict, and all five new tests pass against the merged library. Two cases are deliberately accepted rather than rejected, and now say so in `_make_puzzle_field_width`'s docstring: `rhash_len == 0`, which is what the derived path yields for the default `random=0` and what a `Length = 4` parameter parses back to; and a `version` other than 1, which is treated as HIPv2 exactly as both readers' `version == 1` guards do. Coverage holds at 100% statement and branch on all three changed modules, with statements 1385 -> 1414 and branches 316 -> 338; 30 tests / 434 subtests -> 35 / 455. Scoped unit tier green: 291 passed, 1191 subtests. No EXPECTED_FAILURES entry moved -- still 45, with the same four HIP entries. No changelog entry on this branch: that is consolidated in #657.
… the contents Every padding site in the two HIP modules computed the pad from the contents length alone -- `(8 - (pkt['len'] % 8)) % 8` -- which aligns `Length` rather than `Length + 4`. RFC 7401 section 5.2.1 requires the *total* length of a TLV parameter, Type and Length fields included, to be a multiple of 8, and states the arithmetic outright: `Total Length = 11 + Length - (Length + 3) % 8`. Aligning the contents instead put every parameter pcapkit wrote at 4 (mod 8), for every possible `Length`: measured across 0..63, never a multiple of 8 and never the RFC's answer, both over- and under-padding by turns. - Replace 93 of the 95 copies of that expression -- 45 of the 46 `PaddingField` callbacks in `schema/internet/hip.py` and 48 of the 49 data-model record lengths in `internet/hip.py` -- with one `parameter_total_len()` stating the RFC formula once, plus a `parameter_padding_len()` the padding fields share. So this is a padding correction for **45 of the 46 HIP parameters**. - Leave `LOCATOR_SET` on the old expression, deliberately, at both of its sites, with the reason at the site rather than as a cross-reference. Two defects there cancel exactly: `padding`'s callback never receives the parameter's `len` (`ListField` packs each nested `Locator` into the shared packet context, whose `len` shadows it, so the value seen is always 4 for an IPv6 locator), and the parameter's `len` is `sum(Locator.len)` in 4-octet units where the RFC's `Length` is a byte count. Always-4 padding gives `4 + 24n + 4 = 24n + 8`, and because `24n` is a multiple of 8 the RFC total for `Length = 24n` is the same `24n + 8`. Measured at n = 1, 2, 5 on this branch and on `a18846c8f`: 32, 56 and 128 octets on both, equal to the RFC total each time. Correcting the padding alone would leave `24n + 4`, so #679 fixes the pair together. That cancellation covers plain IPv6 locators only, and the comment says so: an empty set packs 4 octets where the RFC wants 8, one SPI locator packs 35, two pack 63, and a mixed pair packs 59 or 60 -- none 8-aligned, and all byte-identical before and after, which is the point. A new `test_hip_padding_helpers_cover_every_parameter_but_locator_set` asserts the exclusion is exactly one parameter and which one, so it cannot grow or vanish unnoticed. - Fix `EncryptedParameter.data`'s length callback to subtract the four `reserved` octets as well as the `iv`. Not opportunism: the two four-octet errors cancelled at four of the eight residues of `Length` (measured, `Length % 8` in {0, 5, 6, 7}), so fixing the padding alone would have taken `ENCRYPTED` from right at those four to four octets too long at all eight. - `HIP.make`'s `total_length // 8 + 4` was *not* wrong; it is exact now that each parameter is 8-aligned, and RFC 7401 section 5.1.3 defines Header Length as the header and parameters in 8-byte units excluding the first 8. - Invert the pin the old behaviour had: a one-parameter HIP packet is accepted, not rejected. `HIP_COPIES` stays at 2 for unrelated defects its note now names, chiefly `R1_COUNTER`'s 4-octet `counter` where section 5.2.3 requires 8 (filed as #672). - Drop `hip-parameter/ENCRYPTED` from `EXPECTED_FAILURES`; its cycle closes. - Add a note, and no change in value, to the two LOCATOR_SET byte pins in `tests/corekit/test_fields_ipaddress.py`. Those literals went red on an earlier revision of this branch, which is what surfaced the cancellation; with LOCATOR_SET excluded they are correct again and byte-identical to `a18846c8f` -- a diff of that file against origin/main shows two comment lines and no changed literal. The note records why they are already right, and why a shorter pin here would silently bless the four-octet shortfall. Verified against the RFC rather than against a round trip, which cannot see this defect because pcapkit's writer and reader shared the error. The arithmetic is checked over all eight residues as subtests and over the whole 16-bit domain of the `len` field, so a formula that agrees on small values and diverges later cannot pass. HIP unit and round-trip suites 39 passed / 528 subtests against `a18846c8f`'s 35 / 455, and the RFC-only walk over the regenerated `options-internet.pcap` goes from 43 violations to 1 -- the remaining one being the empty `LOCATOR_SET`, which packs 4 octets where the RFC wants 8, identically on `a18846c8f`, and is owned by #679. Fixes #651
4729f3b to
45a3e1d
Compare
Cross-review of the narrowing: GOOD TO GOSecond cross-review, on a different model — Sonnet — against Verdict GOOD TO GO, with three findings. All three were real, I reproduced each What it confirmed, by methods other than mine
The three findings, all reproduced and fixed
One correction in the other directionThe reviewer flagged its 34-vs-my-33 as possibly a plugin difference. It was not: Not verified, and said soNo CRUX/AutoSDE-equivalent gate exists on this repo, so there was nothing of that |
… unit (#672) (#679) * `R1CounterParameter.counter` was a `UInt32Field` where :rfc:`7401#section-5.2.3` states eight octets twice -- "R1 generation counter, 8 bytes" in the diagram, "a 64-bit unsigned integer in network byte order" in the prose. The parameter declared the correct `len=12` and packed 12 octets where the RFC total is 16, four short at `4 (mod 8)`. Now `UInt64Field`, so both codes that reach the class are fixed: `R1_Counter` (128) and `R1_COUNTER` (129). RFC 5201 section 5.2.3 gives the same layout, so no version excused it (#672). * `_make_param_locator_set` wrote `len` 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` counts bytes -- `4n` declared for `24n` octets of contents. Now `sum(8 + Locator.len * 4)`, the octets each locator record occupies (#679). * `LocatorSetParameter.padding` read the nested `Locator.len` rather than the parameter's, because `ListField` packs each nested schema into the enclosing packet context and `Schema.pack` opens with `packet.update(self.__dict__)`. The shadowed value is 4 for any IPv6 locator, so the old expression appended four octets at every locator count. `locator_set_len_callback` now snapshots the parameter's `Length` under `LOCATOR_SET_LEN` before the list packs, and `locator_set_padding_len` reads that, deferring to `parameter_padding_len` for the arithmetic (#679). * `_read_param_locator_set` reports `parameter_total_len(schema.len)`, the last of the 49 record lengths still on the pre-#651 expression (#679). * `_hip_overrides()` gives both R1_COUNTER codes a non-zero `counter`. A zero-valued field cannot discriminate a width defect from a correct one, and that is why the RFC-only fixture walk reported nothing on this parameter (#672). The two had to land together: the padding correction and the `Length` unit cancelled at homogeneous plain-IPv6 sets, so #651 and #664 excluded this one site deliberately. Measured across shapes on `f0999858e` and after -- empty 4 -> 8, plain n=1/2/5 32/56/128 unchanged, SPI n=1 35 -> 32, SPI n=2 63 -> 64, mixed 59/60 -> 56/56 -- every shape is now `11 + Length - (Length + 3) % 8`. The reader was starved by the same quantity: a one-copy `LOCATOR_SET` of n plain locators parsed one truncated locator and had its unconsumed 20n octets read as a second, fabricated parameter; all eight shapes now parse back exactly and repack identically. An RFC-only walk over `options-internet.pcap` goes from 2 violations over 91 records to 0 over 92, with a non-zero counter present so the 0 cannot be concealing a mis-stride. `EXPECTED_FAILURES` is byte-identical at 44 entries: `hip-parameter/R1_Counter` stays, its cause being the schema registry key (#690), not the width. Coverage stays 100% on all three HIP modules, 586 -> 623 subtests in the same scope. pylint, mypy and isort are at exact parity with `f0999858e`. Fixes #672 Fixes #679
… unit (#672) (#679) * `R1CounterParameter.counter` was a `UInt32Field` where :rfc:`7401#section-5.2.3` states eight octets twice -- "R1 generation counter, 8 bytes" in the diagram, "a 64-bit unsigned integer in network byte order" in the prose. The parameter declared the correct `len=12` and packed 12 octets where the RFC total is 16, four short at `4 (mod 8)`. Now `UInt64Field`, so both codes that reach the class are fixed: `R1_Counter` (128) and `R1_COUNTER` (129). RFC 5201 section 5.2.3 gives the same layout, so no version excused it (#672). * `_make_param_locator_set` wrote `len` 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` counts bytes -- `4n` declared for `24n` octets of contents. Now `sum(8 + Locator.len * 4)`, the octets each locator record occupies (#679). * `LocatorSetParameter.padding` read the nested `Locator.len` rather than the parameter's, because `ListField` packs each nested schema into the enclosing packet context and `Schema.pack` opens with `packet.update(self.__dict__)`. The shadowed value is 4 for any IPv6 locator, so the old expression appended four octets at every locator count. `locator_set_len_callback` now snapshots the parameter's `Length` under `LOCATOR_SET_LEN` before the list packs, and `locator_set_padding_len` reads that, deferring to `parameter_padding_len` for the arithmetic (#679). * `_read_param_locator_set` reports `parameter_total_len(schema.len)`, the last of the 49 record lengths still on the pre-#651 expression (#679). * `_hip_overrides()` gives both R1_COUNTER codes a non-zero `counter`. A zero-valued field cannot discriminate a width defect from a correct one, and that is why the RFC-only fixture walk reported nothing on this parameter (#672). The two had to land together: the padding correction and the `Length` unit cancelled at homogeneous plain-IPv6 sets, so #651 and #664 excluded this one site deliberately. Measured across shapes on `f0999858e` and after -- empty 4 -> 8, plain n=1/2/5 32/56/128 unchanged, SPI n=1 35 -> 32, SPI n=2 63 -> 64, mixed 59/60 -> 56/56 -- every shape is now `11 + Length - (Length + 3) % 8`. The reader was starved by the same quantity: a one-copy `LOCATOR_SET` of n plain locators parsed one truncated locator and had its unconsumed 20n octets read as a second, fabricated parameter; all eight shapes now parse back exactly and repack identically. An RFC-only walk over `options-internet.pcap` goes from 2 violations over 91 records to 0 over 92, with a non-zero counter present so the 0 cannot be concealing a mis-stride. `EXPECTED_FAILURES` is byte-identical at 44 entries: `hip-parameter/R1_Counter` stays, its cause being the schema registry key (#690), not the width. Coverage stays 100% on all three HIP modules, 586 -> 623 subtests in the same scope. pylint, mypy and isort are at exact parity with `f0999858e`. Fixes #672 Fixes #679
1 participant
Fixes #651.
The RFC text, fetched
https://www.rfc-editor.org/rfc/rfc7401.txt, 309,319 octets, sha25609366b9f83dc80593172304ffcf05f57afd186aacb468ac6170a8fe26944c4be. Section5.2.1, "TLV Format", verbatim:
and, from the same section's field list,
Lengthis "Length of the Contents, inbytes, excluding Type, Length, and Padding" with
Padding"0-7 bytes, added ifneeded". So it is the total that must be 8-aligned, and
Lengthis only thecontents.
Where the padding changed
One site, not 95 — for 93 of the 95. Both expressions were byte-identical at every
occurrence (
git grep -c '% 8': 46 inschema/internet/hip.py, 49 ininternet/hip.py, andsort -uover the matched lines returns exactly one distinctline in each file), so 45 of the 46 and 48 of the 49 collapse into the helpers below.
The two remaining sites are
LOCATOR_SET's, excluded deliberately — see the #679section.
grep '% 8'over the two files now returns five lines, and each one is accountedfor — four in the schema module and one in the protocol module:
The helper takes no
version, and RFC 5201 is whyWorth stating because HIPv1 and HIPv2 differ on several parameter field widths,
and #655 is about the builders not consulting
version— so a reader couldreasonably ask whether padding needs it too. It does not. RFC 5201 §5.2.1, fetched
(
240,492octets, sha2568b42d181…9cef6) and quoted rather than paraphrased:The same formula as RFC 7401 §5.2.1, to the character. So
parameter_total_lenis correctly version-independent, and nothing here needs to change when #655
lands.
HIP.make'slencomputation was not the second defecttotal_length // 8 + 4is correct, and RFC 7401 section 5.1.3 says why:The fixed header is 40 octets, so
Header Length = (40 - 8 + params) / 8 = 4 + params / 8. The repo's own note called it "lossless only when the parameteroctets are a multiple of eight" — which is exactly the precondition the padding
fix now establishes, so the floor division is exact rather than exact-in-pairs.
_make_hip_paramsums realpack()lengths, every one of which is now amultiple of 8. Left alone deliberately.
One coupled fix, forced rather than opportunistic
EncryptedParameter.data's length callback subtracted the 16-octetivbut notthe four
reservedoctets, while_make_param_encryptedwriteslen = 4 + len(iv) + len(data). The old record total was therefore8 + Length + (-Length % 8)against the RFC's4 + Length + pad.Correction to an earlier revision of this description, which said those
"coincide only when
Length % 8 == 0" — that is wrong, and the earlier tableonly sampled residues 0 and 4, which is how it went unnoticed. Measured by
packing through
_make_param_encryptedat every residue on0c7f2b7c9:LengthLength % 8So they agreed at
Length % 8in {0, 5, 6, 7} and differed at {1, 2, 3, 4}— four residues, not one.
Length = 8, which the unit suite happened to use, isone of the four, which is why the module emitted RFC-conformant
ENCRYPTEDoctets there while getting both halves wrong.
Fixing the padding alone would still have been a regression, and for a stronger
reason than the original wording gave: with only the padding corrected the total
becomes
8 + Length + padagainst a correct4 + Length + pad, a uniformfour-octet surplus with no residue left where it cancels. So
ENCRYPTEDwould have gone from right at four of the eight residues to wrong at all eight.
The two go together;
encrypted_data_lenis the third new helper, and theresidue sets above are now asserted in
test_hip_encrypted_data_length_excludes_reserved_and_ivrather than onlyclaimed in prose.
Widths, and why they discriminate
The defect was exactly
4 (mod 8), so a convenient spread of multiples of 8would prove nothing.
test_hip_parameter_total_length_matches_the_rfc_7401_formulasweeps every
Lengthin 0..63 — all eight residues — against arfc_totaltranscribed inline from the RFC rather than imported from the implementation
(comparing an implementation with itself asserts nothing). What each residue
class rules out:
Length % 8 == 4(4— a wholeSEQ;20— a wholeSOLUTION): theRFC wants zero padding. The old rule appended 4; a rule that dropped the
outer
% 8(8 - (Length + 4) % 8) would append 8. Only this residueseparates the right answer from both.
Length % 8 == 0(0,8,16): contents are 8-aligned alone, so theold rule appended nothing and left the record 4 octets short. This is the
under-padding residue, invisible to any "result is at least as long as the
contents" check.
Lengthnot a multiple of 4 (1,2,3,5,6,7→ pad 3,2,1,7,6,5):catches a formula that only ever moves in steps of four, which both the old
and the fixed one resemble at a glance.
The test also asserts
total != pre_651_total(length)at everyLength, i.e.there is no residue at which it would have passed before this change.
A gap the cross-review found, now closed. Eight residues are enough for any
formula that is periodic in
Length % 8— but not for one that is not. Aparameter_total_lenmasking its argument (length & 0xFF) agrees on 0..63 anddiverges at 256, and nothing in the generator builds a parameter that long, so it
passed the entire suite. The test now also checks the whole domain of the
16-bit
lenfield in one loop. Verified by planting exactly that formula in ascratch copy of this branch:
Failing, then passing
Both runs import from a tree that is printed and asserted first, with
PYTHONPATHset and the editable finder accounted for (see the trap below).New tests against the pristine
0c7f2b7c9library. An earlier revision of thisdescription gave one figure without saying which files it covered, which
invited reading it as both modules; here are both, from a run whose library was
checked by md5 against
git show 0c7f2b7c9:and whose test files were checked bymd5 against this branch:
Both are real and differ only in file selection. Broken down, because
pytest-subtestscounts a failing subtest as its own "failed" entry:The round-trip module adds 3 failures — the two subtests of
test_a_hip_packet_carrying_one_parameter_round_trips, plus the ENCRYPTED case intest_round_trip_is_identity_or_a_recorded_gap, which on the old library stillMISMATCHes while this branch's table no longer records it.Worth flagging because it tripped the cross-review:
74is the number ofsubtests that passed in the one-module run and, coincidentally, the number that
failed in the two-module run. Same integer, different quantity.
The failures are real octet counts rather than an
AttributeError— theoctet-level test deliberately imports no new helper:
Over-padding at
4,1; under-padding at0,5. Same tests on this branch:against
30 passed, 434 subtestson0c7f2b7c9for the same two modules.Fixture evidence that does not trust pcapkit at all
examples/generators/make_samples.pyregenerates the set (neverrm -rf examples/captures—dhcp.pcapngandin.pcapare committed). A standalonewalker then parses
options-internet.pcapwith no pcapkit, taking its stridesolely from
11 + Length - (Length + 3) % 8, so a record written to any otherrule desynchronises it immediately:
The one remaining violation is
LOCATOR_SET, and it is present identically onb34f132f6—frame 102: type 193 padding not zeroed: 00c10000. The walker trustsLength, andLOCATOR_SET'sLengthis in 4-octet units, so it advances 8 octetsinto a 32-octet record and desynchronises. That is #679's defect, not the padding's:
the record itself is the RFC's
24n + 8octets on both trees. So the honest figureis 43 → 1, with the 1 unchanged from
main.It also checks that each record's padding is zeroed, as 5.2.1 requires of the
sender, and that the header's
lenaccounts for exactly the octets walked.Read "0 violations" narrowly, though — the cross-review found that it conceals
one record, and it is worth spelling out because it is the same
two-defects-cancelling shape as everything else here.
R1_COUNTERdeclaresLength = 12but its schema packs only 12 octets in total, four short of theRFC's 16, because of the separate
counterwidth defect described below. ALength-driven reader therefore advances 16 and lands four octets inside the
second copy, where it reads a phantom
Type = 0, Length = 0record out of thatcopy's own zeroed contents — whose "padding" is zero, so the zero-padding check
passes and the walk ends tidily on the area boundary. The generator builds
R1_COUNTERwithcounter = 0(it has no entry in_hip_overrides()), which isexactly what makes the desynced octets indistinguishable from real padding.
Measured, by patching only the second copy's counter in a scratch copy of the
file and re-walking:
So the honest reading is "no padding violations among the records a conformant
reader can find", not "the capture is conformant". The padding fix is not
implicated — the concealment is entirely the
counterwidth defect — and43 → 0remains a real difference, since all 43 pre-fix violations were paddingones. Two follow-ups this suggests, neither taken here: give
R1_COUNTERanon-zero
counterin the generator's_hip_overrides()so the fixture stopshiding it (outside this PR's remit for that file, which is
HIP_COPIESand itsnote), and fix the width itself.
HIP_COPIES = 1, measured — and why it stays at 2Measured over the generator's 49 HIP codes, at both settings, on both trees:
0c7f2b7c9So 41 codes that could not survive alone now can, which is the strongest
available statement that the padding was what made a lone parameter
unrepresentable — and better evidence than any assertion in this PR.
It still stays at 2, because the four codes that fail at one copy fail for
reasons unrelated to padding:
R1_COUNTERandR1_Counter(thecounterwidth),HOST_ID(declareslen=8, packs 18) andHIP_TRANSFORM(HIPv1-only, built hereat version 2). Flipping the constant would trade this fix's workaround for one new
expected-failure entry, one changed status, and the loss of
R1_COUNTERfrom theround-tripping set — a change about those defects.
R1_COUNTERdeserves a sentence, because it is the one code that round-trips atone copy before this PR and not after, which looks like a regression and is not.
At
Length = 12the old contents-aligning rule appended exactly four surplusoctets, and those happened to fill this parameter's own four-octet shortfall,
bringing the record to 16. Two defects cancelling. Correcting the padding removes
the compensation and leaves the shortfall visible, which is the right outcome; at
two copies the shortfall sums to 8 and the code round-trips either way.
The single-parameter case is not lost meanwhile — it is asserted directly, and
now positively, by
test_a_hip_packet_carrying_one_parameter_round_trips.EXPECTED_FAILURES: exactly one entry movedCorrection to an earlier revision of this description: it said the table is
"
**-free", which is wrong — 30 of its entries are produced by two**{f'pcapng-option/{name}': Gap(...) for name in (...)}comprehensions(
test_option_roundtrip_unit.py:460and:504), which is exactly why greppingit cannot enumerate it. It was therefore checked two independent ways instead:
hip-parameter/ENCRYPTED MISMATCH→OK.So:
hip-parameter/ENCRYPTED:MISMATCH→OK, entry deleted. Not deletedmerely because it passes: its two recorded causes are both genuinely gone (the
cipher=half by Two dropped-keyword/wrong-cast defects flagged in review and never filed (hip.py:3533, ipv6_route.py:207) #556, thedata-length half by this PR), and the reasoningis kept as prose where the entry was, in this module's existing style.
changed : []is the whole check.hip-parameter/R1_Counter,hip-parameter/HIP_TRANSFORMandhip-parameter/HOST_IDkeep their recordedstatus, fragment and defect string byte for byte.
UInt32Fieldpack asymmetry was not touched.Note it is not a
Gapentry at all but the prose block attest_option_roundtrip_unit.py:205-230, explaining thatSIDOption.sidwas aUInt32Fieldwhere RFC 791 §3.1 gives the Stream ID 16 bits, so the optionre-emitted as
880400000037where the wire holds88040037.git diff 0c7f2b7c9over that file matches none ofUInt32Field,UInt16Field,SIDOptionor880400000037, i.e. the block is untouched.test_a_single_hip_parameter_cannot_be_constructedis renamed totest_a_hip_packet_carrying_one_parameter_round_tripsand inverted — it requiredthe library to reject its own single-parameter packets, which is the defect.
Narrowed per #679: this fixes 45 of 46 parameters, not all 46
An earlier revision of this PR corrected the padding at every HIP parameter, and
that made
LOCATOR_SETfour octets short — it had been accidentally conformant, bytwo defects cancelling. The owner's decision on #679 is option (b): narrow this PR
and leave
LOCATOR_SETalone, so the pair can be fixed together there. Done, atboth of that parameter's sites.
Why it was already right. Two defects cancel exactly:
LocatorSetParameter.padding's callback never receives the parameter'slen.ListFieldpacks each nestedLocatorinto the shared packet context, whose ownlenkey overwrites the parameter's, andpaddingis evaluated after the list —so the value seen is the last locator's
len, always 4 for an IPv6 locator.Measured by building the schema directly with a parameter
len = 9over onelocator of
len = 4: it pads by the amount for 4, not the 3 that 9 would give._make_param_locator_setsets the parameter'slentosum(Locator.len), in4-octet units, where RFC 7401 §5.2.1's
Lengthis a byte count —4nwherethe contents are
24noctets.Always-4 padding gives
4 + 24n + 4 = 24n + 8; and because24n ≡ 0 (mod 8), theRFC total for
Length = 24nis11 + 24n - 3, the same24n + 8. Identical forevery locator count.
Verified byte-for-byte against
b34f132f6, not merely "a test passes":b34f132f6Length = 24nand the two literals the corekit field test pins come back identical on both trees:
The exclusion is documented at the site, not as a cross-reference. The comment on
LocatorSetParameter.paddingopens by telling the next reader not to "finish" #651 bychanging that line alone, and gives the arithmetic for why. The matching reported
record length in
_read_param_locator_setis left on the old expression too, with apointer to it, so the parameter is unchanged in its data model as well as its octets.
And the exclusion is guarded, so it can neither grow nor evaporate.
test_hip_padding_helpers_cover_every_parameter_but_locator_setintrospects eachparameter schema's declared
paddingfield and asserts the taxonomy:Those three are correct rather than an omission, and unchanged by this PR: their
contents are a fixed 20 octets, and
11 + 20 - (20 + 3) % 8 == 24 == 4 + 20, so theRFC asks for no padding. Measured on both trees — all three pack to 24 octets. They
are listed by name in the assertion so that a fourth appearing there would mean a
parameter had quietly lost its padding field.
Found and deliberately not fixed## Found and deliberately not fixed
R1_COUNTER's counter is four octets where RFC 7401 section 5.2.3 requireseight. The RFC diagram reads "R1 generation counter, 8 bytes" with
Length 12, and the prose says "a 64-bit unsigned integer in network byteorder";
R1CounterParameter.counteris aUInt32Field, so the schema declareslen=12and packs 12 octets where the RFC total is 16.HIP_COPIES = 2hid it,because two identical 4-octet misalignments sum to 8 — and, before this PR, the
old padding rule hid it a second way by appending exactly the four octets it was
missing. This is the same shape of defect as #608 and wants its own issue and
review — not folded in here. Its consequences show up twice above: it is why
R1_COUNTERno longer round-trips at one copy, and why the fixture walker's"0 violations" conceals one record. Now filed as #672, carrying the RFC §5.2.3
quote, the
file:line, the measured octet counts for both codes 128 and 129, thethree separate things that were hiding it, and the walker reproduction.
Also not fixed, and also pre-existing:
_make_hip_param'sbytesandSchemabranches are unreachable. Both append packed octets to a list that
Schema_HIP.parampacks as aListFieldofSchemaFields, soHIP(parameters=[b'...'])andHIP(parameters=[some_schema])both raiseAttributeError: 'bytes' object has no attribute 'type'— identically on0c7f2b7c9and on this branch. Worth recording because it is what makes thetotal_length // 8 + 4truncation hole unreachable in practice: only the(code, kwargs)branch works, and every parameter it produces is 8-aligned afterthis fix. On
0c7f2b7c9that same one-parameter call raisedProtocolError: HIPv2: invalid format; on this branch it yields a 48-octet packetwith
Header Length = 5, which is the whole fix in one line.Also recorded, for the sibling #653/#654/#655 work rather than for this PR:
hip-parameter/HIP_TRANSFORM'sdefectstring inEXPECTED_FAILURESblames"the make-side arithmetic", but measured on this branch the parameter packs 8
octets for
len=0, exactly the RFC total. Its actual cause is that_read_param_hip_transformraises for anyversion != 1while the generatorbuilds it at version 2 — which sounds like #655's territory, so the entry is
left untouched here.
Not touched
protocols/protocol.py,application/http.py,corekit/io.py,dumpkit/common.py,transport/tcp.py,tests/_support.py,misc/pcapng.py,.github/**,docs/source/changelog/1.5.0.rst,CHANGELOG.md. No changelogbullet on this branch — that goes to #657 /
docs/changelog-1.5.0separately.Traps hit while measuring, for the record
PYTHONSAFEPATH=1is what causes the editable-install trap rather thanavoiding it.
__editable__.pypcapkit-1.4.1.post2.pthinstalls an_EditableFinderhard-wired toMAPPING = {'pcapkit': '/local/home/jarryx/GitHub/PyPCAPKit/pcapkit'}— the main checkout — andPYTHONSAFEPATH=1removes the script-directory entry that would otherwise shadowit. Measured: the first
make_samples.pyrun in this session produced anoptions-internet.pcapcarrying pre-fix 20-octetESP_INFOrecords while theworktree's own library packed 16, because the generator had silently imported the
main checkout. Every measurement above was redone with
PYTHONPATH=<tree>and aprinted, asserted
pcapkit.__file__.Second, the finder is appended to
sys.meta_pathas a class, sotype(f).__module__readsbuiltinsand a filter on that misses it entirely;f.__module__is what carries the__editable__prefix.