MySQL: parser v2 #11842

QianKaiLin · 2024-09-27T01:27:48Z

Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.

Contribution style:

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

I have updated the User Guide (in doc/userguide/) to reflect the changes made
I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
(including schema descriptions)

Link to ticket: OISF/suricata-verify#2067

Describe changes:

fix mysql parser bug.
fix mysql logger bug.
add mysql detection keywords mysql.command and mysql.rows
add SV tests.

SV_REPO=
SV_BRANCH=OISF/suricata-verify#2067

catenacyber

Thanks for the work so far, still more to do

CI : 🔴 could you fix it ?
Commits segmentation : I think we will squash all in the end, especially the commits like mysql: fix, so we do not have a buggy point in our git history if we can avoid it
Commit messages : ok, we can see after squashing
Git ID set : ok
CLA : ❓ I do not have access
Doc update : 🟠 I think there is more to do, like a note in upgrade guide cc @jufajardini
Redmine ticket : ok
Rustfmt : ok
Tests : thanks for the SV test
Dependencies added: none
Code : will look into it

catenacyber

CI is to be fixed

etc/schema.json

catenacyber · 2024-10-01T12:52:38Z

suricata.yaml.in

+    mysql:
+      enabled: no
+      # Stream reassembly size for MySQL. By default, track it completely.
+      stream-depth: 0


Why that default ?

I copied from pgsql configuration, now i know what it means, i will remove it.

src/detect-mysql-rows.c

rust/src/mysql/logger.rs

etc/schema.json

rust/src/mysql/mysql.rs

catenacyber · 2024-10-01T13:01:13Z

rust/src/mysql/mysql.rs

+                        if tx_completed {
+                            tx.complete = true;
+                        }
+                    }


Should we not create a new transaction with the response if self.find_or_create_tx() returns None ?

Sorry, I don't understand the question.

rust/src/mysql/mysql.rs

rust/src/mysql/parser.rs

catenacyber · 2024-10-01T13:11:10Z

There is an amazing amount of work in here :-)

jufajardini · 2024-10-02T23:53:03Z

I've adjusted the way the SV PR is passed, hopefully the step for preparing dependencies should work now.
Locally, I see that 3 of the sv tests are failing, expecting that the CI will highlight that, if it happens here, too.
For your next versions, you can follow this style. Paste the link to your SV PR in the SV_BRANCH option, like so:

SV_BRANCH=https://github.com/OISF/suricata-verify/pull/2067

:)

kudos on aaalll the work done so far, here!
For the CI failures, let us know if you need any guidance navigating and fixing them, ok?

jufajardini

About the documentation:

we could indeed have a note on the Upgrading section (upgrading from 7 to 8, an example: 70ed9f91d84d)
we should also document the log and keyword additions (one example: 70ed9f91d84d)

About the copyright note:

all newly files should have it
If this work is all being done by a single author, could you please standardize the author name and e-mail used when you're adding those to the file author annotations?

As Philippe said, there is a massive amount of work done here, so it will take some time for us to do proper rounds of reviews. But thanks for incorporating the feedback, and the time dedicated to this!

rust/src/mysql/mod.rs

rust/src/mysql/parser.rs

rust/src/mysql/mysql.rs

codecov · 2024-10-03T18:51:10Z

Codecov Report

Attention: Patch coverage is 11.28350% with 629 lines in your changes missing coverage. Please review.

Project coverage is 66.45%. Comparing base (45eb7e4) to head (90496a5).
Report is 12 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (45eb7e4) and HEAD (90496a5). Click for more details.

HEAD has 2 uploads less than BASE

Flag BASE (45eb7e4) HEAD (90496a5)

suricata-verify 1 0

unittests 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11842       +/-   ##
===========================================
- Coverage   82.60%   66.45%   -16.15%     
===========================================
  Files         912      852       -60     
  Lines      249351   151008    -98343     
===========================================
- Hits       205965   100356   -105609     
- Misses      43386    50652     +7266

Flag	Coverage Δ
fuzzcorpus	`59.69% <11.00%> (-0.91%)`	⬇️
livemode	`18.64% <11.28%> (-0.08%)`	⬇️
pcap	`43.52% <10.15%> (-0.54%)`	⬇️
suricata-verify	`?`
unittests	`?`

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

victorjulien

a few comments, not a full review yet

etc/schema.json

rust/src/mysql/detect.rs

QianKaiLin · 2024-10-09T03:33:51Z

About the documentation:
* we could indeed have a note on the Upgrading section (upgrading from 7 to 8, an example: [70ed9f91d84d](https://github.com/OISF/suricata/commit/70ed9f91d84d))

* we should also document the log and keyword additions (one example: [70ed9f91d84d](https://github.com/OISF/suricata/commit/70ed9f91d84d))
About the copyright note:
* all newly files should have it

* If this work is all being done by a single author, could you please standardize the author name and e-mail used when you're adding those to the file author annotations?
As Philippe said, there is a massive amount of work done here, so it will take some time for us to do proper rounds of reviews. But thanks for incorporating the feedback, and the time dedicated to this!

I have added the document in upgrade and log and keyword

jufajardini · 2024-10-16T02:21:52Z

About the documentation:
* we could indeed have a note on the Upgrading section (upgrading from 7 to 8, an example: [70ed9f91d84d](https://github.com/OISF/suricata/commit/70ed9f91d84d))

* we should also document the log and keyword additions (one example: [70ed9f91d84d](https://github.com/OISF/suricata/commit/70ed9f91d84d))
About the copyright note:
* all newly files should have it

* If this work is all being done by a single author, could you please standardize the author name and e-mail used when you're adding those to the file author annotations?
As Philippe said, there is a massive amount of work done here, so it will take some time for us to do proper rounds of reviews. But thanks for incorporating the feedback, and the time dedicated to this!
I have added the document in upgrade and log and keyword

Thanks for incorporating the feedback shared. This currently needs a rebase, due to conflicts. When you can, then, it'd be good to share a new, the most current version, rebased. Probably also after creating a new SV version :)

Task OISF#3446

#Task 3446

Task OISF#3446

QianKaiLin · 2024-10-17T04:56:16Z

About the documentation:
* we could indeed have a note on the Upgrading section (upgrading from 7 to 8, an example: [70ed9f91d84d](https://github.com/OISF/suricata/commit/70ed9f91d84d))

* we should also document the log and keyword additions (one example: [70ed9f91d84d](https://github.com/OISF/suricata/commit/70ed9f91d84d))
About the copyright note:
* all newly files should have it

* If this work is all being done by a single author, could you please standardize the author name and e-mail used when you're adding those to the file author annotations?
As Philippe said, there is a massive amount of work done here, so it will take some time for us to do proper rounds of reviews. But thanks for incorporating the feedback, and the time dedicated to this!
I have added the document in upgrade and log and keyword
Thanks for incorporating the feedback shared. This currently needs a rebase, due to conflicts. When you can, then, it'd be good to share a new, the most current version, rebased. Probably also after creating a new SV version :)

I have resolved conflicts, please check again, thx.

jufajardini · 2024-10-18T18:58:05Z

About the documentation:
* we could indeed have a note on the Upgrading section (upgrading from 7 to 8, an example: [70ed9f91d84d](https://github.com/OISF/suricata/commit/70ed9f91d84d))

* we should also document the log and keyword additions (one example: [70ed9f91d84d](https://github.com/OISF/suricata/commit/70ed9f91d84d))
About the copyright note:
* all newly files should have it

* If this work is all being done by a single author, could you please standardize the author name and e-mail used when you're adding those to the file author annotations?
As Philippe said, there is a massive amount of work done here, so it will take some time for us to do proper rounds of reviews. But thanks for incorporating the feedback, and the time dedicated to this!
I have added the document in upgrade and log and keyword
Thanks for incorporating the feedback shared. This currently needs a rebase, due to conflicts. When you can, then, it'd be good to share a new, the most current version, rebased. Probably also after creating a new SV version :)
I have resolved conflicts, please check again, thx.

Please, submit a new PR, to make it easier for us to review the cleaned up and rebased code, as well as to ensure that the CI checks will run against a rebased and updated SV PR :)

jufajardini · 2024-10-18T18:54:27Z

rust/src/mysql/mysql.rs

+        self.transactions.back_mut()
+    }
+
+    fn request_next_state(


It seems that this function is doing way more than just get the next state based on the request. Can we refactor it?

jufajardini · 2024-10-18T18:56:37Z

rust/src/mysql/mysql.rs

+                    if let Some(state) = self.request_next_state(request, flow) {
+                        self.state_progress = state;
+                    }


Is it possible for this to return None?

QianKaiLin requested review from jasonish, victorjulien and a team as code owners September 27, 2024 01:27

Kotodian force-pushed the dev-3346-mysql-proto-v2 branch from da3fc12 to 90496a5 Compare September 29, 2024 01:43

catenacyber mentioned this pull request Oct 1, 2024

MySQL protocol parser v1 #11800

Closed

3 tasks