dns: unify dns eve object; add additionals section - v5 #11305
Conversation
Feature: 7011 Add additionals to DNSMessage struct. Add parsing logic to populate additional section data. Patch dns tests to account for additional section parsing.
Feature: 7011 dns_log_json_answer: log additional section records. update schema.json with new "additionals" section.
Feature: 7017 Add DNSRDataOPT struct and DNSRData enum type OPT. Add OPT parsing function and test function. Add DNSRData OPT type to lua.rs match. Log OPT rdata.
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
- answer object - top level rrname and rrtype - header level details in answers Bug: OISF#6281
While not normally in seen in a DNS query, there is nothing preventing a query from containing other data sections. And it is normal in some types of requests like a zone update, and very common in mDNS. So log all sections even for requests.
jasonish
requested review from
jufajardini,
victorjulien and
a team
as code owners
|
NOTE: This PR may contain new authors. |
| "flags": "100", | ||
| "rd": true, |
There was a problem hiding this comment.
I decided to not change the flags within the scope of this PR.
I'd like to further review "default" and "extended" mode for DNS as a separate task, but before 8.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11305 +/- ##
==========================================
+ Coverage 82.45% 82.48% +0.02%
==========================================
Files 961 961
Lines 251710 251798 +88
==========================================
+ Hits 207552 207683 +131
+ Misses 44158 44115 -43
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 21085 |
|
I think it needs a rebase to get green CI |
There was a problem hiding this comment.
Thanks for the work :-)
- CI : 🟠 needs a rebase
- Code : I should review
- Commits segmentation : good for me
- Commit messages :
dns parsing: add additional section->dns: parse add additional section? - Git ID set : looks fine for me
- CLA : 🟡 I cannot check for Nathan
- Doc update : nice, I wonder if we should communicate about this breaking change
- Redmine tickets : ok, does this also solve https://redmine.openinfosecfoundation.org/issues/6400 ?
Should you also handle https://redmine.openinfosecfoundation.org/issues/6098 ?
And https://redmine.openinfosecfoundation.org/issues/6073 ? - Rustfmt : looks good
- Tests : I will comment on SV
- Dependencies added: none
|
New format makes sense to me. I do think we need to keep supporting v:2 for 8, then remove it in 9. In alerts: use v:3 |
Fixed. |
There was a problem hiding this comment.
Documentation-wise, I really like the log output comparison that goes along with the upgrading note.
Should we also update https://docs.suricata.io/en/latest/output/eve/eve-json-format.html#event-type-dns ?
| Suricata 8.0 modifies the DNS logging in ``dns`` and ``alert`` records | ||
| to a version ``3`` logging format. These changes address a lack of | ||
| fidelity in alerts for DNS responses, as well as unify the format of | ||
| the ``dns`` object accross ``dns`` and ``alert`` objects. |
|
#11433 allows a user to keep v2 for dns records. |
Fixed in the new PR: #11439 |
Previous PR: #11283
https://redmine.openinfosecfoundation.org/issues/7011
https://redmine.openinfosecfoundation.org/issues/7017
https://redmine.openinfosecfoundation.org/issues/6281
Changes from last PR:
SV_BRANCH=OISF/suricata-verify#1916