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: add keyword for dns.response.rrname (feat 7012) - v2 #12500

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jasonish
Copy link
Member

Rebase of: #11647

Changes:

  • Rename to dns.response.rrname
  • Replace function to get rrname with safe function

SV_BRANCH=OISF/suricata-verify#2264

scrivs86 and others added 5 commits January 28, 2025 16:34
Feature: 7012
Add dns.response sticky buffer to match on dns response fields.
Add rust functions to return dns response packet data.
Unit tests verifying signature matching.
This is a better name as the keyword is looking at all rrname type
fields in the response.
These arrays are manually formatted for readability.
Make the function safe by returning a reference to the DNSName object,
the unsafe C wrapper can do the conversion to pointers.
@jasonish jasonish mentioned this pull request Jan 29, 2025
4 tasks
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 98.79130% with 15 lines in your changes missing coverage. Please review.

Project coverage is 80.65%. Comparing base (a2905ae) to head (d6a553b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12500      +/-   ##
==========================================
+ Coverage   80.58%   80.65%   +0.07%     
==========================================
  Files         925      926       +1     
  Lines      259313   260554    +1241     
==========================================
+ Hits       208955   210157    +1202     
- Misses      50358    50397      +39     
Flag Coverage Δ
fuzzcorpus 56.08% <7.01%> (-0.07%) ⬇️
livemode 19.38% <7.01%> (-0.02%) ⬇️
pcap 44.13% <7.01%> (-0.12%) ⬇️
suricata-verify 63.40% <70.61%> (+0.02%) ⬆️
unittests 58.67% <98.79%> (+0.22%) ⬆️

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 24417

@victorjulien
Copy link
Member

Is this to be considered for merge? The conversation in the prev PR suggests there is more to do?

@jasonish
Copy link
Member Author

Is this to be considered for merge? The conversation in the prev PR suggests there is more to do?

Other than more S-V coverage, to be comparable with the unit tests I think its ready for review. Something else standing out?

There's this, #11647 (comment), but it doesn't really affect the completeness of this PR.

Currently its a blocker for more discrete keywords to provide parity, for which I'll need very similar tests as well. So I could do those to complete this off, then they'd be ready for https://redmine.openinfosecfoundation.org/issues/5642.

@catenacyber
Copy link
Contributor

Currently its a blocker for more discrete keywords to provide parity

What are discrete keywords ?

Do you say this PR is blocking adding other keywords ? Why so ?

Is this to be considered for merge? The conversation in the prev PR suggests there is more to do?

@scrivs86 said yesterday that he was going to do another version cf #11647 (comment)

This means squashing in your commits I guess

Comment on lines +191 to +193
``rdata`` field matching supports a subset of types that contain
domain name structured data, for example: "www.suricata.io". The list
of types inspected is:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have an example with rdata, too?

Other than that, doc looks good to me :)

Copy link
Member Author

@jasonish jasonish Jan 31, 2025

Choose a reason for hiding this comment

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

dns.reponse.rrname matches on rdata that is formatted as a resource name record. There is no additional keyword.

@scrivs86
Copy link
Contributor

Currently its a blocker for more discrete keywords to provide parity

What are discrete keywords ?

Do you say this PR is blocking adding other keywords ? Why so ?

Is this to be considered for merge? The conversation in the prev PR suggests there is more to do?

@scrivs86 said yesterday that he was going to do another version cf #11647 (comment)

This means squashing in your commits I guess

I reached out to @jasonish , and he already implemented the changes I was going to do. Thanks :)

@jasonish
Copy link
Member Author

What are discrete keywords ?

For example, dns.answer.rdata, to bring parity with the logging. This PR has some functions that are useful there so I'd rather build on it.

@jasonish
Copy link
Member Author

This means squashing in your commits I guess

Why? We have a bit of a history of keeping the "story" when the work ends up being a collaboration.

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.

6 participants