dns: log queries as an array - v3 #11283
Conversation
If multiple queries exist in a DNS request, Suricata would log a discrete DNS event for each request. However, in an alert on a DNS request, the DNS object would contain an array of query objects. This brings the "dns" object into alignment with respect to DNS requests in alerts and "dns" records. This introduces a new function for logging DNS messages that is common for requests and responses which should reduce code, but for this commit is only used for requests, so doesn't cover responses yet. As this is a breaking change, rename the "query" array in alerts to "queries" to be more in alignment with how we log "answers" and "authorities". Note that this is a breaking change. Bug: OISF#6281
By using the same function for logging DNS requests and responses we can better ensure that the format stays consistent between. Bug: OISF#6281
jasonish
requested review from
jufajardini,
victorjulien and
a team
as code owners
| "queries": [ | ||
| { | ||
| "rrname": "www.suricata.io", | ||
| "rrtype": "A" | ||
| } | ||
| ] |
There was a problem hiding this comment.
This format is critical for mDNS as well.
There was a problem hiding this comment.
Elasticsearch is pretty good with arrays like this, you can search on dns.queries.rrname. So it means searches/reports, etc will need to change from dns.rrname to dns.queries.rrname.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11283 +/- ##
==========================================
+ Coverage 82.45% 82.47% +0.01%
==========================================
Files 961 961
Lines 251710 251662 -48
==========================================
Hits 207552 207552
+ Misses 44158 44110 -48
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 21035 |
|
Looks a nice clean up, thanks for taking on this |
| "version": 2, | ||
| "type": "answer", | ||
| "id": 0, | ||
| "flags": "8180", |
There was a problem hiding this comment.
Any thoughts on if flags would look better as:
"flags": ["qr", "rd", "ra"],
There was a problem hiding this comment.
Looks better as ["qr", "rd", "ra"], than 8180 as a string ?
There was a problem hiding this comment.
We'd probably want to keep the raw value (8180 in this case). The array would replace "rd": true and friends.
There was a problem hiding this comment.
How would that affect things like elastic and splunk wrt querying for them?
There was a problem hiding this comment.
Currently you'd have to do something like:
dns.flags.rd:true dns.flags.z:true
with this change:
dns.flags:rd dns.flags:z
Previous PR: #11238
Changes from previous PR:
Ticket: https://redmine.openinfosecfoundation.org/issues/6281
SV_BRANCH=OISF/suricata-verify#1907