Repository navigation
Conversation
The DNS tracer parsed every packet but forwarded only responses with at
least one answer, so a query that gets NXDOMAIN or no reply at all (the
shape of one-way DNS exfiltration) never reached a rule.
Behind the new dnsRuleForwarding flag (default off, behaviour unchanged
when off) the tracer also forwards queries and error responses
(NXDOMAIN, SERVFAIL, REFUSED), marked rule-only:
- rule-only events reach the rule manager only, not the DNS manager
(which would issue its own lookup), the network stream, metrics,
profile learning or third-party receivers
- they match rule expressions declared as eventType "dnsQuery", so the
existing "dns" rules (R0005 in particular) keep seeing answered
responses only and do not fire on ndots search-suffix churn; older
agents load "dnsQuery" expressions harmlessly
- NODATA responses and names under .cluster.local., .in-addr.arpa.,
.ip6.arpa. and configured suffixes are dropped at the tracer, before
enqueue and enrichment
- rule-only events are deduplicated for 5s on (mntns, pid, name, qr,
qtype); answered responses are still never deduplicated
- new CEL fields on DNS events: qtype, qr, numAnswers, rcode (responses
only, has() is false on a query)
- alerts on rule-only events carry qr, qtype, numAnswers, rcode and
nameserver in their arguments
- the tracer now classifies the packet in place and copies only what it
forwards, removing a DeepCopy per dropped packet
- counter node_agent.dns.rule_forwarding.total{outcome=...}
Docs: docs/features/dns-query-rule-forwarding.md, docs/CONFIGURATION.md.
Signed-off-by: Ben <ben@armosec.io>
slashben
added
the
ai-assisted
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 36.96% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 46 functions across 18 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary change: forwarding DNS queries and error responses to rule evaluation. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 36.96% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 46 functions across 18 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Request changes for one confirmed detection false negative.
Reviewed head 2eafeaac96b68faef5aa13d1e5c68372ee4119e6 against main at c7a73937cb2af722c5fce3c9e6599145ea071036 (20 files). The feature is needed: the target tracer drops every query and zero-answer response. Opt-in forwarding addresses that gap, preserves answered-response consumers, and keeps existing dns rules isolated through dnsQuery. Independent correctness and architecture reviews found routing, adapter selection, copied-packet ownership, and release paths sound.
Blocker (P2): the DNS dedup key omits rcode, so a distinct matching error response can disappear before evaluation; see inline finding. An isolated regression reproduces SERVFAIL followed by NXDOMAIN with the same pid/name/type: expected one NameError event at the rule manager, actual zero. Adding rcode to the key in a disposable validation copy makes the regression pass while identical-response dedup still passes. No source-branch changes were made.
Nonblocking: new non-DNS CEL qr returns Q on DatasourceEvent, contrary to the documented empty default; the test covers StructEvent only. Guard non-DNS accessors and add a datasource test when convenient.
History: #247 intentionally filtered unresolved DNS for existing consumers; rule-only dispatch preserves that boundary. #781 removed unhelpful DNS dedup; answered responses remain exempt here. Closed #356 changed old rules to query-only; it had an approval and no stated closure rationale, so closure is not evidence of rejection. #846 was explicitly closed because #806 merged; those address DNS wildcard matching, not forwarding. Open #875 supplies correlation state, a complementary capability. No duplicate forwarding change found.
Searches covered open/closed/merged PRs and issues using DNS, dns.go, NXDOMAIN, exfiltration, dnsQuery, ComputeDNSKey, and forwarding/rule-only variants, capped at 100 results per query; broad rule-only search reached the cap. GitHub text search does not exhaust diffs or private ticket history.
Validation: credential-free, network-isolated bubblewrap execution: full tests for config, dedupcache, utils/..., rulemanager/..., containerwatcher/v2 and tracers passed, excluding gadget *Fields tests (missing tracers.tar). Targeted new DNS tests, go vet for those packages, gofmt and diff --check passed. All 39 current GitHub checks succeeded. No live kernel/eBPF or enabled-feature load test was run.
| // parallel A/AAAA retries repeat them within seconds. | ||
| case utils.DnsEventType: | ||
| if e, ok := event.(utils.DNSEvent); ok && utils.IsRuleOnlyEvent(event) { | ||
| return dedupcache.ComputeDNSKey(mntns, e.GetPID(), e.GetDNSName(), string(e.GetQr()), e.GetQtype()), dedupTTLDNS, true |
There was a problem hiding this comment.
[P2] Preserve distinct DNS response codes in the dedup key. With forwarding and default dedup enabled, a process receiving SERVFAIL and then NXDOMAIN for the same name/qtype within five seconds gets identical keys. The second response is discarded before rule evaluation, so has(event.rcode) && event.rcode == 'NameError' never sees its matching event. This can occur when retrying another configured resolver. I reproduced this using the real dispatch fixture and DedupCache: zero NameError events reached the rule manager; including rcode in a disposable key experiment restored delivery. Add rcode to ComputeDNSKey and cover SERVFAIL → NXDOMAIN, alongside repeated-identical-response dedup.
There was a problem hiding this comment.
Fixed in 4064b5b: rcode is now part of ComputeDNSKey (empty on queries, so the query key is unchanged). Added TestEventHandlerFactory_DNSDedupKeepsDistinctResponseCodes on the dispatch fixture (SERVFAIL then NXDOMAIN both delivered, a repeated NXDOMAIN absorbed), TestComputeDNSKey_ResponseCodesAreDistinct, and the rcode variant in the key test. The nonblocking qr point is in the same commit: DatasourceEvent.GetQr now returns empty for non-DNS events like GetQtype and GetRcode, with a datasource-backed exec case in TestCEL_DNSFields_NonDNSEventIsEmpty. Feature doc updated.
…on-DNS events A SERVFAIL from the first configured resolver and the NXDOMAIN from the retry share name, type and qr, so the NXDOMAIN was marked Duplicate and a rule gated on event.rcode never saw it. rcode is now part of ComputeDNSKey; identical responses still collapse. DatasourceEvent.GetQr returned Q for every non-DNS event, contrary to the documented empty value; it now checks the event type like GetQtype and GetRcode, and the CEL test covers a datasource-backed exec event. Signed-off-by: Ben Hirschberg <ben@armosec.io> Signed-off-by: Ben <ben@armosec.io>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Approved after re-reviewing 4064b5bdeedfa8f5ed62d31e0307ae20aa84f14a against main at c7a73937cb2af722c5fce3c9e6599145ea071036.
The previous P2 blocker is resolved: DNS dedup now includes rcode, so SERVFAIL followed by NXDOMAIN reaches rule evaluation as two distinct responses while repeated identical responses remain deduplicated. My original failing reproduction now passes on this head. The author added both dispatch-level and key-level regression tests. The nonblocking non-DNS qr default is also corrected and covered with a datasource-backed CEL test.
Reviewed all seven follow-up files with independent correctness and architecture checks. No remaining material blockers found. The opt-in forwarding feature remains needed, existing answered-DNS consumers/rules remain isolated, and routing and packet ownership remain sound. Related history and bounded search limitations are documented in the initial review; refreshed dnsRuleForwarding/NXDOMAIN searches found no superseding change.
Fresh credential-free, network-isolated validation passed: go test ./pkg/config ./pkg/dedupcache ./pkg/utils/... ./pkg/rulemanager/... ./pkg/containerwatcher/v2 ./pkg/containerwatcher/v2/tracers -skip 'Fields$' -count=1, the original reproduction and new targeted regressions, go vet for those packages, gofmt, and diff --check. All 39 current GitHub checks succeeded. Gadget *Fields tests remain excluded because tracers.tar is absent; live eBPF and enabled-feature load testing were not performed. No source branch changes were made.
This branch has not been deployed
2 participants
Summary
The DNS tracer parses every DNS packet but forwarded only responses with at least one answer, so a query that gets NXDOMAIN or no reply at all (the shape of one-way DNS exfiltration) never reached a rule. This adds an opt-in flag,
dnsRuleForwarding, that forwards queries and error responses to the rule manager only, exposesqtype/qr/numAnswers/rcodeto CEL, and keeps existingdnsrules untouched via adnsQueryopt-in expression type. Flag off is behaviour-identical to today.Ticket
SUB-9110
Changes
pkg/containerwatcher/v2/tracers/dns.go:WithDNSRuleForwardingoption, pureclassifyDNS(answered → as before; query / NXDOMAIN / SERVFAIL / REFUSED → forwarded rule-only; NODATA, empty names and.cluster.local./.in-addr.arpa./.ip6.arpa./ configured suffixes → dropped at the tracer, before enqueue and enrichment). The subscribe callback now classifies in place andDeepCopys only forwarded packets, removing a copy per dropped packet. Counternode_agent.dns.rule_forwarding.total{outcome}.pkg/utils:RuleOnlyEventmarker onDatasourceEventandStructEvent;RuleEventType()returns the aliasdnsQueryfor rule-only DNS events;DNSEvent.GetQtype/GetRcode; CEL fieldsqtype,qr,numAnswers,rcode(has(event.rcode)false on a query).pkg/containerwatcher/v2/event_handler_factory.go:ruleOnlyHandlerslist (dns→ rule manager only), rule-only events skip third-party receivers and are dropped when duplicate; DNS dedup case for rule-only events only (5s, key includes pid, qr, qtype). Answered responses are never deduplicated, as before.pkg/rulemanager: rule expressions are selected byutils.RuleEventTypeinReportEnrichedEventandCEL.EvaluateRule; eval-contexteventTypestaysdns.pkg/rulemanager/ruleadapters/adapters/dns.go: rule-only alerts carryqr,qtype,numAnswers,rcode,nameserverarguments; answered-response alerts unchanged.pkg/config:DNSRuleForwardingConfig{Enabled, ExcludedSuffixes}, default off, suffixes normalized on load; only wired whenruntimeDetectionEnabled.docs/features/dns-query-rule-forwarding.md,docs/CONFIGURATION.md.Why the
dnsQueryaliasAbout nine rules are written for
eventType: "dns"on the assumption the event is an answered response. With ndots:5,api.github.com.ec2.internal.and friends are queried on every external lookup and never enter the learned egress baseline, so routing queries todnsexpressions would fire R0005 in every pod. A guard such asevent.qr == 'R'cannot be retrofitted either: an older agent disables an expression that references a field it does not know. Rules opt in by adding adnsQueryexpression; older agents load it and never match it.Not in this PR
nodeAgent.config.dnsRuleForwarding); until then use thenodeAgent.config.extrapassthrough.Testing
go test ./pkg/rulemanager/... ./pkg/containerwatcher/v2/ ./pkg/utils/... ./pkg/dedupcache/ ./pkg/config/→ 1030 passed.go test ./pkg/containerwatcher/v2/tracers/ -skip 'Fields$'→ 96 passed (the*Fieldsgadget tests needtracers.tar, absent locally, and fail identically on main). New tests:TestClassifyDNS(flag on/off × packet kinds × suffixes, plus a real datasource packet),TestEventHandlerFactory_DNS*(rule-only reaches the rule manager only; answered reaches all four handlers and third-party; dedup marks only rule-only events),TestCEL_DNS*(fields,has(event.rcode), alias routing both directions),TestDnsAdapter_*,TestLoadConfig_DNSRuleForwarding*, updatedComputeDNSKeytests.go vetclean.Docs
docs/features/dns-query-rule-forwarding.md(new),docs/CONFIGURATION.md(DNS Rule Forwarding section).AI Context
AI-skills: armosec-shared-rules:agent-dispatch-policy | cmds: /model
Summary by CodeRabbit
dnsQueryevent type.