Skip to content

fix(ospf,ngap): keep the parsed OSPF Packet Length and refuse octets after the NGAP-PDU - #1304

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1249-ospf-ngap-roundtrip
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1249-ospf-ngap-roundtrip

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort). I ran the Makefile's flags on ospf.py/ngap.py only, not make: they report nothing new.
  • make test passes, and a test case covers the change. I ran the touched modules one by one, plus tests/project and the protocols --exclude protocols/internet unittest leg.
  • Added a changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Description

Closes #1249
Closes #1250
Closes #1252
Refs #1251 (OSPF auth_data half only; HTTP/2 PING is another PR)

Probe (OSPF.from_data(OSPF(w, len(w)).info) / NGAP): before the fix, the crypto+digest packet rebuilds as Length 003c, and Lengths 0000/001e rebuild as 002c. auth_type=2 with bytes raised AttributeError, b'abc' was padded, and NGAP+ffff came back as 58 octets. After the fix, every OSPF repro rebuilds byte-exactly, and the bad inputs raise ProtocolError.

Tests: new test_ospf_ngap_roundtrip_unit.py gives 8 passed, 21 subtests passed. Every new test fails without its fix; the exact-PDU case is a control. Other runs: test_ospf_unit 3 passed, test_ngap_unit 23 passed, test_ospf_cryptographic_authentication_unit 4 passed, tests/project 389 passed, 1 skipped, and the unittest leg 858 tests, 0 failures.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) 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 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on c777570a8: NEEDS CHANGES (Sonnet reviewed; the author ran on Opus). The behaviour is correct. One citation is not.

  • Must fix: the PR body and the ngap.py comment cite TS 38.412 §7 for "one NGAP message per SCTP DATA chunk". §7 (v16.0.0) only fixes PPID 60 and SCTP as the transport; it says nothing about one message per chunk. The rule actually rests on SCTP preserving message boundaries (RFC 4960) and on X.691 11.1.4, which pads the outermost APER encoding only to the next octet.
  • Suggested: _make_data now reads data.len directly, so a hand-built Data_OSPF without len raises. That is why test_ospf_unit.py needed len=24. A getattr fallback would avoid this.
  • Confirmed:
    • RFC 2328 D.4.3: the digest is not counted in the packet length. Six OSPF probes rebuild byte-exactly.
    • Auth types 0 and 1 produce the same bytes as main.
    • packet_length is the correct name; length really is swallowed by Protocol.__init__.
    • The Charpy decode matches from_aper across 3000 mutations.
    • All 1605 fixture frames are unchanged.
    • Leg: 856 tests, 0 failures.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 88.91% (unit tier, Python 3.14, 17bf03c43, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1034 2342 845 90.24%
pcapkit/corekit 1878 91 580 22 94.34%
pcapkit/dumpkit 142 0 42 0 100.00%
pcapkit/foundation 2442 102 852 39 94.38%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15792 189 4048 165 98.19%
pcapkit/toolkit 487 65 144 1 85.42%
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.

…after the NGAP-PDU

- OSPF: make() takes packet_length, and _make_data passes the parsed len, so
  the RFC 2328 D.4.3 digest outside Packet Length is no longer counted and a
  declared Length is not rewritten (#1249). Data without len falls back to
  24 + len(payload); a packet_length that is not an int in 0..65535 raises
  ProtocolError.
- OSPF: 8 raw octets are accepted as cryptographic auth_data and unpacked
  into the schema (#1250); auth_data of any other width raises ProtocolError
  instead of being padded or cut (#1251, OSPF half).
- NGAP: octets after the aligned PER NGAP-PDU raise ProtocolError instead of
  being dropped (#1252).

Closes #1249
Closes #1250
Closes #1252
Refs #1251
@JarryShaw
JarryShaw force-pushed the fix/1249-ospf-ngap-roundtrip branch from c777570 to 17bf03c Compare October 7, 2026 18:48
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on 17bf03c43: GOOD TO GO. I read the delta against c777570a8 myself; all three round-1 blockers are fixed.

  1. Citation. The TS 38.412 §7 claim is gone from both the comment and the PR body. It now cites RFC 9260 §1.3 (each user message occupies its own DATA chunk) and §3.3.1 (an unfragmented message has B and E set), plus X.691 (02/2021) §11.1.4 (zero to seven zero bits of padding) and §11.1.5 (the result is the complete encoding). Those four carry the "a whole octet left over is not part of the NGAP-PDU" conclusion without needing the NGAP spec at all.
  2. Missing len. _make_data passes data.get('len'), not getattr(..., None) — right, because the test fixture's __getattr__ raises KeyError rather than AttributeError, which a getattr default would not catch. When len is absent, make() falls back to 24 + len(payload) through the packet_length is None branch.
  3. packet_length validation. A non-int, a bool, or a value outside 0..65535 raises ProtocolError: OSPF: invalid packet length. Rejecting bool is the right call even though it is an int subclass.

The round-1 DummyData edit was reverted, so test_ospf_unit.py now differs from main only in the _make_encrypt_auth assertion. Both new tests fail against the previous head — the first outright, the second in all five subtests.

One honesty note rather than a change: the PR body's 389 passed, 1 skipped for tests/project is from round 1 and was not re-run after this amend. The protocols --exclude protocols/internet leg was (858 tests, 0 failures), and this amend touches no file that leg misses.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 7, 2026
@JarryShaw
JarryShaw merged commit e00c8ab into main Oct 7, 2026
38 checks passed
@JarryShaw
JarryShaw deleted the fix/1249-ospf-ngap-roundtrip branch October 7, 2026 20:02
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 7, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 25, 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) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

1 participant