-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
devguide/transactions: fix image filename #6245
Conversation
- extending/applayer/transactions: third draft - typo fix - add definition of `flow` as found in inliniac's blog - strike off parenthesis text about per-flow state that didn't make much sense - doc/devguide: add transactions.rst - doc/devguide/extending/app-layer/index.rst: add transactions.rst
Add explanation about how transaction state is used for rule matching
Grammar refinement. Add examples of simple protocols. Refine some statements. Nit: fix markup.
add sequence diagrams as pngs generated by mscgen via a script Add image description to Sequence Diagrams Change table of contents be built with markup tell git to ignore *.png files
mscgen is used for generating Sequence Diagrams images. Let's make sure it's present before generating devguide docs.
Codecov Report
@@ Coverage Diff @@
## master #6245 +/- ##
==========================================
- Coverage 76.94% 76.93% -0.02%
==========================================
Files 611 611
Lines 186146 186146
==========================================
- Hits 143236 143215 -21
- Misses 42910 42931 +21
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
- detail Template Protocol Sequence Diagram further - add note about HTTP2 transactions
@@ -2322,6 +2322,18 @@ fi | |||
AC_DEFINE([CLS],[64],[L1 cache line size]) | |||
fi | |||
|
|||
# mscgen for devguide images | |||
AC_PATH_PROG([HAVE_MSCGEN], mscgen, "no") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know if this should be part of this PR, but disable-docs
would be a nice configure option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this.
Is there some URL where we can see the nice thing with generated images ?
mscgen -T png -F Arial $FILE | ||
# if command fails, lets inform about that | ||
if [ $? -ne 0 ]; then | ||
echo "$FILE couldn't be converted in the devguide" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not exit there ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about that, what if that was just with a specific file, because of bad naming, for instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like not supposed to happen, so let's stop the work so that the user notices ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes way more sense 😅 sorry for sounding stubborn sometimes. Patched.
_`General Concepts` | ||
=================== | ||
|
||
Transactions are abstractions that help detecting and logging in Suricata. They also help during the detection phase, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why also
? You already mentioned detecting
=================== | ||
|
||
Transactions are abstractions that help detecting and logging in Suricata. They also help during the detection phase, | ||
when dealing with protocols that can have large PDUs, like TCP, in controlling state for partial rule matching, in case of rules that mention more than one field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem clear to me as an introduction. I would rather begin with an example like a HTTP request and its response constitute one transaction
Rule Matching | ||
~~~~~~~~~~~~~ | ||
|
||
Transaction progress is also used for certain keywords to know what is the minimum state before we can expect a match: until that, Suricata won't even try to look for the patterns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be nice to have the list of such keywords when running ./src/suricata --list-keywords=all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be like a linked document, which I'd mention here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather like something automatically generated...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, could we generate that list automatically, but have the list on a different page, to prevent this one from getting too long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but my point was that this requires another development work first.
I do not know how to get that list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok, sorry, I had gotten it wrongly at first. So we need something that allows us to list what keywords need that minimum stage, before matching? Should I create a ticket for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we need something that allows us to list what keywords need that minimum stage, before matching?
I would first like to have the list of keywords that are matched within a transaction (and the what the other ones are matched on)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I must dig more in order to get a better grasp at this...
- rs: rust | ||
- tc: to client | ||
- ts: to server | ||
- tx: transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the files ?
I think it is worth mentioning the current work to have files be part of the transaction cf https://redmine.openinfosecfoundation.org/issues/4444
It may also be worth mentioning the current work to put a limit on the number of transactions per flow cf https://redmine.openinfosecfoundation.org/issues/4530
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you suggest we add those? As a work in progress changes section? Or one with advanced topics? Or something else entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
work in progress changes section seems good for me
|
||
- Initial state value: ``0``. | ||
- Simpler scenarios: state is simply an int. ``1`` represents transaction completion, per direction. | ||
- Complex Transaction State in Suricata: ``enum`` (Rust: ``i32``). Completion is indicated by the highest enum value (some examples are: SSH, HTTP, DNS, SMB). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTP2 also, they have a state diagram in their RFC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me realize I'm confused with DNS. That's because I'm mentioning it here as an example of a complex transaction state, while I also mention it before as an instance of a simpler protocol. Is that ok, could others get confused by that, as well? In that case, what can we do to dispel such confusion? I think this is a question to @jasonish .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can keep DNS as a simplicity example when it fits
I can set up something locally, would that work? |
No need if it is not ready yet, I will make it work locally |
Addressed topics that are (somewhat) settled in the following PR: #6265 |
Transactions documentation - v9
Describe changes:
(waiting for final approval to figure out the squashing process... If that's not ideal, I can fix that now.)
Link to Redmine issue: https://redmine.openinfosecfoundation.org/issues/4396
Previous PR: #6241