Skip to content

fix(pcapng): warn on a malformed TLS key-log line instead of aborting (#1985) - #1989

Merged
JarryShaw merged 3 commits into
mainfrom
fix/1985-tls-keylog-malformed-line
Oct 20, 2026
Merged

fix(pcapng): warn on a malformed TLS key-log line instead of aborting (#1985)#1989
JarryShaw merged 3 commits into
mainfrom
fix/1985-tls-keylog-malformed-line

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 20, 2026

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — a bug fix

Description of your pull request and other information

A malformed line in a Decryption Secrets Block's key log no longer aborts extract. TLSKeyLog.post_process and WireGuardKeyLog.post_process now handle a line that does not parse, wherever it sits, the same way:

  • It raises a ProtocolWarning: "invalid TLS key log format: '…'; line kept as captured".
  • It is left out of entries, and the valid entries and every frame are kept.
  • The block rebuilds byte-exact, because the data model already keeps the log text verbatim.

A line cut by the capture is still dropped silently, as in #1983. A cut last line that still fits inside the pad now gets this warning instead of raising.

Three existing tests that asserted the old FieldValueError now assert the warning. The new test_pcapng_tls_keylog_malformed_1985_unit.py fails on main (19 failed) and passes here.

Not covered: a WireGuard secret with invalid base64 still parses silently, because b64decode drops the bad characters.

Closes #1985

@JarryShaw
fix(pcapng): warn on a malformed TLS key-log line instead of aborting (…
cc0a0f3 
…#1985)

A TLS or WireGuard key log line that did not parse raised FieldValueError
from post_process, which aborted the whole extraction, wherever the line
sat in the log. It now raises a ProtocolWarning ("...; line kept as
captured"), is left out of the entries, and the well-formed entries around
it are kept. The log text is already kept verbatim on the data model, so
the block still rebuilds octet for octet. A last line the capture or the
block cut is still left out silently, as #1983 does.

Tests that pinned the old refusal now pin the warning.

Closes #1985
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 20, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 20, 2026
@JarryShaw

JarryShaw commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at cc0a0f38d. A Sonnet review found the rebuild byte-exact across TLS and WireGuard DSBs: malformed first, middle and last lines, CRLF line endings, an empty line, non-ASCII. It found exactly one warning per malformed line, and no caller in pcapkit/ relies on the old raise. Two things to fix:

  • CLIENT_RANDOM zz yy leaves an empty entries['CLIENT_RANDOM'] bucket, because entries[label] is touched before bytes.fromhex fails.
  • The test docstrings and names use timed wording ("no longer aborts", "now raises").

Question for you: the warning quotes the whole malformed line, so it can include a TLS secret or a WireGuard private key. main's old error quoted it the same way. Should the warning name only the label, with the values elided, or keep the full line?

@JarryShaw JarryShaw added needs: decision Waiting on the maintainer to decide — not blocked by other work review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 20, 2026
@JarryShaw JarryShaw moved this from In review to Needs decision in PyPCAPKit Oct 20, 2026
@JarryShaw
fix(pcapng): keep a malformed key-log line out of entries entirely (#…
13d72b9 
…1985)

TLSKeyLog.post_process indexed entries[label] before bytes.fromhex ran, so a line with a known label but fields that do not decode left an empty bucket for that label. Both values are decoded first and entries is touched only once they are. WireGuardKeyLog decodes before it adds, so it needs no change.

Also drops timed wording from the test docstrings and rewraps the post_process docstring.

Refs #1985
@JarryShaw

JarryShaw commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

Pushed 13d72b971. TLSKeyLog.post_process now decodes both fields before it touches entries, so a malformed line leaves no bucket behind (new test). The timed wording is removed. The warning text is unchanged until you decide. The delta review runs once that's settled.

@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 20, 2026
@JarryShaw

JarryShaw commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

the warning name only the label, with the values elided

@JarryShaw

JarryShaw commented Oct 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner Author

The code at 13d72b971 reviewed GOOD TO GO: a Sonnet delta review confirmed valid entries are collected exactly as before, and the no-empty-bucket test fails on cc0a0f38d. Per your 17:45Z ruling, the warning will name only the label, with the values elided. That needs one more commit and a re-review.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet needs: decision Waiting on the maintainer to decide — not blocked by other work review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 20, 2026
@JarryShaw JarryShaw moved this from Needs decision to In review in PyPCAPKit Oct 20, 2026
@JarryShaw
fix(pcapng): elide key-log values from the malformed-line warning (#1985
1476720 
)

The malformed-line warning quoted the whole key-log line, which carries key material. _malformed_line_message names only the label, when the key log knows it, and elides the values. An unrecognised label is not quoted either, since it may be key material too. The WireGuard parser's internal ValueError no longer carries the line.

Tests assert the new text and that no warning quotes a hex secret or base64 key from their input.

Refs #1985
@JarryShaw

JarryShaw commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

Pushed 1476720ea, which applies your ruling.

  • Known label: the warning reads invalid TLS key log line for label 'CLIENT_RANDOM'; values elided, line kept as captured. WireGuard is the same, using the key type.
  • Unrecognised label: … with an unrecognised label; line elided, kept as captured. The label isn't quoted, since it could itself be key material.
  • Elsewhere: nothing else on the key-log path quotes a line or a secret.

The tests assert the exact text, and that no warning contains the secrets from their inputs. A delta review is running.

@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 20, 2026
@JarryShaw

JarryShaw commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 1476720ea. A Sonnet delta review ran malformed TLS and WireGuard lines carrying a marker secret through extract, and the secret appears in no warning or exception. The quoted label is always a canonical enum name: hostile labels give the unrecognised form. Putting {line!r} back fails 27 tests, and the rebuilds stay byte-exact.

@JarryShaw JarryShaw removed the review: running A cross-review is in flight against the current head - no verdict yet label Oct 20, 2026
@JarryShaw JarryShaw added the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 20, 2026
@github-actions

github-actions Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Coverage: 91.53% (unit tier, Python 3.15, 1476720ea, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19597 881 2618 867 92.11%
pcapkit/corekit 2540 71 808 37 96.54%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2918 23 1016 25 98.78%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17459 223 4598 188 98.12%
pcapkit/toolkit 872 36 298 10 95.21%
pcapkit/utilities 522 2 158 4 99.12%
pcapkit/vendor 4431 1995 1018 167 51.73%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 6272730 into main Oct 20, 2026
59 checks passed
@JarryShaw
JarryShaw deleted the fix/1985-tls-keylog-malformed-line branch October 20, 2026 21:34
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 20, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 20, 2026
@JarryShaw JarryShaw mentioned this pull request Oct 20, 2026
Closed
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.5

Development

Successfully merging this pull request may close these issues.

fix(pcapng): a malformed TLS key-log line in a Decryption Secrets Block aborts extraction

1 participant

@JarryShaw