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.

Sign up for GitHub

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

Jump to bottom

Suricata alerts #9322

Merged
merged 15 commits into from
Jul 27, 2021
Merged

Suricata alerts #9322

merged 15 commits into from
Jul 27, 2021

Conversation

rogercoll
Copy link
Contributor

Required for all PRs:

  • [ x ] Updated associated README.md.
  • [ x ] Wrote appropriate unit tests.

resolves #9321

The code add the ability to parse logs of "alert" event type. Currently, there is only support for "stats" type. The PR separates the parse in different functions depending on the event type, it can be very helpful for future types integration.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 1, 2021
srebhan
srebhan reviewed Jun 1, 2021
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thank you very much @rogercoll for this PR. It's very nicely written. I do have only some minor comments in the code. Can you please check those.

@rogercoll @srebhan
Update plugins/inputs/suricata/suricata.go
8c87593 
Typo

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@srebhan srebhan self-assigned this Jun 1, 2021
@rogercoll @srebhan
Update plugins/inputs/suricata/suricata.go
00c9a46 
Use double # for comments

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@srebhan srebhan added the plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins label Jun 1, 2021
rogercoll and others added 2 commits June 1, 2021 18:21
@rogercoll @srebhan
Update plugins/inputs/suricata/suricata.go
f599df1 
Give more info about flattening error

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@rogercoll @srebhan
Update plugins/inputs/suricata/suricata.go
a051766 
Only check for alerts presence

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@rogercoll
Copy link
Contributor Author

@srebhan Thanks, sure! Very good comments. Just reviewing them and running tests again.

srebhan
srebhan reviewed Jun 2, 2021
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

@rogercoll thanks for the quick response. Found two more cosmetics :-) and would be happy if you can check them. I'll also trigger the CI tests and if those are ok I guess we are good to go.

rogercoll and others added 3 commits June 2, 2021 19:34
@rogercoll @srebhan
Update plugins/inputs/suricata/suricata.go
abd4959 
Simplify variable definition in type assertion

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@rogercoll @srebhan
Fix linter complain
0bf3d7f 
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@rogercoll
gofmt to suricata.go file
9300989
@rogercoll
Copy link
Contributor Author

rogercoll commented Jun 2, 2021 •
edited
Loading

@srebhan All comments closed. I see that the testing pipeline for mac is failing because a timeout. I thing this is not related to this PR, am I right?

Thanks

@rogercoll
Copy link
Contributor Author

@srebhan Is there any update on this? Can we rerun the pipelines?

Thanks

@srebhan
Copy link
Member

srebhan commented Jun 10, 2021

!retry-failed

srebhan
srebhan approved these changes Jun 10, 2021
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for submitting this!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jun 10, 2021
reimda
reimda suggested changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

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

Would you add a test case that covers the output metric format? In suricata_test.go, the func TestSuricata covers the existing "suricata" metric but we also need to test the new "suricata_alert" metric.

@rogercoll
Copy link
Contributor Author

@reimda sure, I will get back to you.

Thanks

@telegraf-tiger
Copy link
Contributor

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

@rogercoll
Copy link
Contributor Author

@reimda "suricata_alert" metric test added in new commits.

Thanks!

srebhan
srebhan approved these changes Jul 13, 2021
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Still looks good to me. :-)

@srebhan srebhan requested a review from reimda July 13, 2021 07:57
sspaink
sspaink approved these changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

lgtm

@sspaink sspaink merged commit 837eb31 into influxdata:master Jul 27, 2021
phemmer added a commit to phemmer/telegraf that referenced this pull request Aug 13, 2021
@phemmer
Merge remote-tracking branch 'origin/master' into postgres
fa5f937 
* origin/master: (183 commits)
  fix: CrateDB replace dots in tag keys with underscores (influxdata#9566)
  feat: Pull metrics from multiple AWS CloudWatch namespaces (influxdata#9386)
  fix: improve Clickhouse corner cases for empty recordset in aggregation queries, fix dictionaries behavior (influxdata#9401)
  fix(opcua): clean client on disconnect so that connect works cleanly (influxdata#9583)
  fix: Refactor ec2 init for config-api (influxdata#9576)
  fix: sort logs by timestamp before writing to Loki (influxdata#9571)
  fix: muting tests for udp_listener (influxdata#9578)
  fix: Do not return on disconnect to avoid breaking reconnect (influxdata#9524)
  fix: Fixing k8s nodes and pods parsing error (influxdata#9581)
  feat: OpenTelemetry output plugin (influxdata#9228)
  feat: Support AWS Web Identity Provider (influxdata#9411)
  fix: upgraded sensu/go to v2.9.0 (influxdata#9577)
  fix: Normalize unix socket path (influxdata#9554)
  docs: fix aws ec2 readme inconsistency (influxdata#9567)
  feat: Modbus Rtu over tcp enhancement (influxdata#9570)
  docs: information on new conventional commit format (influxdata#9573)
  docs: Add logo (influxdata#9574)
  docs: Adding links to net_irtt and dht_sensor external plugins (influxdata#9569)
  Upgrade hashicorp/consul/api to 1.9.1 (influxdata#9565)
  Update vmware/govmomi to v0.26.0 (influxdata#9552)
  Do not skip good quality nodes after a bad quality node is encountered (influxdata#9550)
  fix test so it hits a fake service (influxdata#9564)
  Update changelog
  Fix procstat plugin README to match sample config (influxdata#9553)
  Fix metrics reported as written but not actually written  (influxdata#9526)
  Prevent segfault in persistent volume claims (influxdata#9549)
  Update procstat to support cgroup globs & include systemd unit children (Copy of influxdata#7890) (influxdata#9488)
  Fix attempt to connect to an empty list of servers. (influxdata#9503)
  Fix handling bool in sql input plugin (influxdata#9540)
  Suricata alerts (influxdata#9322)
  Linter fixes for plugins/inputs/[fg]* (influxdata#9387)
  For Prometheus Input add ability to query Consul Service catalog (influxdata#5464)
  Support Landing page on Prometheus landing page (influxdata#8641)
  [Docs] Clarify tagging behavior (influxdata#9461)
  Change the timeout from all queries to per query (influxdata#9471)
  Attach the pod labels to the `kubernetes_pod_volume` & `kubernetes_pod_network` metrics. (influxdata#9438)
  feat(http_listener_v2): allows multiple paths and add path_tag (influxdata#9529)
  Bug Fix Snmp empty metric name (influxdata#9519)
  Worktable workfile stats (influxdata#8587)
  Update Go to v1.16.6 (influxdata#9542)
  ...
@wmandra
Copy link

wmandra commented Oct 4, 2021

@rogercoll this works well, however I'd like to make one suggestion. The current implementation will only output source/destination ip/port fields for rules where the "target" metadata keyword is used. Unfortunately none of the rules signatures provided by et/open, oisf, tgreen/hunting, or sslbl use this keyword, and neither do any of the default rules.

However, src_ip, src_port, dest_ip, dest_port are included in the json data heading along with other useful fields like in_iface and proto.

The test data in the PR doesn't accurately reflect 90+% of what will be generated by actual rules in production. I would suggest adding test4.json as:
{"timestamp":"2021-05-30T20:07:13.208777+0200","flow_id":1696236471136137,"in_iface":"s1-suricata","event_type":"alert","src_ip":"10.0.0.5","src_port":18715,"dest_ip":"179.60.192.3","dest_port":80,"proto":"TCP","alert":{"action":"allowed","gid":1,"signature_id":6,"rev":0,"signature":"Corrupted HTTP body","severity": 3,"category":"Misc activity","severity":3},"flow":{"pkts_toserver":1,"pkts_toclient":0,"bytes_toserver":174,"bytes_toclient":0,"start":"2021-05-30T20:07:13.208777+0200"}}

This is more accurate and will then cause the unit test to fail as the plugin will not parse the src/dest fields in the event.

@rogercoll
Copy link
Contributor Author

@wmandra Thanks for pointing that about!

Your test data makes more sense to me. I will open an issue asap.

@rogercoll
Copy link
Contributor Author

@wmandra Your testdata does not make the tests to fail, but it actually will produce an alert output without source.ip, source.port, target.ip and target.port.

The alerts without source and target information could use src_ip, dest_ip, ... fields to provide these information to the Telegraf output. The issue is that the actual implementation just parses the data inside "alert" or "stats", nothing outside those json keys. Thus we would have to change a bit the logic.

@wmandra Do you know if src_ip, dest_ip, src_port and dest_port are always provided when an alert is generated? In some stats suricata outputs they are not provided.

@wmandra
Copy link

wmandra commented Oct 11, 2021

As far as I can see on a live Suricata installation src_ip, src_port, dest_ip, dest_port, and proto fields are always included in the outer json of the alert event. I haven't seen a single alert event where source and target are included within the inner "alert" json.

@powersj powersj mentioned this pull request Apr 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sspaink sspaink sspaink approved these changes

@srebhan srebhan srebhan approved these changes

@reimda reimda Awaiting requested review from reimda

Assignees

@srebhan srebhan

Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add alert metrics for suricata input
5 participants
@rogercoll @srebhan @wmandra @sspaink @reimda