Conversation
Only a 0-byte stub existed under NotImplemented/, so IP protocol 132 fell through to Raw and nothing could ride SCTP. RFC 9260, following the shape TCP uses for options: a common header, then chunks dispatched through a table, each with a read handler and a make counterpart. Covers all 13 chunk types the RFC defines, all 8 chunk parameters and all 13 error causes. Unassigned, reserved and extension types fall through to a generic handler that records raw flags and value rather than raising - as do ECNE and CWR, whose numbers the RFC assigns but whose formats it reserves. CRC32c is both recorded verbatim and verifiable: unlike TCP and UDP the digest covers only the SCTP packet with the field zeroed, no IP pseudo-header, so it can be checked from the SCTP bytes alone. The table is generated at import from the reflected polynomial and asserted against RFC 9260 Appendix A. The wire bytes are little-endian, which a test pins against the big-endian form. Dispatch is keyed on the DATA chunk's Payload Protocol Identifier rather than a port, so register_sctp(60, ...) is what NGAP will later call. Deliberately not wired into register_apptype's fan-out, which writes port numbers. Const modules under pcapkit/const/sctp/ are hand-written, but generated by driving the real Vendor.context() against the IANA CSVs, so they match what a future crawler would emit rather than diverging from it. Two corekit weaknesses were worked around locally rather than fixed in shared code: Schema.pack shares one packet dict with nested schemas, so a nested parameter's length overwrote the enclosing chunk's; and ListField.unpack can loop forever on a truncated list of SchemaField items.
There was a problem hiding this comment.
🟡 Changes recommended
SCTP PPID fallback currently mutates the class-level PPID registry by inserting a None key when a PPID is unregistered, which is unintended behavior that should be fixed before merging.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds first-class SCTP (RFC 9260) support to PyPCAPKit’s transport layer, including parsing/serialization, constants, registry wiring, and comprehensive unit/docs coverage, enabling correct decoding of IP protocol 132 and future PPID-based upper-layer protocols like NGAP.
Changes:
- Implement SCTP protocol support (schemas + data models + transport implementation) with CRC32c calculation/validation and PPID-based next-layer dispatch.
- Add SCTP constant enumerations (chunk types, parameters, causes, PPIDs) and expose them through package exports.
- Add extensive SCTP unit tests and documentation updates, plus Internet-layer dispatch for protocol number 132 and a
register_sctphelper.
File summaries
| File | Description |
|---|---|
| tests/protocols/transport/test_sctp_unit.py | Adds comprehensive SCTP unit tests (round-trip, RFC wire conformance, scapy cross-check). |
| pcapkit/protocols/transport/sctp.py | Implements SCTP transport protocol, chunk/param/cause dispatch, CRC32c helpers, PPID decoding. |
| pcapkit/protocols/transport/transport.py | Updates base transport docs to include SCTP. |
| pcapkit/protocols/transport/init.py | Exposes SCTP in transport package exports and TODO list. |
| pcapkit/protocols/transport/NotImplemented/sctp.py | Removes prior SCTP stub (no longer needed). |
| pcapkit/protocols/schema/transport/sctp.py | Adds SCTP packet/chunk/parameter/cause schemas, including padding/length helpers. |
| pcapkit/protocols/schema/transport/init.py | Re-exports SCTP schemas from the transport schema package. |
| pcapkit/protocols/schema/init.py | Re-exports SCTP schemas at the top-level schema package. |
| pcapkit/protocols/data/transport/sctp.py | Adds SCTP data models for packet/chunks/parameters/causes. |
| pcapkit/protocols/data/transport/init.py | Re-exports SCTP data models from the transport data package. |
| pcapkit/protocols/data/init.py | Re-exports SCTP data models at the top-level data package. |
| pcapkit/protocols/internet/internet.py | Registers IP protocol 132 dispatch to the SCTP implementation. |
| pcapkit/protocols/init.py | Exposes SCTP at the top-level pcapkit.protocols exports. |
| pcapkit/foundation/registry/protocols.py | Adds register_sctp helper and documents why register_apptype doesn’t fan out to SCTP. |
| pcapkit/foundation/registry/init.py | Exposes register_sctp from the registry package. |
| pcapkit/foundation/init.py | Exposes register_sctp from the foundation package. |
| pcapkit/const/sctp/chunk.py | Adds SCTP chunk type enumeration. |
| pcapkit/const/sctp/parameter.py | Adds SCTP chunk parameter type enumeration. |
| pcapkit/const/sctp/cause_code.py | Adds SCTP error cause code enumeration. |
| pcapkit/const/sctp/payload_protocol_identifier.py | Adds SCTP PPID enumeration. |
| pcapkit/const/sctp/init.py | Adds SCTP const package exports. |
| pcapkit/const/init.py | Re-exports SCTP constants at the top-level const package. |
| pcapkit/all.py | Adds SCTP and register_sctp to “import everything” exports. |
| pcapkit/init.py | Adds SCTP to top-level package exports. |
| docs/source/pep.rst | Updates “More Protocols” notes to reflect SCTP is implemented. |
| docs/source/pcapkit/protocols/transport/transport.rst | Updates transport base protocol docs to include SCTP. |
| docs/source/pcapkit/protocols/transport/sctp.rst | Adds SCTP protocol documentation page (API, schemas, data models). |
| docs/source/pcapkit/protocols/transport/index.rst | Adds SCTP to transport docs index and updates TODO list. |
| docs/source/pcapkit/protocols/index.rst | Updates protocol hierarchy diagram/links to include SCTP. |
| docs/source/pcapkit/foundation/registry.rst | Documents register_sctp in registry docs. |
| docs/source/pcapkit/const/sctp.rst | Adds documentation page for SCTP constant enumerations. |
| docs/source/pcapkit/const/index.rst | Adds SCTP to const docs index. |
| docs/source/ext.rst | Updates extensibility docs to include SCTP and register_sctp. |
Review details
- Files reviewed: 31/33 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… state The const modules under pcapkit/const/sctp/ had no pcapkit/vendor/sctp/ counterpart, which every other const package has and the project's rule requires for registry-derived enums. Four crawlers now generate them from the IANA SCTP parameters registry: chunk types (-1), chunk parameter types (-2), error cause codes (-24) and payload protocol identifiers (-25). The committed modules regenerate with no member or value changing. The only differences are docstrings and #: comments, and four of those are corrections: the throwaway script that first produced these files split on every newline rather than on CRLF, which made csv.reader swallow the separator inside IANA's multi-line quoted reference fields and ran words together - "Transport over SCTP",November and "Bearer Independent CallControl protocol" among them. The crawlers' docstrings replace the "maintained manually" wording, which is no longer true, here and in the docs and on the Help Wanted page. Separately, a Copilot finding on the PR: an unregistered Payload Protocol Identifier was blanked to None before dispatch, and __proto__ is a defaultdict, so reading it inserted a None key into class-level state shared by every later SCTP instance and dispatched the payload under alias None. The PPID now reaches the next layer unchanged, as Internet does with an unregistered protocol number, and an _import_next_layer override looks the registry up without mutating it, falling back to the Raw the registry already declares. An unregistered PPID now yields Raw with the payload intact, info.protocol set to the real identifier, and no new registry key. The same defaultdict-read shape affects __chunk__, __parameter__ and __cause__ at six further call sites: those insert the real code rather than None, so dispatch stays right, but the growth is unbounded and it makes a later legitimate register_chunk() warn that the code is already registered. Left for its own change. Full suite: 473 passed, 4 skipped, 249 subtests passed.
There was a problem hiding this comment.
🔵 Needs a closer look
The PR introduces a large new transport protocol implementation plus registry/public-API surface updates, which warrants final human verification despite only minor review findings.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
pcapkit/protocols/data/transport/sctp.py:315
- Docstring has a duplicated word (“parameter parameter”), which looks like a typo and will show up in rendered documentation.
pcapkit/protocols/schema/transport/sctp.py:501 - Docstring has a duplicated word (“parameter parameter”), which reads like a typo and will propagate into generated docs.
- Files reviewed: 31/41 changed files
- Comments generated: 0 new
- Review effort level: Lite
JarryShaw
added
the
feat
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
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.
Clears SCTP from the Help Wanted transport list, and is the prerequisite for NGAP: discussion #251 asks for NGAP on a TCP/UDP port, but real 5G N2 traffic rides SCTP, which did not exist here — only a 0-byte stub under
NotImplemented/, so IP protocol 132 fell through toRaw.Shape
Modelled on TCP, since both are "a header followed by typed, length-prefixed things": a common header, then chunks dispatched through a
__chunk__table, each with a_read_chunk_*and a_make_chunk_*counterpart — 68 methods, every read paired, which keeps the codebase's read/make invariant intact.Covered: all 13 chunk types RFC 9260 defines, all 8 chunk parameters (heartbeat info, IPv4/IPv6 address, state cookie, unrecognized parameter, cookie preservative, host name, supported address types), and all 13 error causes.
Falling through to a generic handler that records raw flags and value rather than raising: AUTH, I-DATA, DTLS, ASCONF/ASCONF-ACK, RE-CONFIG, PAD, FORWARD-TSN, I-FORWARD-TSN, every unassigned or reserved type, extension parameters (32768+) and extension cause codes — and ECNE/CWR, whose numbers the RFC assigns but whose formats it reserves, so there is nothing to parse. Nothing is guessed.
Two documented narrowings: reserved chunk-flag bytes on the chunks that define none are not exposed and are emitted as zero, per the RFC's "set to 0 on transmit, ignored on receipt"; and
UnrecognizedParameterplus the unrecognized-chunk/parameter causes keep their payload as bytes rather than recursing, since by definition the sender did not recognise it.CRC32c is recorded and verifiable
Data_SCTP.chksumkeeps the four wire octets verbatim, as TCP and UDP do, pluscrc32c(),calculate_checksum(),validate_checksum()and achecksum_validproperty;make(chksum=None)computes it.Worth doing here specifically because SCTP's digest covers only the SCTP packet with the field zeroed — no IP pseudo-header — so it is verifiable from the SCTP bytes alone, unlike TCP/UDP. The table is generated at import from the reflected polynomial
0x82F63B78and asserted against the values quoted in RFC 9260 Appendix A. The orientation trap has its own test: the wire bytes are the CRC32c little-endian, and the test asserts it is not the big-endian form.PPID dispatch, for NGAP later
SCTP.__proto__is keyed on the DATA chunk's Payload Protocol Identifier, not a port.read()finds the first DATA chunk, exposes its PPID, and_get_payload()returns that chunk's user data, bypassingTransport's port lookup. NGAP will call either of:Deliberately not added to
register_apptype's fan-out: that helper writes port numbers, and a port number in a PPID-keyed registry would dispatch on the wrong namespace.Constants
pcapkit/const/sctp/is hand-maintained — there is no crawler — but it was produced by driving the realVendor.context()against the IANA CSVs, so it is byte-identical in style and naming to what a crawler would emit and a later regeneration will not churn. That is also why the PPID member is the unlovelyPayloadProtocolIdentifier_3GPP_NG_Application_Protocol: the generator's digit-leading fallback, precedentTransType_3PC. Their docstrings say "maintained manually", not TCP's "automatically generated from", which would be untrue.A future
pcapkit/vendor/sctp/crawler has a target:https://www.iana.org/assignments/sctp-parameters/sctp-parameters.xhtml, covering Chunk Types, Chunk Parameter Types, Error Cause Codes and PPIDs. Chunk flags are per-chunk bitfields, not value enums, so they stayBitFieldnamespaces.Verification
Round trip plus wire conformance per chunk type —
makethenreadback, and a hand-written byte string whose expected fields come from RFC 9260 — and then cross-checked against scapy: seven scapy-built frames written to a temp pcap and read back, all reachingEthernet:IPv4:SCTPwith scapy's own CRC32c validating. Field by field on INIT (length, init_tag, a_rwnd, in/outbound streams, init_tsn, parameter order, address, supported types), DATA (tsn/sid/ssn/ppid/payload and theI/U/B/Eflag bits against scapy's own names, which is what pins the bit positions), SACK (cum_tsn_ack, a_rwnd, gap blocks, duplicates), and the rest.33 tests / 58 subtests. Full suite: 472 passed, 4 skipped, 249 subtests passed. One of those 33 was skipped in development behind a runtime probe for the
BitFieldconstruction bug; #374 having landed, it now runs and passes on its own.Two corekit weaknesses, worked around locally
Both were hard blockers, both are worked around inside this branch's own files rather than by touching shared code, and both deserve their own issues:
Schema.packshares onepacketdict with nested schemas, so a nested parameter'slengthoverwrote the enclosing chunk's before the chunk's padding was sized — INIT ACK packed three octets too long and the next chunk misparsed. The six chunks with nested TLV lists fold chunk padding into the list's span instead. That also makes the parser accept a chunk whose length omits the final parameter's padding, which RFC 9260 §3.2 requires, and there is a test for it.ListField.unpackcan loop forever on a truncated list ofSchemaFielditems, because a zero-length parse subtracts nothing from its budget. This hung the suite. Clamped locally to the octets actually present.Merge note
The one line in
pcapkit/protocols/internet/internet.pyregistering protocol 132 is appended after theHIPentry; #378 (ESP) adds protocol 50 next toAHin the same table. Different lines, so the conflict should be trivial or absent.