Skip to content

fix(pcapng): repoint TLSKeyLabel at RFC 9850, add ECH_SECRET/ECH_CONFIG - #883

Merged
JarryShaw merged 1 commit into
mainfrom
fix-882-tlskeylabel-rfc9850
Sep 28, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix-882-tlskeylabel-rfc9850

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Description of your pull request and other information

TLSKeyLabel tracked the Mozilla NSS key-log wiki, but draft-ietf-opsawg-pcapng-06 S4.7 now
points at [I-D.ietf-tls-keylogfile] (draft-05), published as RFC 9850, "The SSLKEYLOGFILE Format for TLS", whose S4.2 creates the IANA "TLS
SSLKEYLOGFILE Labels" registry.

Against that registry we were missing ECH_SECRET and ECH_CONFIG (RFC 9850 SS2.3/4.2), so
TLSKeyLabel('ECH_SECRET') raised ValueError on an otherwise-legitimate Decryption Secrets
Block. Both are added with a #: comment transcribed from the registry table; ECH_SECRET keeps
the file's # nosec B105 bandit convention for *_SECRET members, ECH_CONFIG doesn't need it.

RSA has no row in RFC 9850's registry -- it's an NSS-only label the NSS wiki records as removed
in NSS 3.34. Kept rather than deleted (removing a public member is a breaking change) and
documented as the historical exception.

Also repoints secrets_type.py's stale # NSS Key Log Format comment at RFC 9850, and cites RFC
9850 in TLSKeyLabel's docstring. Added a test pinning the member set against RFC 9850 plus the
documented RSA exception, and asserting ECH_SECRET/ECH_CONFIG/RSA all resolve.

Fixes #882

- pcapkit/protocols/misc/pcapng.py: TLSKeyLabel drifted from the Mozilla NSS
  key-log wiki that draft-ietf-opsawg-pcapng-06 S4.7 used to cite; it now
  points at RFC 9850's S4.2 "TLS SSLKEYLOGFILE Labels" registry. Add the two
  labels that registry defines and we were missing, ECH_SECRET and
  ECH_CONFIG, each with a #: comment transcribed from the registry table.
  ECH_SECRET keeps the file's # nosec B105 convention (bandit flags the
  SECRET substring); ECH_CONFIG does not need it. RSA has no row in that
  registry -- it is an NSS-only label removed in NSS 3.34 -- so it is kept,
  not deleted, and documented as the historical exception. The class
  docstring now cites RFC 9850.
- pcapkit/vendor/pcapng/secrets_type.py: repoint the "NSS Key Log Format"
  comment at RFC 9850, matching the new authority.
- tests/protocols/misc/test_pcapng_unit.py: pin TLSKeyLabel's member set
  against RFC 9850's registry plus the documented RSA exception, and assert
  ECH_SECRET, ECH_CONFIG and RSA all resolve.

Verified: new test fails on the pre-fix pcapng.py (missing ECH_SECRET and
ECH_CONFIG) and passes after; full test_pcapng_unit.py (94 tests) is green.

Fixes #882
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 28, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO on 3b024c00c — cross-review (opus; author sonnet). No changes required.

It went past the RFC and checked the live registry, which is the right instinct given RFC 9850's registry is Specification Required and could have moved since July 2026. Fetched https://www.iana.org/assignments/tls-parameters/tls-sslkeylogfile-labels.csv today and diffed it against the enum imported from the PR tree:

IANA registry size: 10          enum size: 11
registry - enum (MISSING from pcapkit): []
enum - registry (EXTRA in pcapkit): ['RSA']

I re-ran that fetch myself: 10 rows, the same 10 labels, and grep -c RSA over RFC 9850 returns 0 — RSA appears nowhere in the document, so the spurious-member framing holds and no registered label is missed.

Other findings, each with its own evidence:

  • Fails without the fix. Replaced only pcapng.py with git show origin/main: — AssertionError: Items in the second set but not the first: 'ECH_SECRET' 'ECH_CONFIG', run=1 failures=1. A real failure on the defect, not on an import.
  • # nosec B105 is right in both directions, measured with bandit 1.9.4 rather than reasoned from the substring: the PR file is clean with 8 skipped (7 pre-existing + ECH_SECRET), and stripping the marker from ECH_SECRET alone produces B105 hardcoded_password_string … pcapng.py:290. So it is load-bearing there and genuinely unnecessary on ECH_CONFIG.
  • Descriptions byte-exact against the IANA CSV's unwrapped Description column, including the ECHConfig casing.
  • No consumer can break. 22 TLSKeyLabel hits, all annotations, fixtures or prose; no len(), no exhaustive dispatch. The docs directive is autoclass with :undoc-members:, so the .rst needs no edit.
  • 94 tests OK (skipped=1) against the PR tree, with pcapkit.__file__ printed.

And it caught an imprecision in my own issue text, which I have verified and fixed in the PR description. Draft-06 §4.7 does not literally name RFC 9850 — line 2380 reads "This format is described in [I-D.ietf-tls-keylogfile]", resolved at line 3206 as draft-ietf-tls-keylogfile-05, which was then published as RFC 9850. The substance is unaffected (that draft already registers both labels, and the draft contains zero NSS or Mozilla references, so the old pointer really is gone), but the citation chain is transitive and the description now says so.

Not verified, stated rather than glossed: make pylint / make mypy / make isort were not run, and no lint leg exists in this PR's checks — CI's "Analyze" is CodeQL. The diff is four comment lines, two enum members whose values equal their names, and one test method; the one linter with anything to say here was bandit, which was run.

CI: 55 green, 0 failed, 3 still in flight. Unpublished and unmerged, yours to merge.

@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 28, 2026
@JarryShaw
JarryShaw merged commit a245c06 into main Sep 28, 2026
63 checks passed
@JarryShaw
JarryShaw deleted the fix-882-tlskeylabel-rfc9850 branch September 28, 2026 21:48
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 28, 2026
JarryShaw added a commit that referenced this pull request Sep 28, 2026
util/changelog_md.py regenerated from 1.5.0.rst; --check exit 0.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
util/changelog_md.py regenerated from 1.5.0.rst; --check exit 0.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
util/changelog_md.py regenerated from 1.5.0.rst; --check exit 0.
JarryShaw added a commit that referenced this pull request 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 from the new
  canonical module instead of defining it, and re-exports it under the
  same name so `from pcapkit.protocols.misc.pcapng import TLSKeyLabel`
  keeps working; the three other internal importers are left importing
  through that re-export, to avoid splitting their existing combined
  import with WireGuardKeyLabel, which stays put. 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 added a commit that referenced this pull request 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 added a commit that referenced this pull request Sep 29, 2026
…registry (#890)

- 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 added a commit that referenced this pull request Sep 29, 2026
The paragraph claimed the defect programme ran "across some 140 issues and
pull requests between #326 and #509" and reached "#805 by the entries below".
Both are wrong, and re-measuring on this revision gives:

* 254 distinct issue/PR references in the entries, not ~140.
* 87 of them fall in [#326, #509]; 164 are above #509 and 3 below #326.
* The highest is #883, not #805.

The range framing was the root cause -- it encoded the window the programme
opened on as though it were its extent, so every merge since made it more
wrong. Replaced with a snapshot that says it is one, and regenerated
CHANGELOG.md so the two stay in step (`util/changelog_md.py --check` exits 0).
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
@JarryShaw JarryShaw moved this to Done in PyPCAPKit Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(pcapng): TLSKeyLabel has drifted from RFC 9850's IANA registry -- RSA spurious, ECH_SECRET and ECH_CONFIG missing

1 participant