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 v2.1 #10040

Closed
wants to merge 6 commits into from

Conversation

catenacyber
Copy link
Contributor

@catenacyber catenacyber commented Dec 12, 2023

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

Describe changes:

  • analyze DNS over HTTP2
SV_BRANCH=pr/1540

OISF/suricata-verify#1540

Draft to get feedback about approach...
Leaving comments on the code for specific questions

TODO :

  • Fix dns json schema and logging first (CI will be red)
  • Use config for dns logging...
  • Merge Output alert applayer v17.1 #9870 so as not to have a short-lived output-json-doh2.c
  • need to check (ok let's be honest, /check/fix/) behavior when DOH2 is disabled

@jasonish why is DNS not logging the same thing for alerts and dns events ?
That is why do we log multiple dns events for a single packet having multiple queries, and one alert will have an array in .dns.query with the same data
Same goes for answers and it turns out the schema is incomplete because everything in dns needs to be put in ./dns/answer like authorities and grouped

Functionnaly, in terms of output :

  • The flow will have doh2 as app_proto (and http2 as app_proto_orig)
  • There are doh2 events that have both http2 and dns fields. dns logging is done like alerts, not like dns events...
  • DNS and HTTP and HTTP2 signatures work on DOH2
  • Signatures can be DOH2 specific. These signatures can combine http, http2 and dns keywords.
  • DNS Frames do not work on DoH2

Memory management

  • a HTTP2 tx can own 2 DNS tx (one to client, one to server)
  • a HTTP2 tx stores the streamed DNS response content until it is complete before parsing it (limiting to U16_MAX bytes)

API

  • There is a new DOH2 app-layer protocol which resorts to HTTP2 for most of the things
  • HTTP2 parsing discovering DOH2 changes the app_proto to DOH2, doh2 can be enabled or disabled in suricata.yaml config
  • There are more alproto "comparison" functions
  • Every DNS keyword needs to check the flow alproto to change the transaction
  • Every DNS/HTTP keyword is automatically registered for DOH2 (hacking DetectAppLayerMpmRegister2...)

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
@jasonish
Copy link
Member

@jasonish why is DNS not logging the same thing for alerts and dns events ?
That is why do we log multiple dns events for a single packet having multiple queries, and one alert will have an array in .dns.query with the same data
Same goes for answers and it turns out the schema is incomplete because everything in dns needs to be put in ./dns/answer like authorities and grouped

https://redmine.openinfosecfoundation.org/issues/6281

It appears I started this but then got sidetracked.

return (alproto == ALPROTO_DOH2) || (alproto == ALPROTO_DNS);
case ALPROTO_HTTP2:
return (alproto == ALPROTO_DOH2) || (alproto == ALPROTO_HTTP2);
case ALPROTO_DOH2:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to check if this case is needed

@jasonish
Copy link
Member

@jasonish why is DNS not logging the same thing for alerts and dns events ?
That is why do we log multiple dns events for a single packet having multiple queries, and one alert will have an array in .dns.query with the same data
Same goes for answers and it turns out the schema is incomplete because everything in dns needs to be put in ./dns/answer like authorities and grouped

https://redmine.openinfosecfoundation.org/issues/6281

It appears I started this but then got sidetracked.

Ah, yes. I'm waiting for this to be approved #9920, as I want to build on the unification of request and response into a simple DNS message struct.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 17024

@victorjulien victorjulien self-assigned this Dec 14, 2023
@catenacyber catenacyber added the needs rebase Needs rebase to master label Dec 19, 2023
@catenacyber
Copy link
Contributor Author

https://redmine.openinfosecfoundation.org/issues/6281
It appears I started this but then got sidetracked.

Ah, yes. I'm waiting for this to be approved #9920, as I want to build on the unification of request and response into a simple DNS message struct.

So #9920 was merged as #10045 and now you can work on https://redmine.openinfosecfoundation.org/issues/6281 before I fix this PR by using the fixed logging for dns over HTTP2.

Am I understanding correctly @jasonish ?

@jasonish
Copy link
Member

https://redmine.openinfosecfoundation.org/issues/6281
It appears I started this but then got sidetracked.

Ah, yes. I'm waiting for this to be approved #9920, as I want to build on the unification of request and response into a simple DNS message struct.

So #9920 was merged as #10045 and now you can work on https://redmine.openinfosecfoundation.org/issues/6281 before I fix this PR by using the fixed logging for dns over HTTP2.

Am I understanding correctly @jasonish ?

Yes, but it might take a bit longer than just the code. There is some discussion over adding a new version to the DNS objects, or do we just tack on the Suricata version and let downstream alert consumers deal with it with no compatibility option.

#9620 (comment)

@catenacyber
Copy link
Contributor Author

Continued in #10114

@catenacyber catenacyber closed this Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
4 participants