fix(esp): the extension= keyword is accepted, stored and never read

Describe the bug

ESP accepts an extension= keyword, stores it, and then never reads it. Extension mode is dead code for that one protocol.

pcapkit/protocols/internet/esp.py:1276 assigns self._extf = extension, and no line anywhere in the file reads self._extf. Contrast pcapkit/protocols/internet/ah.py, its sibling under IPsec, which reads it three times (:75, :87, :99) to guard name, protochain and payload, and assigns it at :233.

Counted across the eight implemented IPv6 extension headers:

                _extf   UnsupportedCall   if extension: return
HOPOPT            4            7                   1
IPv6_Route        4            7                   1
IPv6_Opts         4            7                   1
IPv6_Frag         4            7                   1
MH                4            7                   1
HIP               4            7                   1
AH                4            7                   1
ESP               1            0                   0

ESP is the only outlier on all three markers: it has no UnsupportedCall guard properties and no if extension: return short-circuit in read.

Reproduction

Not run against a capture — read from source at origin/main. ESP.read declares extension: 'bool' = False at :996-997 and __post_init__ at :1259-1260, so ESP(..., extension=True) is accepted; :1276 records it; nothing consumes it. The parse therefore behaves identically whether extension is True or False.

Expected behavior

One of two things, and which is a design decision rather than a mechanical fix:

ESP should support extension mode — then it needs the three UnsupportedCall guard properties and the if extension: return short-circuit its seven siblings have, and _extf becomes load-bearing.
ESP should not — then extension= should be removed from read and __post_init__ rather than accepted and silently discarded, and :1276 deleted.

Accepting a keyword that changes nothing is the worst of the three, because a caller cannot tell it had no effect.

System information

Source-only; no library version involved. Read at origin/main.

Additional context

Found while measuring the IPv6_Ext shared-base proposal on #891, where it decides whether ESP joins that base. Noted there in #891 so the refactor is not silently made to carry this fix: giving ESP the base would supply the missing guards as a side effect, turning a behaviour-preserving refactor into a behaviour change. This issue exists so that question gets answered on its own merits.

Direction settled by the maintainer's ruling on #891, verbatim:

If per IANA, ESP is an extension header, then it MUST honour the same contracts - that's an implementation defect. And thus we should have ESP double inherit as AH then.

The condition holds. IANA's protocol-numbers-1.csv marks 50,ESP,Encap Security Payload,**Y**,[RFC4303] in the IPv6 Extension Header column, alongside 51,AH,...,Y,[RFC4302] — and this library's own IANA-derived registry already says the same at pcapkit/const/ipv6/extension_header.py:33 (ESP = 50).

So of the two options in the body above, the first is chosen: ESP gains the extension-mode contract. The extension= keyword is not removed.

Concretely that means giving ESP what its seven siblings have — the three UnsupportedCall guard properties reading self._extf (compare ah.py:75,87,99) and the if extension: return short-circuit in read — so that _extf, already assigned at esp.py:1276, becomes load-bearing.

Sequencing: the maintainer also ruled that ESP double-inherits the forthcoming IPv6_Ext base as ESP(IPsec, IPv6_Ext), mirroring AH. Since that base supplies exactly these members, this fix and that re-parent are one change rather than two — implementing the guards by hand now and then hoisting them a PR later would be wasted work. So this issue is deliberately held until the IPv6_Ext base lands, and it is then satisfied by the re-parent.

Marking blocked accordingly. Checkable blocker: the IPv6_Ext base issue, which is itself filed only after #891 merges. Two commands settle whether this is still blocked at any moment:

gh issue list -R JarryShaw/PyPCAPKit --state open --search "IPv6_Ext in:title"
gh issue view 891 -R JarryShaw/PyPCAPKit --json state -q .state

If it turns out the base slips or is abandoned, this unblocks and gets the guards written by hand instead — the defect is real either way and must not be lost to the sequencing.

Scope correction, so this is not closed wholesale when #924 merges: #924 discharges the defect here but not the optimisation.

The substantive half is satisfied by construction. _extf was assigned at esp.py and never read anywhere — a keyword accepted and silently ignored, which is what this issue is about. Under #924 it becomes load-bearing for ESP: payload and protochain read it through the new IPv6_Ext base, and protocol reads it by hand. So extension= now changes behaviour rather than being swallowed.

What #924 does not do: ESP.read still has no if extension: return short-circuit, which its seven siblings all have. That saves wasted decode work rather than changing behaviour, so it is an optimisation this issue never actually asked for — but it is the one difference left between ESP and the other extension headers, and it will look like an oversight to whoever reads the family next.

So when #924 merges, the right move is to close this on the defect and either drop the short-circuit or re-file it as its own small perf item, rather than closing silently and leaving the asymmetry unrecorded. I am not deciding that — flagging it while the context is fresh.

blocked stays until #924 merges; #917 is still open.

refactor(ipv6): rename IPv6_GenericExt to IPv6_Ext and make it the sh…

Unblocked — #917 closed and #924 merged as 93cf940b3.

The defect is discharged. _extf was assigned at esp.py and never read anywhere, which is exactly what this issue reported: a keyword accepted and silently ignored. Under #924 it is load-bearing — payload and protochain read it through the new IPv6_Ext base, and protocol reads it by hand — so extension= now changes behaviour.

What remains is one asymmetry, and it is not what this issue asked for. ESP.read still has no if extension: return short-circuit, which its seven siblings all have. That saves wasted decode work rather than changing behaviour.

So this can be closed on the defect. I am not closing it myself because the choice is yours: close it now and let the short-circuit go unrecorded, or close it and let me file the short-circuit as a small perf follow-up so the difference between ESP and the other seven is written down somewhere. I would file the follow-up — an unexplained asymmetry in a family of eight is the kind of thing that reads as an oversight to whoever touches it next.

Removing blocked; leaving the issue open pending that choice.

i say the short-circuit must be done in ESP cause it's part of the IPv6 extension headers.

unless RFC tells otherwise.

I am reversing my own PR. Your qualifier — "unless RFC tells otherwise" — resolves against the change, and #928 as written is wrong.

I posted my record of the ruling before your qualifier arrived; I read the channel at 18:28 and did not re-read before replying at 18:35. That is my miss, and the qualifier turns out to be the operative half.

The RFC does tell otherwise, and the library already encodes it. :rfc:8200#section-4.5 sets ESP aside — "For this purpose, the Encapsulating Security Payload (ESP) is not considered an extension header" — and the next sentence lists ESP among "examples of upper-layer headers". IPv6.__generic_ext_codes__ (pcapkit/protocols/internet/ipv6.py:116) matches that exactly, and it is seven codes, not eight:

[0, 43, 44, 51, 60, 135, 139]     # HOPOPT, Route, Frag, AH, Opts, MH, HIP
ESP/50 in __generic_ext_codes__:  False

So the premise "it's part of the IPv6 extension headers" is the one thing that does not hold for ESP — AH is in that set, ESP deliberately is not.

Two consequences, both against the change:

The guard is unreachable from the chain walk. IPv6._decode_next_layer only passes extension=True for codes in that frozenset, so it never constructs ESP that way. The short-circuit can only fire for a caller who passes extension=True by hand.
For that caller it would lose data, not save work. ESP's Next Header lives inside the encrypted trailer, so IPv6's walk cannot read it and cannot continue past ESP. The decrypted plaintext reaches _decode_next_layer and nowhere else — esp.py:1186-1187 is the only place inner is passed on. Skipping it discards every inner layer with nothing downstream to recover them. That is the opposite of the seven siblings, where next is in cleartext and the walk genuinely does the work instead.

So my recommendation is to close #928 unmerged, and record the asymmetry as deliberate rather than as an oversight: ESP has no short-circuit because it is not an extension header, which is the same reason it is absent from __generic_ext_codes__.

That leaves #895 fully discharged by #924 on the defect, with nothing outstanding. Labelling needs: decision since this reverses your stated premise and I would rather you confirmed than have me act on my own reading of the RFC. If you would rather keep the guard for the hand-passed case, say so and I will keep it but restrict it to the undecrypted path, where there is no plaintext to lose.

Reversal confirmed by an independent review and by my own measurement. My recommendation to close #928 stands, with one refinement that makes the claim narrower and more defensible.

Measured by me on RFC 3602 §4 case 7 (tunnel-mode ESP, AES-CBC-128), the vector already in tests/protocols/internet/test_esp_unit.py:

extension=False                      extension=True
  status     DECRYPTED                 status     DECRYPTED
  plaintext  84 octets                 plaintext  84 octets      <- identical
  info.next  4 (IPv4)                  info.next  4 (IPv4)       <- identical
  info.ipv4  present                   info.ipv4  ABSENT
  protochain ESP:IPv4:ICMP             protochain UnsupportedCall

The refinement: it is not byte loss. info.plaintext is byte-identical either way, and so is info.next. What is destroyed is every parsed inner layer — info.ipv4, a public data-model field holding the fully parsed inner datagram with ICMP under it, is simply gone — together with the __next_type__ / __next_name__ keys that _decode_next_layer writes (internet.py:227-228) and that protocol.py:1657,1663 reads back to descend. So even a caller who knew to look has no handle. That is the accurate framing; "the plaintext is thrown away" would have been the stronger claim and it is false.

Two further findings from the review, both against the change:

There was no third unguarded path. An AST enumeration of ESP.read found exactly 9 Return nodes — 7 × _make_opaque, the new return esp, and the _decode_next_layer call — with _make_opaque called only from read. So the defect is that the guards should not exist, not that one was missing.
The read-tail guard had no runtime test at all. All five of my tests take the no-keys _make_opaque route; the decrypted guard was covered only by the source-level family sweep, which cannot see behaviour. A field-by-field decrypted comparison would have failed on info.ipv4 immediately. That is the gap that let me write a wrong change and believe it tested.

So: close #928 unmerged, and record ESP's lack of a short-circuit as deliberate — the same reason it is absent from IPv6.__generic_ext_codes__. #895 is then fully discharged by #924 on the defect, with nothing outstanding.

Still needs: decision, because closing it reverses your stated premise and that is your call, not mine. If you would rather keep a guard for the hand-passed case, the _make_opaque half is safe in isolation — there is no plaintext to lose on the undecrypted path — but it would be unreachable from the chain walk and I would not bother.

confused. what exactly does RFC say about ESP? is it an extension header? if so, then it should follow the other extension headers with the short circuits.

IPv6.__generic_ext_codes__ does not have ESP probably bc we didnt implement ESP back then. that's a miss not a pattern.

Both answered with measurement, and I owe you a correction on my own framing first.

I said "the RFC does tell otherwise" leaning on :rfc:8200#section-4.5. That was one-sided. The RFCs are split, and by the authority that matters most, ESP is an extension header:

IANA's IPv6 Extension Header Types registry lists ESP (50). I fetched it: 11 entries, [0, 43, 44, 50, 51, 60, 135, 139, 140, 253, 254]. :rfc:8200#section-4 makes that registry authoritative for what counts.
:rfc:8200#section-4.5 scopes its exclusion: "For this purpose," — the fragmentation discussion — ESP "is not considered an extension header", and the next sentence lists it among upper-layer headers.
:rfc:4303#section-3.1.1 places ESP after the IP header in both IPv4 and IPv6, i.e. in the chain.

So your instinct was right on taxonomy: ESP is an extension header. My earlier framing overstated §4.5 by dropping its scoping clause, which is exactly the overstatement #926 fixed in the prose two hours ago. I should not have repeated it.

Your second point is refuted, though, and this is the part that decides it. The __generic_ext_codes__ exclusion is not a legacy miss:

esp.py added                 bdb7e5971  2026-09-14  Implement ESP … (#378)
__generic_ext_codes__ added  b3551cb63  2026-09-29  fix(ipv6): parse unrecognised
                                                    extension headers generically (#904)
ESP mentions in that commit's ipv6.py: 11

It was written today, two weeks after ESP shipped, by a commit that mentions ESP eleven times — and it carries an explicit reason at ipv6.py:90-100: "it is excluded because :rfc:4303 places the real Next Header byte inside the encrypted trailer."

Which is why "it's an extension header, so it should short-circuit" does not follow. The seven siblings can short-circuit because their next is readable in cleartext, so IPv6._decode_next_layer genuinely continues the walk and does the work instead. ESP's next is inside the encrypted trailer, so the walk cannot read it and stops. Nothing downstream picks up the plaintext — which is what I measured on RFC 3602 case 7: info.ipv4 present without the guard, absent with it.

So ESP is an extension header and it cannot short-circuit — not a contradiction, just a consequence of where its Next Header lives. Still needs: decision, and I would close #928.

okay, that makes sense now.

Ruling recorded, verbatim:

okay, that makes sense now.

Taken as accepting the reversal, so #928 is closed unmerged and this issue closes with it.

Where that leaves #895. The defect it reported — extension= accepted and silently ignored — was discharged by #924 as 93cf940b3: _extf became load-bearing, so payload, protochain and protocol all consult it. The short-circuit half is settled as not applicable to ESP, on the mechanism rather than on taxonomy:

ESP is an IPv6 extension header. IANA's IPv6 Extension Header Types registry lists 50 among its 11 entries, and :rfc:8200#section-4 makes that registry authoritative. :rfc:8200#section-4.5's exclusion is scoped to "For this purpose," — the fragmentation discussion — and I was wrong to quote it unscoped earlier.
It still cannot short-circuit, because :rfc:4303 places the real Next Header byte inside the encrypted trailer. The seven siblings can defer to IPv6._decode_next_layer precisely because their next is readable in cleartext; ESP's is not, so the walk stops there and nothing downstream picks up the plaintext.

Measured on RFC 3602 §4 case 7, tunnel-mode ESP: with the guard, info.ipv4 — the fully parsed inner datagram, ICMP beneath it — is absent, and protochain raises UnsupportedCall, where without it they read ESP:IPv4:ICMP. info.plaintext and info.next survive byte-identically, so it is loss of every parsed inner layer rather than loss of bytes.

Already documented, so nothing new needs writing: pcapkit/protocols/internet/ipv6.py:90-100 states the exclusion and its reason, added by #904 today — two weeks after ESP shipped in #378, by a commit mentioning ESP eleven times. So it was a deliberate choice with full knowledge of ESP, not the legacy miss it looked like.

Closing. Branch fix/895-esp-extension-short-circuit deleted as abandoned.

Navigation Menu

fix(esp): the extension= keyword is accepted, stored and never read #895

1 remaining item

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

fix(esp): the extension= keyword is accepted, stored and never read #895

Description

Activity

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

1 remaining item

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

JarryShaw commented on Sep 29, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions