fix(hip): size the SOLUTION parameter as two whole-octet fields (#608) - #629
Merged
Merged
Conversation
`_make_param_solution` declared `len = 4 + ceil(max(bits) / 4)`. RFC 7401 section 5.2.5 spells the SOLUTION parameter's Length `4 + RHASH_len / 4`, but over a `Random #I` and a `Puzzle solution #J` of `RHASH_len / 8` octets *each* -- `/ 4` is twice `/ 8`, an identity that holds only because `RHASH_len` is a whole number of octets. Applied to an arbitrary `int.bit_length()` it breaks, and the builder emitted an odd contents width that `_read_param_solution`'s `(len - 4) % 2` guard rejects, because `SolutionParameter` splits that width into two equal `(len - 4) // 2` halves. `random=0x1, solution=0xfff` declared `len=7` and raised `ProtocolError: HIPv2: [ParamNo 321] invalid format` on the library's own output; the same undersized length also truncated `solution=0xfff` to `0xff` on the wire, silently. - `len = 4 + 2 * math.ceil(max(...) / 8)`, matching the sibling PUZZLE builder and even by construction, so the reader's guard can never trip. Loosening the reader instead was rejected: an odd contents width has no meaning in a format whose two fields are equal-width by definition. - Record on both sites why the RFC's `/ 4` shorthand must not be reused on a width that is not a whole number of octets. - Eight-width regression coverage in `tests/protocols/internet/test_hip_unit.py`, five widths deliberately not multiples of eight, plus a 57-bit case pinning the 20-octet length RFC 5201 section 5.2.5 requires of HIPv1. Fixes #608 Verified on CPython 3.14.7: the new tests fail 6/1 (5 SUBFAILED + the HIPv1 case) against an unmodified snapshot of the base commit and pass 2/2 with 8 subtests after. `test_hip_unit.py` goes 22 tests / 68 subtests -> 24 / 76; `hip.py` was already at 100% statement and branch coverage before and after, so the subtest count is the axis that carries the evidence. `test_option_roundtrip_unit.py`, `test_docstring_contract.py`, `test_tier_guard.py` and `tests/project` all green.
Owner
Author
GOOD TO GOIndependent cross-review, per the standing rule that an agent-raised change is It built its own checkouts ( Verdicts
It fetched both RFCs itself and matched §5.2.5 verbatim in each, confirmed It also attacked the "round both fields independently" alternative I had not It disputed two things. Both were checked here and both stand.1. A factual error in my description — accepted and fixed. I described RFC 7401's 2. A fourth latent defect I had not found. Neither builder consults Pre-existing, present identically in the untouched PUZZLE builder, and not made worse Neither disputed point changes the verdict: the load-bearing claims are 3 and 5, and |
This was referenced Sep 22, 2026
JarryShaw
added a commit
that referenced
this pull request
Sep 22, 2026
…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. 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: that is consolidated in #657.
JarryShaw
added a commit
that referenced
this pull request
Sep 22, 2026
…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.
JarryShaw
added a commit
that referenced
this pull request
Sep 22, 2026
…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.
JarryShaw
added
the
breaking
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #608.
The defect, re-verified on current
mainThe issue was measured against
493020f83; re-measured here against6c3d1b0d9(and the branch is now rebased onto
ead73b204), CPython 3.14.7, importing fromthis worktree rather than the editable install:
The headline case reproduces exactly as filed. The reproduction also turned up a
second symptom the issue did not name: the undersized
lendoes not merelymis-state the parameter, it truncates the value. Both fields take their width
from
len, sosolution=0xfffwas packed into the one octetlen=7allowed andcame back as
0xff, silently:The RFC, actually fetched
Fetched
https://www.rfc-editor.org/rfc/rfc7401.txtandhttps://www.rfc-editor.org/rfc/rfc5201.txt. The section in the issue brief isoff by one: RFC 5201 §5.2.4 is PUZZLE; SOLUTION is §5.2.5 in both RFCs.
RFC 7401 §5.2.5 (HIPv2), verbatim:
with the diagram's two payload fields labelled
Random #I, n bytesandPuzzle solution #J, RHASH_len / 8 bytes, andRHASH_lendefined in §2.2 as"the natural output length of RHASH in bits". RFC 5201 §5.2.5 (HIPv1) fixes
both at 8 bytes and
Lengthat 20.There is no numbered figure for either parameter. RFC 7401 has exactly two
numbered figures — Figure 1, the base-exchange diagram in §4.1, and Figure 2, the
simplified HIP state diagram in §4.4.4 — and RFC 5201 has none at all. The §5.2.x
packet diagrams are unnumbered inline ASCII art, so there is nothing to cite by
figure number.
Formula, before and after
4 + math.ceil(max(random.bit_length(), solution.bit_length()) / 4)len=74 + 2 * math.ceil(max(random.bit_length(), solution.bit_length()) / 8)len=8RHASH_len / 4is twiceRHASH_len / 8— but only becauseRHASH_lenis ahash output length in bits and therefore a whole number of octets. Substituting an
arbitrary
int.bit_length()breaks the identity, andceil(b/4) != 2*ceil(b/8)for every
bthat is not a multiple of 8. The sibling_make_param_puzzlealreadyuses the
ceil(bits / 8)form; this brings SOLUTION in line with it, and theresult is even by construction so the reader's guard can never trip.
Why loosening the reader is the wrong fix
The obvious alternative is to drop or weaken
_read_param_solution's(schema.len - 4) % 2guard. Three reasons that is wrong:SolutionParametersizesrandomandsolutionat(pkt['len'] - 4) // 2each(
pcapkit/protocols/schema/internet/hip.py:454,:456). Withlen - 4oddthose two halves sum to
len - 5, leaving one octet of declared contentsunaccounted for — the parameter is mis-framed, not merely unusual.
RHASH_len / 8octets, sothey are equal-width by definition and the RFC says nothing about which of the
two would take an extra octet. A laxer reader would have to invent that
framing, and two implementations would invent it differently.
undersized
len, not by the parity check. Acceptinglen=7would stillhave written
0xfffor0xfff. Only a correctlenfixes both symptoms.The builder is the side that is wrong, so the builder is what changed. The guard
and the schema's
// 2sizing are untouched.Failing, then passing
Both runs import from a tree whose
pcapkit.__file__is printed and asserted, sothe editable install's
MetaPathFindercannot substitute the primary checkout.The "before" run is an immutable
git archiveexport of the base commit with onlythe new test file overlaid;
sha256sumofpcapkit/protocols/internet/hip.py,pcapkit/protocols/schema/internet/hip.pyand the test file was taken beforeand after each run and was identical each time
(
fb542eb639acfeab…/b56203d2a044e69a…/ce9d80c439321914…).Before (
/tmp/hip608/before,git archive 6c3d1b0d9, unfixedhip.py):with
AssertionError: 7 != 8on the headline case andAssertionError: 19 != 20on the HIPv1 case. Exit code read from a file, not a pipeline:
cat /tmp/hip608/exitcode.txt→1.After (this branch):
cat /tmp/hip608/exitcode.txt→0.Note which subtests failed: exactly the five non-byte-aligned widths, with
the three byte-aligned controls passing on the unfixed tree. That asymmetry is
the test discriminating, visible in the output itself.
Why these inputs discriminate
Four plausible expressions for the field-pair width, evaluated at each case's bit
length. The correct one is
2 * ceil(b/8); the others are the defect(
ceil(b/4)), a floor variant (2 * floor(b/8)), and one field's worth insteadof two (
ceil(b/8)):/42*floor(/8)ceil(/8)Five of the eight widths are deliberately not multiples of eight, which is the
whole point: at a multiple of eight the defect coincides with the correct width,
which is why every fixture that ever reached this builder passed through it
unharmed. The 8/16/24-bit cases are kept as controls, asserting the repair changes
nothing on the path that already worked.
The HIPv1 case uses 57 bits, not 64, for the same reason:
4 + ceil(64/4)and4 + 2*ceil(64/8)are both 20, so a full-width 64-bit value would pass eitherway. At 57 bits the defect gives 19, which fails both the HIPv1
len != 20checkand the parity check, while the correct formula gives the 20 RFC 5201 §5.2.5
requires. A 57-bit value is an ordinary 8-octet field with seven leading zero bits.
EXPECTED_FAILURESintests/protocols/test_option_roundtrip_unit.pywasimported, not grepped (it is built by
**unpacking): 45 entries, fourHIP-related, and neither
SOLUTIONnorPUZZLEamong them. So there is no entryto remove, and the round-trip suite stays green in both directions.
Coverage — which axis carries the evidence
Coverage cannot see this change, and the numbers say so:
hip.pywas already at 100% statement and 100% branch coverage. The changedline was already executed by the existing
_make_param_solutiontests — they justnever checked the value it produced. Saturated before, saturated after: coverage
cannot go backwards, and it cannot go forwards either.
The subtest count is the meaningful axis.
tests/protocols/internet/test_hip_unit.pyalone: 22 tests / 68 subtests → 24 tests / 76 subtests.
Regression runs
All scoped to the modules this change touches — the full suite was deliberately
not run (it has reached 41 GB RSS in a worktree on this host). Exit codes read
from a file.
test_hip_unit.py+test_option_roundtrip_unit.py+test_docstring_contract.py+tests/project133 passed, 915 subtests passed, exit 0+ test_tier_guard.py(pre-rebase)158 passed, 934 subtests passed, exit 0util/changelog_md.py --checktests/project/test_changelog_md.pyis the changelog gate and is in that run.CHANGELOG.mdwas regenerated withpython util/changelog_md.py, never hand-edited.The rebase onto
ead73b204merged cleanly and kept #628's bullet ahead of mine.Found, and deliberately not fixed
The issue records two further
hip.pyclaims as uninvestigated. Both are real —I verified them while measuring this one — and both are left alone, because each
is a distinct defect with its own blast radius:
Padding is computed from the contents length, not the record length.
RFC 7401 §5.2.1, verbatim: "All of the encoded TLV parameters have a length
(that includes the Type and Length fields), which is a multiple of 8 bytes."
Every parameter schema in
pcapkit/protocols/schema/internet/hip.pypads with(8 - (pkt['len'] % 8)) % 8, which alignslenrather thanlen + 4. So aSOLUTIONwithlen=8emits a 12-octet record where the RFC requires 16.pcapkit round-trips it fine because its writer and reader share the wrong
formula; a real HIP peer would not. This affects every HIP parameter, not
just SOLUTION, and correcting it changes the bytes of every HIP fixture — far
too wide to ride along with a one-line length fix.
Re-serialising a parsed parameter loses leading zero octets. Given
param=, both_make_param_puzzleand_make_param_solutionrecompute thewidth from
param.random.bit_length(), andData_SolutionParametercarries nofield width to recover the original from. An 8-octet
Random #Iof0x01readsback as
1and re-emits in one octet. My fix does not make this worse (itrounds up, never down), but it does not fix it either: doing so means
threading the on-wire width through the data model, which is a design change.
A third one, not in the issue at all, found while measuring this. RFC 7401
§5.2.5 names the SOLUTION parameter's second octet
Reserved— "zero when sent,ignored when received" — but
SolutionParameternames itlifetimeand bothsides round-trip it as a
2**(v-32)second duration, which is PUZZLE'ssemantics (§5.2.4), not SOLUTION's. Two consequences, both measured:
So the builder cannot emit the zero the RFC mandates, and asking it to — which is
what its own default
lifetime=0does — escapes a bareValueErrorout ofmath.log2(0)rather than an exception frompcapkit.utilities.exceptions. Thesame
log2(lifetime)sits in_make_param_puzzle, whereLifetimeis a realfield, so the bare-
ValueErrorhalf is shared and not SOLUTION-specific. Leftalone: it is a semantics and error-handling change across two builders, and
nothing about it depends on or is fixed by the length arithmetic here.
A fourth, raised by the cross-review and reproduced here. Neither
_make_param_solutionnor_make_param_puzzleconsultsversionwhen sizing,so under HIPv1 — where RFC 5201 §5.2.4/§5.2.5 fix the fields at 8 octets each and
Lengthat 12 and 20 respectively — any value narrower than the full field buildsa parameter its own reader rejects. That is HIP SOLUTION builder sizes with ceil(bits/4), emitting a parameter its own reader rejects #608's exact symptom in a second
guise. Measured on both trees,
version=1:Pre-existing, not a regression, and not made worse: this PR rounds up, so it
fixes the 57-bit row (19 → 20) and changes nothing else. Fixing it properly means
the builders sizing to a fixed 8 octets when
version == 1rather than tobit_length(), which is a behaviour change in two builders and belongs with thePUZZLE builder this PR deliberately does not touch. The 57-bit test here pins the
one HIPv1 row that this change does move; it does not claim general HIPv1
correctness.
Also noted, not a defect: the round-trip generator's SOLUTION case
(
examples/generators/options.py:977) passes only{'lifetime': 1}, sorandomand
solutiondefault to0and the case exercises this formula at zero bits— maximally non-discriminating. It is left as-is because
examples/is outsidethis change's scope and altering it would regenerate committed fixtures.