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

pgsql: initial support #5700

Closed
wants to merge 1 commit into from
Closed

Conversation

jufajardini
Copy link
Contributor

  • pgsql/parser: add startup message structure, initial nom parser and
    corresponding unit test
  • pgsql/mod: list pgsql modules that must be compiled
  • lib: add pgsql to the module's list

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

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

Describe changes:

  • pgsql/parser: add startup message structure, initial nom parser and
    corresponding unit test
  • pgsql/mod: list pgsql modules that must be compiled
  • lib: add pgsql to the module's list

#suricata-verify-pr:
#suricata-verify-repo:
#suricata-verify-branch:
#suricata-update-pr:
#suricata-update-repo:
#suricata-update-branch:
#libhtp-pr:
#libhtp-repo:
#libhtp-branch:

- pgsql/parser: add startup message structure, initial nom parser and
corresponding unit test
- pgsql/mod: list pgsql modules that must be compiled
- lib: add pgsql to the module's list
@victorjulien
Copy link
Member

Please have a look at the failing QA tests, they show how the code can be tidied up a bit.

use super::*;

#[test]
fn test_parse_pgsql_startup_packet() {
Copy link
Member

@victorjulien victorjulien Jan 5, 2021

Choose a reason for hiding this comment

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

I think a good next step would be to add a test for invalid input. For example one where the length field contains a value smaller than PGSQL_LENGTH_SIZE + PGSQL_PROTO_SIZE

>> proto_version: bits!(tuple!(
take_bits!(16u16),
take_bits!(16u16)))
>> data: take!(len as usize - PGSQL_LENGTH_SIZE - PGSQL_PROTO_SIZE)
Copy link
Member

Choose a reason for hiding this comment

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

I always find this hard to read. Would len as usize - (PGSQL_LENGTH_SIZE + PGSQL_PROTO_SIZE) be the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try that and see if it works! Although this will likely not be necessary anymore, following Jason's reasoning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it does, I've changed it and code runs, test still working.

@victorjulien
Copy link
Member

I think this is a great start @jufajardini 👍

@victorjulien
Copy link
Member

I think a good next step would be to add tests for incomplete records. Handling these will be an important part of the parser (see https://github.com/OISF/suricata/blob/master/doc/devguide/extending/app-layer/parser.rst for what will be expected when you integrate the parser). While TCP data often gives you a full PDU, it doesn't have to be that way. So we need to handle all cases.

@jufajardini jufajardini mentioned this pull request Jan 6, 2021
3 tasks
@jufajardini
Copy link
Contributor Author

Closed with: #5706

@jufajardini jufajardini closed this Jan 6, 2021
@jufajardini jufajardini deleted the pgsql-support-v1 branch January 21, 2022 15:14
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.

2 participants