Skip to content

fix(hip): HIP_CIPHER warns at six Cipher IDs although RFC 7401 allows six - #1111

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1092-hip-cipher-limit
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1092-hip-cipher-limit

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — Makefile flags run on the touched files; mypy and isort clean, pylint adds nothing on the changed lines
  • make test passes, and a test case covers the change — modules run: new test_hip_cipher_limit_unit.py, test_hip_unit.py, test_option_roundtrip_unit.py, tests/project
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Closes #1092. RFC 7401 §5.2.8 allows up to six Cipher IDs, and a recipient keeps the first six and drops the rest. _read_param_hip_cipher now warns only above six and truncates to six. length is still the whole wire record, so parsing bookkeeping is unchanged. The maker had no limit and is left alone.

IDs before (warn / kept) after
5 none / 5 none / 5
6 warn / 6 none / 6
7 warn / 7 warn / 6

test_hip_unit.py pinned the old warning at six; it now uses seven. Without the fix the new module gives 2 failed, 1 passed (6 and 7 fail, 5 passes); with it, 2 passed. test_hip_unit 34 passed, round-trip 6 passed, tests/project 379 passed, 1 skipped.

… six

RFC 7401 section 5.2.8 allows up to six Cipher IDs, and a recipient
accepts the first six and drops the rest. The reader warned at six and
kept every ID at seven or more. It now warns only above six and keeps
the first six; the reported length is still the whole wire record.

The existing HIP unit test pinned the old warning at six; it now uses
seven. New module tests/protocols/internet/test_hip_cipher_limit_unit.py
covers 5, 6 and 7 IDs.

Closes #1092
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 883350174: GOOD TO GO (ran on Sonnet; author Opus)

  • Fix confirmed on main. main warns at 6 Cipher IDs and keeps 7, 8 and 9. With the PR, 5–6 IDs parse silently, and 7 or more warn and keep [1..6].
  • RFC. The comment matches RFC 7401 §5.2.8 word for word (fetched).
  • No desync after truncation. The walk uses the schema len. A following TRANSACTION_PACING parameter still parses (min_ta=10), and frame sizes are right: 72 and 64 octets.
  • Fails without the fix. With hip.py reverted, the new module fails 2. The edited test_hip_unit case (7 IDs, len=14) is consistent, at 34 passed.
  • By design: a parsed 7-ID parameter now re-makes as 6 IDs (64 octets), so it no longer round-trips byte-identically. That is the RFC's "dropping the rest". 5–6 IDs still round-trip.

@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 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.74% (unit tier, Python 3.14, 883350174, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1035 2342 846 90.23%
pcapkit/corekit 1874 91 578 22 94.33%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2422 143 842 34 92.62%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15655 187 3942 162 98.19%
pcapkit/toolkit 487 71 144 3 84.15%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

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

@JarryShaw
JarryShaw merged commit 0aff5d8 into main Oct 6, 2026
41 checks passed
@JarryShaw
JarryShaw deleted the fix/1092-hip-cipher-limit branch October 6, 2026 20:31
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(hip): HIP_CIPHER warns at six Cipher IDs although RFC 7401 allows six

1 participant