feat(security): security findings dashboard#183
Open
janosmiko wants to merge 3 commits into
Open
Conversation
Summary by CodeRabbit
WalkthroughImplements a pluggable security-finding subsystem: new security package and multiple source adapters (falco, heuristic, trivy-operator, policyreport, kubescape, gatekeeper), a concurrent caching Manager with indexing and TTLs, K8s client virtual |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/tabs.go (1)
645-679:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReinitialize security state when loading a session-restored tab for the first time.
Tabs built via
buildSessionTabState()start with emptysecurityManager/ availability state. ThisneedsLoadpath restores those zero values, but never rerunsrefreshSecuritySources()orloadSecurityAvailability(). Switching to an inactive restored tab on another context will therefore come up with missing or stale Security sidebar state until the user manually changes navigation.Proposed fix
if needsLoad { m.tabs[idx].needsLoad = false m.applyPinnedGroups() + m.refreshSecuritySources() + securityCmd := m.loadSecurityAvailability() // Load contexts for the left column breadcrumb. contexts, _ := m.client.GetContexts() resourceTypes := model.BuildSidebarItems(model.SeedResources()) if discovered := m.discoveredResources[m.nav.Context]; len(discovered) > 0 { @@ switch m.nav.Level { case model.LevelResources: @@ - return m.loadResources(false) + return tea.Batch(m.loadResources(false), securityCmd) case model.LevelResourceTypes: @@ - return m.loadPreview() + return tea.Batch(m.loadPreview(), securityCmd) default: @@ - return m.refreshCurrentLevel() + return tea.Batch(m.refreshCurrentLevel(), securityCmd) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/tabs.go` around lines 645 - 679, The restored-tab loading path doesn't reinitialize security state, so when needsLoad is true call the security reinitialization helpers (e.g., refreshSecuritySources() and loadSecurityAvailability()) after applyPinnedGroups() and before any branch-specific loading (before loadResources/loadPreview/refreshCurrentLevel) so the Security sidebar state is rebuilt for session-restored tabs created by buildSessionTabState(); ensure these calls run unconditionally in the needsLoad block so securityManager and availability are populated before returning.
🧹 Nitpick comments (4)
internal/security/falco/falco.go (1)
163-163: 💤 Low valueUnsafe type assertion on stream closer.
The
stream.(io.Closer).Close()type assertion will panic ifstreamdoesn't implementio.Closer. TheStream()method returnsio.ReadCloser, so it should always be safe, but the assertion pattern is unnecessarily fragile.Safer close pattern
- _ = stream.(io.Closer).Close() + _ = stream.Close()Since
GetLogs(...).Stream(ctx)returns(io.ReadCloser, error), you can callClose()directly on the returned value without the type assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/security/falco/falco.go` at line 163, The current code uses an unsafe type assertion _ = stream.(io.Closer).Close() which can panic; replace it by calling Close directly on the returned reader (since GetLogs(...).Stream(ctx) returns an io.ReadCloser) — e.g., call stream.Close() and handle or log the returned error instead of asserting the type; locate the call site using the symbols GetLogs, Stream and the local variable stream to update the cleanup to a direct Close() call.internal/app/commands_load_preview.go (1)
116-121: ⚡ Quick winUse the shared security API-group constant instead of a raw string.
Line 116 hardcodes
"_security", while other paths already usemodel.SecurityVirtualAPIGroup. Using the constant avoids drift and keeps dispatch rules centralized.Proposed patch
- if m.nav.ResourceType.APIGroup == "_security" { + if m.nav.ResourceType.APIGroup == model.SecurityVirtualAPIGroup {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/commands_load_preview.go` around lines 116 - 121, Replace the hard-coded string "_security" in the API group check with the shared constant model.SecurityVirtualAPIGroup: in the conditional that reads if m.nav.ResourceType.APIGroup == "_security" (inside the method that calls loadSecurityAffectedResources), change the right-hand side to model.SecurityVirtualAPIGroup so the check uses the centralized constant used elsewhere and avoids string drift; ensure any necessary import/namespace references for model.SecurityVirtualAPIGroup remain valid.internal/app/security_source_entries.go (1)
39-69: 💤 Low valueClarify: Emoji characters in Icon field values.
The
model.Iconstruct'sEmojifield contains emoji characters (e.g., "🛡️", "🔍", "📋"). The coding guidelines state "No emojis in code, comments, or documentation." However, these appear to be functional UI data rendered based on user terminal preferences, not decorative content.If the
Emojifield is an intentional feature for emoji-capable terminals, this is likely acceptable as data. If emojis should be avoided entirely, these would need removal or replacement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/security_source_entries.go` around lines 39 - 69, The Emoji characters in the displayByName map (and the fallback meta.icon) violate the "No emojis in code" guideline; remove them by replacing each model.Icon Emoji value (e.g., in the map entries for "heuristic","trivy-operator","policy-report","falco","kube-bench","gatekeeper","kubescape" and the fallback Icon) with an empty string or a plain-text fallback, and ensure the created SecuritySourceEntry uses those updated model.Icon values (references: displayByName map, meta.icon, model.Icon, SecuritySourceEntry).internal/k8s/security.go (1)
247-304: 💤 Low value
findingToItemis currently unused in production code flows.The function has dedicated unit tests, but neither
getSecurityFindings(viagroupFindings+findingGroupToItem) norGetSecurityAffectedResources(viaaffectedResourceToItem) invoke it. If this is reserved for future use (e.g., flat finding lists without grouping), consider adding a comment explaining its intended purpose. Otherwise, remove it along with its tests to reduce dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/k8s/security.go` around lines 247 - 304, The function findingToItem is dead code (not referenced by getSecurityFindings / groupFindings / findingGroupToItem or by GetSecurityAffectedResources / affectedResourceToItem) so either document its intended future use or remove it and its tests; update code accordingly by either adding a clear comment above findingToItem explaining that it’s intentionally retained for future flat-finding rendering (include its expected consumers and why it differs from affectedResourceToItem/findingGroupToItem), or delete the findingToItem function and remove/adjust its unit tests to avoid testing unreachable code, ensuring all references in tests and the codebase are cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/app/commands_load_test.go`:
- Around line 927-934: The test is too permissive: for a "no-fetch" contract you
must assert loadYAML() does not return a command rather than allowing any
non-yamlLoadedMsg; update the check around cmd/msg so that if the scenario
expects no fetch then assert cmd == nil directly (i.e. fail the test if cmd !=
nil), otherwise (if a command is expected) keep the existing behavior of
invoking cmd() and asserting the resulting msg is a yamlLoadedMsg with no error;
refer to the cmd variable, the yamlLoadedMsg type, and the loadYAML() call to
locate and change the assertion logic.
In `@internal/app/security_availability_cache.go`:
- Around line 37-41: The SecurityAvailabilityCacheState struct currently stores
and logs the raw Host value (populated via HostForContext()), which can contain
full API server URLs or embedded credentials; remove the Host field from
SecurityAvailabilityCacheState (or replace it with a redacted identifier such as
a hostname-only or hash) and update all places that log or serialize this struct
(including the code paths that log the host and write the YAML snapshot) to use
the redacted identifier instead; ensure HostForContext() outputs are never
logged or written verbatim and add a small helper to produce the safe redaction
used by the cache and logger.
In `@internal/app/security_hook.go`:
- Around line 26-31: The shared availability map must be defensively copied at
hook boundaries: in setSecurityHookState(mgr *security.Manager, avail
map[string]bool) make a clone of avail (e.g., allocate a new map and copy
entries) before assigning it to the package variable securityHookAvailability
while holding securityHookMu; likewise, in currentSecurityHookState() return a
cloned copy of securityHookAvailability (not the original) so callers get an
independent map; keep using securityHookMu for synchronization and leave
securityHookManager assignment as-is.
In `@internal/app/security_refresh.go`:
- Around line 26-29: Resolved context stored in kctx (computed from
m.nav.Context with fallback m.client.CurrentContext()) is not being reused when
calling loadSecurityAvailabilityCacheForContext, causing cache skips; update all
calls that currently pass m.nav.Context (e.g., the
loadSecurityAvailabilityCacheForContext invocations) to pass kctx instead and
reuse kctx for subsequent availability-cache or probing calls so the resolved
context is consistently used (ensure any other branches that call
loadSecurityAvailabilityCacheForContext or similar functions also use kctx).
In `@internal/app/tabs_test.go`:
- Around line 980-1039: This test mutates package-level hook state via
loadTab(...) and fails to restore it; capture the prior hook state at the start
with currentSecurityHookState() and register a t.Cleanup that restores it (call
the package setter, e.g. setSecurityHookState or equivalent) so that after the
test (even on failure) you call setSecurityHookState(prevMgr, prevAvail); add
the capture and t.Cleanup before any calls to saveCurrentTab()/loadTab() in
TestTabSwitchPreservesSecurityState.
In `@internal/app/tabs.go`:
- Around line 69-75: The isVirtualResourceKind helper incorrectly omits the
actual port-forward row kind used by portForwardItems; update
isVirtualResourceKind(kind string) to treat "__port_forward_entry__" (in
addition to the existing "__port_forwards__") as a virtual kind so port-forward
rows short-circuit direct actions; modify the boolean expression in
isVirtualResourceKind to include equality check for "__port_forward_entry__"
(and keep the existing prefix check for "__security_") so any code paths that
receive a selected item's Kind will no-op for port-forward rows.
In `@internal/app/update_actions_default.go`:
- Line 26: The debug log currently emits the fully expanded custom shell command
(expandedCmd) via m.addLogEntry, which may leak secrets; change this to never
log the verbatim expandedCmd — either log a generic message or a safely redacted
version instead. Modify the call site that uses m.addLogEntry("DBG",
fmt.Sprintf("$ sh -c %q", expandedCmd)) so it logs a constant placeholder (e.g.
"$ sh -c <redacted>") or pass a sanitized string produced by a new helper (e.g.
redactSecrets or redactCommandTemplate) that strips or masks any
tokens/credentials before logging; ensure you update references to expandedCmd
in that logging call only and keep command execution unchanged.
In `@internal/security/falco/parse.go`:
- Around line 185-187: The code incorrectly maps str("k8s.rc.name") to
ReplicaSet; change the branch that checks str("k8s.rc.name") to set ref.Kind =
"ReplicationController" instead, and add a new branch that checks
str("k8s.rs.name") (or reuse the same pattern) to set ref.Kind = "ReplicaSet" so
ReplicaSets are handled; update the conditions around the existing
str("k8s.rc.name") check and the ref.Name assignment to preserve behavior while
correcting the Kind values.
In `@internal/security/gatekeeper/gatekeeper.go`:
- Around line 42-44: The availability probe currently only checks
templates.gatekeeper.sh/v1 (ConstraintTemplateGVR) and will fail on clusters
where Gatekeeper serves ConstraintTemplate as v1beta1; update the IsAvailable
probe to try v1 first and, if that fails, fall back to probing
templates.gatekeeper.sh/v1beta1 (e.g., create a second
schema.GroupVersionResource for v1beta1 or attempt discovery with Version
"v1beta1") and return available if either check succeeds; reference
ConstraintTemplateGVR and the IsAvailable function so the probe attempts both
versions before reporting unavailable.
In `@internal/security/heuristic/heuristic.go`:
- Around line 69-73: The loop currently only iterates pod.Spec.Containers,
missing init and ephemeral containers; update the iteration in heuristic.go to
also run each check in allChecks against pod.Spec.InitContainers and
pod.Spec.EphemeralContainers (e.g., iterate over pod.Spec.InitContainers and
pod.Spec.EphemeralContainers just like pod.Spec.Containers) and append their
results to findings so allChecks is applied uniformly to init, regular, and
ephemeral containers.
- Around line 58-61: The current Pod listing uses a single unbounded List call
(s.client.CoreV1().Pods(namespace).List) and the scan only iterates
pod.Spec.Containers; change the list call to paginate using metav1.ListOptions
with Limit and loop on the Continue token to handle large clusters, and update
the scanning logic (where you iterate containers for each pod) to include
pod.Spec.InitContainers and pod.Spec.EphemeralContainers alongside
pod.Spec.Containers so all container types are evaluated by the heuristic.
In `@internal/security/manager.go`:
- Around line 271-275: The sourceResult being sent into results currently omits
availability on the error path, causing skipped/unavailable sources to appear as
Available:true; update all send-sites that write to the results channel (the
branches around sourceResult{name, findings, err} and the error-only send that
currently sends only {name}) to explicitly set Available:false when a source
returns an error or is skipped, and ensure the successful/no-error path sets
Available:true; adjust any consumers (e.g., the anySourceSucceeded check) to
rely on the Available field instead of inferring availability from the
presence/absence of findings. Ensure this change is applied consistently for the
sourceResult construction in the blocks around the existing send sites
referenced (including the ranges shown: ~271, 300-307, 328-340, 362-383).
- Around line 446-474: BuildFindingIndex currently uses seen := map[string]bool
and ignores later duplicates, which can under-report if a later duplicate has
higher severity; change dedup tracking to store the recorded Severity (e.g.,
seen := map[string]Severity) keyed by dedup := f.Resource.Key()+"|"+f.Title, and
on each finding: if dedup not seen, record seen[dedup]=f.Severity and increment
the appropriate counter in idx.counts[key]; if dedup seen and f.Severity >
seen[dedup], decrement the previous severity counter for idx.counts[key],
increment the new (higher) severity counter, and update seen[dedup]=f.Severity;
keep idx.bySource[f.Source]++ behavior unchanged. This ensures BuildFindingIndex
preserves the highest severity per (resource, Title).
In `@internal/security/policyreport/parse.go`:
- Around line 96-102: The current filter in parse.go that skips PolicyReport
objects uses a name substring check
strings.Contains(strings.ToLower(u.GetName()), "falco") which is too broad and
causes false negatives; remove that name-based check and only rely on explicit
metadata labels/annotations (the existing isFalcoSource checks on
labels["app.kubernetes.io/managed-by"] and labels["created-by"] and any other
explicit Falco-specific metadata) to detect falcosidekick-created reports,
updating the conditional around labels := u.GetLabels() and isFalcoSource
accordingly so only explicit source labels/annotations cause the report to be
skipped.
In `@internal/security/policyreport/policyreport.go`:
- Around line 93-96: The current block in the ClusterPolicyReport fetch swallows
all errors and returns (findings, nil); change it to only ignore NotFound
errors: import and use the Kubernetes API error helper (errors.IsNotFound) to
detect a missing ClusterPolicyReport CRD and return (findings, nil) only in that
case; for any other error (RBAC errors, timeouts, transient API errors) return
the error (e.g., return nil, err) so callers surface the failure—apply this
change where the variable err is checked after attempting to list/get
ClusterPolicyReport and mirror the error propagation pattern used above for
PolicyReport.
---
Outside diff comments:
In `@internal/app/tabs.go`:
- Around line 645-679: The restored-tab loading path doesn't reinitialize
security state, so when needsLoad is true call the security reinitialization
helpers (e.g., refreshSecuritySources() and loadSecurityAvailability()) after
applyPinnedGroups() and before any branch-specific loading (before
loadResources/loadPreview/refreshCurrentLevel) so the Security sidebar state is
rebuilt for session-restored tabs created by buildSessionTabState(); ensure
these calls run unconditionally in the needsLoad block so securityManager and
availability are populated before returning.
---
Nitpick comments:
In `@internal/app/commands_load_preview.go`:
- Around line 116-121: Replace the hard-coded string "_security" in the API
group check with the shared constant model.SecurityVirtualAPIGroup: in the
conditional that reads if m.nav.ResourceType.APIGroup == "_security" (inside the
method that calls loadSecurityAffectedResources), change the right-hand side to
model.SecurityVirtualAPIGroup so the check uses the centralized constant used
elsewhere and avoids string drift; ensure any necessary import/namespace
references for model.SecurityVirtualAPIGroup remain valid.
In `@internal/app/security_source_entries.go`:
- Around line 39-69: The Emoji characters in the displayByName map (and the
fallback meta.icon) violate the "No emojis in code" guideline; remove them by
replacing each model.Icon Emoji value (e.g., in the map entries for
"heuristic","trivy-operator","policy-report","falco","kube-bench","gatekeeper","kubescape"
and the fallback Icon) with an empty string or a plain-text fallback, and ensure
the created SecuritySourceEntry uses those updated model.Icon values
(references: displayByName map, meta.icon, model.Icon, SecuritySourceEntry).
In `@internal/k8s/security.go`:
- Around line 247-304: The function findingToItem is dead code (not referenced
by getSecurityFindings / groupFindings / findingGroupToItem or by
GetSecurityAffectedResources / affectedResourceToItem) so either document its
intended future use or remove it and its tests; update code accordingly by
either adding a clear comment above findingToItem explaining that it’s
intentionally retained for future flat-finding rendering (include its expected
consumers and why it differs from affectedResourceToItem/findingGroupToItem), or
delete the findingToItem function and remove/adjust its unit tests to avoid
testing unreachable code, ensuring all references in tests and the codebase are
cleaned up.
In `@internal/security/falco/falco.go`:
- Line 163: The current code uses an unsafe type assertion _ =
stream.(io.Closer).Close() which can panic; replace it by calling Close directly
on the returned reader (since GetLogs(...).Stream(ctx) returns an io.ReadCloser)
— e.g., call stream.Close() and handle or log the returned error instead of
asserting the type; locate the call site using the symbols GetLogs, Stream and
the local variable stream to update the cleanup to a direct Close() call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 882f1883-b281-40d6-8e7a-3170d5fc7145
📒 Files selected for processing (88)
internal/app/app.gointernal/app/app_init.gointernal/app/app_types.gointernal/app/commands_coverage_test.gointernal/app/commands_load.gointernal/app/commands_load_preview.gointernal/app/commands_load_preview_test.gointernal/app/commands_load_test.gointernal/app/commands_security.gointernal/app/commands_security_test.gointernal/app/commands_yaml.gointernal/app/cursor_test.gointernal/app/messages_security.gointernal/app/security_availability_cache.gointernal/app/security_availability_cache_test.gointernal/app/security_hook.gointernal/app/security_ignores.gointernal/app/security_ignores_test.gointernal/app/security_navigation.gointernal/app/security_navigation_test.gointernal/app/security_refresh.gointernal/app/security_source_entries.gointernal/app/security_source_entries_test.gointernal/app/security_state.gointernal/app/tabs.gointernal/app/tabs_test.gointernal/app/update.gointernal/app/update_actions.gointernal/app/update_actions_default.gointernal/app/update_actions_security.gointernal/app/update_actions_security_menu.gointernal/app/update_actions_security_test.gointernal/app/update_actions_test.gointernal/app/update_bookmarks_session.gointernal/app/update_keys_actions.gointernal/app/update_keys_actions_test.gointernal/app/update_navigation.gointernal/app/update_navigation_security_jump_test.gointernal/app/view.gointernal/app/view_right.gointernal/app/view_status.gointernal/app/view_status_test.gointernal/k8s/client.gointernal/k8s/client_operations.gointernal/k8s/client_resources.gointernal/k8s/security.gointernal/k8s/security_group.gointernal/k8s/security_group_test.gointernal/k8s/security_test.gointernal/model/actions.gointernal/model/metadata.gointernal/model/sidebar.gointernal/model/sidebar_test.gointernal/model/types.gointernal/model/types_test.gointernal/security/falco/falco.gointernal/security/falco/falco_test.gointernal/security/falco/parse.gointernal/security/gatekeeper/gatekeeper.gointernal/security/gatekeeper/gatekeeper_test.gointernal/security/heuristic/checks.gointernal/security/heuristic/fetch_test.gointernal/security/heuristic/heuristic.gointernal/security/heuristic/heuristic_test.gointernal/security/kubescape/kubescape.gointernal/security/kubescape/kubescape_test.gointernal/security/kubescape/parse.gointernal/security/manager.gointernal/security/manager_test.gointernal/security/pagination.gointernal/security/pagination_test.gointernal/security/policyreport/parse.gointernal/security/policyreport/policyreport.gointernal/security/policyreport/policyreport_test.gointernal/security/source.gointernal/security/source_test.gointernal/security/testing.gointernal/security/trivyop/parse.gointernal/security/trivyop/trivyop.gointernal/security/trivyop/trivyop_test.gointernal/ui/config_keybindings.gointernal/ui/explorer_format.gointernal/ui/explorer_format_columns.gointernal/ui/explorer_format_test.gointernal/ui/explorer_sec_column.gointernal/ui/explorer_sec_column_test.gointernal/ui/findingdetails.gointernal/ui/findingdetails_test.go
Comment on lines
+927
to
+934
| if cmd == nil { | ||
| return | ||
| } | ||
| msg := cmd() | ||
| if loaded, ok := msg.(yamlLoadedMsg); ok { | ||
| assert.NoError(t, loaded.err, "synthetic security item must not produce a yamlLoadedMsg with unknown-resource-type error") | ||
| } | ||
| } |
There was a problem hiding this comment.
Regression assertion is too permissive for a “no-fetch” contract.
This test currently passes even if loadYAML() returns a non-nil command and a non-yamlLoadedMsg. If the contract is short-circuit/no fetch, assert that directly.
Suggested tightening
cmd := m.loadYAML()
- if cmd == nil {
- return
- }
- msg := cmd()
- if loaded, ok := msg.(yamlLoadedMsg); ok {
- assert.NoError(t, loaded.err, "synthetic security item must not produce a yamlLoadedMsg with unknown-resource-type error")
- }
+ assert.Nil(t, cmd, "synthetic security item must short-circuit without dispatching YAML fetch")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if cmd == nil { | |
| return | |
| } | |
| msg := cmd() | |
| if loaded, ok := msg.(yamlLoadedMsg); ok { | |
| assert.NoError(t, loaded.err, "synthetic security item must not produce a yamlLoadedMsg with unknown-resource-type error") | |
| } | |
| } | |
| if cmd == nil { | |
| return | |
| } | |
| assert.Nil(t, cmd, "synthetic security item must short-circuit without dispatching YAML fetch") | |
| } |
Suggested change
| if cmd == nil { | |
| return | |
| } | |
| msg := cmd() | |
| if loaded, ok := msg.(yamlLoadedMsg); ok { | |
| assert.NoError(t, loaded.err, "synthetic security item must not produce a yamlLoadedMsg with unknown-resource-type error") | |
| } | |
| } | |
| cmd := m.loadYAML() | |
| assert.Nil(t, cmd, "synthetic security item must short-circuit without dispatching YAML fetch") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/app/commands_load_test.go` around lines 927 - 934, The test is too
permissive: for a "no-fetch" contract you must assert loadYAML() does not return
a command rather than allowing any non-yamlLoadedMsg; update the check around
cmd/msg so that if the scenario expects no fetch then assert cmd == nil directly
(i.e. fail the test if cmd != nil), otherwise (if a command is expected) keep
the existing behavior of invoking cmd() and asserting the resulting msg is a
yamlLoadedMsg with no error; refer to the cmd variable, the yamlLoadedMsg type,
and the loadYAML() call to locate and change the assertion logic.
Comment on lines
+37
to
+41
| type SecurityAvailabilityCacheState struct { | ||
| SchemaVersion int `json:"schema_version"` | ||
| Host string `json:"host"` | ||
| UpdatedAt time.Time `json:"updated_at"` | ||
| Availability map[string]bool `json:"availability"` |
There was a problem hiding this comment.
Redact the kube host before logging or persisting it.
Lines 69-80 log host verbatim, and Lines 100-104 write it straight into the YAML snapshot. HostForContext() can resolve to the full API server URL, so this can leak internal endpoints and, in some kubeconfigs, embedded credentials. The cache filename is already host-scoped, so please drop this field or store/log only a redacted identifier.
As per coding guidelines, "Always redact logs and never paste secrets (API keys, tokens, passwords, JWTs) in code or output".
Also applies to: 69-80, 100-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/app/security_availability_cache.go` around lines 37 - 41, The
SecurityAvailabilityCacheState struct currently stores and logs the raw Host
value (populated via HostForContext()), which can contain full API server URLs
or embedded credentials; remove the Host field from
SecurityAvailabilityCacheState (or replace it with a redacted identifier such as
a hostname-only or hash) and update all places that log or serialize this struct
(including the code paths that log the host and write the YAML snapshot) to use
the redacted identifier instead; ensure HostForContext() outputs are never
logged or written verbatim and add a small helper to produce the safe redaction
used by the cache and logger.
Comment on lines
+980
to
+1039
| func TestTabSwitchPreservesSecurityState(t *testing.T) { | ||
| mgrA := security.NewManager() | ||
| mgrB := security.NewManager() | ||
| availA := map[string]bool{"heuristic": true} | ||
| availB := map[string]bool{"heuristic": true, "trivy-operator": true, "falco": true, "policy-report": true} | ||
|
|
||
| m := Model{ | ||
| tabs: []TabState{ | ||
| { | ||
| nav: model.NavigationState{Context: "cluster-a"}, | ||
| // Tab A: heuristic-only cluster. | ||
| securityManager: mgrA, | ||
| securityAvailabilityByName: availA, | ||
| securityActiveGroup: "groupA", | ||
| showSecurityIgnored: true, | ||
| }, | ||
| { | ||
| nav: model.NavigationState{Context: "cluster-b"}, | ||
| // Tab B: full-stack cluster. | ||
| securityManager: mgrB, | ||
| securityAvailabilityByName: availB, | ||
| securityActiveGroup: "groupB", | ||
| showSecurityIgnored: false, | ||
| }, | ||
| }, | ||
| activeTab: 0, | ||
| // Mirror Tab 0's state into the active model fields so | ||
| // saveCurrentTab has something to persist. Model embeds | ||
| // securityModelState, so the fields are promoted but must be | ||
| // initialised via the embedded struct literal. | ||
| securityModelState: securityModelState{ | ||
| securityManager: mgrA, | ||
| securityAvailabilityByName: availA, | ||
| securityActiveGroup: "groupA", | ||
| showSecurityIgnored: true, | ||
| }, | ||
| } | ||
|
|
||
| // Switch to Tab B. | ||
| m.saveCurrentTab() | ||
| m.loadTab(1) | ||
| assert.Same(t, mgrB, m.securityManager, "Tab B's manager must be active") | ||
| assert.Equal(t, availB, m.securityAvailabilityByName, "Tab B's availability map must be active") | ||
| assert.Equal(t, "groupB", m.securityActiveGroup) | ||
| assert.False(t, m.showSecurityIgnored, "Tab B's toggle must be active") | ||
| hookMgr, hookAvail := currentSecurityHookState() | ||
| assert.Same(t, mgrB, hookMgr, "hook state must publish Tab B's manager so sidebar renders B's sources") | ||
| assert.Equal(t, availB, hookAvail) | ||
|
|
||
| // Switch back to Tab A. | ||
| m.saveCurrentTab() | ||
| m.loadTab(0) | ||
| assert.Same(t, mgrA, m.securityManager) | ||
| assert.Equal(t, availA, m.securityAvailabilityByName) | ||
| assert.Equal(t, "groupA", m.securityActiveGroup) | ||
| assert.True(t, m.showSecurityIgnored) | ||
| hookMgr, hookAvail = currentSecurityHookState() | ||
| assert.Same(t, mgrA, hookMgr) | ||
| assert.Equal(t, availA, hookAvail) | ||
| } |
There was a problem hiding this comment.
Restore global hook state in test cleanup to prevent cross-test leakage.
This test updates package-level hook state via loadTab(...) but doesn’t restore prior values. Add t.Cleanup to keep tests order-independent.
Suggested hardening
func TestTabSwitchPreservesSecurityState(t *testing.T) {
+ prevMgr, prevAvail := currentSecurityHookState()
+ t.Cleanup(func() {
+ setSecurityHookState(prevMgr, prevAvail)
+ })
+
mgrA := security.NewManager()
mgrB := security.NewManager()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/app/tabs_test.go` around lines 980 - 1039, This test mutates
package-level hook state via loadTab(...) and fails to restore it; capture the
prior hook state at the start with currentSecurityHookState() and register a
t.Cleanup that restores it (call the package setter, e.g. setSecurityHookState
or equivalent) so that after the test (even on failure) you call
setSecurityHookState(prevMgr, prevAvail); add the capture and t.Cleanup before
any calls to saveCurrentTab()/loadTab() in TestTabSwitchPreservesSecurityState.
Comment on lines
+69
to
+73
| for _, c := range pod.Spec.Containers { | ||
| for _, check := range allChecks { | ||
| findings = append(findings, check(pod, c)...) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the file
find . -name "heuristic.go" -path "*/internal/security/heuristic/*" | head -5Repository: janosmiko/lfk
Length of output: 100
🏁 Script executed:
# Once found, examine the content around lines 69-73
cd . && git ls-files "**/internal/security/heuristic/**"Repository: janosmiko/lfk
Length of output: 39
🏁 Script executed:
# Look for the file in the repository
fd "heuristic.go" internal/securityRepository: janosmiko/lfk
Length of output: 98
🏁 Script executed:
# Examine the file to see the code at lines 69-73 and surrounding context
cat -n ./internal/security/heuristic/heuristic.go | sed -n '50,100p'Repository: janosmiko/lfk
Length of output: 1105
🏁 Script executed:
# Check the imports and type definitions
cat -n ./internal/security/heuristic/heuristic.go | head -50Repository: janosmiko/lfk
Length of output: 2056
🏁 Script executed:
# Search for any existing usage of InitContainers or EphemeralContainers in codebase
rg "InitContainers|EphemeralContainers" --type goRepository: janosmiko/lfk
Length of output: 3713
🏁 Script executed:
# Check if there are any tests for the Fetch function
rg "TestFetch|func Test" ./internal/security/heuristic/ --type go -A 5Repository: janosmiko/lfk
Length of output: 8131
🏁 Script executed:
# Check the Kubernetes types to understand EphemeralContainer vs Container
rg "EphemeralContainer" ./internal/k8s/orphan_refs.go -B 2 -A 2Repository: janosmiko/lfk
Length of output: 282
🏁 Script executed:
# Look at the test that uses EphemeralContainers
cat -n ./internal/k8s/orphan_refs_test.go | grep -A 20 "TestBuildRefSet_InitAndEphemeralContainers"Repository: janosmiko/lfk
Length of output: 1021
🏁 Script executed:
# See more of the EphemeralContainer test
cat -n ./internal/k8s/orphan_refs_test.go | sed -n '111,130p'Repository: janosmiko/lfk
Length of output: 873
🏁 Script executed:
# Search for how EphemeralContainer is used elsewhere to understand its structure better
rg "EphemeralContainer" ./internal/k8s -B 1 -A 3 | head -60Repository: janosmiko/lfk
Length of output: 1184
🏁 Script executed:
# Let's verify the actual structure by checking if EphemeralContainerCommon has all the fields needed
# First, let's see what the checkFn functions actually check
cat -n ./internal/security/heuristic/heuristic.go | tail -200Repository: janosmiko/lfk
Length of output: 3095
🏁 Script executed:
# Get the actual check functions to understand what fields they use
cat -n ./internal/security/heuristic/heuristic.go | sed -n '78,250p'Repository: janosmiko/lfk
Length of output: 205
🏁 Script executed:
# Get the file size first
wc -l ./internal/security/heuristic/heuristic.goRepository: janosmiko/lfk
Length of output: 103
🏁 Script executed:
# Read the entire file to see all the checks
cat ./internal/security/heuristic/heuristic.goRepository: janosmiko/lfk
Length of output: 2542
🏁 Script executed:
# List all files in the heuristic package
ls -la ./internal/security/heuristic/Repository: janosmiko/lfk
Length of output: 426
🏁 Script executed:
# Search for where allChecks is defined
rg "allChecks" ./internal/security/heuristic/ --type goRepository: janosmiko/lfk
Length of output: 762
🏁 Script executed:
# Examine the checks.go file to see the allChecks definition and understand what each check does
cat -n ./internal/security/heuristic/checks.go | head -100Repository: janosmiko/lfk
Length of output: 4640
🏁 Script executed:
# Continue reading the checks to see what fields are accessed
cat -n ./internal/security/heuristic/checks.go | tail -100Repository: janosmiko/lfk
Length of output: 4051
🏁 Script executed:
# Search for how EphemeralContainer fields are accessed in the codebase
rg "\.Env|\.EnvFrom|\.Image|\.Name|\.SecurityContext|\.Resources" ./internal/security/heuristic/checks.goRepository: janosmiko/lfk
Length of output: 2522
🏁 Script executed:
# Check if there's Kubernetes documentation or examples of EphemeralContainer conversion
rg "EphemeralContainerCommon" . --type go -B 2 -A 2 | head -50Repository: janosmiko/lfk
Length of output: 404
Include init and ephemeral containers in heuristic checks.
Scanning only pod.Spec.Containers misses risky init/ephemeral container settings, which creates false negatives in security findings. Init containers can run privileged setup code, and ephemeral containers are commonly used for debugging—both should be validated against the same security checks.
Proposed patch
for _, c := range pod.Spec.Containers {
for _, check := range allChecks {
findings = append(findings, check(pod, c)...)
}
}
+ for _, c := range pod.Spec.InitContainers {
+ for _, check := range allChecks {
+ findings = append(findings, check(pod, c)...)
+ }
+ }
+ for _, ec := range pod.Spec.EphemeralContainers {
+ c := corev1.Container{
+ Name: ec.Name,
+ Image: ec.Image,
+ SecurityContext: ec.SecurityContext,
+ Resources: ec.Resources,
+ }
+ for _, check := range allChecks {
+ findings = append(findings, check(pod, c)...)
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/security/heuristic/heuristic.go` around lines 69 - 73, The loop
currently only iterates pod.Spec.Containers, missing init and ephemeral
containers; update the iteration in heuristic.go to also run each check in
allChecks against pod.Spec.InitContainers and pod.Spec.EphemeralContainers
(e.g., iterate over pod.Spec.InitContainers and pod.Spec.EphemeralContainers
just like pod.Spec.Containers) and append their results to findings so allChecks
is applied uniformly to init, regular, and ephemeral containers.
Comment on lines
+271
to
+275
| type sourceResult struct { | ||
| name string | ||
| findings []Finding | ||
| err error | ||
| } |
There was a problem hiding this comment.
Preserve the unavailable path instead of folding it into Available: true.
The nil-error collector path currently marks skipped sources as Available: true, because the unavailable branch only sends {name} through results. That makes uninstalled/disabled sources look healthy in res.Sources, and anySourceSucceeded will then treat an all-unavailable scan as a successful fetch.
Proposed fix
type sourceResult struct {
name string
+ skipped bool
findings []Finding
err error
}
@@
if !available {
- results <- sourceResult{name: s.Name()}
+ results <- sourceResult{name: s.Name(), skipped: true}
return
}
@@
for r := range results {
+ if r.skipped {
+ res.Sources = append(res.Sources, SourceStatus{Name: r.name})
+ continue
+ }
if r.err != nil {
res.Errors[r.name] = r.err
res.Sources = append(res.Sources, SourceStatus{Name: r.name, LastError: r.err})
continue
}Also applies to: 300-307, 328-340, 362-383
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/security/manager.go` around lines 271 - 275, The sourceResult being
sent into results currently omits availability on the error path, causing
skipped/unavailable sources to appear as Available:true; update all send-sites
that write to the results channel (the branches around sourceResult{name,
findings, err} and the error-only send that currently sends only {name}) to
explicitly set Available:false when a source returns an error or is skipped, and
ensure the successful/no-error path sets Available:true; adjust any consumers
(e.g., the anySourceSucceeded check) to rely on the Available field instead of
inferring availability from the presence/absence of findings. Ensure this change
is applied consistently for the sourceResult construction in the blocks around
the existing send sites referenced (including the ranges shown: ~271, 300-307,
328-340, 362-383).
Comment on lines
+446
to
+474
| func BuildFindingIndex(findings []Finding) *FindingIndex { | ||
| idx := &FindingIndex{ | ||
| counts: make(map[string]SeverityCounts), | ||
| bySource: make(map[string]int), | ||
| } | ||
| // Track (resource + title) pairs to deduplicate per-container findings | ||
| // in the per-resource severity counters only. | ||
| seen := make(map[string]bool) | ||
| for _, f := range findings { | ||
| idx.bySource[f.Source]++ | ||
| key := f.Resource.Key() | ||
| dedup := key + "|" + f.Title | ||
| if seen[dedup] { | ||
| continue | ||
| } | ||
| seen[dedup] = true | ||
| c := idx.counts[key] | ||
| switch f.Severity { | ||
| case SeverityCritical: | ||
| c.Critical++ | ||
| case SeverityHigh: | ||
| c.High++ | ||
| case SeverityMedium: | ||
| c.Medium++ | ||
| case SeverityLow: | ||
| c.Low++ | ||
| } | ||
| idx.counts[key] = c | ||
| } |
There was a problem hiding this comment.
Keep the highest severity for a deduped (resource, Title) key.
BuildFindingIndex drops every duplicate after the first one, so if the same unique check arrives twice with different severities, the badge severity depends on iteration order. That can under-report the affected resource when a later duplicate is more severe.
Proposed fix
func BuildFindingIndex(findings []Finding) *FindingIndex {
idx := &FindingIndex{
counts: make(map[string]SeverityCounts),
bySource: make(map[string]int),
}
- // Track (resource + title) pairs to deduplicate per-container findings
- // in the per-resource severity counters only.
- seen := make(map[string]bool)
+ bestByCheck := make(map[string]Finding)
for _, f := range findings {
idx.bySource[f.Source]++
- key := f.Resource.Key()
- dedup := key + "|" + f.Title
- if seen[dedup] {
- continue
+ dedup := f.Resource.Key() + "|" + f.Title
+ prev, ok := bestByCheck[dedup]
+ if !ok || severityRank(f.Severity) > severityRank(prev.Severity) {
+ bestByCheck[dedup] = f
}
- seen[dedup] = true
+ }
+ for _, f := range bestByCheck {
+ key := f.Resource.Key()
c := idx.counts[key]
switch f.Severity {
case SeverityCritical:
c.Critical++
@@
}
idx.counts[key] = c
}
return idx
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/security/manager.go` around lines 446 - 474, BuildFindingIndex
currently uses seen := map[string]bool and ignores later duplicates, which can
under-report if a later duplicate has higher severity; change dedup tracking to
store the recorded Severity (e.g., seen := map[string]Severity) keyed by dedup
:= f.Resource.Key()+"|"+f.Title, and on each finding: if dedup not seen, record
seen[dedup]=f.Severity and increment the appropriate counter in idx.counts[key];
if dedup seen and f.Severity > seen[dedup], decrement the previous severity
counter for idx.counts[key], increment the new (higher) severity counter, and
update seen[dedup]=f.Severity; keep idx.bySource[f.Source]++ behavior unchanged.
This ensures BuildFindingIndex preserves the highest severity per (resource,
Title).
Comment on lines
+96
to
+102
| // Skip entire reports created by falcosidekick (common label/annotation). | ||
| labels := u.GetLabels() | ||
| if isFalcoSource(labels["app.kubernetes.io/managed-by"]) || | ||
| isFalcoSource(labels["created-by"]) || | ||
| strings.Contains(strings.ToLower(u.GetName()), "falco") { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Over-broad Falco name filter can hide valid findings
Line 100 uses strings.Contains(..., "falco"), which can skip unrelated PolicyReport objects whose names happen to contain that substring. That creates silent false negatives in the security view. Prefer explicit metadata/source checks only.
Proposed fix
- if isFalcoSource(labels["app.kubernetes.io/managed-by"]) ||
- isFalcoSource(labels["created-by"]) ||
- strings.Contains(strings.ToLower(u.GetName()), "falco") {
+ if isFalcoSource(labels["app.kubernetes.io/managed-by"]) ||
+ isFalcoSource(labels["created-by"]) {
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Skip entire reports created by falcosidekick (common label/annotation). | |
| labels := u.GetLabels() | |
| if isFalcoSource(labels["app.kubernetes.io/managed-by"]) || | |
| isFalcoSource(labels["created-by"]) || | |
| strings.Contains(strings.ToLower(u.GetName()), "falco") { | |
| return nil | |
| } | |
| // Skip entire reports created by falcosidekick (common label/annotation). | |
| labels := u.GetLabels() | |
| if isFalcoSource(labels["app.kubernetes.io/managed-by"]) || | |
| isFalcoSource(labels["created-by"]) { | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/security/policyreport/parse.go` around lines 96 - 102, The current
filter in parse.go that skips PolicyReport objects uses a name substring check
strings.Contains(strings.ToLower(u.GetName()), "falco") which is too broad and
causes false negatives; remove that name-based check and only rely on explicit
metadata labels/annotations (the existing isFalcoSource checks on
labels["app.kubernetes.io/managed-by"] and labels["created-by"] and any other
explicit Falco-specific metadata) to detect falcosidekick-created reports,
updating the conditional around labels := u.GetLabels() and isFalcoSource
accordingly so only explicit source labels/annotations cause the report to be
skipped.
Comment on lines
+93
to
+96
| if err != nil { | ||
| // ClusterPolicyReport CRD may not exist — non-fatal. | ||
| return findings, nil //nolint:nilerr // optional CRD, not an error | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "policyreport.go" | grep -E "(security|policyreport)"Repository: janosmiko/lfk
Length of output: 106
🏁 Script executed:
cat -n internal/security/policyreport/policyreport.go | head -120 | tail -40Repository: janosmiko/lfk
Length of output: 1734
🏁 Script executed:
head -20 internal/security/policyreport/policyreport.goRepository: janosmiko/lfk
Length of output: 592
Distinguish between missing CRD and transient errors for ClusterPolicyReport.
Line 95 currently treats every error as success-with-partial-data. Only NotFound should be ignored; other errors (RBAC denial, timeouts, API transience) must surface to keep cluster health visible. This also aligns with the PolicyReport error handling above (line 84), which propagates errors rather than silently suppressing them.
Proposed patch
if namespace == "" {
cprList, err := security.ListPaginated(ctx, s.client.Resource(ClusterPolicyReportGVR))
if err != nil {
- // ClusterPolicyReport CRD may not exist — non-fatal.
- return findings, nil //nolint:nilerr // optional CRD, not an error
+ // ClusterPolicyReport CRD may not exist — non-fatal.
+ if apierrors.IsNotFound(err) {
+ return findings, nil
+ }
+ return findings, fmt.Errorf("list cluster policy reports: %w", err)
}
for i := range cprList.Items {
findings = append(findings, parsePolicyReport(&cprList.Items[i])...)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err != nil { | |
| // ClusterPolicyReport CRD may not exist — non-fatal. | |
| return findings, nil //nolint:nilerr // optional CRD, not an error | |
| } | |
| if err != nil { | |
| // ClusterPolicyReport CRD may not exist — non-fatal. | |
| if apierrors.IsNotFound(err) { | |
| return findings, nil | |
| } | |
| return findings, fmt.Errorf("list cluster policy reports: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/security/policyreport/policyreport.go` around lines 93 - 96, The
current block in the ClusterPolicyReport fetch swallows all errors and returns
(findings, nil); change it to only ignore NotFound errors: import and use the
Kubernetes API error helper (errors.IsNotFound) to detect a missing
ClusterPolicyReport CRD and return (findings, nil) only in that case; for any
other error (RBAC errors, timeouts, transient API errors) return the error
(e.g., return nil, err) so callers surface the failure—apply this change where
the variable err is checked after attempting to list/get ClusterPolicyReport and
mirror the error propagation pattern used above for PolicyReport.
Adds a top-to-bottom security findings feature: a Manager that aggregates
six SecuritySource adapters (heuristic, falco, trivy-operator,
policy-report, gatekeeper, kubescape), a per-resource SEC badge
rendered in the explorer table, a Security category in the sidebar
that filters to the cluster's actually-installed sources, and an
overlay-driven action menu for ignoring noisy findings.
Source adapters
- heuristic: walks Pod specs and flags privileged containers, host
namespaces, missing security contexts, etc. — no CRDs required.
- falco: tails the falco daemon logs and surfaces rule hits with the
resource ref extracted from each entry's output_fields.
- trivy-operator: reads VulnerabilityReport + ConfigAuditReport CRDs.
- policy-report: reads wgpolicyk8s.io PolicyReport / ClusterPolicyReport
CRDs (Kyverno + others).
- gatekeeper: reads OPA Gatekeeper Constraint CRDs. IsAvailable probes
ConstraintTemplate via the dynamic client (ctx-aware) so a stalled
API server can't block the WaitGroup and starve every other source's
probe result; Fetch wraps the Discovery() enumeration of dynamic
constraint kinds in a ctx-bounded goroutine for the same reason.
Severity from spec.enforcementAction (deny -> high, warn -> medium,
dryrun -> low).
- kubescape: reads WorkloadConfigurationScan CRDs from
spdx.softwarecomposition.kubescape.io and surfaces failed controls;
1-10 scoreFactor bucketed to the explorer's coarse Severity scale.
Manager + caching
- Parallel IsAvailable probe with per-source 3s context timeout (so
one slow source can't starve the rest, now that every source's
probe honours ctx).
- FetchAll runs sources in parallel via WaitGroup.Go; per-source
errors flow into res.Errors so partial failure stays visible.
- Result cache keyed by (kubeCtx, namespace) with a 5-minute refresh
TTL; clean clusters (zero findings) are also cacheable so navigation
ticks don't re-hammer the sources.
- BuildFindingIndex aggregates per-resource severity counts for the
SEC badge; bySource counts every emission so cross-source overlaps
don't undercount.
- Per-host on-disk availability cache lives next to kubectl's discovery
cache ($KUBECACHEDIR/discovery/<host>/lfk-security-availability.yaml)
so the sidebar populates instantly on subsequent runs.
- IgnoreChecker + showIgnored guarded with an RWMutex on the k8s
client; the persisted ignore-list is async-saved off the Update
goroutine so an fsync stall can't freeze the UI.
Explorer integration
- Synthetic ResourceTypes ("__security_<source>__") routed through
Client.GetResources via the _security APIGroup so the standard
loadResources path renders findings as model.Items without bespoke
rendering.
- Drill-in: __security_finding_group__ rows at LevelResources expand
to __security_affected_resource__ rows at LevelOwned; Enter on an
affected resource jumps to the underlying real Kubernetes resource
(resolved from the per-row "__resource_key__" column).
- Right pane shows a split view (affected resources on top, group
details below) for finding groups, mirroring the Pod -> Containers
layout.
- Action menu (#) gains "Ignore (Group)" / "Ignore (This Resource)" /
"Un-ignore" / "Refresh" entries and a per-cluster
SecurityIgnoreState persisted to YAML.
- shift+r and the Refresh menu entry both invalidate the manager
cache and re-probe availability so newly-installed CRDs appear
without restarting.
- Per-tab security state (manager, availability, index, active
group, showIgnored) lives on TabState so two tabs on different
clusters don't share sources via the global SecuritySourcesFn hook.
Synthetic-kind safety
- All action handlers (loadYAML, copy/export, directAction*, Diff,
enterFullView, navigateChild*) gate on isVirtualResourceKind /
onSecurityView so users no longer trip "Warning: unknown resource
type: __security_*" on Y/L/E/D/Del/Enter for synthetic rows.
- Sidebar/header rendering honors DisplayName for synthetic kinds so
the user sees "FALCO" / "PORT FORWARDS" instead of
"__SECURITY_FALCO__".
- Loader entry "(probing sources...)" with the new
model.SecurityLoaderKind sentinel surfaces while the availability
probe is in flight (no cached result), and is non-navigable.
- Refresh + watch tick at LevelOwned in security view route through
loadSecurityAffectedResources instead of GetOwnedResources (which
has no case for synthetic kinds and would silently empty the list).
Tests cover every new package plus the integration paths: source
adapters, Manager cache invalidation + index clearing, IgnoreChecker
race, async ignore-state save, Enter drill-in, Enter-jump,
synthetic-kind action no-ops, column header DisplayName, loader
sentinel navigation no-op, on-disk availability cache round-trip,
and tab-switch security state preservation.
Avoid stressing the Kubernetes control plane during security findings fetches. On a real cluster (746 PolicyReports + 442 VulnerabilityReports + 524 ConfigAuditReports) one of the user's control planes froze under the original load pattern (one large list per source, all sources fanned out concurrently, no pagination, all-errored fetches re-fired on every navigation tick). Pagination - New ListPaginated helper wrapping client-go's tools/pager via EachListItem, page size 200. Auto-recovers from continue-token expiration on long lists. - Wired into trivyop (VulnerabilityReports + ConfigAuditReports), policyreport (PolicyReports + ClusterPolicyReports), kubescape (WorkloadConfigurationScans), and gatekeeper (per-kind Constraints). - Per-page response stays bounded; HTTP/2 streams stop collapsing on large lists, etcd doesn't materialise the whole list in memory. Concurrency cap - New Manager.SetMaxFetchConcurrency (default 2) caps simultaneous source Fetches via a buffered-channel semaphore acquired AFTER the IsAvailable hint check (unavailable sources cost zero slots) and honouring ctx cancellation while waiting. - A 6-source cluster fans out 2-at-a-time instead of all-at-once; peak control-plane load drops roughly 3x. Throttling-spiral fixes (related) - Manager.SetAvailability lets callers publish a per-source availability hint so FetchAll skips its own IsAvailable probe when the app already paid that cost. Wired from updateSecurityAvailabilityLoaded. Halves API request volume per navigation tick. - New errorTTL (default 30s) caches all-errored fetch results briefly so a throttled cluster doesn't re-fire the same failing list calls on every navigation tick. - gatekeeper.discoverConstraintKinds returns (nil, nil) on NotFound, so a Gatekeeper webhook installed without ConstraintTemplates no longer spams "the server could not find the requested resource" per fetch. Tests cover the pagination helper (multi-page accumulation, empty list, single page, error propagation, cancellation), the concurrency cap (3-wave fan-out for 6 sources), the unbounded escape hatch, the availability hint skip, the negative-cache TTL, and the gatekeeper NotFound path.
Rebase onto main and apply review fixes. Bugs: - falco: k8s.rc.name is ReplicationController (not ReplicaSet); add k8s.rs.name -> ReplicaSet mapping that was missing entirely. - heuristic: scan InitContainers and EphemeralContainers in addition to Spec.Containers — privileged init containers were silently invisible to the dashboard. - policyreport: only swallow NotFound errors when listing ClusterPolicyReports; RBAC denials and API timeouts now surface to the manager instead of being reported as zero findings. Drop the substring "falco" name filter that incorrectly skipped legitimate Kyverno reports whose policy name happened to contain that token. - manager: unavailable sources (IsAvailable=false) are no longer reported with Available:true in res.Sources, restoring the negative cache semantics for clusters with zero installed security tools. - manager: namespace-filter no longer reuses Findings backing array (Findings[:0] aliased the cached slice so a later FetchAll could overwrite findings a concurrent cache-hit reader was still reading). - manager: snapshot ignoredNamespaces under RLock before filtering so a concurrent SetIgnoredNamespaces can't race with the read. - security_hook: defensively clone the availability map at set / get boundaries so publisher mutation (Model.maps.Copy after a probe result) cannot be observed by hook readers outside the lock. - security_refresh: reuse the resolved kctx for both the disk-cache lookup and the IgnoreChecker — previously the cache lookup got m.nav.Context directly and was skipped when nav.Context was empty. - tabs: session-restored tabs now call refreshSecuritySources / loadSecurityAvailability when promoted via needsLoad so the Security sidebar populates on the restored tab's context instead of waiting for the first manual navigation. - update_actions_default: stop logging the expanded custom-action command verbatim — templates can interpolate user-controlled fields (env, annotations, secret names) that should not echo into the log. - falco: defer stream.Close() inside a per-iteration func so it runs on scanner error too, and raise bufio.Scanner's buffer cap so long JSON alert lines don't hit bufio.ErrTooLong and abort silently. Nits: - use model.SecurityVirtualAPIGroup constant instead of the "_security" string literal in commands_load_preview and commands_security. - extract isVirtualResourceKind into virtual_kinds.go to keep tabs.go under the 800-line cap. Regression tests cover: workload-kind mapping in extractRefFromOutputFields, init/ephemeral container scanning, unavailable-source cache decision, errored-source negative cache, ignored-namespace filter slice insulation, and security hook clone semantics.
janosmiko
force-pushed
the
feat/security-dashboard-v2
branch
from
6917779 to
3206373
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/app/update_actions_security_menu.go (1)
164-172: ⚡ Quick winUse
logger.Errorfor error conditions.
logger.Infois used to log a failure to save security ignores. Since this represents an error condition,logger.Errororlogger.Warnwould be more appropriate for log-level filtering and alerting.Proposed fix
func (m Model) updateSecurityIgnoresSaveErr(msg securityIgnoresSaveErrMsg) (tea.Model, tea.Cmd) { - logger.Info("Failed to save security ignores", "error", msg.err) + logger.Error("Failed to save security ignores", "error", msg.err) m.setStatusMessage("Failed to save ignore rule", true) return m, scheduleStatusClear() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/update_actions_security_menu.go` around lines 164 - 172, In updateSecurityIgnoresSaveErr, replace the logger.Info call with logger.Error to correctly record this failure as an error-level event; keep the same context keys (e.g., "Failed to save security ignores", "error", msg.err) so the error message and stack are preserved when logging from updateSecurityIgnoresSaveErr(msg securityIgnoresSaveErrMsg).internal/k8s/security.go (1)
253-304: 💤 Low value
CreatedAt: time.Now()may misrepresent finding age.Using the current time as
CreatedAtfor every item means the "Age" column will always show near-zero, which doesn't reflect when the finding was actually detected. Ifsecurity.Findinghas a timestamp field (or can derive one from the source), consider using it instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/k8s/security.go` around lines 253 - 304, The findingToItem function sets CreatedAt to time.Now(), which hides the finding's real age; change it to use the timestamp provided by the security.Finding (e.g., f.Timestamp or f.DetectedAt / f.FirstSeen — whichever field exists on security.Finding) and only fall back to time.Now() when that field is zero; update the CreatedAt assignment in findingToItem to read that finding timestamp (with a zero-time check) so the Item's age reflects the actual detection time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/app/update_actions_security_menu.go`:
- Around line 164-172: In updateSecurityIgnoresSaveErr, replace the logger.Info
call with logger.Error to correctly record this failure as an error-level event;
keep the same context keys (e.g., "Failed to save security ignores", "error",
msg.err) so the error message and stack are preserved when logging from
updateSecurityIgnoresSaveErr(msg securityIgnoresSaveErrMsg).
In `@internal/k8s/security.go`:
- Around line 253-304: The findingToItem function sets CreatedAt to time.Now(),
which hides the finding's real age; change it to use the timestamp provided by
the security.Finding (e.g., f.Timestamp or f.DetectedAt / f.FirstSeen —
whichever field exists on security.Finding) and only fall back to time.Now()
when that field is zero; update the CreatedAt assignment in findingToItem to
read that finding timestamp (with a zero-time check) so the Item's age reflects
the actual detection time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7f8d0a9-b50b-4190-8975-db343b009f07
📒 Files selected for processing (83)
internal/app/app.gointernal/app/app_init.gointernal/app/app_types.gointernal/app/commands_coverage_test.gointernal/app/commands_load.gointernal/app/commands_load_preview.gointernal/app/commands_load_preview_test.gointernal/app/commands_load_test.gointernal/app/commands_security.gointernal/app/commands_security_test.gointernal/app/commands_yaml.gointernal/app/cursor_test.gointernal/app/messages_security.gointernal/app/security_availability_cache.gointernal/app/security_availability_cache_test.gointernal/app/security_hook.gointernal/app/security_hook_test.gointernal/app/security_ignores.gointernal/app/security_ignores_test.gointernal/app/security_navigation.gointernal/app/security_navigation_test.gointernal/app/security_refresh.gointernal/app/security_source_entries.gointernal/app/security_source_entries_test.gointernal/app/security_state.gointernal/app/tabs.gointernal/app/tabs_test.gointernal/app/update.gointernal/app/update_actions.gointernal/app/update_actions_default.gointernal/app/update_actions_security.gointernal/app/update_actions_security_menu.gointernal/app/update_actions_security_test.gointernal/app/update_actions_test.gointernal/app/update_bookmarks_session.gointernal/app/update_keys_actions.gointernal/app/update_keys_actions_test.gointernal/app/update_navigation.gointernal/app/update_navigation_security_jump_test.gointernal/app/view.gointernal/app/view_right.gointernal/app/view_status.gointernal/app/view_status_test.gointernal/app/virtual_kinds.gointernal/k8s/client.gointernal/k8s/client_operations.gointernal/k8s/client_resources.gointernal/k8s/security.gointernal/k8s/security_group.gointernal/k8s/security_group_test.gointernal/k8s/security_test.gointernal/model/actions.gointernal/model/metadata.gointernal/model/sidebar.gointernal/model/sidebar_test.gointernal/model/types.gointernal/model/types_test.gointernal/security/falco/falco.gointernal/security/falco/falco_test.gointernal/security/falco/parse.gointernal/security/gatekeeper/gatekeeper.gointernal/security/gatekeeper/gatekeeper_test.gointernal/security/heuristic/checks.gointernal/security/heuristic/fetch_test.gointernal/security/heuristic/heuristic.gointernal/security/heuristic/heuristic_test.gointernal/security/kubescape/kubescape.gointernal/security/kubescape/kubescape_test.gointernal/security/kubescape/parse.gointernal/security/manager.gointernal/security/manager_test.gointernal/security/pagination.gointernal/security/pagination_test.gointernal/security/policyreport/parse.gointernal/security/policyreport/policyreport.gointernal/security/policyreport/policyreport_test.gointernal/security/source.gointernal/security/source_test.gointernal/security/testing.gointernal/security/trivyop/parse.gointernal/security/trivyop/trivyop.gointernal/security/trivyop/trivyop_test.gointernal/ui/config_keybindings.go
✅ Files skipped from review due to trivial changes (8)
- internal/model/types_test.go
- internal/app/security_state.go
- internal/app/virtual_kinds.go
- internal/app/update_actions_security_test.go
- internal/app/app_init.go
- internal/app/security_navigation.go
- internal/app/app.go
- internal/security/heuristic/checks.go
🚧 Files skipped from review as they are similar to previous changes (62)
- internal/app/commands_yaml.go
- internal/app/update_actions_default.go
- internal/app/security_availability_cache.go
- internal/k8s/client_resources.go
- internal/app/view.go
- internal/app/update.go
- internal/app/security_source_entries.go
- internal/app/commands_load.go
- internal/app/update_bookmarks_session.go
- internal/app/messages_security.go
- internal/security/source_test.go
- internal/app/update_actions_test.go
- internal/app/view_status_test.go
- internal/model/types.go
- internal/app/commands_load_test.go
- internal/security/source.go
- internal/app/commands_load_preview_test.go
- internal/app/update_keys_actions.go
- internal/security/pagination.go
- internal/model/actions.go
- internal/app/view_status.go
- internal/app/commands_load_preview.go
- internal/app/security_source_entries_test.go
- internal/k8s/client_operations.go
- internal/k8s/client.go
- internal/security/testing.go
- internal/security/pagination_test.go
- internal/app/cursor_test.go
- internal/security/falco/falco_test.go
- internal/app/view_right.go
- internal/app/security_navigation_test.go
- internal/app/update_navigation_security_jump_test.go
- internal/app/update_actions_security.go
- internal/app/commands_coverage_test.go
- internal/model/sidebar_test.go
- internal/security/trivyop/trivyop.go
- internal/security/kubescape/parse.go
- internal/security/policyreport/policyreport.go
- internal/app/tabs_test.go
- internal/security/policyreport/policyreport_test.go
- internal/app/commands_security.go
- internal/security/trivyop/parse.go
- internal/k8s/security_group.go
- internal/model/sidebar.go
- internal/security/kubescape/kubescape.go
- internal/security/gatekeeper/gatekeeper.go
- internal/security/heuristic/heuristic.go
- internal/k8s/security_group_test.go
- internal/app/security_hook.go
- internal/k8s/security_test.go
- internal/app/security_ignores.go
- internal/security/falco/parse.go
- internal/security/policyreport/parse.go
- internal/security/falco/falco.go
- internal/app/update_navigation.go
- internal/security/heuristic/heuristic_test.go
- internal/security/manager_test.go
- internal/app/commands_security_test.go
- internal/security/trivyop/trivyop_test.go
- internal/app/security_ignores_test.go
- internal/security/manager.go
- internal/ui/config_keybindings.go
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
1 participant
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.
Summary
A top-to-bottom security findings feature for
lfk, plus thecontrol-plane-friendliness fixes that make it safe to point at busy
clusters.
Dashboard (commit
6de0931)Manager:heuristic (privileged-pod-style misconfigurations, no CRDs needed),
falco (daemon log tail), trivy-operator
(VulnerabilityReport + ConfigAuditReport), policy-report (Kyverno
PolicyReport / ClusterPolicyReport), gatekeeper (Constraints
discovered dynamically), kubescape (WorkloadConfigurationScans).
FindingIndex aggregating severity counts.
actually-installed sources via a per-host on-disk availability
cache mirroring kubectl's
$KUBECACHEDIR/discovery/<host>/.Enter jumps to the underlying real Kubernetes object.
per-cluster
SecurityIgnoreStatepersisted to YAML.group, showIgnored) on
TabStateso two tabs on different clustersdon't bleed sources via the global
SecuritySourcesFnhook.Diff, copy/export, navigateChild) gates on
isVirtualResourceKind/
onSecurityViewso users no longer tripWarning: unknown resource type: __security_*.Control-plane-friendliness fixes (commit
6917779)The original fan-out pattern (one large list per source, all sources
parallel, no pagination, all-errored fetches re-fired on every nav)
froze a control plane in testing — 746 PolicyReports + 442
VulnerabilityReports + 524 ConfigAuditReports all in flight at once.
/ gatekeeper Fetch via a
ListPaginatedhelper around client-go'stools/pager. HTTP/2 streams stop collapsing on big lists; etcdno longer materialises the full set in memory.
SetMaxFetchConcurrency) on simultaneoussource Fetches — a 6-source cluster fans out 2-at-a-time. Peak
control-plane load drops ~3x.
SetAvailability) letsFetchAllskip its own
IsAvailableprobe when the app already paid thatcost — halves API request volume per navigation tick.
errorTTL) for all-errored fetches so athrottled cluster doesn't re-fire the same failing list calls on
every navigation tick.
to query" instead of an error — webhook-installed-but-no-templates
no longer spams the log per fetch.
Test plan
go test -race ./...(all packages green)make lint(0 issues)error propagation, cancellation)
Gatekeeper + Kubescape installed), confirm sidebar populates,
drill into each source, navigate between sources, verify
no control-plane stress
cache without removing previously-known sources on a transient
error
security state without leaking sources between them