Skip to content

refactor(vendor,pcapng): generate TLSKeyLabel from the RFC 9850 IANA registry - #890

Merged
JarryShaw merged 1 commit into
mainfrom
worktree-agent-a0feac1569add5104
Sep 29, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
worktree-agent-a0feac1569add5104

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Summary

Closes #886.

TLSKeyLabel moves from a hand-written class in pcapkit/protocols/misc/pcapng.py
to a generated one at pcapkit/const/pcapng/tls_key_label.py, crawled from the
IANA "TLS SSLKEYLOGFILE Labels" registry (RFC 9850 section 4.2), matching its
const/pcapng siblings' shape. RSA has no registry row (NSS-only, removed in
NSS 3.34) and is hand-carried by the crawler with its #883 comment kept
verbatim. Matching stays by uppercase value, not name, unchanged.

The canonical import path changes, per the owner's ruling on #886 ("go with
(1)"); pcapkit/protocols/misc/pcapng.py now re-exports the class under its own
name, so from pcapkit.protocols.misc.pcapng import TLSKeyLabel keeps working.

Two further public-contract effects breaking already covers but are worth
naming explicitly:

  • Base class: the generated class derives from aenum.StrEnum (matching
    every other const/pcapng string registry) instead of
    pcapkit.utilities.compat.StrEnum. isinstance/issubclass against the
    compat class now returns False. repr, str, format, %-formatting,
    json.dumps, == 'ECH_SECRET', hash, isinstance(m, str), .value/.name
    are all unaffected — checked directly, not assumed.

  • New API surface: mixing in EnumRegistry adds get/get_all/register/
    register_alias(es), which the hand-written class never had. No lookup path
    mints — T(...), .get(...), .get_all(...), T[...] on an unknown label
    all still raise/miss exactly as before — only the explicit T.register(...)
    grows the member set, which is the documented EnumRegistry contract.

  • Searched for similar pull requests (fix(pcapng): repoint TLSKeyLabel at RFC 9850, add ECH_SECRET/ECH_CONFIG #883 is the related, already-merged one)

  • Followed the coding style (pylint/mypy/isort run against the changed files; no new findings)

  • make test passes, and a test case covers the change

  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md -- N/A -- changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

  • refactor — changes neither behaviour nor performance

Test plan

  • Ran the crawler twice through the real pcapkit.vendor.__main__ entry point; byte-identical (sha256 9466eef...58446d) both times
  • tests/protocols/misc/test_pcapng_unit.py — 94/94, extended with re-export identity + per-member value pins
  • tests/const/test_const_enum_builtin_parity.py, test_const_enum_lookup.py, test_const_enum_get.py, test_const_enum_no_mint.py, test_const_registry_protocol.py — all green with updated sweep counts
  • tests/vendor/test_vendor_missing_body_unit.py, test_vendor_dest_path_unit.py, tests/project/test_public_api.py, test_isort_clean.py — all green

@JarryShaw JarryShaw added refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix) enhancement Issues requesting a new capability (set by the feature request template) const Regenerated IANA or vendor constant tables; members keep their numeric values breaking Breaks public-facing behaviour or API (apply alongside the type label) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 28, 2026
@JarryShaw
JarryShaw force-pushed the worktree-agent-a0feac1569add5104 branch from 6144606 to 476fe92 Compare September 29, 2026 00:15
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO on 476fe923d — cross-review was opus (author sonnet). Its one blocking find is fixed, the CI failure is fixed, and I have re-run the affected files myself:

TREE /tmp/v890f/pcapkit/__init__.py
  test_const_registry_protocol.py     run=74 failures=0 errors=0
  test_const_enum_builtin_parity.py   run=33 failures=0 errors=0
  test_pcapng_unit.py                 run=94 failures=0 errors=0

The CI failure was one defect across five legs, not five: test_const_registry_protocol.py pins a population count over pcapkit/const and the new module made it stale — AssertionError: 122 != 121. A second sweep over the same population lives in that file, separate from the one in test_const_enum_builtin_parity.py that was already updated. pcapkit/const goes 139 → 140 .py files, so the +1 is exactly this change.

One judgement call in the fix, flagged rather than waved through. len(generated) deliberately stays 105, because the per-file loop at :790-796 asserts class \w+\(EnumRegistry, IntEnum\) — literally IntEnum — so a StrEnum file inside generated would fail that regex outright rather than just shift a number; and 105 is a historical census anchored at 05468a06b, which a later-added file was never part of. The arithmetic holds: 122 − 6 converted − 11 excluded = 105.

The cost is that tls_key_label.py now sits in EXCLUDED_STILL_BESPOKE, whose own comment describes modules that override the base's generic implementation, plus TransportProtocol for never mixing in EnumRegistry at all. This file fits neither — it mixes in EnumRegistry and overrides nothing. That is documented in a comment rather than hidden, and the alternative (widening the regex to admit StrEnum) would weaken the assertion for all 105 files, which is worse. @JarryShaw — if you would rather this were its own named frozenset, or the census anchored by sha instead of a live rglob, say so and I will have it split; I did not spin a third round for a naming preference.

The review's blocking find is also fixed: pcapkit/foundation/reassembly/data/data.py asserted TLSKeyLabel derives from compat.StrEnum, which this PR falsifies (issubclass(T, compat.StrEnum) goes True → False, since the generated file takes StrEnum from aenum as every other StrEnum const does). It now records the move explicitly and notes that json.dumps and string equality still hold while isinstance/issubclass against compat.StrEnum does not. The author's original report that the file "only names it in a docstring" was literally true and still missed it — a docstring can be a falsifiable claim about the thing you changed.

Two disclosure gaps closed in the PR body: the base-class move is a second public-contract effect beyond the import path (type identity flips; repr/str/format/%/json.dumps/==/hash/isinstance(m, str)/.value/.name all measured identical), and the class gains get/get_all/register/register_alias(es) from EnumRegistry where the hand-written one had none — additive, with only explicit register(...) able to mint. Nine lookup attacks left the member count at 11 throughout.

One latent crawler bug fixed unprompted: the citation builder matched \[RFC(\d+)\] with a '9850' fallback and appended #section-4.2 unconditionally, so a future row citing anything else would have silently got a false reference. It now anchors the match and raises on an unparseable reference. Output still byte-identical (sha256 9466eef…58446d) since all ten live rows are [RFC9850].

Determinism, RSA survival, uppercase values, WireGuardKeyLabel untouched, the registration pattern, the relocated autoclass, and all five parity counts were confirmed in the earlier review round and needed no rework. Unpublished and unmerged, yours to merge — it closes #886.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
Comment thread pcapkit/protocols/misc/pcapng.py Outdated
@JarryShaw

Copy link
Copy Markdown
Owner Author

added inline comment and synced from main branch

@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Sep 29, 2026
…registry

- Add pcapkit/vendor/pcapng/tls_key_label.py, a new crawler fetching the
  IANA "TLS SSLKEYLOGFILE Labels" CSV (RFC 9850 section 4.2) and generating
  pcapkit/const/pcapng/tls_key_label.py, matching the shape of its
  const/pcapng siblings (EnumRegistry + StrEnum, no _missing_, so lookup
  stays closed and raises exactly as it did before this move). The RFC
  citation is parsed from each row rather than assumed, and raises loudly
  on a reference it does not recognise instead of guessing one.
- RSA has no row in the registry (NSS-only, removed in NSS 3.34) and is
  hand-carried by the crawler with its #883 comment preserved verbatim;
  the other 8 "*_SECRET*" members keep their `# nosec B105` suppression.
- pcapkit/protocols/misc/pcapng.py now imports TLSKeyLabel as
  Enum_TLSKeyLabel, matching the alias every one of its seven
  pcapkit.const.pcapng siblings already carries in this file, and
  re-exports it under the original bare name (`TLSKeyLabel =
  Enum_TLSKeyLabel`) so `from pcapkit.protocols.misc.pcapng import
  TLSKeyLabel` keeps resolving to the identical object; the three other
  internal importers are left importing through that re-export, to avoid
  splitting their existing combined import with WireGuardKeyLabel, which
  stays put and hand-written (the alias convention is about imports of
  const enums, not local definitions). Updated the one docstring elsewhere
  that named TLSKeyLabel's old StrEnum base, which this move changes from
  compat.StrEnum to aenum.StrEnum.
- Register the new crawler/const module in the vendor/const __init__.py
  index files and docs, and pin the const-enum sweep tests' updated
  counts in both files that census pcapkit/const's population.
- Extend test_pcapng_unit.py to check the re-export identity, pin every
  member's value, and confirm unknown/lower-case labels still raise.

Build/test: ran the crawler twice (byte-identical sha256), plus
tests/protocols/misc/test_pcapng_unit.py (94/94) and the affected
tests/const + tests/vendor + tests/project sweeps, all green.
@JarryShaw
JarryShaw force-pushed the worktree-agent-a0feac1569add5104 branch from ce8931d to 7def2d1 Compare September 29, 2026 01:07
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO on 7def2d117 — cross-review (opus; author sonnet), third head, parent re-confirmed as origin/main (fde4cb20c).

The Enum_TLSKeyLabel alias is in at :38 with TLSKeyLabel = Enum_TLSKeyLabel at :294 preserving the old path, and I measured the identity myself: from pcapkit.protocols.misc.pcapng import TLSKeyLabel is <canonical> → True, both module attributes resolving to the same object, WireGuardKeyLabel.__module__ still local. Tests on that head: test_pcapng_unit.py 94/0/0, test_const_registry_protocol.py 74/0/0, test_public_api.py 10/0/0. The 122 != 121 CI failure is gone.

The rebinding turns out to earn its keep in a way I had not considered. Because pickle's find_class does a plain getattr on the module, a stored pickle naming the pre-#886 location still resolves — verified:

cpcapkit.protocols.misc.pcapng\nTLSKeyLabel\n(S'ECH_SECRET'\ntR.
  -> <TLSKeyLabel.ECH_SECRET: 'ECH_SECRET'>   is canonical member: True

A bare import … as Enum_TLSKeyLabel with no rebinding would have broken every such pickle silently. It is the only module-level CapWords-to-CapWords rebinding in pcapkit/, so it is the odd pattern — but the alternatives are a reimport or an isort-hostile double alias, and it carries a comment explaining itself. Also checked: __module__ stays canonical so the const sweeps count it once; the docs use explicit autoclass and the block was relocated, so Sphinx is unaffected; __all__ is still ['PCAPNG'], unchanged from when the class was hand-written.

On EXCLUDED_STILL_BESPOKE, I was half wrong and am dropping the objection. I said the file fits neither of the set's stated rationales. In fact the name was already a misnomer before this PR: of the eleven entries, five already mix in EnumRegistry and three of those are already StrEnum. So the set's operative meaning is already "outside this file's IntEnum-only census", which is exactly what tls_key_label.py is — adding it is consistent with what the set does, not a new overload. And the second-frozenset idea I floated is worse: it would sort this one file honestly while leaving the other five misfiled. No change wanted here.

One pre-existing defect found, not this PR's. tests/const/test_const_registry_protocol.py:1536-1539 says its case is "synthetic, since no shipped aenum.StrEnum registry mixes in the base yet". I censused it independently: 10 shipped StrEnum registries mix in EnumRegistry — ftp.command.Command, ftp.command.FEATCode, http.method.Method, pcapng.option_type.OptionType, reg.apptype.AppType and its four transport subclasses, plus this PR's TLSKeyLabel. Nine of the ten predate #890, so the sentence was already false, and #890 only adds the tenth. Filing that together with the set rename as their own issue rather than widening this PR.

Unpublished and unmerged, yours to merge — it closes #886.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
@JarryShaw
JarryShaw merged commit 5e7f91e into main Sep 29, 2026
63 checks passed
@JarryShaw
JarryShaw deleted the worktree-agent-a0feac1569add5104 branch September 29, 2026 01:29
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 29, 2026
JarryShaw added a commit that referenced this pull request Sep 29, 2026
…L_BESPOKE

- test_default_still_resolves_when_registered_str called its str-valued
  case "synthetic, since no shipped aenum.StrEnum registry mixes in the
  base yet". Measured on main (walking pcapkit.const, filtering for
  EnumRegistry+str subclasses): ten already do (Command, FEATCode,
  Method, OptionType, TLSKeyLabel, AppType and its four transport
  subclasses). Rewrote the docstring to say so instead of the stale claim.
- EXCLUDED_STILL_BESPOKE described its members as overriding the base's
  generic implementation, plus TransportProtocol for never mixing in the
  base at all. In fact most entries do mix in EnumRegistry -- the set's
  real, operative meaning is "outside this file's IntEnum-only historical
  census, anchored at commit 05468a0". Renamed to
  EXCLUDED_FROM_INTENUM_CENSUS and re-grouped its eleven entries by which
  of three reasons applies: not an IntEnum, overrides the base, or never
  mixes in the base (apptype.py carries two of the three, in one file).
  return_code.py/status_code.py's actual override is a custom __new__ plus
  _unregistered_member, not get()/register() -- their get/get_all/
  register/register_alias all resolve to the base's own implementation;
  ResponseKind and GroupingInformation, co-resident in return_code.py,
  override nothing at all. Updated the other four EXCLUDED_STILL_BESPOKE
  references (one behavioural, three prose) to the new name.
- No behaviour change: the set's file-membership, and the 6/11/105/122
  counts it feeds, are unchanged (122/11/105 following #890's merge,
  which added pcapkit/const/pcapng/tls_key_label.py to both).

Build/test: tests/const/test_const_registry_protocol.py (run=74,
failures=0, errors=0) and tests/const/test_const_enum_builtin_parity.py
(run=33, failures=0, errors=0), both via the worktree's own pcapkit.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
…L_BESPOKE

- test_default_still_resolves_when_registered_str called its str-valued
  case "synthetic, since no shipped aenum.StrEnum registry mixes in the
  base yet". Measured on main (walking pcapkit.const, filtering for
  EnumRegistry+str subclasses): ten already do (Command, FEATCode,
  Method, OptionType, TLSKeyLabel, AppType and its four transport
  subclasses). Rewrote the docstring to say so instead of the stale claim.
- EXCLUDED_STILL_BESPOKE described its members as overriding the base's
  generic implementation, plus TransportProtocol for never mixing in the
  base at all. In fact most entries do mix in EnumRegistry -- the set's
  real, operative meaning is "outside this file's IntEnum-only historical
  census, anchored at commit 05468a0". Renamed to
  EXCLUDED_FROM_INTENUM_CENSUS and re-grouped its eleven entries by which
  of three reasons applies: not an IntEnum, overrides the base, or never
  mixes in the base (apptype.py carries two of the three, in one file).
  return_code.py/status_code.py's actual override is a custom __new__ plus
  _unregistered_member, not get()/register() -- their get/get_all/
  register/register_alias all resolve to the base's own implementation;
  ResponseKind and GroupingInformation, co-resident in return_code.py,
  override nothing at all. Updated the other four EXCLUDED_STILL_BESPOKE
  references (one behavioural, three prose) to the new name.
- No behaviour change: the set's file-membership, and the 6/11/105/122
  counts it feeds, are unchanged (122/11/105 following #890's merge,
  which added pcapkit/const/pcapng/tls_key_label.py to both).

Build/test: tests/const/test_const_registry_protocol.py (run=74,
failures=0, errors=0) and tests/const/test_const_enum_builtin_parity.py
(run=33, failures=0, errors=0), both via the worktree's own pcapkit.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
…L_BESPOKE (#894)

- test_default_still_resolves_when_registered_str called its str-valued
  case "synthetic, since no shipped aenum.StrEnum registry mixes in the
  base yet". Measured on main (walking pcapkit.const, filtering for
  EnumRegistry+str subclasses): ten already do (Command, FEATCode,
  Method, OptionType, TLSKeyLabel, AppType and its four transport
  subclasses). Rewrote the docstring to say so instead of the stale claim.
- EXCLUDED_STILL_BESPOKE described its members as overriding the base's
  generic implementation, plus TransportProtocol for never mixing in the
  base at all. In fact most entries do mix in EnumRegistry -- the set's
  real, operative meaning is "outside this file's IntEnum-only historical
  census, anchored at commit 05468a0". Renamed to
  EXCLUDED_FROM_INTENUM_CENSUS and re-grouped its eleven entries by which
  of three reasons applies: not an IntEnum, overrides the base, or never
  mixes in the base (apptype.py carries two of the three, in one file).
  return_code.py/status_code.py's actual override is a custom __new__ plus
  _unregistered_member, not get()/register() -- their get/get_all/
  register/register_alias all resolve to the base's own implementation;
  ResponseKind and GroupingInformation, co-resident in return_code.py,
  override nothing at all. Updated the other four EXCLUDED_STILL_BESPOKE
  references (one behavioural, three prose) to the new name.
- No behaviour change: the set's file-membership, and the 6/11/105/122
  counts it feeds, are unchanged (122/11/105 following #890's merge,
  which added pcapkit/const/pcapng/tls_key_label.py to both).

Build/test: tests/const/test_const_registry_protocol.py (run=74,
failures=0, errors=0) and tests/const/test_const_enum_builtin_parity.py
(run=33, failures=0, errors=0), both via the worktree's own pcapkit.
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 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) const Regenerated IANA or vendor constant tables; members keep their numeric values enhancement Issues requesting a new capability (set by the feature request template) refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

refactor(vendor,pcapng): generate TLSKeyLabel from RFC 9850's IANA registry instead of maintaining it by hand

1 participant