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

alert/metadata: no pgsql object encapsulation (7.0.x backport) - v1 #11665

Closed
wants to merge 3 commits into from

Conversation

jufajardini
Copy link
Contributor

Trying to decouple this backport from progress (or delays) of #11635

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

Describe changes:

  • clean cherry-pick of 69e26de
  • add alert metadata logging for pgsql (from what I could see, the way this is seen in 7 is different from master, so couldn't simply backport what we have for that
  • [extra] clean cherry-pick of ce1556c as this seemed like something that should be backported, too

Provide values to any of the below to override the defaults.

SV_REPO=
SV_BRANCH=OISF/suricata-verify#2020

jufajardini and others added 3 commits August 27, 2024 10:41
Before, the JsonBuilder object for the pgsql event was being created
from the C-side function that actually called the Rust logger.

This resulted that if another module - such as the Json Alert called the
PGSQL logger, we wouldn't have the `pgsql` key present in the log output
- only its inner fields.

Bug OISF#6983

(cherry picked from commit 69e26de)
It was brought to my attention by GLongo that Pgsql parser handled eof
diffrently for requests and responses, and apparently there isn't a good
reason for such a difference therefore, apply same logic used for
rs_pgsql_parse_request for checking for eof when parsing a response.

(cherry picked from commit ce1556c)
@jasonish
Copy link
Member

Re. commit: 56dbcd6

Maybe a wording issue. But this adds pgsql metadata to alerts if available right? Maybe:

output/json: add pgsql metadata logging to alerts

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

Looks like a resonable choice of commits to backport.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 22270

@catenacyber
Copy link
Contributor

Why do we need pgsql: check for eol when parsing response ?
(Genuine question)

@jufajardini
Copy link
Contributor Author

Why do we need pgsql: check for eol when parsing response ? (Genuine question)

As this seemed to be something that should have been there from the beginning, it made sense to me that it would be backported, once it was discovered and fixed.

@jufajardini
Copy link
Contributor Author

Re. commit: 56dbcd6

Maybe a wording issue. But this adds pgsql metadata to alerts if available right? Maybe:

output/json: add pgsql metadata logging to alerts

Okie, do I submit a new PR to fix the commit?

@jasonish
Copy link
Member

Re. commit: 56dbcd6
Maybe a wording issue. But this adds pgsql metadata to alerts if available right? Maybe:

output/json: add pgsql metadata logging to alerts

Okie, do I submit a new PR to fix the commit?

Yup :)

@jufajardini
Copy link
Contributor Author

Properly new PR for updating commit message :P #11681

@jufajardini jufajardini deleted the bug-7066/v1 branch September 9, 2024 16:18
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.

4 participants