Skip to content

Comments

detect: dns.opcode as first-class integer#10280

Closed
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:detect-dns-opcode-range-5446-v6
Closed

detect: dns.opcode as first-class integer#10280
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:detect-dns-opcode-range-5446-v6

Conversation

@catenacyber
Copy link
Contributor

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

Describe changes:

  • make keyword dns.opcode use helper for integers so that they can have all features such as range
  • integer keywords now accept negated range

#10208 rebased after merge of #10246 (and updated doc to point to rules-integer-keywords)

Ticket: 5446

That means it can accept ranges
@codecov
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (244a35d) 73.31% compared to head (6d4c370) 82.32%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #10280       +/-   ##
===========================================
+ Coverage   73.31%   82.32%    +9.00%     
===========================================
  Files         895      978       +83     
  Lines      148215   271999   +123784     
===========================================
+ Hits       108666   223912   +115246     
- Misses      39549    48087     +8538     
Flag Coverage Δ
fuzzcorpus 63.48% <75.00%> (-0.01%) ⬇️
suricata-verify 61.48% <75.00%> (-0.05%) ⬇️
unittests 62.85% <85.71%> (?)

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 17946

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

LGTM! :)

arg2: 0,
},
0b0010_0000_0000_0000,
((0b0010_0000_0000_0000 >> 11) & 0xf) as u8,
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a couple of tests for range and negative range too if they're allowed for opcode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why ?

There are already tests for generic DetectUintData for range and negative range.
Feels to me like duplicating...

Copy link
Member

Choose a reason for hiding this comment

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

ok sure. I just saw the other modes in the test here and thought it was missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just translated the existing unit tests, right ?

Copy link
Member

Choose a reason for hiding this comment

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

yep. thought that since this PR enabled range acceptance, why was that mode missing in tests when there were others. I mean this is cool with me if there are tests already. Thanks :)

@victorjulien victorjulien added this to the 8.0 milestone Feb 2, 2024
@victorjulien
Copy link
Member

Merged in #10321, thanks!

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.

5 participants