Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dns over http2 5773 v16 #11461

Closed
wants to merge 9 commits into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5773

Describe changes:

  • analyze DNS over HTTP2

SV_BRANCH=OISF/suricata-verify#1970

#11428 with DNS v3 logging, no longer a draft, cf last commit

Should there be a squash up of commits ?

@jasonish here for a DOH2 tx, we log a bidirectional HTTP2 transaction, and then if any, a DNS transaction, preferring the answer... What do you think about it ? This allows to keep the same format as for regular dns.
Another option would be to log two doh2 events : one for the DNS request and one for the DNS answer, with HTTP2 getting logged twice... not sure how it would work out for alerts...

by making tx parsing and creation more easily available,
without needing a dns state.

Dns event NotResponse is now set on the right tx, and not the one
before.

Also debug log for Z-flag on request says "request" instead of
"response"

Also rustfmt dns.rs
Now a flow alproto can be changed by a call to AppLayerParserParse
when HTTP2 forces the flow to turn into DOH2.
Ticket: 5773

Handles both directions the same way for data if content type is
application/dns-message
So as to consume less memory for HTTP2Transaction
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 92.80000% with 27 lines in your changes missing coverage. Please review.

Project coverage is 82.51%. Comparing base (090079c) to head (6e9bb35).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11461      +/-   ##
==========================================
- Coverage   82.52%   82.51%   -0.02%     
==========================================
  Files         938      938              
  Lines      248297   248533     +236     
==========================================
+ Hits       204917   205077     +160     
- Misses      43380    43456      +76     
Flag Coverage Δ
fuzzcorpus 60.54% <64.26%> (+0.09%) ⬆️
livemode 18.68% <6.13%> (-0.02%) ⬇️
pcap 43.90% <79.73%> (+0.12%) ⬆️
suricata-verify 61.63% <85.06%> (+0.09%) ⬆️
unittests 59.39% <33.33%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21444

@jasonish
Copy link
Member

@jasonish here for a DOH2 tx, we log a bidirectional HTTP2 transaction, and then if any, a DNS transaction, preferring the answer... What do you think about it ? This allows to keep the same format as for regular dns.

Probably fine, as these are doh events, not dns event. Some thoughts tho:

  • will there be a doh3 at some point and so on?
  • DNS is valuable metadata, I wonder if spreading it over multiple event types will limit its usefulness. My head already hurts thinking about creating DNS reports that cover tradition DNS and DoH.

@victorjulien
Copy link
Member

In dcerpc, we don't care if it comes over smb or udp/tcp directly, right? I feel dns records shouldn't be different regardless of their transport. It should be recognizable somehow as Dns over HTTP/2, Dns over HTTP, etc still, but that can be done in various ways.

@catenacyber
Copy link
Contributor Author

So, you would want dns event instead of doh2
We know it is doh2 because this event has http object in it

@victorjulien
Copy link
Member

So, you would want dns event instead of doh2 We know it is doh2 because this event has http object in it

Yeah that sounds good.

@catenacyber
Copy link
Contributor Author

Thanks

Continued in #11498

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants