Skip to content

fix(hip): correct 7 HIP parameter wire defects - #2400

Merged
JarryShaw merged 1 commit into
mainfrom
fix/hip-wire-correctness-v2
Oct 27, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/hip-wire-correctness-v2

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 27, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — bug fix

Description

Closes #2271. Closes #2275. Closes #2276. Closes #2281. Closes #2282. Closes #2283. Closes #2299.

Seven independent RFC-level defects, fixed together since all seven edit hip.py:

Breaking data-model/signature changes carry versionchanged:: 1.5.0 notes.

Judgment calls flagged for a second look:

  1. fix(hip): REG_INFO, REG_REQUEST and REG_RESPONSE lifetimes decode as seconds, not RFC 8003 #2281's duration semantics are a silent default-wire change: REG_INFO's default max-lifetime wire octet moves from 0x0f to 0x5f, since the maker's numeric default (15) is now read as 15 seconds (encoding to octet 0x5f, ~14.67s) rather than copied as the raw octet 0x0f (~14.3ms). RFC 8003 §5 sets a 10s MUST-floor on this value, which 0x0f's ~14.3ms violated and 0x5f's ~14.67s satisfies — the new default happens to be more RFC-correct, though that wasn't the point of the change.
  2. fix(hip): REG_INFO, REG_REQUEST and REG_RESPONSE lifetimes decode as seconds, not RFC 8003 #2281 encoding floors to the nearest lower octet (inherent to the RFC's log encoding) — a requested 255 s round-trips as 234.75 s; raw-octet round-trips stay byte-exact (verified all 256 values). Narrower than that inherent floor: round-tripping a decoded timedelta straight back into the maker is lossier still — 103 of 255 non-zero octets land one octet low (e.g. 127 → 126, 255 → 254), from float/microsecond rounding compounding the floor, not from the RFC itself.
  3. fix(hip): LOCATOR_SET built with spi= raises unless type=1 is also passed #2276's consistency check ((type == 1) != (spi is not None)) means type=2 with an spi raises the new error, while type=2 without one still falls through to locator_value_selector's existing FieldValueError — both reject, but via different exceptions.
  4. fix(hip): CERT group decodes through the DH Group registry, but RFC 8002 has no registry #2283: cert_group being a plain int still silently accepts an Enum.Group member at the call site (stored as the enum) — no guard added, since only the decode path was the actual defect.

Test: new tests/protocols/internet/test_hip_wire_semantics_unit.py (15 tests). Fail-without-fix: 14 of 15 fail against origin/main (the 15th is a deliberate no-regression control for #2276's unchanged path). test_hip_unit.py updated for the three issues it pinned the old wrong semantics for.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) bug Issues reporting a defect (set by the bug report template; a default, not an assessment) breaking Breaks public-facing behaviour or API (apply alongside the type label) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
…2281, #2282, #2283, #2299)

Seven independent RFC-level defects in HIP parameter makers and readers,
fixed together since all seven edit pcapkit/protocols/internet/hip.py:

- #2271: TRANSACTION_ID/OVERLAY_ID lost their leading zero octets on
  rebuild, since len was sized from the integer's own bit_length rather
  than the wire width. A new id_len field keeps the original width; a new
  shared HIP._make_identifier_width helper sizes from it, falling back to
  bit_length only when building from scratch (mirrors #653's
  _make_puzzle_field_width).
- #2275: HIP_TRANSFORM's maker had no version gate, though its reader
  rejects a HIPv2 parameter. Added the matching ProtocolError.
- #2276: LOCATOR_SET's maker set len=5 for an spi= locator but left
  type=0, a combination locator_value_selector itself rejects. type now
  defaults to None and is derived from whether spi is given (1 with spi,
  else 0); an explicitly contradicting type raises naming both values.
- #2281: REG_INFO/REG_REQUEST/REG_RESPONSE lifetimes read as raw seconds
  via timedelta(seconds=octet); RFC 8003 section 4.1 defines a non-zero
  octet as 2**((v-64)/8) seconds and 0 as a dedicated zero case. New
  _read_registration_lifetime/_make_registration_lifetime/
  _make_lifetime_octet mirror PUZZLE's #1323 precedent; makers take
  either a duration or a raw exponent, and the raw octet stays the
  round-trip source on the data models.
- #2282: OVERLAY_TTL's ttl parsed as a timedelta, though RFC 6079 defines
  it as a hop count. ttl is now a plain int throughout schema/data/maker.
- #2283: CERT's group decoded through the DH Group registry, though RFC
  8002 section 2 defines no such registry for it. cert_group is now a
  plain int (UInt8Field), decode-side enum lookup dropped.
- #2299: HOST_ID's maker prefixed errors "[HIPv2]" instead of the house
  "HIPv{version}: [ParamNo {code}]" form used elsewhere.

Breaking data-model and signature changes carry versionchanged:: 1.5.0
notes.

Tests: new tests/protocols/internet/test_hip_wire_semantics_unit.py (15
tests, fail-without-fix verified for 14 of them against origin/main; the
15th is a deliberate no-regression control). tests/protocols/internet/
test_hip_unit.py updated for the three issues it pinned the old wrong
semantics for. tests/project/test_rfc_constants_one_home_1739_unit.py
gains a 65535 UNRELATED allowance matching schema/internet/hip.py's
existing entry, since the new id_len field's two-octet Length bound
collides with the homed IP_MAX_LENGTH constant.
@JarryShaw
JarryShaw force-pushed the fix/hip-wire-correctness-v2 branch from 7b6f167 to e99e058 Compare October 27, 2026 19:13
@JarryShaw

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at e99e0581d (Sonnet; amended from 7b6f16722). All 7 RFC claims independently re-verified against the actual RFC text (6078/6079/7086/8046/8003/8002/7401), each with its own measurement — 70 hand-built TLVs for #2271 (70/70 byte-exact on this head, 20/70 on main), a 65,536-pair REG_INFO grid for #2281, 4 hand-built wires for #2276. 551 passed/12497 subtests (internet tier), 2107 passed/10253 subtests (project tier); pylint 518→511 (2 genuine reductions, 0 new); mypy/vermin/bandit clean.

Two real, small corrections applied in this amend (both prose-only, verified against the suite afterward):

  • PR body judgment call Fix typo in README #1 was backwards. It said REG_INFO's default 0xf is "now interpreted as an exponent" — but exponent and raw-octet are literally the same byte in this encoding, so that claim described a no-op. What actually happens is the maker's numeric default is now read as 15 seconds (a duration), encoding to wire octet 0x5f — rewritten accordingly, and noted that 0x0f's ~14.3ms violated RFC 8003 §5's 10s floor while 0x5f's ~14.67s satisfies it (incidental, not the point of the fix).
  • A typo from RFC 8003 §4.1 itself (2^(63/8) where the RFC means 2^(-63/8), i.e. ~4ms not ~235s) had propagated into a docstring and a test comment. Code was always correct; fixed both prose sites.

Also folded the review's flooring finding into judgment call #2: round-tripping a decoded timedelta straight back through the maker is lossier than the inherent log-encoding floor — 103 of 255 non-zero octets land one octet low, from float/microsecond rounding compounding the floor rather than the RFC itself.

Non-blocking, not acted on (flagging for visibility): #2276's ProtocolError/FieldValueError asymmetry could be closed by validating type not in (0, 1) before the spi-consistency check; a changelog entry may be worth adding given recent sibling breaking PRs each added one (your call, template marks it optional when not user-facing); a few maker edge cases (inf/nan, lifetime_exponent=None, ttl wraparound at 65536, removed cert_group_* kwargs silently absorbed by **kwargs) are unguarded but match the existing code's general looseness elsewhere.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
@github-actions github-actions Bot added this to the PyPCAPKit v1.5 milestone Oct 27, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 91.67% (unit tier, Python 3.15, e99e0581d, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19601 880 2620 867 92.12%
pcapkit/corekit 2596 74 818 37 96.51%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 3044 23 1060 25 98.83%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17527 210 4638 185 98.21%
pcapkit/toolkit 914 36 334 10 95.51%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 4434 1961 1018 167 52.49%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 4a14097 into main Oct 27, 2026
63 checks passed
@JarryShaw
JarryShaw deleted the fix/hip-wire-correctness-v2 branch October 27, 2026 22:33
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 27, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

1 participant