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

userguide: explain rule types and categorization - v9 #12209

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jufajardini
Copy link
Contributor

Previous PR: #12184

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

Please check the built version: https://suri-rtd-test.readthedocs.io/en/doc-sigtypes-et-properties-v9/rules/rule-types.html

Describe changes:

Many of the examples and conclusions documented here were derived from tests and checks as seen on OISF/suricata-verify#2153

Add documentation about the rule types introduced by 2696fda.

Add doc tags around code definitions that are referenced in the docs.

Task #https://redmine.openinfosecfoundation.org/issues/7031
@jufajardini jufajardini added the typo/doc update No code change : only doc or typo fixes label Dec 3, 2024
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.18%. Comparing base (e9173f3) to head (7c79e48).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12209      +/-   ##
==========================================
+ Coverage   83.17%   83.18%   +0.01%     
==========================================
  Files         912      912              
  Lines      257111   257111              
==========================================
+ Hits       213856   213885      +29     
+ Misses      43255    43226      -29     
Flag Coverage Δ
fuzzcorpus 61.01% <ø> (-0.01%) ⬇️
livemode 19.41% <ø> (-0.01%) ⬇️
pcap 44.40% <ø> (+0.01%) ⬆️
suricata-verify 62.77% <ø> (-0.01%) ⬇️
unittests 59.17% <ø> (-0.01%) ⬇️

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

Comment on lines +95 to +104
* - IP Only
- Flow
- Once per direction
- On IP addresses on the flow
- Source/ Destination field of a rule
* - IP Only (contains negated address)(*)
- Flow
- Once per direction
- On IP addresses on the flow
- Source/ Destination field of a rule, containing negated address
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Action is flow if flow exists. Packets that are not part of a flow, .... will always be inspected per packet.

- Source/ Destination field of a rule
* - IP Only (contains negated address)(*)
- Flow
- Once per direction
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All packets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test this with rule profiling to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can have a SV test with like-ip-only

  • alert rule: several alerts
  • drop/pass: should be applied to the flow

- Packet
- Per-packet basis
- Packet-level info (e.g.: header info)
- ``itype``, ``tcp.hdr``, ``tcp.seq``, ``ttl`` etc.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tcp-pkt - add as keyword example here

- Flow, if stateful (**)
- Per stream chunk, if stateful, per-packet if not.
- Against the reassembled stream. If stream unavailable, match per-packet.
(stream payload or packet payload)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move to Inspected, be stream payload AND packet payload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exposes the stream and/or payload data.

* - Type
- Action Scope
- Inspected
- Matches
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Data exposed?


* - Type
- Action Scope
- Inspected
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inspection hook?

- Keyword Examples (non-exhaustive)
* - Decoder Events Only
- Packet
- Per-packet basis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per-broken/ invalid packet

* - Decoder Events Only
- Packet
- Per-packet basis
- Packets that are broken on an IP level
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decoding events

- ``content`` with ``startswith`` or ``depth``
* - Stream
- Flow, if stateful (**)
- Per stream chunk, if stateful, per-packet if not.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stream chunks, or just packets (if not accepted by stream engine)

* - Stream
- Flow, if stateful (**)
- Per stream chunk, if stateful, per-packet if not.
- Against the reassembled stream. If stream unavailable, match per-packet.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exposes stream reassembled payload or packet payload data

Comment on lines +185 to +187
- Application Layer (`app_layer`)
- Protocol Detection Only (`pd_only`)
- Decoder Events Only (`de_only`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ` is wrong.

Comment on lines +189 to +191
The rule examples provided further cover some such cases, but the table below
lists those keywords with more details:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check detect-flowbits.c:632. some cases will make a packet rule become a stateful / app_tx signature.

@jufajardini
Copy link
Contributor Author

@njlavigne There will be a new version of this coming out soon to take into account a review done with Victor, but do feel free to add feedback, please, if you see anything weird or missing :)

aspects aforementioned.

.. list-table:: Suricata Rule Types
:widths: 10, 17, 18, 30, 25
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review width for last 2 columns, especially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the rtd theme isn't properly rendering list-table widths properly - it's ignoring this, basically.

The best workaround I've found was using nested paragraphs, as it seems that the width is being based on the column header, and this seems to be working.

@jasonish
Copy link
Member

jasonish commented Dec 3, 2024

Given the embedded code, etc. I wonder if this is a little too deep to from the rule chapter. Is it better suited at the end with some "rule internals" or part of a Suricata internals guide? At first I was wondering devguide, but I do realize there is some internals that should be documented with a power user audience, and not necessarily a developer audience.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23656

@jufajardini
Copy link
Contributor Author

Given the embedded code, etc. I wonder if this is a little too deep to from the rule chapter. Is it better suited at the end with some "rule internals" or part of a Suricata internals guide? At first I was wondering devguide, but I do realize there is some internals that should be documented with a power user audience, and not necessarily a developer audience.

We have Making sense out of alerts, right after the two Rules chapters. Maybe we could have this part about the rule types before or after that one? I understand it has more advanced stuff, but I think semantically it also makes sense that it remains close to the other chapters. imho, the code excerpts that we show aren't mandatory to understand the contents - more to add hooks for those who like to dig...

@jasonish
Copy link
Member

jasonish commented Dec 4, 2024

Given the embedded code, etc. I wonder if this is a little too deep to from the rule chapter. Is it better suited at the end with some "rule internals" or part of a Suricata internals guide? At first I was wondering devguide, but I do realize there is some internals that should be documented with a power user audience, and not necessarily a developer audience.

We have Making sense out of alerts, right after the two Rules chapters. Maybe we could have this part about the rule types before or after that one? I understand it has more advanced stuff, but I think semantically it also makes sense that it remains close to the other chapters. imho, the code excerpts that we show aren't mandatory to understand the contents - more to add hooks for those who like to dig...

Maybe move it to the end of the rules chapter? My thought are:

  • Making sense of alerts is more about reading the alert. Could be a different audience than rule writers. I think the large majority of our users are not rule writers, but read the alerts.
  • Reading documentation in order. If this new info it must have reading material before writing your first rule, keep it where it is, otherwise if its for the smaller few that really need to know whats going on, move it down

To be fair, advanced rule writing could be its own book I'm sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typo/doc update No code change : only doc or typo fixes
Development

Successfully merging this pull request may close these issues.

3 participants