feat(discovery): VM discovery — network sweep, LDAP, and SSH package inventory - #118
Merged
Conversation
Adds pkg/proxy/discovery implementing the discovery_inventory action: collects OS and package inventory from VMs over SSH with nothing installed on the targets. Collection commands ship as versioned, Ed25519-signed content packs rather than living in the binary, so adding a distro or fixing a command is a pack release instead of a fleet-wide agent upgrade. The agent verifies the signature, evaluates each collector's when-guard against facts probed from the host, runs the commands verbatim, and returns raw output — parsing stays server-side. One host failing never fails the batch: per-target statuses (ssh-refused, ssh-auth-failed, timeout) are the coverage data phase 0 exists to produce. Exports signing.ParsePublicKey so packs verify against the same trust root as signed actions. Refs #113
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new SSH-based discovery proxy package to collect OS and package inventory from VMs. The implementation uses signed, versioned content packs to evaluate guards and run commands without agent updates. The review feedback highlights several critical improvements: resolving a potential deadlock in SSH session pipes by reading stdout and stderr concurrently, fixing a data race in target allowance checks by properly locking configuration fields, ensuring robustness against platform-specific line endings during signature verification, and using the standard errors.As function for idiomatic error handling.
- Drain stdout and stderr concurrently. Reading them in sequence deadlocked once a collector's stderr exhausted the SSH channel window before any stdout was written; the regression test fails against the old code after a 3s stall. - Snapshot the CIDR allowlist under a read lock in isTargetAllowed. Configure could replace it concurrently. - Add known_hosts_file so customers who can supply host keys get real SSH host key verification; unreadable file now fails configuration instead of silently accepting any key. - Normalize line endings before computing signed pack bytes: a CRLF checkout otherwise fails verification indistinguishably from tampering. - Use errors.As for ssh.ExitError so wrapped errors are handled. Refs #113
Adds a test covering every path into command execution — missing pack key, unsigned pack, pack signed by another key, and an uncached version — so a future refactor cannot quietly allow an unverified command through. Documents at the exec call why static analysis flags it: signature verification is the sanitizer, and CodeQL cannot model it. Refs #113
A request now selects a content pack by version; it can no longer carry the pack body. Pack bytes come only from pack_dir, already verified against the configured Ed25519 key. The inline parameter was a demo convenience, not the production model (the ticket specifies a pack ref, and P6 distributes packs to disk). Removing it keeps executable content off the request path entirely rather than relying solely on the signature check to gate it, and resolves the CodeQL command-injection finding honestly instead of by dismissal. Also refuses a pack whose declared version differs from the requested one — mislabelled results would corrupt server-side correlation. Adds a test that output exceeding max_output_bytes returns truncated rather than stalling, covering the second half of the pipe-draining review comment. Refs #113
Contributor
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new SSH-based discovery proxy module to collect OS and package inventory from VMs concurrently using signed, versioned content packs. Feedback on the implementation highlights a potential performance bottleneck due to sequential DNS lookups during target validation, a bug where plain IP addresses without CIDR masks in the allowed CIDRs list are parsed incorrectly, and fragility in stripping the signature line from YAML documents.
Implements P3 (#114): the forager can now find hosts it was not told about, not only inventory a supplied list. discovery_sweep probes CIDRs with plain TCP connects. Safety is part of the contract rather than tuning: only well-formed connections (no crafted packets that destabilize embedded and OT gear), a rate cap the forager enforces regardless of what the server requests, exclusions applied during address expansion so an excluded host is never probed, and requested CIDRs refused unless contained by the datasource's allowed_cidrs — one bad request must not turn a segment collector into a general-purpose scanner. MAC addresses come from reading the kernel neighbour cache after probing rather than sending ARP frames, so the forager needs no raw sockets or CAP_NET_RAW. A discovery node requiring elevated privileges is far harder to get deployed. discovery_ldap lists AD computer objects, skipping tombstones older than active_within_days — importing machines decommissioned years ago would report permanent phantom gaps in the coverage report. A machine that never logged on is kept: that is a new host, not a stale one. objectGUID is decoded with AD's mixed-endian byte order since it is a STRONG merge identifier, and bind failures are redacted because they echo the bind DN back. Refs #114, #113
mayankpande88
changed the title
Caught by CI, not locally: arp_linux.go sits behind a linux build tag, so lint on darwin never compiled it. Verified with GOOS=linux.
Contributor
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new discovery proxy module designed to find VMs in a network segment and collect their OS and package inventory over SSH, as well as query computer objects from Active Directory. Key feedback focuses on optimizing performance by replacing fmt.Sscanf with strconv.ParseInt in ldap.go, handling context cancellation for LDAP operations, and addressing potential performance bottlenecks. Specifically, it is recommended to resolve target hostnames and reverse DNS lookups concurrently with bounded concurrency to prevent blocking or overwhelming DNS resolvers. Additionally, the sweep worker pool should scale with the requested probe rate to avoid capping scan speeds on dead hosts, and a safety check should be added to prevent shift overflow when expanding small CIDR blocks.
… checks Three findings from review: Signature-line stripping now requires exactly one top-level line and matches only at column 0 (tolerating space before the colon). Because verification removes these lines before checking the signature, every line the stripper removes is a line an attacker could add or alter without invalidating it. A second one was already blocked downstream — yaml.v3 rejects the duplicate key — but that is a property of the parser, not a guarantee this package makes, and the signature check must not rest on it. Both injection variants are now regression tests. A bare IP in allowed_cidrs became a hostname entry, so a target given by name that resolved to it never matched: name resolution is only compared against the network list. Bare addresses now become /32 or /128 networks, matching what the sweep parser already did. Scope checks run concurrently. Each hostname target costs a DNS lookup, so a batch of named hosts previously blocked the handler for the sum of every lookup before a single host was contacted. Order is preserved so results stay correlatable. Refs #113, #114
…able workers More review findings: go-ldap has no context-aware operations, so a cancelled action kept a directory query alive for up to timeout_seconds after nobody was waiting. Cancellation now closes the connection, failing an in-flight bind or search immediately. Address-space size was computed as int, so 1<<32 for a /0 overflows on 32-bit platforms: the scope would silently expand to nothing rather than be rejected. Computed as int64 now, with a test. Reverse DNS lookups were unbounded — a sweep finding many hosts fired that many simultaneous queries at the customer's resolver, which looks like a DNS flood and is liable to be treated as one. Bounded to 32. Sweep workers are configurable (default 64, max 512), which helps when a scope is mostly dead addresses and each probe costs a full timeout. A test asserts raising workers cannot raise the probe rate: the limiter owns rate, and the cap is a safety property no request parameter may erode. strconv.ParseInt replaces fmt.Sscanf for the AD timestamp and userAccountControl parsing. Refs #113, #114
RamanKharchee
approved these changes
Aug 3, 2026
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds
pkg/proxy/discovery: the forager can now find VMs on a network segment and collect their OS and package inventory over SSH, with nothing installed on the target hosts — their agent is the sshd and package manager the OS already ships.Three actions, deliberately separate:
discovery_sweepdiscovery_ldapdiscovery_inventorySweeps find, they do not inventory — package data always requires credentials, and that split is why these are separate actions rather than one. Nothing here decides when to run: the server schedules everything and the forager holds no state between actions.
Collection commands do not live in the binary. They ship as a versioned, Ed25519-signed content pack. A request selects a pack by version; the forager reads it from
pack_dir, verifies the signature, evaluates each collector'swhenguard against facts probed from the host, and runs the surviving commands verbatim. Adding a distro or fixing a command is a pack release, not a fleet-wide agent upgrade — the constraint that shaped this design. Output comes back raw; parsing is server-side (nudgebee/nudgebee-enterprise#35405), so a parser fix never touches hosts.Part of the VM discovery epic (nudgebee/nudgebee-enterprise#35404).
Fixes #113
Fixes #114
sweep.goarp_linux.go/arp_other.goldap.gopack.gowhen.gofacts.goos_family/os_id/os_major/archexecutor.goproxy.goDecisions worth reviewing
Sweep safety is contract, not tuning. Only well-formed TCP connects — no crafted packets, because the malformed probes port scanners emit are what destabilize embedded and OT gear, and a discovery tool that knocks over a PLC has failed regardless of what it found. The rate cap is enforced in the forager: the server asks for a rate, the forager still refuses to exceed its own. Exclusions are applied during address expansion so an excluded host is never handed to a prober. Scope is enforced twice — a requested CIDR outside
allowed_cidrsis refused, so one bad or malicious request cannot turn a segment collector into a general-purpose scanner.No raw sockets. MACs come from reading the kernel's neighbour cache after probing rather than sending ARP frames, so the forager needs no
CAP_NET_RAW. A discovery node that requires elevated privileges is a much harder thing to get deployed into a customer's network. The honest cost: a host with none of the probed ports open is invisible to a sweep — which is exactly why the directory and hypervisor sources exist.LDAP staleness filtering is correctness, not optimization. AD accumulates tombstones of machines decommissioned years ago; importing them would report permanent phantom gaps in the coverage report. A machine that has never logged on is kept — that is a new host, not a stale one, and the two are easy to conflate.
Requests select a pack; they cannot carry one. Pack bodies reach
pack_dirthrough the distribution pipeline, never through an action, so no part of an action payload is ever executed.The guard language is intentionally tiny —
==/!=against a quoted literal over four known facts. It should not grow: anything richer is an expression evaluator inside a binary that runs signed content against production hosts.Failures are data, not errors. One unreachable host never fails a batch. Per-target statuses (
ssh-refused,ssh-auth-failed,timeout) are exactly the coverage information Phase 0 exists to produce.Known gaps
known_hosts_fileis set. Discovery finds hosts nobody has catalogued, so their keys are unknown on first contact and change when a VM is re-imaged; strict-by-default would break the module's purpose. Residual risk is bounded — read-only collection, unprivileged credential, signature-verified commands. Server-side key pinning removes the tradeoff and belongs with that work.Type of change
How Has This Been Tested?
make validatepasses: lint 0 issues,-raceclean, package at ~78% coverage.Sweep and LDAP:
objectGUIDdecoded with AD's mixed-endian byte order (wrong order would split one machine into two assets)Inventory (from the earlier rounds):
TestHandleInventory_NoUnverifiedCommandPathpins the core security invariant across all six ways a pack can fail to verify, including that an inline body is refusedReview rounds
Three real defects found in review, each fixed with a regression test:
io.Copy(io.Discard, ...)past the cap so an over-producing command can still exit. Verified the test discriminates: it fails against the old code after a 3s stall. (A first attempt using 256KB of stderr passed against the buggy code — the ~2MB channel window absorbed it. It only reproduces above the window.)isTargetAllowedread the CIDR allowlist without the lockConfigurewrites under. Now snapshotted under a short read lock, kept off the slow DNS path.\nfailed verification after a CRLF checkout, indistinguishably from tampering. Line endings are normalized before computing signed bytes; a test confirms tamper detection still works on CRLF input.The CodeQL command-injection finding was resolved by a design change (packs selected by version, never carried in a request) rather than by dismissing the alert. The insecure-host-key finding was resolved by adding
known_hosts_file.Checklist
make validatepasses (fmt + lint + test)