[Fortigate] Fix startsWith for null value and add support for login events #8670

philippkahr · 2023-12-07T14:44:00Z

Hi,

when running the event pipeline it could happen that the field is not populated and thus errors with a null value does not have startsWith ... therefore I added the null check using the Elvis operator as in:

'ctx.rule?.description?.startsWith(''Dynamic address updated'') ?: false'

and support for login events.

Checklist

I have reviewed tips for building integrations and this pull request is aligned with them.
I have verified that all data streams collect metrics or logs.
I have added an entry to my package's changelog.yml file.
I have verified that Kibana version constraints are current according to guidelines.

elasticmachine · 2023-12-07T15:02:42Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-12-13T19:46:55.486+0000
Duration: 17 min 29 sec

Test stats 🧪

Test	Results
Failed	0
Passed	11
Skipped	0
Total	11

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elasticmachine · 2023-12-07T15:02:52Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`1/1`)	💚
Files	100.0% (`5/5`)	💚 3.929
Classes	100.0% (`5/5`)	💚 3.929
Methods	100.0% (`46/46`)	💚 7.593
Lines	93.29% (`1446/1550`)	👍 4.665
Conditionals	100.0% (`0/0`)	💚

elasticmachine · 2023-12-08T15:39:36Z

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

packages/fortinet_fortigate/data_stream/log/elasticsearch/ingest_pipeline/event.yml

…st_pipeline/event.yml Co-authored-by: Krishna Chaitanya Reddy Burri <[email protected]>

kcreddy · 2023-12-12T09:18:19Z

/test

kcreddy · 2023-12-12T09:20:04Z

@philippkahr can you add sample log to check if the problem is fixed?
Ideally a log containing null value of rule.description that doesn't produce the error null value does not have startsWith ...

philippkahr · 2023-12-12T13:05:21Z

With the following addition of a log line and

<190>devname="firewall" devid="FG201EAB12CD34EF" vd="root" date=2021-05-07 time=08:31:14 eventtime=1620372674900027938 tz="+0100" logid="0112053203" type="event" subtype="connector" level="information" fctemssn="(null)" addr="FCTEMS0000011111_AV-Running" msg="Updated tag FCTEMS0000011111_AV-Running."

Running without null check:

elastic-package test pipeline --generate
2023/12/12 13:58:39  INFO New version is available - v0.95.0. Download from: https:/elastic/elastic-package/releases/tag/v0.95.0
Run pipeline tests for the package
--- Test results for package: fortinet_fortigate - START ---
FAILURE DETAILS:
fortinet_fortigate/log test-fortinet.log:
[0] unexpected pipeline error: [cannot access method/field [description] from a null def reference]


╭────────────────────┬─────────────┬───────────┬───────────────────────┬─────────────────────────────────────────────────────────────────────────────┬──────────────╮
│ PACKAGE            │ DATA STREAM │ TEST TYPE │ TEST NAME             │ RESULT                                                                      │ TIME ELAPSED │
├────────────────────┼─────────────┼───────────┼───────────────────────┼─────────────────────────────────────────────────────────────────────────────┼──────────────┤
│ fortinet_fortigate │ log         │ pipeline  │ test-fortinet-6-2.log │ PASS                                                                        │  28.488208ms │
│ fortinet_fortigate │ log         │ pipeline  │ test-fortinet-7-4.log │ PASS                                                                        │  30.433333ms │
│ fortinet_fortigate │ log         │ pipeline  │ test-fortinet.log     │ FAIL: test case failed: one or more problems with fields found in documents │   45.25075ms │
╰────────────────────┴─────────────┴───────────┴───────────────────────┴─────────────────────────────────────────────────────────────────────────────┴──────────────╯
--- Test results for package: fortinet_fortigate - END   ---
Done
Error: one or more test cases failed

Running with null check:

[0] parsing field value failed: the IP "FCTEMS0000011111_AV-Running" is not one of the allowed test IPs (see: https:/elastic/elastic-package/blob/main/internal/fields/_static/allowed_geo_ips.txt)

So We are now keeping the fortinet.firewall.addr instead of renaming it to the grp keyword. however, I managed to circumvent this issue by adding a convert processor and an on_failure handler. I don't know how I could otherwise "easily" check if the value inside this field is an IP. Some regex might work but that could be expensive and error prone.

I added this bit:

  - convert:
      field: fortinet.firewall.addr
      type: ip
      if: ctx.fortinet?.firewall?.addr != null && ctx.fortinet?.firewall?.addrgrp == null
      on_failure:
        - rename:
            field: fortinet.firewall.addr
            target_field: fortinet.firewall.addrgrp

and we can actually remove the startsWith clause and apply this change to all elements that contain the fortinet.firewall.addr

kcreddy · 2023-12-13T07:22:51Z

Hey @philippkahr,
Apologies for lack of knowledge on Fortigate, but if I understand your intention correctly, in current integration we have this processors in https:/elastic/integrations/blob/main/packages/fortinet_fortigate/data_stream/log/elasticsearch/ingest_pipeline/event.yml#L196-L204:

  - rename:
      field: fortinet.firewall.logdesc
      target_field: rule.description
      ignore_missing: true
  - rename:
      field: fortinet.firewall.addr
      target_field: fortinet.firewall.addrgrp
      if: ctx.rule?.description.startsWith('Dynamic address updated')
      ignore_missing: true

and fortinet.firewall.addr is of type ip and fortinet.firewall.addrgrp is of type keyword.

Since this fails when fortinet.firewall.logdesc is null (doesn't exist), you want to capture the addr into addrgrp instead.
If so, I propose following change to second rename processor, and this is the only change required. No need for convert processor:

  - rename:
      field: fortinet.firewall.addr
      target_field: fortinet.firewall.addrgrp
      if: (ctx.rule?.description == null || (ctx.rule?.description != null && ctx.rule.description.startsWith('Dynamic address updated'))
      ignore_missing: true

philippkahr · 2023-12-13T07:33:08Z

Hi @kcreddy
If you look into the example log message, there is this field: addr="FCTEMS0000011111_AV-Running" and apparently that can either be a keyword or an IP. This key value pair has then the field name: fortinet.firewall.addr. I would ignore the rule.description and rather make sure that the addr we have, is always of type IP and that we don't try to write a keyword field into the mapped IP field. That is what I am doing above with the convert.

kcreddy · 2023-12-13T07:55:21Z

I was a bit unsure when the startsWith was removed completely, but after taking a look at the sample logs for logdesc and its corresponding addr, it makes sense to actually ignore startsWith logic and having a convert.

kcreddy · 2023-12-13T07:55:25Z

/test

kcreddy · 2023-12-13T08:24:53Z

Can you also update version to 1.22.3 inside manifest.yml?
https:/elastic/integrations/blob/main/packages/fortinet_fortigate/manifest.yml#L3

elasticmachine · 2023-12-13T11:16:03Z

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

packages/fortinet_fortigate/data_stream/log/elasticsearch/ingest_pipeline/event.yml

packages/fortinet_fortigate/data_stream/log/elasticsearch/ingest_pipeline/login.yml

…st_pipeline/event.yml Co-authored-by: Krishna Chaitanya Reddy Burri <[email protected]>

…grations into fortigate-fixes

kcreddy · 2023-12-13T17:28:37Z

/test

kcreddy

CI failing on an older README file. Can you run below command and commit the updated README?
elastic-package build && elastic-package format && elastic-package lint && elastic-package check && elastic-package build

packages/fortinet_fortigate/data_stream/log/elasticsearch/ingest_pipeline/login.yml

kcreddy

LGTM 👍🏼

elasticmachine · 2023-12-14T06:27:46Z

Package fortinet_fortigate - 1.23.0 containing this change is available at https://epr.elastic.co/search?package=fortinet_fortigate

elasticmachine · 2023-12-14T06:27:47Z

Package fortinet_fortigate - 1.23.0 containing this change is available at https://epr.elastic.co/search?package=fortinet_fortigate

elasticmachine · 2024-01-11T12:14:16Z

Package fortinet_fortigate - 1.23.1 containing this change is available at https://epr.elastic.co/search?package=fortinet_fortigate

Fix startsWith for null value

5fa4a45

philippkahr added the bug Something isn't working, use only for issues label Dec 7, 2023

philippkahr requested a review from a team as a code owner December 7, 2023 14:44

bugfix pr

0be893a

philippkahr mentioned this pull request Dec 7, 2023

Catch null values and function calls elastic/package-spec#678

Open

jamiehynds added the Team:Security-External Integrations label Dec 8, 2023

jamiehynds added the Integration:Fortinet (Deprecated) Use one of the specific fortinet_X labels. label Dec 8, 2023

kcreddy reviewed Dec 12, 2023

View reviewed changes

packages/fortinet_fortigate/data_stream/log/elasticsearch/ingest_pipeline/event.yml Outdated Show resolved Hide resolved

Update packages/fortinet_fortigate/data_stream/log/elasticsearch/inge…

7468efe

…st_pipeline/event.yml Co-authored-by: Krishna Chaitanya Reddy Burri <[email protected]>

Merge branch 'main' into fortigate-fixes

249b67b

fixing fortinet firewall

430a569

Added support for login events

5d201e9

philippkahr changed the title ~~[Fortigate] Fix startsWith for null value~~ [Fortigate] Fix startsWith for null value and add support for login events Dec 13, 2023

kcreddy reviewed Dec 13, 2023

View reviewed changes

philippkahr and others added 3 commits December 13, 2023 12:55

Update packages/fortinet_fortigate/data_stream/log/elasticsearch/inge…

9b3377a

…st_pipeline/event.yml Co-authored-by: Krishna Chaitanya Reddy Burri <[email protected]>

Fixed the comments

a6dfe39

Merge branch 'fortigate-fixes' of https:/philippkahr/inte…

61402ce

…grations into fortigate-fixes

kcreddy reviewed Dec 13, 2023

View reviewed changes

packages/fortinet_fortigate/data_stream/log/elasticsearch/ingest_pipeline/login.yml Outdated Show resolved Hide resolved

here go you

3881a2b

kcreddy approved these changes Dec 14, 2023

View reviewed changes

kcreddy merged commit 947b69a into elastic:main Dec 14, 2023
4 checks passed

philippkahr deleted the fortigate-fixes branch December 14, 2023 08:48

andrewkroh added Integration:fortinet_fortigate Fortinet FortiGate Firewall Logs and removed Integration:Fortinet (Deprecated) Use one of the specific fortinet_X labels. labels Jul 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fortigate] Fix startsWith for null value and add support for login events #8670

[Fortigate] Fix startsWith for null value and add support for login events #8670

philippkahr commented Dec 7, 2023 •

edited

Loading

elasticmachine commented Dec 7, 2023 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented Dec 7, 2023 •

edited

Loading

elasticmachine commented Dec 8, 2023

kcreddy commented Dec 12, 2023

kcreddy commented Dec 12, 2023

philippkahr commented Dec 12, 2023

kcreddy commented Dec 13, 2023 •

edited

Loading

philippkahr commented Dec 13, 2023

kcreddy commented Dec 13, 2023

kcreddy commented Dec 13, 2023

kcreddy commented Dec 13, 2023

elasticmachine commented Dec 13, 2023

kcreddy commented Dec 13, 2023

kcreddy left a comment

kcreddy left a comment

elasticmachine commented Dec 14, 2023

elasticmachine commented Dec 14, 2023

elasticmachine commented Jan 11, 2024

[Fortigate] Fix startsWith for null value and add support for login events #8670

[Fortigate] Fix startsWith for null value and add support for login events #8670

Conversation

philippkahr commented Dec 7, 2023 • edited Loading

Checklist

elasticmachine commented Dec 7, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

elasticmachine commented Dec 7, 2023 • edited Loading

🌐 Coverage report

elasticmachine commented Dec 8, 2023

kcreddy commented Dec 12, 2023

kcreddy commented Dec 12, 2023

philippkahr commented Dec 12, 2023

kcreddy commented Dec 13, 2023 • edited Loading

philippkahr commented Dec 13, 2023

kcreddy commented Dec 13, 2023

kcreddy commented Dec 13, 2023

kcreddy commented Dec 13, 2023

elasticmachine commented Dec 13, 2023

🚀 Benchmarks report

kcreddy commented Dec 13, 2023

kcreddy left a comment

Choose a reason for hiding this comment

kcreddy left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 14, 2023

elasticmachine commented Dec 14, 2023

elasticmachine commented Jan 11, 2024

philippkahr commented Dec 7, 2023 •

edited

Loading

elasticmachine commented Dec 7, 2023 •

edited

Loading

elasticmachine commented Dec 7, 2023 •

edited

Loading

kcreddy commented Dec 13, 2023 •

edited

Loading