Conversation
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
Ticket: 5773
Ticket: 5773
Ticket: 5773
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
catenacyber
requested review from
a team,
jasonish and
victorjulien
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11533 +/- ##
==========================================
- Coverage 82.54% 82.51% -0.03%
==========================================
Files 923 923
Lines 248460 248696 +236
==========================================
+ Hits 205083 205209 +126
- Misses 43377 43487 +110
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 21626 |
victorjulien
left a comment
victorjulien
left a comment
There was a problem hiding this comment.
Some nits and minor questions inline.
| if unsafe { ALPROTO_DOH2 } != ALPROTO_UNKNOWN { | ||
| // we store DNS response, and process it when complete | ||
| if let Some(doh) = &mut self.doh { | ||
| if doh.is_doh_data[dir.index()] && doh.data_buf[dir.index()].len() < 0xFFFF { |
There was a problem hiding this comment.
should there be an event if we exceed this?
| } | ||
|
|
||
| // doh2 is just http2 wrapped in another name | ||
| parser.name = b"doh2\0".as_ptr() as *const std::os::raw::c_char; |
There was a problem hiding this comment.
should there be a check that http2 is enabled?
| ssh: | ||
| enabled: yes | ||
| #hassh: yes | ||
| doh2: |
There was a problem hiding this comment.
comment to explain it is dns over http2, depends on http2 parsing
| } | ||
| bool r2 = false; | ||
| if (tx_dns) { | ||
| // mix of JsonDnsLogger |
There was a problem hiding this comment.
don't understand this comment
| #endif | ||
|
|
||
| // Get inner transaction for engine | ||
| void *DetectGetInnerTx(void *tx_ptr, AppProto alproto, AppProto engine_alproto, uint8_t flow_flags) |
There was a problem hiding this comment.
could be inline since its called a lot?
|
Wrt the logging, I think we can sort this out after merge. |
| { ALPROTO_TELNET, "telnet" }, | ||
| { ALPROTO_WEBSOCKET, "websocket" }, | ||
| { ALPROTO_LDAP, "ldap" }, | ||
| { ALPROTO_DOH2, "doh2" }, |
There was a problem hiding this comment.
is this an generally used name for DNS over HTTP/2? Or are ppl just using doh? could perhaps use dns-http2 or similar
|
|
||
| #define PACKET_PROFILING_APP_END(dp, id) \ | ||
| if (profiling_packets_enabled) { \ | ||
| BUG_ON((id) != (dp)->alproto); \ |
There was a problem hiding this comment.
what about the other protocols where this should still hold?
| pub doh_data_buf: [Vec<u8>; 2], | ||
| pub dns_request_tx: Option<DNSTransaction>, | ||
| pub dns_response_tx: Option<DNSTransaction>, | ||
| pub doh: Option<DohHttp2Tx>, |
|
Merged in #11536, thanks! |
4 participants
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5773
Describe changes:
SV_BRANCH=OISF/suricata-verify#1980
#11498 with needed rebase
Should there be a squash up of commits ?
@jasonish still same question : 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...