feat(hostmonitoring): finish host profile, SBOM, and malware detection (#976)
* feat(hostmonitoring): finish host profile, SBOM, and malware detection Host monitoring previously ran rules/alerts against the host pseudo-container but application-profile generation silently stalled, SBOM generation was an explicit no-op, and malware detection was excluded outright. - Add pkg/hostidentity: a shared host-identity builder (node-name based, with a machine-id fallback) that synthesizes K8s-shaped shared container data for the host, mirroring the pattern already proven in armosec/private-node-agent. - Unblock application-profile generation by injecting that synthetic data at a single point in containercallback.go, with explicit guards added at every K8s-dependent consumer (profilehelper, creator, containerprofilecache) so a synthetic entry is never mistaken for a real K8s lookup result. Also fixes a premature-finalization bug where the host profile would have been deleted after MaxSniffingTime, and a ContainerInfos nil-map crash on host's first profile save. - Add SBOM generation for host as a wholly separate scan branch (host has no image/mounts to reuse the container path): a new Syft directory-source scanner against the host root filesystem with container-runtime-storage exclusions, a new HostSBOMRescanInterval config for periodic re-scans, and a bounded scan timeout. Vulnerability scanning itself stays external to kubevuln, unchanged from how containers already work. - Add malware detection for host by removing its exclusion and IgnoreContainer checks; detection logic itself needed no changes. See docs/features/host-monitoring-completion.md for full design rationale, including two documented behavior consequences: a narrower ProfileMetadata alert payload for real containers with no cached profile, and host alerts permanently carrying FailOnProfile=false (profile never reaches Completed by design, since it must keep learning indefinitely). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(rulemanager): restore ProfileMetadata.Error signal to alert consumers Found by /code-review: the presence-based fix for host profile metadata (state.Error == nil) accidentally withheld ProfileMetadata entirely whenever a profile was absent, dropping the Error field alert consumers previously relied on. The old "if state != nil" check was dead code (GetContainerProfileState never returns nil), so ProfileMetadata was always effectively attached before this feature branch; restore that behavior for both ApplicationProfile and NetworkProfile, surfacing state.Error via the Error field as before, for host and real containers alike. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(sbommanager): guard nil Annotations on existing host SBOM Found by CodeRabbit review on #976: - host_sbom.go: Storage.GetSBOMMeta returns the K8s object directly, and ObjectMeta.Annotations is optional -- an existing host SBOM fetched with nil Annotations would panic on the map write that stamps ToolVersionMetadataKey. The host scan runs on its own goroutine with no recovery, so this could take down the whole node-agent process. Initialize Annotations to an empty map before the status switch. Added a regression test seeding a nil-annotation SBOM and asserting prepareHostSbom doesn't panic. - lifecycle_host_timer_test.go: the non-host polling closure read entry.data before acquiring entry.mu, racing against the timer cleanup path setting entry.data to nil under that mutex. Moved the nil check inside the lock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * docs: add missing docstrings to close CodeRabbit coverage gap Adds Go doc comments to the 9 diff-touched functions that lacked them (LoadConfigOptional, CreateMalwareManager, GetPodSpec, CreateSbomManager, ContainerCallback in sbom_manager.go, GetContainerProfileState mock, CreateContainerProfileDirect and GetContainerProfile mocks, and the trimTrailingNewline helper in the new hostidentity package). No behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * refactor(hostmonitoring): address code-review cleanup findings All 7 low-severity findings from a second-pass /code-review of PR #976: - Add utils.IsHost(containerID string), the single choke point for the "is this the host" check wherever only a containerID (not a full *containercollection.Container) is available. Replaces independently re-implemented `== armotypes.HostContainerID` equality at 6 sites across dns_manager.go (including its own now-removed private isHost helper), profilehelper.go, creator.go, processtree_creator.go (2 sites) and network_stream.go. - Cache hostidentity.ResolveHostID's result on ContainerWatcher (sync.Once): the host add-container notification can be delivered more than once, and resolution can read /etc/machine-id from disk for a value that never changes for the process lifetime. - Extract creator.go's near-identical ApplicationProfile/NetworkProfile ProfileMetadata construction into a shared buildProfileMetadata helper. - Remove host_sbom.go's always-empty reportHostFailureOmitted() no-op and its 4 call sites; its rationale is now a doc comment on processHostSbom. - Dedupe prepareHostSbom's TooLarge/default switch tail via fallthrough. - Guard pkg/hostsensormanager's hostFSPrefix with atomic.Pointer[string] instead of a plain string: HostFSPrefix()/SetHostFSPrefixForTest were unsynchronized, a latent race under go test -race for any future parallel test or concurrent host-scan goroutine. Left unaddressed: the reviewer-flagged synchronous size.Of() reflection walk on the host SBOM mirrors the exact pattern the container path already uses (sbom_manager.go) -- changing size measurement for host only would diverge from an established, correctness-critical (TooLarge-gating) convention without matching review of the container path, which is out of proportion for this cleanup pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(containerwatcher): retry host-ID resolution after a transient failure Found by CodeRabbit review on #976: - containercallback.go: the sync.Once-based caching added in the previous cleanup commit permanently cached a FAILED resolution too, not just a successful one. cfg.NodeName is fixed at startup, but the machine-id fallback reads from disk and can fail transiently (e.g. HOST_ROOT not yet mounted on an early replay of the host add-container notification) -- sync.Once would lock that failure in forever, permanently breaking host shared-data injection for the process lifetime. Replaced with a mutex- guarded cache that only stores a successful hostID, so a later retry can still succeed. Added TestResolveHostID_RetriesAfterFailure to lock this in. - lifecycle_host_timer_test.go: the non-host container's short MaxSniffingTime (50ms) means its finalization timer could, in principle, already have fired and cleared entry.data between the earlier require.Eventually and this direct entry.data.timer read, causing a nil pointer panic instead of a clean test failure. Added a require.NotNil guard inside a closure (so the deferred RUnlock still runs even if require.NotNil exits via runtime.Goexit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(sbommanager): preserve content on a rescan of an Incomplete host SBOM Found by /code-review low: - host_sbom.go: prepareHostSbom's default case reported hadContent=false for an existing SBOM with status Incomplete. But Incomplete is only ever set when a prior scan already had content but was still oversized (see the write path's hadContent==true branch). Misreporting hadContent=false on a rescan routed a still-too-large Incomplete SBOM into the TooLarge branch, which wipes wipSbom.Spec -- destroying content Incomplete specifically exists to preserve, and permanently blocking further rescans via the TooLarge one-way door. Fixed by grouping Incomplete with Learning (both mean "content exists"), leaving Initializing/interrupted-run/TooLarge- released (never had content) in the false branch. Added Test_PrepareHostSbom_IncompleteRetainsContent. - creator.go: buildProfileMetadata dereferenced state's fields with no nil check, where the removed pre-refactor code guarded this with `if state != nil`. Every current GetContainerProfileState implementation happens to never return nil, so this isn't reachable today, but nil is still a valid value under the interface contract a future or alternate implementation could return. Added a defensive nil check plus TestBuildProfileMetadata_NilStateDoesNotPanic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(sbommanager): distinguish content-bearing from empty Incomplete SBOMs Found by CodeRabbit review on #976 (correcting my own prior fix in this same area): treating every Incomplete-status host SBOM as hadContent=true was itself wrong. handleGenericFailure marks a host SBOM Incomplete via an annotation-only patch (markSBOMStatus never touches Spec) after repeated scan failures -- this can fire on an SBOM that never completed a single scan, not only on one that previously had good/oversized content. The container path's own convention (wipSbomHadContent) confirms Incomplete is never unconditionally treated as "has content" there either -- only Learning is. Disambiguate using ResourceSizeMetadataKey: set exactly once a scan actually completes (success or oversized), and never cleared by the annotation-only failure patch, so its presence reliably distinguishes a content-bearing Incomplete SBOM from one that never got that far. Assuming either answer unconditionally is wrong in a different direction: always true lets a content-less SBOM dodge the TooLarge size gate forever; always false (my prior fix) wipes genuinely retained content via the TooLarge branch on the next oversized scan. Split the single Incomplete test into Test_PrepareHostSbom_IncompleteContentBearingRetainsContent and Test_PrepareHostSbom_IncompleteEmptyDoesNotClaimContent, covering both origins as requested by the review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix: address Copilot review findings, plus two more IgnoreContainer gaps found while fixing them Found by GitHub Copilot review (pullrequestreview-5263505786): - containerprofilemanager/v1/lifecycle.go: saveContainerProfile uses container.K8s.Namespace directly for the CR's own Namespace field, but the real host pseudo-container (GetHostAsContainer) has an empty K8s.Namespace/ PodName -- the first host profile save would fail to create against an empty namespace. The shared test fixture (newHostPseudoContainer) pre-populated those fields, masking the bug entirely. Fixed with hostContainerWithIdentity, which copies the synthetic identity from sharedData into a local copy before use, without mutating the container object shared with every other event subscriber. Corrected the test fixture to match the real (empty) production shape, turning the existing content-population test into a genuine regression test. - sbommanager: hostTooLargeReleased compared against ScannerMemoryLimitAnnotation/ scannerMemLimit, the sidecar scanner's memory limit -- irrelevant to host, which always scans in-process with Syft and whose scannerMemLimit is always 0. This meant increasing cfg.MaxSBOMSize could never unblock a stuck TooLarge host SBOM. Added a dedicated HostMaxSBOMSizeAnnotation compared against cfg.MaxSBOMSize instead. - sbommanager: hostFSPrefix (from hostsensormanager.HostFSPrefix(), fallback /host_fs) and hostRoot (local HOST_ROOT lookup, fallback /host -- matching the DaemonSet's actual mount) could silently disagree with no HOST_ROOT override set, opening a path the DaemonSet never mounts. Fixed by reusing hostRoot directly. - containerwatcher/v2/containercallback.go: the outer containerCallback's IgnoreContainer gate runs before any callback (including the host branch) is dispatched, and can drop the host pseudo-container under realistic production configs (an IncludeNamespaces allow-list that doesn't list "", or cfg.NamespaceName itself being empty). Added an IsHostContainer exemption, mirroring the convention already used elsewhere. While fixing that last one, found and fixed the SAME bug independently present in two more managers that Copilot's review didn't flag: containerprofilemanager/v1/lifecycle.go's own ContainerCallback and rulemanager/containercallbacks.go's ContainerCallback -- both ran IgnoreContainer unconditionally, ahead of their own downstream host bypasses (rule_manager.go's startRuleManager bypass is unreachable if containercallbacks.go's earlier gate already dropped the event). This means the rule/alert engine's host support -- the one piece this whole feature originally assumed already worked -- could itself have been silently broken under an IncludeNamespaces config. malwaremanager and sbommanager already guarded correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix: address second Copilot/CodeRabbit review round on host monitoring Copilot (pullrequestreview-5263783785): - containerprofilemanager, containerprofilecache: the host ContainerProfile CR was being stored/read in namespace "host" -- a synthetic identity label embedded in the Wlid/InstanceID, not a real Kubernetes namespace. The first host profile create would fail with NotFound on any real cluster. Fixed by using node-agent's own deployment namespace (cfg.NamespaceName, guaranteed to exist) for the actual storage calls in both the write path (hostContainerWithIdentity) and the read path (containerprofilecache's ContainerCallback), keeping them consistent since one reads what the other writes. - sbommanager: hostSbomName's sanitizer (character replacement + 63-char truncation) is lossy enough that two distinct hostIDs could collide on the same storage key (e.g. "node.a"/"node-a", or two long names sharing a 63-char prefix), letting one node silently overwrite another's SBOM. Fixed by appending an 8-char hash of the raw (pre-sanitize) hostID. - event_handler_factory.go: a second, separate IgnoreContainer gate on the runtime EVENT stream (ProcessEvent) was missed by the earlier container-lifecycle fix -- host would get correctly registered but every actual exec/open/syscall/network event would still be silently dropped here under an IncludeNamespaces config, starving the profile/rule/malware handlers of all host behavior data while appearing to work. Exempted the same way as the lifecycle callback. - Investigated and REFUTED the "Preserve SBOM content when oversized rescans fail" claim: wipSbom.Spec.Syft is unconditionally overwritten with the fresh scan's content before the size check runs, so a Learning SBOM that turns oversized on rescan persists the new (if oversized) content, not GetSBOMMeta's metadata-only shape. Verified empirically before deciding not to change this code. - hostidentity.go (flagged as "previously missed, code unchanged since last review" -- a real, standalone gap): ResolveHostID's machine-id fallback read via hostsensormanager.HostFSPrefix(), whose own default (/host_fs) differs from the /host default sbom_manager.go and the deployed chart actually use. With NodeName empty and no HOST_ROOT override, host identity resolution would fail even though the host filesystem is mounted. Fixed by resolving HOST_ROOT independently in this package (mirroring sbom_manager.go's identical inline logic, which this package cannot import without a cycle) rather than changing hostsensormanager's shared default, which an established, separate host-sensor feature also depends on. CodeRabbit (pullrequestreview-5263761626): - container_operations.go, lifecycle.go: addContainerWithTimeout replaced the container map entry unconditionally rather than checking for an existing one first. A replayed AddContainer notification (the container-watcher collection is known to do this, notably for the host pseudo-container) would orphan the earlier entry's monitor goroutine forever -- deleteContainer only ever signals "the current" entry in the map. Fixed with an atomic get-or-insert (addContainerEntryIfAbsent); a duplicate add is now a no-op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix: host profile terminal status, deletion hang, and SBOM label collision Third review round (Copilot) on host-monitoring-completion found two more real gaps and extended the earlier CR-name collision fix to labels: - monitorContainer now transitions the host profile to Completed once its learning window elapses, without stopping the loop -- previously it could never reach a terminal status, so containerprofilecache's tryPopulateEntry (which only caches Completed/TooLarge) would never surface it to profile-dependent CEL rules regardless of collected content. The same tick now also fires the completionNotifier exactly once, matching every other Completed/TooLarge transition in this file, which real consumers of that notification were previously never told about for host. - deleteContainer no longer risks a 30-second GetTerminationExitCode hang for host: it sets Completed directly instead of retrying for a Kubernetes pod status that will never exist. - hostSbomLabels now uses the same collision-resistant hash-suffixed identifier as the CR name, closing a gap where two hostnames colliding under lossy sanitization still shared identical labels despite distinct CR names. Two new regression tests for the fixes above initially read WatchedContainerData fields from outside monitorContainer's owning goroutine without synchronization; go test -race caught it. Fixed by synchronizing on the entry.ready channel and the completionNotifier hook instead of polling. Verified: go build/vet clean, go test -race -count=15 on the new tests clean, go test -race -count=1 across all touched packages clean, and go test -count=1 ./... shows only the two known pre-existing sandbox failures (tracers, validator -- both MEMLOCK/eBPF privilege limitations, confirmed against unmodified main). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix: HOST_ROOT empty-string handling, registration-retry gap, hash strength Fourth review round (CodeRabbit + Copilot) on host-monitoring-completion: - Treat an explicitly empty HOST_ROOT the same as unset in both places that resolve it (sbom_manager.go's CreateSbomManager, hostidentity.go's machineIDHostRoot) -- os.LookupEnv reports an empty env var as present, so both previously kept "" instead of falling back to /host, which fails the host Syft scan and misresolves machine-id-based host identity. - Fix a registration-retry gap: a replayed AddContainer racing in while an earlier attempt for the same container is still pending, and that attempt then failing, could leave the container silently untracked forever. entry.ready closes on both success and failure, so the "already tracked" branch now waits for it and retries the get-or-insert if the earlier attempt didn't survive, instead of assuming closure means success. - Widen the host SBOM's collision-resistant hash suffix from 8 to 32 hex characters (32 to 128 bits) -- collisionResistantLabel's maxBaseLen already accounted for the suffix length dynamically. - Record LearningPeriod for host even though its max-sniffing-time timer is never armed -- both previously lived behind the same host guard, so host profiles reported a 0s learning period despite genuinely converging on a schedule. monitorContainer's own Completed-transition deadline for host now reuses this recorded value instead of recomputing it a second time (which would silently drift, since calculateSniffingTime applies random jitter). - Fix the LearningPeriod write itself racing with concurrent reads: it was not synchronized with entry.mu the way the adjacent watchedContainerData/ timer field writes are, a pre-existing gap only now exercised concurrently. - Fix TestResolveHostID_RetriesAfterFailure forcing its failure case through hostsensormanager.SetHostFSPrefixForTest, which no longer affects the path hostidentity.ResolveHostID's machine-id fallback actually takes (it reads HOST_ROOT directly); the test could pass or fail depending on whether the sandbox running it has a real /host/etc/machine-id. Now also sets HOST_ROOT. Four other findings in this round's Copilot overview (host profile terminal state, SBOM content preservation on oversized rescans, host profile storage namespace, host event namespace filtering) were carried-forward stale findings from its own prior review round, already fixed earlier in this PR -- verified against current code before deciding not to change anything. Verified: go build/vet clean, go test -race -count=15 on the new/changed tests clean, go test -race -count=1 across all touched packages clean, and go test -count=1 ./... shows only the two known pre-existing sandbox failures (tracers, validator -- both MEMLOCK/eBPF privilege limitations). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix: deleteContainer could leak a still-running host monitor goroutine Fifth review round (Copilot) on host-monitoring-completion: deleteContainer's "signal termination only if monitoring is still active" guard keyed off status alone (GetStatus() != Completed && != TooLarge) -- correct for a real container, whose monitorContainer loop actually returns once it reaches one of those statuses, but wrong for host: an earlier fix in this PR made host's loop deliberately keep running past Completed. Once host reached Completed (which happens on every normal run, once its learning window elapses), removal would skip the termination-signal block entirely, remove the entry from the map anyway, and leave the still-running monitor goroutine ticking forever against a removed entry -- the underlying cause of a "container not found" log line seen earlier in this PR's history and previously attributed only to test timing. Fixed by making the "monitoring is still active" check unconditionally true for host regardless of its current status, since host's loop is only ever stopped by this explicit termination signal (unlike a real container's, which can also stop on its own). Added TestDeleteContainer_HostStopsMonitorEvenAfterReachingCompleted: drives host to Completed, removes it, then asserts no further profile saves occur in a window that would otherwise contain several more ticks. Also corrected a stale doc claim that pkg/hostidentity and the SBOM host branch share hostsensormanager.HostFSPrefix() -- that approach was tried and reverted earlier in this PR (its /host_fs fallback disagrees with the /host fallback this feature needs); both now resolve HOST_ROOT independently. Four other findings in this round's Copilot overview (host profile terminal state, SBOM content preservation, host profile storage namespace, host event namespace filtering) were carried-forward stale findings already fixed earlier in this PR -- verified against current code before deciding not to change anything. Verified: go build/vet clean, go test -race -count=15 on the new/changed tests clean, go test -race -count=1 across all touched packages clean, and go test -count=1 ./... shows only the two known pre-existing sandbox failures (tracers, validator -- both MEMLOCK/eBPF privilege limitations). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix: stale registration-failure cleanup could delete a newer entry Sixth review round (Copilot) on host-monitoring-completion: The fourth round's duplicate-registration retry (addContainerWithTimeout) created a new race: addContainer's own internal early-failure branches and addContainerWithTimeout's outer error/timeout branches both clean up after the same failure, and both used the unconditional removeContainerEntry (containerID) -- keyed only by containerID, not by which entry they actually own. If a replayed AddContainer retried registration (via the fourth round's fix) between the failing attempt's first cleanup call (which already removed its entry) and its second, that second call would delete the replay's newer, successfully-registered entry instead -- leaving its monitor goroutine running with no tracked entry left to ever signal it to stop. Fixed with removeContainerEntryIfMatch(containerID, expected), a compare-and-delete that only removes an entry if it is still exactly the one the caller holds. All five cleanup call sites in addContainer/ addContainerWithTimeout now use it; deleteContainer's own removal is unaffected, since it is not part of this retry loop. Added TestAddContainerWithTimeout_StaleFailureCleanupDoesNotDeleteNewerEntry, which drives addContainer's real internal failure path against an already-cancelled context, installs a newer entry the way a successful replay would, then proves a stale cleanup call for the failed attempt's own (already-removed) entry is a no-op rather than deleting the newer one. Four other findings in this round's Copilot overview (host profile terminal state, SBOM content preservation, host profile storage namespace, host event namespace filtering) were carried-forward stale findings already fixed earlier in this PR -- reverified against current code before deciding not to change anything. Verified: go build/vet clean, go test -race -count=15 on the new/changed tests clean, go test -race -count=1 across all touched packages clean, and go test -count=1 ./... shows only the two known pre-existing sandbox failures (tracers, validator -- both MEMLOCK/eBPF privilege limitations). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * docs: document hostSBOMRescanInterval config key Seventh review round (Copilot) on host-monitoring-completion found no new code issues -- the sixth round's registration-cleanup fix is confirmed resolved, and the four recurring stale findings (host profile terminal state, SBOM content preservation, host profile storage namespace, host event namespace filtering) remain already fixed, reverified against current code. One real gap: hostSBOMRescanInterval (default 24h) was missing from docs/CONFIGURATION.md's timing table, leaving operators with no documented way to discover or override the host SBOM rescan cadence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * docs: correct stale TooLarge-release comment and PR description Eighth review round (Copilot) on host-monitoring-completion found two stale comments, both describing pre-fix behavior, and no new code logic issues -- the four recurring carryovers (host profile terminal state, SBOM content preservation, host profile storage namespace, host event namespace filtering) remain already fixed, reverified against current code. - prepareHostSbom's TooLarge-release doc comment still named "the scanner memory limit" as an escape hatch; host never uses scannerMemLimit (it has no sidecar) -- the actual escape hatch is cfg.MaxSBOMSize via HostMaxSBOMSizeAnnotation, fixed in an earlier round. - The PR description said host alerts would permanently carry FailOnProfile=false, which became stale once monitorContainer was fixed (also earlier in this PR) to transition host to Completed. Updated on GitHub. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> --------- Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
M
Matthias Bertschy committed
9b414600d80200eba95bde14344f38479145fcd7
Parent: 5acac56
Committed by GitHub <noreply@github.com>
on 9/22/2026, 11:01:18 AM