Implement ESP, with optional payload decryption - #378
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A few concrete issues remain around context/packet forwarding semantics and avoiding potentially sensitive context repr output in engine warnings.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds first-class support for IPsec ESP (protocol 50) including optional payload decryption, and introduces a generic “protocol-keyed parsing context” mechanism so protocols can consume caller-supplied out-of-band state (e.g., ESP Security Associations) without protocol-specific Extractor kwargs.
Changes:
- Implement
pcapkit.protocols.internet.esp.ESPwith SA-driven trailer/ICV handling and optional decryption viacryptography(extrapypcapkit[crypto]). - Introduce
pcapkit.corekit.context.ContextRegistryand propagate it through extraction and nested protocol parsing via__context__/_get_context(). - Add comprehensive ESP unit tests and update docs/registries so ESP is importable, documented, and reachable from IP protocol 50.
File summaries
| File | Description |
|---|---|
| tests/protocols/internet/test_esp_unit.py | New unit tests covering ESP registry, parsing, decryption, failure modes, and extraction end-to-end. |
| tests/foundation/engines/test_runtime_engines.py | Updates runtime-engine test helper to include an extractor context registry. |
| pyproject.toml | Adds optional dependency extra crypto = ["cryptography>=3.4"] and includes it in all. |
| Pipfile | Adds cryptography to the dev environment dependencies. |
| pcapkit/protocols/schema/internet/esp.py | Adds ESP schema (SPI/SEQ + opaque payload field). |
| pcapkit/protocols/schema/internet/init.py | Exports ESP schema in the internet schema package. |
| pcapkit/protocols/protocol.py | Adds context plumbing: stores __context__, exposes _get_context(), forwards context to next layers. |
| pcapkit/protocols/internet/NotImplemented/esp.py | Removes abandoned ESP sketch implementation. |
| pcapkit/protocols/internet/ipv6.py | Forwards __context__ when importing next layers. |
| pcapkit/protocols/internet/ipv4.py | Threads __packet__ (src/dst) for downstream protocols (used by ESP destination matching). |
| pcapkit/protocols/internet/ipsec.py | Updates docs/comments now that ESP is implemented. |
| pcapkit/protocols/internet/internet.py | Registers ESP in protocol registry and forwards __context__ to next layers. |
| pcapkit/protocols/internet/esp.py | New ESP implementation (SA, algorithms, decryption/encryption, and opaque fallback). |
| pcapkit/protocols/internet/init.py | Exports ESP from the internet protocols package and updates TODO list. |
| pcapkit/protocols/data/internet/esp.py | Adds ESP data model capturing ciphertext/plaintext split and status. |
| pcapkit/protocols/data/internet/init.py | Exports ESP data model in the internet data package. |
| pcapkit/protocols/init.py | Exports ESP at the top-level protocols namespace. |
| pcapkit/interface/core.py | Adds context= argument to extract() public API and documents its use. |
| pcapkit/foundation/extraction.py | Stores normalized caller context on the Extractor (_exctx). |
| pcapkit/foundation/engines/scapy.py | Warns when context is supplied but ignored by Scapy engine. |
| pcapkit/foundation/engines/pyshark.py | Warns when context is supplied but ignored by PyShark engine. |
| pcapkit/foundation/engines/pcapng.py | Passes __context__ into PCAPNG parsing path. |
| pcapkit/foundation/engines/pcap.py | Passes __context__ into PCAP frame parsing path. |
| pcapkit/foundation/engines/dpkt.py | Warns when context is supplied but ignored by DPKT engine. |
| pcapkit/corekit/context.py | New ContextRegistry / ProtocolContext implementation and documentation. |
| pcapkit/init.py | Exports ESP at package top-level. |
| docs/source/pep.rst | Updates Help Wanted / ESP status notes to reflect implementation. |
| docs/source/pcapkit/protocols/internet/ipsec.rst | Removes “ESP not implemented” footnote references. |
| docs/source/pcapkit/protocols/internet/index.rst | Adds ESP to the internet protocol docs toctree and updates TODO. |
| docs/source/pcapkit/protocols/internet/esp.rst | New ESP documentation page. |
| docs/source/pcapkit/corekit/index.rst | Adds context channel to corekit docs index. |
| docs/source/pcapkit/corekit/context.rst | New documentation page for parsing context mechanism. |
Review details
Suppressed comments (1)
pcapkit/protocols/internet/esp.py:1038
- Passing
packet=packet or Nonedrops an empty-but-intentionally-shared packet-info dict (and is inconsistent with how other protocols forwardpacket). Forward the dict as-is so downstream layers can rely on object identity.
return self._decode_next_layer(esp, next_type, len(inner), packet=packet or None,
version=version, payload=inner)
- Files reviewed: 32/32 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces substantial new crypto-adjacent parsing and a new context-propagation mechanism across core extraction/protocol plumbing, warranting final human review despite limited nits.
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in ESP’s UNSUPPORTED handling and packet-context propagation that can lead to wrong status/ICV reporting and missing schema context in nested parsing.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 38/44 changed files
- Comments generated: 4
- Review effort level: Lite
|
@copilot resolve the merge conflicts in this pull request |
Merged |
RFC 4303 ESP was advertised but absent: pcapkit/protocols/internet/ipsec.py
claimed id() == ('AH', 'ESP'), the docs carried a broken xref to the class, and
IP protocol 50 fell through to Raw. The only source was a 98-line sketch under
NotImplemented/ whose read() body was `pass`.
Parsing splits at the SA boundary, which the wire does not carry. Without
context ESP reports SPI and sequence and hands the remainder over as opaque
ciphertext, guessing no trailer and never raising. With an SA it derives the
ICV split, decrypts, strips padding by Pad Length and dispatches the plaintext
through Next Header, so an ESP-tunnelled datagram decodes as its inner
protocol.
Getting that context to a protocol needed a channel, since nothing
user-supplied reached one during extraction: pcapkit/corekit/context.py keys
caller-supplied state on Protocol.id(), Extractor and extract() take a
context= argument, and _import_next_layer threads it down as __context__.
Keyed on Protocol.id() rather than made ESP-specific, so any protocol needing
caller state can use it.
Ciphers via cryptography, declared optional: AES-CBC, AES-GCM and NULL, with
HMAC-SHA1/SHA2 integrity, per RFC 8221's mandatory-to-implement set. pcapkit
imports and works without it, and ESP degrades to the no-keys path.
Extended sequence numbers, TFC padding and anti-replay are not implemented; an
ESN packet fails its integrity check cleanly rather than being decoded.
Tests use published vectors - RFC 3602 cases 5 and 7, draft-mcgrew-gcm-test-01
- and cover wrong keys, a truncated ICV, and that keys stay out of dumps and
reprs.
- The cached cryptography import used an object() sentinel for "not yet
attempted"; NotImplemented now marks that state, with None still meaning
attempted-and-absent, so the tri-state reads without a private object.
- Three engine warnings interpolated the caller's context with !r. That is
exactly the material this protocol keeps out of logs, and a caller's context
need not have a safe __repr__, so the messages name the keyword instead.
- ESP replaced a caller-supplied empty packet dict with a fresh one through
`or {}`, and passed `packet or None` down to the next layer; both now test
for None, as IPv4 and IPv6 do, so an intentionally empty dict survives.
- ProtocolBase.__init__ adopts an already-normalised ContextRegistry instead of
calling make(), which copies: every nested layer normalised what it was
handed, so a capture paid a dict copy per protocol for nothing. make() keeps
copying, since the public `context` property relies on it to hand out a
registry that cannot reach into the protocol's own.
Full suite: 467 passed, 4 skipped, 227 subtests passed.
Cipher and Integrity were IANA registry enums living in the protocol module.
They are IKEv2 Transform Type 1 and Type 3 transform IDs, so under the
project's rule they belong in const with a vendor crawler, and they now do:
pcapkit/const/esp/{cipher,integrity}.py generated by pcapkit/vendor/esp/, from
ikev2-parameters-5.csv and -7.csv. Named per consumer, as every other const
subpackage is, with both __init__ docstrings noting the registry is IKEv2's and
ESP the only consumer so far.
Neither CSV matches Vendor.process()'s default layout - column 2 is Status, not
the reference - so each crawler carries its own process(). Registry names are
kept verbatim and the prefix-stripped spelling is emitted as an alias, which is
what keeps Cipher.AES_CBC resolving in the docstring examples elsewhere.
Registration is not support. The registry contributes 36 ciphers and 15
integrity algorithms; pcapkit implements 5 of each, and that is now an explicit
table in esp.py rather than properties on the enum, so a registered but
unimplemented algorithm is refused at SA construction with a message naming
what is implemented. Every value and parameter for the supported algorithms was
compared against the old inline enums and is identical.
ESPStatus stays in esp.py: it describes pcapkit's own decrypt outcome and
appears in no registry.
Also normalised the three pyshark engine warnings to 'Extractor(engine=pyshark)',
matching dpkt and scapy, and fixed the adjacent "dose not support" typo.
Full suite: 467 passed, 4 skipped, 227 subtests passed.
904140b to
37a7b95
Compare
read() checked association.unavailable() ahead of the ICV split and the truncation check, so with cryptography missing two things went wrong: the ICV was never reported even though the security association declares its length independently of any crypto backend, and a packet too short to hold that ICV came back UNSUPPORTED instead of TRUNCATED. The length comes from the SA, so both the split and the truncation check are possible either way, and they now run first. An UNSUPPORTED packet reports the ICV it carries -- for a combined-mode algorithm that is the authentication tag, which a caller may well want to see -- and payload_data excludes it, matching every other status. Addresses Copilot's review comment on #378.
|
All ten inline comments triaged. Six were already fixed in earlier pushes (the three Fixed here ( Filed as #382, not fixed here — the three Verification on this head: 3.14 full suite 502 passed / 4 skipped / 285 subtests; 3.10 CI selection 362 passed / 28 skipped / 0 failed — the configuration that was previously red, now green on the rebase onto #381. |
Closes the ESP item on the Help Wanted page. Scope chosen by @JarryShaw: full ESP including decryption, accepting the crypto dependency and key-management surface.
ESP was already advertised, and absent
pcapkit/protocols/internet/ipsec.pyreturnedid() == ('AH', 'ESP')— asserted bytests/protocols/internet/test_ah_unit.py— the docs carried a broken xref topcapkit.protocols.internet.esp.ESP, and IP protocol 50 fell through toRaw. The only source was a 98-line sketch underNotImplemented/whoseread()body waspass, in a directory with no__init__.py. It was read for intent, then the real thing written fresh; the sketch is deleted.The structural problem, and the general fix
RFC 4303 does not put the payload length, the Pad Length/Next Header trailer or the ICV length on the wire — they need SA context. And nothing user-supplied reached a protocol during extraction:
Protocol._import_next_layerpassed a fixed kwarg set, andExtractor.__init__a fixed keyword list.So this adds a channel rather than an ESP special case:
pcapkit/corekit/context.py, keying caller-supplied state onProtocol.id().Extractorandextract()take acontext=argument, and_import_next_layerthreads it down as__context__, mirroring the existing__packet__convention. Any protocol needing caller state can use it; a protocol finds its own entry without knowing how the caller spelled the registry. All three signatures gained keyword-only arguments with defaults, so nothing existing changes shape.Behaviour with and without keys
Without an SA — reports SPI and sequence, hands the remainder over as opaque ciphertext with
status=NO_SA, leavesnext/pad_len/padding/plaintextasNonerather than guessing a trailer, and surfaces the ciphertext asRawsopayloadandprotochainbehave normally. It never raises. Verified on this branch:0x8765 seq=1, payload classRaw.With an SA — derives the ICV split, decrypts, strips padding by Pad Length, and dispatches the plaintext through Next Header.
Algorithms
Supported:
ENCR_NULL,ENCR_AES_CBC(128/192/256),ENCR_AES_GCM_8/12/16; integrityAUTH_NONE,AUTH_HMAC_SHA1_96,AUTH_HMAC_SHA2_256_128/384_192/512_256— RFC 8221's mandatory-to-implement anchor. Not implemented and rejected with a clear error rather than half-working: AES-CCM, ChaCha20-Poly1305, 3DES, DES, AES-XCBC-96, AES-GMAC, HMAC-MD5-96. Documented in two tables rendered on the ESP docs page.cryptographyis an optional extra; pcapkit imports and works without it, and ESP degrades to the no-keys path with a warning if a key is supplied while it is missing.Deliberately not implemented
ICV failures are reported, not raised, so one bad packet cannot abort a capture.
Keys are kept out of anything that gets written
Checked rather than asserted: the only thing serialised is
frame.info.to_dict(), and the ESP data model holds no key field by construction;ProtocolBase.__repr__renders_infoonly, so an SA held on the context cannot appear;SecurityAssociation.__repr__prints SPI, algorithm names, ICV length and destination. A test extracts a real capture to atreedump and asserts the key hex is absent from it.Tests
28 tests / 36 subtests, on published vectors: RFC 3602 §4 cases 5 and 7 (AES-CBC-128, transport and tunnel mode — case 7 asserts the tunnelled datagram decodes to
IPv4:ESP:IPv4:ICMPwith the inner addresses), and draft-mcgrew-gcm-test-01 §4 for AES-GCM at 128 and 256 bits. HMAC-SHA-256-128 has no published ESP packet vector, so its expected ICV is both recomputed with stdlibhmacover the RFC 4303 §2.8 coverage and pinned as hex.Also covered: wrong cipher key →
DECRYPT_FAILEDwithplaintext=None, wrong AEAD key →AUTH_FAILED, wrong integrity key →AUTH_FAILEDbefore decryption, corrupted and truncated ICV, non-block-multiple ciphertext,makeround trips includingencrypt=Trueacross 20 payload sizes and all three GCM ICV lengths, degradation withcryptographymocked absent, destination-keyed SA matching, and ESP as an IPv6 extension header.Full suite on this branch: 467 passed, 4 skipped, 227 subtests passed — exactly +28/+36 over
main, nothing else moved.