devguide/transactions: fix image filename #6245

jufajardini · 2021-07-01T15:06:43Z

Transactions documentation - v9

Describe changes:

Previous PR somehow went through with an image name wrong. Fixing that.

(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

- 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 · 2021-07-01T15:28:59Z

Codecov Report

Merging #6245 (32d34aa) into master (b8499de) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            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

Flag	Coverage Δ
fuzzcorpus	`52.77% <ø> (-0.05%)`	⬇️
suricata-verify	`51.16% <ø> (+<0.01%)`	⬆️
unittests	`63.08% <ø> (ø)`

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

suricata-qa · 2021-07-04T08:00:11Z

field	test	baseline	% diff
	tlpr1_stats_chk
.tcp.insert_list_fail	219	255	86.0%

- detail Template Protocol Sequence Diagram further - add note about HTTP2 transactions

catenacyber · 2021-07-12T07:58:50Z

configure.ac

@@ -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")


I do not know if this should be part of this PR, but disable-docs would be a nice configure option

catenacyber

Thanks for this.
Is there some URL where we can see the nice thing with generated images ?

catenacyber · 2021-07-12T08:00:19Z

doc/devguide/extending/app-layer/img/generate-images.sh

+    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"


Should we not exit there ?

I'm not sure about that, what if that was just with a specific file, because of bad naming, for instance?

This looks like not supposed to happen, so let's stop the work so that the user notices ;-)

That makes way more sense 😅 sorry for sounding stubborn sometimes. Patched.

catenacyber · 2021-07-12T08:01:00Z

doc/devguide/extending/app-layer/transactions.rst

+_`General Concepts`
+===================
+
+Transactions are abstractions that help detecting and logging in Suricata. They also help during the detection phase,


Why also ? You already mentioned detecting

catenacyber · 2021-07-12T08:02:58Z

doc/devguide/extending/app-layer/transactions.rst

+===================
+
+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.


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

catenacyber · 2021-07-12T08:04:51Z

doc/devguide/extending/app-layer/transactions.rst

+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.


Would it be nice to have the list of such keywords when running ./src/suricata --list-keywords=all

Could it be like a linked document, which I'd mention here?

I would rather like something automatically generated...

Ok, could we generate that list automatically, but have the list on a different page, to prevent this one from getting too long?

Yes, but my point was that this requires another development work first.
I do not know how to get that list

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?

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)

Ok, I think I must dig more in order to get a better grasp at this...

catenacyber · 2021-07-12T08:08:52Z

doc/devguide/extending/app-layer/transactions.rst

+- rs: rust
+- tc: to client
+- ts: to server
+- tx: transaction


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

How do you suggest we add those? As a work in progress changes section? Or one with advanced topics? Or something else entirely?

work in progress changes section seems good for me

catenacyber · 2021-07-12T08:10:17Z

doc/devguide/extending/app-layer/transactions.rst

+
+- 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).


HTTP2 also, they have a state diagram in their RFC

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 .

You can keep DNS as a simplicity example when it fits

jufajardini · 2021-07-12T15:29:56Z

Thanks for this.
Is there some URL where we can see the nice thing with generated images ?

I can set up something locally, would that work?

catenacyber · 2021-07-12T15:39:46Z

I can set up something locally, would that work?

No need if it is not ready yet, I will make it work locally

jufajardini · 2021-07-12T17:00:25Z

Addressed topics that are (somewhat) settled in the following PR: #6265

jufajardini added 11 commits July 1, 2021 14:50

devguide/transactions: change formatting

9676d10

devguide/transactions: fix typos

ea95909

devguide/transactions: improve contents

00b3b27

Add explanation about how transaction state is used for rule matching

doc/devguide: Transaction v6

d82697b

Grammar refinement. Add examples of simple protocols. Refine some statements. Nit: fix markup.

devguide/transactions: add sequence diagrams

d450d8a

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

config/make/doc: check for mscgen package

b100dff

mscgen is used for generating Sequence Diagrams images. Let's make sure it's present before generating devguide docs.

workflows: add mscgen to Ubuntu 18.04 Cocci builds

85a6478

devguide/transactions: adjust formatting

676b0cc

devguide/transactions: add examples from Template

abb5c85

devguide/transactions: fix broken image filename

8a971fa

jufajardini requested review from norg, victorjulien and a team as code owners July 1, 2021 15:06

jufajardini mentioned this pull request Jul 1, 2021

devguide/transactions: add template examples #6241

Closed

devguide/transactions: small updates

32d34aa

- detail Template Protocol Sequence Diagram further - add note about HTTP2 transactions

catenacyber reviewed Jul 12, 2021

View reviewed changes

jufajardini mentioned this pull request Jul 12, 2021

devguide/transactions: refinements and add WIP section #6265

Closed

jufajardini closed this Jul 12, 2021

jufajardini deleted the tx-documentation-v9 branch September 30, 2021 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

devguide/transactions: fix image filename #6245

devguide/transactions: fix image filename #6245

jufajardini commented Jul 1, 2021

codecov bot commented Jul 1, 2021 •

edited

Loading

suricata-qa commented Jul 4, 2021

catenacyber Jul 12, 2021

catenacyber left a comment

catenacyber Jul 12, 2021

jufajardini Jul 12, 2021

catenacyber Jul 12, 2021

jufajardini Jul 12, 2021

catenacyber Jul 12, 2021

catenacyber Jul 12, 2021

catenacyber Jul 12, 2021

jufajardini Jul 12, 2021

catenacyber Jul 12, 2021

jufajardini Jul 12, 2021

catenacyber Jul 12, 2021

jufajardini Jul 12, 2021

catenacyber Jul 12, 2021

jufajardini Jul 12, 2021

catenacyber Jul 12, 2021

jufajardini Jul 12, 2021

catenacyber Jul 12, 2021

catenacyber Jul 12, 2021

jufajardini Jul 12, 2021 •

edited

Loading

catenacyber Jul 12, 2021

jufajardini commented Jul 12, 2021

catenacyber commented Jul 12, 2021

jufajardini commented Jul 12, 2021 •

edited

Loading

devguide/transactions: fix image filename #6245

devguide/transactions: fix image filename #6245

Conversation

jufajardini commented Jul 1, 2021

codecov bot commented Jul 1, 2021 • edited Loading

Codecov Report

suricata-qa commented Jul 4, 2021

Choose a reason for hiding this comment

catenacyber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jufajardini Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jufajardini commented Jul 12, 2021

catenacyber commented Jul 12, 2021

jufajardini commented Jul 12, 2021 • edited Loading

codecov bot commented Jul 1, 2021 •

edited

Loading

jufajardini Jul 12, 2021 •

edited

Loading

jufajardini commented Jul 12, 2021 •

edited

Loading