fix(pcap): don't widen explicit host_spec with the interface subnet (Refs #758) by leoparente · Pull Request #790 · netboxlabs/pktvisor

leoparente · 2026-06-26T17:05:38Z

Refs #758. (Does not auto-close — see discussion there.)

Problem

On a libpcap live-capture input with an explicit host_spec (e.g. a VIP /32), pktvisord also appends the capture interface's address at its netmask prefix (e.g. 192.168.50.23/24) to the "host" set used for direction classification. The user's host_spec is silently widened to the whole subnet.

The result is DNS direction misclassification. With host_spec: 192.168.50.2/32, the query client → VIP classifies correctly (dst=VIP wins on the /32 → toHost), but the response VIP → client has dst=client, which matches the interface /24 → toHost instead of fromHost. Depending on the handler this either breaks transaction pairing (v2: query in the in map, response looked up in the out map → never pairs → dns_xact_in stays 0) or mis-increments (v1: pairs, but counts xacts_out instead of xacts_in). Either way server-side DNS is reported as client-side, the symptom in #758.

The af_packet source never does interface host-detection (hence pcap_source: af_packet is the known workaround), and pcap_file never auto-detects — so the defect is specific to live libpcap capture.

Fix

When parse_host_spec() has produced a non-empty host set, the explicit host_spec is authoritative and interface auto-detection is skipped (libpcap now matches af_packet). When no usable host_spec is given, the prior behavior is preserved exactly: the interface address is auto-added at its on-link/subnet prefix (the intentional inside/outside distinction).

The gate is !_hostIPv4.empty() || !_hostIPv6.empty() — the lists parse_host_spec() filled — and deliberately not config_exists("host_spec"): the CLI's default tap always emits host_spec (the empty string when -H is omitted), so config_exists would wrongly skip auto-detection for a plain pktvisord <iface> and classify every packet as unknown.

The decision + entry construction were extracted into a pure, unit-testable helper lib::utils::append_interface_host_subnets(...), so the new logic is covered without binding a live NIC. PcapInputStream::_get_hosts_from_libpcap_iface() keeps the OS getifaddrs gather and delegates to it.

libs/visor_utils/{utils.h,utils.cpp,test_utils.cpp} — the helper + a unit test covering both gate branches and multi-address ordering.
src/inputs/pcap/PcapInputStream.cpp — rewire the libpcap host detection through the helper.
cmd/pktvisord/main.cpp, cmd/pktvisor-reader/main.cpp — -H help text now documents that, for live capture, it defines the host set explicitly and disables automatic detection (was "append to any automatic detection").

Behavior / compatibility

No change for inputs that don't set a usable host_spec — including a plain pktvisord <iface> (empty host_spec → auto-detect, unchanged).
Inputs with a non-empty host_spec on a libpcap interface now get exactly the host set they configured; this shifts all direction-derived metrics for them (net in/out, flow, dns_xact_in/out) — they were previously misclassified.
-H semantics change (intentional, per pcap host_spec is effectively widened by interface netmask, causing DNS xact direction misclassification #758): a non-empty -H is now authoritative (replace), not append. Help text updated in both binaries.
A host_spec in either family disables interface auto-detection for both families; on a dual-stack interface list every family you care about (the other family's traffic would otherwise be direction unknown, and is dropped if the v2 DNS handler's only_xact_directions filter excludes unknown).

Testing

New unit-tests-visor-utils case appendInterfaceHostSubnets (skip-when-provided / append-when-not / multiple same-family addresses): pass (19 assertions; full util suite 45).
unit-tests-input-pcap: pass, no regression.
The live getifaddrs path is not CI-reachable (no real NIC in CI); the pure helper carries the automated coverage. Manually verifiable on a live host: host_spec: <VIP>/32 → dns_xact_in_total increments for server-side DNS; no -H → auto-detect unchanged.

Reviews

Built spec → plan → 5 rounds of adversarial review (which caught a critical gate bug: an earlier draft gated on config_exists("host_spec"), which would have broken the dominant pktvisord <iface> path) → subagent-driven implementation with per-task + whole-branch review → a final adversarial review over the commits (build + tests run). All clean.

🤖 Generated with Claude Code

… detection (Refs #758)

…Refs #758)

… (Refs #758)

Copilot

Pull request overview

This PR fixes direction misclassification for live libpcap captures when users provide an explicit host_spec, by preventing pktvisord from silently widening the configured host set with the capture interface’s on-link subnet (Refs #758).

Changes:

Skip libpcap interface host auto-detection when parse_host_spec() already produced a non-empty host set.
Extract interface-host append logic into a unit-testable helper (lib::utils::append_interface_host_subnets) and add unit coverage for both gate branches and ordering.
Update -H help text in both binaries to document the new “authoritative host set” semantics for live capture.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/inputs/pcap/PcapInputStream.cpp	Collect iface addresses then delegate host-subnet appending to the new helper, gated by whether `host_spec` populated host lists.
libs/visor_utils/utils.h	Exposes the new `append_interface_host_subnets(...)` helper API.
libs/visor_utils/utils.cpp	Implements the helper that appends interface host subnets only when no host set was provided.
libs/visor_utils/test_utils.cpp	Adds unit tests covering skip/append behavior and multi-address ordering.
cmd/pktvisord/main.cpp	Updates `-H` help text to reflect authoritative semantics for live capture.
cmd/pktvisor-reader/main.cpp	Updates `-H` help text to reflect authoritative semantics for live capture.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

github-actions · 2026-06-26T17:15:11Z

LCOV of commit `e9be5cb` during Debug Builds #184

  lines......: 82.5% (14321 of 17365 lines)
  functions..: 73.1% (1451 of 1984 functions)
  branches...: no data found

Files changed coverage rate: n/a

Full coverage report

…net_ntop (Refs #758)

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

leoparente · 2026-06-26T18:18:10Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2028129693

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…hosts on restart (Refs #758)

leoparente · 2026-06-26T18:54:53Z

@codex review

chatgpt-codex-connector · 2026-06-26T18:58:31Z

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: df50edac73

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

…epth (Refs #758)

…s, move k8s example (#797) * chore(deps): bump cpp-httplib 0.27.0 -> 0.47.0, opt out of getaddrinfo_a cpp-httplib >= 0.28 defaults to CPPHTTPLIB_USE_NON_BLOCKING_GETADDRINFO and links libanl on Linux, which does not exist on musl and broke the static cross builds (the reason we were pinned to 0.27.0 since #784). The recipe now exposes use_non_blocking_getaddrinfo; set it to False so no anl is linked. httplib only backs the REST server and in-process test servers, so blocking getaddrinfo is fine. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(netprobe): cover literal-IPv6 auto-detect and ip_version:6 DNS round-trip Adds the two CI-deterministic cases from the ICMPv6 design spec that were never written: a literal "::1" target must be accepted without an ip_version hint and produce one probe, and a DNS target with ip_version: 6 must reach start() and echo the value via info_json. Both run through the tcp test type because a ping stream opens raw sockets and cannot start unprivileged in CI; the parse/route path under test is shared by ping and tcp. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs: fix stale -H/content_failures wording; move k8s example under centralized_collection - README: the -H usage text still said live-capture host_spec "will append to any automatic detection"; #790 made it replace auto-detection. Sync both usage blocks with the CLI help text. - netprobe README: content_failures now covers every v3 response assertion (body, JSON, size, header, version), not just expected_body(_regex). - Move k8s/ to centralized_collection/k8s/ next to the other collection examples, fix its apply/dashboard paths, and link it from centralized_collection/README.md and the top-level README (it was unreferenced before). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * chore(deps): bump catch2 3.16.0, openssl 3.6.4, libcurl 8.22.0 Newest ConanCenter versions of each. Not bumped on purpose: - openssl 4.0.x: every consumer recipe (libcurl incl. 8.22.0, libnghttp2, sentry-crashpad, cpp-httplib) still pins openssl/[<4]; moving needs a force override against the recipes' declared support. - protobuf 7.35.0: excluded by opentelemetry-cpp 1.26.0's protobuf/[<7]. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * chore(deps): drop comment on cpp-httplib option Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(build): stop declaring OPENTELEMETRY_PROTO_API as dllimport on Windows The Windows job has failed since the opentelemetry-cpp/1.26.0 ConanCenter recipe revision a671ccb (2026-08-18): it now forces OTELCPP_PROTO_LIB_TYPE to STATIC_LIBRARY and disables upstream's shared/static selection, so opentelemetry_proto.lib is a static library on Windows. Our CMake still defined the (non-propagated) export macro as __declspec(dllimport) there, a workaround from #784 for the old revision that built the proto library as a DLL, so every opentelemetry::proto symbol resolved to an __imp_ import that no library provides (1500+ LNK2019/LNK2001 across all targets). The first failure on develop is the #796 merge only because it was the first build after that recipe revision; last green build (07-22) used revision 9d81768. Define the macro as empty everywhere, matching what upstream's project_build_tools_set_static_library_declaration does for MSVC. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ci(windows): only install Release Conan dependencies Visual Studio is a multi-config generator, so the cmake-conan provider defaults to installing (and, on a cache miss, building) every dependency for both Release and Debug. The Windows jobs only ever build --config Release, so the Debug pass is wasted: on a cold cache it doubled the Configure step to over an hour. Pin the provider to the configured build type. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

leoparente added 3 commits June 26, 2026 13:52

feat(utils): add append_interface_host_subnets() helper for pcap host…

f164e42

… detection (Refs #758)

fix(pcap): don't widen explicit host_spec with the interface subnet (…

1c74d32

…Refs #758)

docs(cli): -H replaces interface host auto-detection for live capture…

67669d6

… (Refs #758)

leoparente requested a review from Copilot June 26, 2026 17:06

leoparente self-assigned this Jun 26, 2026

Copilot started reviewing on behalf of leoparente June 26, 2026 17:07

View session

Copilot AI reviewed Jun 26, 2026

View reviewed changes

Comment thread src/inputs/pcap/PcapInputStream.cpp Resolved

Comment thread libs/visor_utils/utils.cpp Resolved

Comment thread libs/visor_utils/utils.cpp Resolved

fix(pcap): early-return before getifaddrs when host_spec set; guard i…

2028129

…net_ntop (Refs #758)

leoparente requested a review from Copilot June 26, 2026 17:25

Copilot started reviewing on behalf of leoparente June 26, 2026 17:26

View session

Copilot AI reviewed Jun 26, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Jun 26, 2026

View reviewed changes

Comment thread src/inputs/pcap/PcapInputStream.cpp Resolved

fix(pcap): recompute host set on each parse_host_spec to avoid stale …

df50eda

…hosts on restart (Refs #758)

leoparente marked this pull request as ready for review June 26, 2026 18:59

leoparente requested review from Copilot, jajeffries and samiura June 26, 2026 18:59

Copilot started reviewing on behalf of leoparente June 26, 2026 19:00

View session

Copilot AI reviewed Jun 26, 2026

View reviewed changes

Comment thread src/inputs/pcap/PcapInputStream.cpp Resolved

fix(pcap): pass computed host_set_provided to helper for defense-in-d…

e9be5cb

…epth (Refs #758)

samiura approved these changes Jun 26, 2026

View reviewed changes

leoparente merged commit 7acee80 into develop Jun 26, 2026
23 checks passed

leoparente deleted the fix/pcap-host-spec-broadening branch June 26, 2026 19:40

leoparente mentioned this pull request Jun 26, 2026

pcap host_spec is effectively widened by interface netmask, causing DNS xact direction misclassification #758

Closed

leoparente mentioned this pull request Sep 22, 2026

chore: fix Windows build, cpp-httplib 0.47 + dep bumps, spec gap fixes, move k8s example #797

Merged

Navigation Menu

Repository navigation

fix(pcap): don't widen explicit host_spec with the interface subnet (Refs #758) - #790

fix(pcap): don't widen explicit host_spec with the interface subnet (Refs #758)#790
leoparente merged 6 commits into
developfrom
fix/pcap-host-spec-broadening

leoparente commented Jun 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

leoparente commented Jun 26, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

leoparente commented Jun 26, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jun 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

leoparente commented Jun 26, 2026

Problem

Fix

Behavior / compatibility

Testing

Reviews

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LCOV of commit e9be5cb during Debug Builds #184

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

leoparente commented Jun 26, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

leoparente commented Jun 26, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jun 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jun 26, 2026 •

edited

Loading

LCOV of commit `e9be5cb` during Debug Builds #184