Allow alert from unexpected Ports, allow Port=0 as intentional wildcard - #905
Allow alert from unexpected Ports, allow Port=0 as intentional wildcard#905entlein wants to merge 3 commits into
Conversation
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed the port/protocol projection and selector-matching changes. The mechanics (AddrPortGroup matching, PeerSelector namespace/label matching, wildcard-zero-port semantics) look correct and are well covered by the new unit/component tests. Left two inline comments:
- The R0011 default-rule change from
!net.is_private_ip(...)to only excluding127.is a significant scope increase (alerts on all private/internal egress now) — this is the exact question the PR description itself raises as unresolved, so it reads as a blocker for taking this out of draft, plus IPv6 loopback isn't covered by the new check. - A
Port: 0wildcard entry mixed with other protocols' specific ports in the same neighbor collapses the whole entry to fully open (all protocols/ports), which may be broader than intended.
Not approving yet since this is still a draft and the PR body flags an open design question — happy to re-review once that's settled.
| ruleExpression: | ||
| - eventType: "network" | ||
| expression: "event.pktType == 'OUTGOING' && !net.is_private_ip(event.dstAddr) && !cp.was_address_in_egress(event.containerId, event.dstAddr)" | ||
| expression: "event.pktType == 'OUTGOING' && !event.dstAddr.startsWith('127.') && !cp.was_address_port_protocol_in_egress(event.containerId, event.dstAddr, event.dstPort, event.proto)" |
There was a problem hiding this comment.
Blocker (already flagged by the PR author): dropping !net.is_private_ip(event.dstAddr) in favor of only excluding 127. is a big scope change for R0011 — it now alerts on egress to any private/internal address (other pods, cluster services, node IPs, etc.) unless it's allowlisted with the right port/protocol. The test fixture changes in this PR (clusterDNS, kube-api entries needed just to keep DNS/kube-api egress quiet) are a preview of how much extra noise this adds to any profile that doesn't explicitly enumerate its internal peers. Given the PR description itself says "Still need to talk to myself if this is a good default", this should be resolved (or explicitly called out as a follow-up decision with default rules left conservative) before this leaves draft.
Separately: startsWith('127.') only covers IPv4 loopback. IPv6 loopback (::1) isn't excluded, whereas the previous net.is_private_ip presumably handled both families.
| ports := make(map[string]struct{}, len(n.Ports)) | ||
| wildcard := len(n.Ports) == 0 | ||
| for _, p := range n.Ports { | ||
| if p.Port == nil || *p.Port == 0 { | ||
| wildcard = true | ||
| continue |
There was a problem hiding this comment.
wildcard is computed once per neighbor entry, not per protocol/port. If a single NetworkNeighbor mixes a Port: 0 entry for one protocol with specific ports for another (e.g. {TCP, 0} + {UDP, 53}), the if wildcard { ports = nil } below drops all of that entry's ports — so the address becomes open on both TCP and UDP, any port, even though UDP was explicitly restricted to 53. Worth either scoping the wildcard to its own protocol (e.g. map[string]struct{} keyed loosely, or drop only matching-protocol port entries) or documenting that mixing a Port: 0 entry with other protocols' specific ports in the same neighbor isn't supported and will fully open that address.
There was a problem hiding this comment.
Very true,
The easiest solution to this might be to backtrack and treat the null as the wildcard (i.e. if the port is not specified, it is treated as ANY) . Thats however not consistent with the narrative of the profiles.
Guess, its a good point in time, to review all our settings for what they mean is absent, null, wrong, or wildcarded.
So, the base premise is: if it aint explicitely listed, it aint allowed, so it'll alert
Oh dear oh dear 🤣😅,That ll break things, so maybe, this mantra needs the extension unless its a non-mandatory field , in which case the absence of the entry means ANYTHING goes.
(I think, I just found another bug (beyond this one), darn)... THANKS FOR THE REVIEW!
EDIT: bug=logic bug, not code
…rt if delcared and violated Signed-off-by: entlein <einentlein@gmail.com>
| ruleExpression: | ||
| - eventType: "network" | ||
| expression: "event.pktType == 'OUTGOING' && !net.is_private_ip(event.dstAddr) && !cp.was_address_in_egress(event.containerId, event.dstAddr)" | ||
| expression: "event.pktType == 'OUTGOING' && !event.dstAddr.startsWith('127.') && !cp.was_address_port_protocol_in_egress(event.containerId, event.dstAddr, event.dstPort, event.proto)" |
There was a problem hiding this comment.
Still an open blocker, unchanged since my earlier comment: this line now alerts R0011 on egress to any private/internal address, not just external non-loopback traffic — a real scope increase from the old !net.is_private_ip(...) guard, and IPv6 loopback (::1) still isn't excluded. The PR description still says "still need to talk to myself if this is a good default", so this reads as an intentional open question rather than an oversight — but it should be resolved (either accepted explicitly, or the default kept conservative) before this leaves draft.
| return false | ||
| } | ||
|
|
||
| func (l *containerProfileNetworkLibrary) wasSelectorInIngress(containerID, namespace, podLabels ref.Val) ref.Val { |
There was a problem hiding this comment.
was_selector_in_ingress/was_selector_in_egress (plus the new event.dstNamespace/event.dstPodLabels fields in pkg/utils/cel.go) aren't referenced by any rule in tests/chart/templates/node-agent/default-rules.yaml on this branch — there's no R0012 or similar consuming them yet. The implementation and tests (selector_test.go, selector_compile_test.go) look solid, but as it stands this is dead code shipped ahead of its consumer. Worth calling out explicitly as groundwork for a follow-up rule (or landing it together with that rule) so it doesn't read as an accidental leftover.
There was a problem hiding this comment.
How do you stack a PR on this upstream-remote? This PR is assumed stacked on #902 , so thats why it looks dead , but I m too dumb to find the gh button to make it stack, so you see the incremenetal diff (it works in my own repo).
I m tracking the various inconsistencies here for now k8sstormcenter#91 ( I think the AI didnt report them all correctly, but I need to retest if that s actually what the code now does)
There was a problem hiding this comment.
ok got you, I don't think we can stack PRs on external branches, do you want me to merge #902 ?
2 participants
Still need to talk to myself if
!event.dstAddr.startsWith('127.')is a good default, cause a lot of attacks to come from 127.0.0.1 .Thoughts are: here in the test suite we exclude 127 , as port-fwds or proxies often cause noise during testing
But we exclude it in the rulelibrary default -> this needs a good set of realistic apps on clusters to decide.
This PR is assumed stacked onto #902
Why I think we should have the code (w/o defaults discussion)
We are carrying the port/protocol stanza pretty much everywhere, but the ports no matching is currently a dud (it doesnt alert).
If we find that it doesnt impact performance, it would align better with the expectations (if there is a stanza in a profile, it should either do something or be removed)