postgresql: add initial support (WIP) - checkpoint #6281

jufajardini · 2021-07-28T10:29:27Z

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

Previous PR: #6062

Describe changes:
This version covers most of the messages related to startup phase and simple query, except for NegotiateProtocolVersion - must understand how this happens, still.

The logger has filterable fields (eve.json), but might be too verbose at this point.

Considerations:

Transactions need refining, to ensure messages are in the correct transaction.
Doesn't upgrade to TLS (STARTTLS) yet.
In same cases, it seems that some messages are not logged (happens with pgsql-jdbc.pcap, available in gdrive). This may be due to them being part of extended query protocol message flow. Shall investigate further.

Mostly tested with the pcaps available here:
https://drive.google.com/drive/folders/1BPJICt_ybkofDUPrMdGglvfQeefFCQF_?usp=sharing

rust/src/applayerpgsql/parser.rs: - add nom parsers for decoding most messages involved in Startup Phase - add more unittests src/Makefile.am fix formatting issue rust/src/applayerpgsql/pgsql.rs: - fix unused documentation comments warning output-json-pgsql.c: - update CreateEveHeader call.

pgsql: adapt decoder for multi-message transaction style (WIP)

- midway WIP, deciding if should keep this style or rollback

make sure PostgreSQL isn't enabled by default.

pgsql: Add directions to re-write parse_request and parse_response tests/fuzz: add PostgreSQL to confyaml.c pgsql: rewrite parse_request pgsql: rewrite parse_request (WIP) PostgreSQL: add debug to probe fn

PostgreSQL: parse SimpleQuery subprotocol (WIP) - parse Query request - WIP parse RowDescription message (nom parser) parser/pgsql: parse RowDescription messages (compiles) pgsql/pgsql: adjust unittest for probe_response probe_tc must check for ssl_responses, otherwise suricata misses some messages in case of gap. But that rendered some of the unittests bonky. parser/pgsql: add RowData nom parser (WIP) Pgsql: parse more Simple Query messages Pgsql: integrate new nom parsers (WIP) PostgreSQL: parse Simple Query (WIP)

rust/pgsql: - add PgsqlTransaction to cbindgen context - add postgresql messages to logger, with filterable fields - make fields and structs public so logger can access those src/output-json-pgsql: - convert logger to jsonbuilder

codecov · 2021-07-28T10:52:25Z

Codecov Report

Merging #6281 (ff8b534) into master (843c4b2) will decrease coverage by 0.09%.
The diff coverage is 18.51%.

@@            Coverage Diff             @@
##           master    #6281      +/-   ##
==========================================
- Coverage   76.97%   76.88%   -0.10%     
==========================================
  Files         611      613       +2     
  Lines      186249   186330      +81     
==========================================
- Hits       143368   143262     -106     
- Misses      42881    43068     +187

Flag	Coverage Δ
fuzzcorpus	`52.70% <14.28%> (-0.17%)`	⬇️
suricata-verify	`51.18% <16.88%> (-0.06%)`	⬇️
unittests	`63.05% <16.66%> (-0.03%)`	⬇️

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

jufajardini · 2021-07-28T10:55:19Z

the commit-check fail is, as far as I could understand, from a commit when the logger hadn't been converted into JsonBuilder yet...

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

ERROR: QA failed on generic_collect.

Pipeline 3639

victorjulien · 2021-07-29T09:52:59Z

rust/src/pgsql/log.rs

+            PgsqlErrorNoticeFieldType::TerminatorToken => {
+                // do nothing, as that's the terminator token ?
+            },
+            PgsqlErrorNoticeFieldType::UnknownFieldType => {


is this unknown to suri, or unknown in the pgsql protocol spec?

The specification lists existing field types, but states that more types can be added in the future - therefore, frontends should silently ignore unrecognized field types.

victorjulien

Some comments and questions. Nothing major. Nice work :)

victorjulien · 2021-07-29T12:09:16Z

rust/src/pgsql/parser.rs

+}
+
+impl PgsqlFEMessage {
+    pub fn get_message_type(&self) -> &str {


this method turns a type into a string, so its name should reflect that

victorjulien · 2021-07-29T15:25:46Z

rust/src/pgsql/parser.rs

+    }
+}
+
+impl From<u8> for PgsqlErrorNoticeFieldType {


can these 2 funcs somehow be merged, or use a common util func?

Checking this properly, now, those two cannot, as this From<u8> is for the ErrorMessages and NoticeMessages types, not the front end messages.

The From functions are also used by the nom parsers, in the cases where that makes sense, and return types compatible with said parsers. Whereas the get_message_type is used by the app-layer parsing the requests and responses and the logger. So I'm guessing that some documentation is needed here, to explain that (plus proper naming the get_message_typeone)...

victorjulien · 2021-07-29T15:29:21Z

rust/src/pgsql/pgsql.rs

+    }
+}
+#[derive(Debug, PartialEq)]
+pub enum PgsqlStateProgress {


this looks quite similar to PgsqlTransactionState, is there a need for both to exist?

They're indeed similar, and I'm still trying to figure that out, to be honest. First, we thought that progress would be tracked per transaction, then, due to how postgresql protocol transactions work, we thought that maybe tracking it just in state would be enough. Currently, basically only PgsqlStateProgress is being used. But I would say I must investigate the logs a bit more, to understand if Suri can interpret the traffic correctly with StateProgress only.

victorjulien · 2021-07-29T15:30:54Z

rust/src/pgsql/pgsql.rs

+                self.transactions.push(tx);
+            }
+            // If we don't have to create a new transaction, just return the current one
+            return self.transactions.last_mut().unwrap();


is it possible that there is no tx yet, so this unwrap would fail?

victorjulien · 2021-07-29T15:32:49Z

rust/src/pgsql/pgsql.rs

+                PgsqlStateProgress::CommandCompletedReceived => {
+                    match parser::parse_request(start) {
+                        // TODO Question should I also add "&& self.transactions.is_empty()"
+                        Ok((rem, request)) => {


think it would be good to move this block into a util func to keep the main loop more readable

victorjulien · 2021-08-30T15:39:52Z

@jasonish can you have a look at the EVE output and see if that is what we'd want vs how verbose it is?

Bug: OISF#6281

- answer object - top level rrname and rrtype - header level details in answers Bug: OISF#6281

If multiple queries exist in a DNS request, Suricata would log a discrete DNS event for each request. However, in an alert on a DNS request, the DNS object would contain an array of query objects. This brings the "dns" object into alignment with respect to DNS requests in alerts and "dns" records. This introduces a new function for logging DNS messages that is common for requests and responses which should reduce code, but for this commit is only used for requests, so doesn't cover responses yet. As this is a breaking change, rename the "query" array in alerts to "queries" to be more in alignment with how we log "answers" and "authorities". Note that this is a breaking change. Bug: OISF#6281

By using the same function for logging DNS requests and responses we can better ensure that the format stays consistent between. Bug: OISF#6281

Bug: OISF#6281

- answer object - top level rrname and rrtype - header level details in answers Bug: OISF#6281

By using the same function for logging DNS requests and responses we can better ensure that the format stays consistent between. Bug: OISF#6281

Bug: OISF#6281

- answer object - top level rrname and rrtype - header level details in answers Bug: OISF#6281

If multiple queries exist in a DNS request, Suricata would log a discrete DNS event for each request. However, in an alert on a DNS request, the DNS object would contain an array of query objects. This brings the "dns" object into alignment with respect to DNS requests in alerts and "dns" records. This introduces a new function for logging DNS messages that is common for requests and responses which should reduce code, but for this commit is only used for requests, so doesn't cover responses yet. As this is a breaking change, rename the "query" array in alerts to "queries" to be more in alignment with how we log "answers" and "authorities". Note that this is a breaking change. Bug: OISF#6281

By using the same function for logging DNS requests and responses we can better ensure that the format stays consistent between. Bug: OISF#6281

Bug: OISF#6281

- answer object - top level rrname and rrtype - header level details in answers Bug: OISF#6281

V3 style DNS logging fixes the discrepancies between request and response logging better dns records and alert records. The main change is that queries and answers are always logged as arrays, and header fields are not logged in array items. For alerts this means that answers are now logged as arrays, queries already were. DNS records will get this new format as well, but with a configuration parameter. Bug: OISF#6281

DNS v3 logging fixes the discrepancies between request and response logging with the main difference being queries always being placed in an array. Bug: OISF#6281

Bug: OISF#6281

DNS v3 logging fixes the discrepancies between request and response logging with the main difference being queries always being placed in an array. Bug: OISF#6281

Bug: OISF#6281

V3 style DNS logging fixes the discrepancies between request and response logging better dns records and alert records. The main change is that queries and answers are always logged as arrays, and header fields are not logged in array items. For alerts this means that answers are now logged as arrays, queries already were. DNS records will get this new format as well, but with a configuration parameter. Bug: OISF#6281

DNS v3 logging fixes the discrepancies between request and response logging with the main difference being queries always being placed in an array. Bug: OISF#6281

Bug: OISF#6281

V3 style DNS logging fixes the discrepancies between request and response logging better dns records and alert records. The main change is that queries and answers are always logged as arrays, and header fields are not logged in array items. For alerts this means that answers are now logged as arrays, queries already were. DNS records will get this new format as well, but with a configuration parameter. Bug: OISF#6281

DNS v3 logging fixes the discrepancies between request and response logging with the main difference being queries always being placed in an array. Bug: OISF#6281

Bug: OISF#6281

jufajardini added 22 commits July 27, 2021 13:14

PostgreSQL: adjust copyright years (mod/logger)

0d2e506

PostgreSQL: add TransactionState

5531536

pgsql: adapt decoder for multi-message transaction style (WIP)

PostgreSQL: find_or_create_tx for pgsql

635cb6b

- midway WIP, deciding if should keep this style or rollback

PostgreSQL: add logic for State Progress control

12e0346

Add author info to files

3365325

PgsqlState: store backend process info & change state progress

9824931

PostgreSQL: rename rust directory

23659a4

PostgreSQL: update yaml file

8bc3573

make sure PostgreSQL isn't enabled by default.

PostgreSQL: use PgsqlStateProgress (WIP)

26b0225

pgsql: Add directions to re-write parse_request and parse_response tests/fuzz: add PostgreSQL to confyaml.c pgsql: rewrite parse_request pgsql: rewrite parse_request (WIP) PostgreSQL: add debug to probe fn

Pgsql: fix infinite loop. compiles

5b87954

src/output-json-pgsql: fix formatting (clang)

f57202f

PostgreSQL: begin work on pgsql logger (WIP)

d30adfe

pgsql/log: rename from logger to log

0e065e9

yaml: reorder pgsql parameters appearance

1b70821

pgsql: convert logger to JsonBuilder

da63230

rust/pgsql: - add PgsqlTransaction to cbindgen context - add postgresql messages to logger, with filterable fields - make fields and structs public so logger can access those src/output-json-pgsql: - convert logger to jsonbuilder

rust/pgsql/parser: add message types, refine code

3657e28

rust/pgsql/pgsql: handle eof, use Debug calls.

3f71cef

rust/pgsql/pgsql: fix SSLrequestaccepted bug

6327270

pgsql: clean up logs (Debug prints, log msgs).

21cf855

rust/pgsql: handle eof, rewrite message types

ff8b534

jufajardini mentioned this pull request Jul 28, 2021

PostgreSQL: add initial support (WIP) #6062

Closed

3 tasks

victorjulien reviewed Jul 29, 2021

View reviewed changes

victorjulien added this to the 7.0 milestone Aug 23, 2021

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 21, 2024

doc: upgrade guide for dns logging changes

f472bcd

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 21, 2024

schema: remove dns fields no longer used

02d193b

- answer object - top level rrname and rrtype - header level details in answers Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 21, 2024

dns: use common logging functions for alerts

240ba40

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 21, 2024

doc: upgrade guide for dns logging changes

5ca729e

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 21, 2024

schema: remove dns fields no longer used

8a1d279

- answer object - top level rrname and rrtype - header level details in answers Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 21, 2024

dns: use common logging functions for alerts

bb09f57

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 21, 2024

doc: upgrade guide for dns logging changes

1fc6b0f

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 21, 2024

schema: remove dns fields no longer used

598652c

- answer object - top level rrname and rrtype - header level details in answers Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 27, 2024

dns: use common logging functions for alerts

68b166e

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 27, 2024

doc: upgrade guide for dns logging changes

d03b245

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jun 27, 2024

schema: remove dns fields no longer used

bc34fa2

- answer object - top level rrname and rrtype - header level details in answers Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 5, 2024

dns: add v3 dns logging

38124e0

DNS v3 logging fixes the discrepancies between request and response logging with the main difference being queries always being placed in an array. Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 5, 2024

doc: upgrade guide for dns logging changes

b8a2346

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 5, 2024

doc: upgrade guide for dns logging changes

a178fdb

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 5, 2024

dns: add v3 dns logging

61da314

DNS v3 logging fixes the discrepancies between request and response logging with the main difference being queries always being placed in an array. Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 5, 2024

doc: upgrade guide for dns logging changes

0203273

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 5, 2024

dns: add v3 dns logging

729ca50

DNS v3 logging fixes the discrepancies between request and response logging with the main difference being queries always being placed in an array. Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 5, 2024

doc: upgrade guide for dns logging changes

6551bd8

Bug: OISF#6281

jasonish added a commit to jasonish/suricata that referenced this pull request Jul 5, 2024

doc: upgrade guide for dns logging changes

091df00

Bug: OISF#6281

victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Jul 9, 2024

doc: upgrade guide for dns logging changes

d3c08b9

Bug: OISF#6281

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

postgresql: add initial support (WIP) - checkpoint #6281

postgresql: add initial support (WIP) - checkpoint #6281

jufajardini commented Jul 28, 2021 •

edited

Loading

codecov bot commented Jul 28, 2021

jufajardini commented Jul 28, 2021

suricata-qa commented Jul 29, 2021

victorjulien Jul 29, 2021

jufajardini Jul 29, 2021

victorjulien left a comment

victorjulien Jul 29, 2021

victorjulien Jul 29, 2021

jufajardini Aug 24, 2021 •

edited

Loading

victorjulien Jul 29, 2021

jufajardini Jul 29, 2021

victorjulien Jul 29, 2021

victorjulien Jul 29, 2021

victorjulien commented Aug 30, 2021

postgresql: add initial support (WIP) - checkpoint #6281

postgresql: add initial support (WIP) - checkpoint #6281

Conversation

jufajardini commented Jul 28, 2021 • edited Loading

codecov bot commented Jul 28, 2021

Codecov Report

jufajardini commented Jul 28, 2021

suricata-qa commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorjulien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jufajardini Aug 24, 2021 • edited Loading

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

victorjulien commented Aug 30, 2021

jufajardini commented Jul 28, 2021 •

edited

Loading

jufajardini Aug 24, 2021 •

edited

Loading