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

Trendmicro Integration #4471

Merged
merged 24 commits into from
Feb 9, 2023
Merged

Trendmicro Integration #4471

merged 24 commits into from
Feb 9, 2023

Conversation

emnp
Copy link
Contributor

@emnp emnp commented Oct 18, 2022

What does this PR do?

Add integration for TrendMicro deep security logs.

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.

Author's Checklist

  • Deep Security
  • Apex Central
  • Kibana Visualizations & Dahsboards

How to test this PR locally

# cd integrations/packages/trendmicro
# elastic-package build
# elastic-package stack up -d -v
# eval "$(elastic-package stack shellinit)"
# elastic-package test  -v

Related issues

Screenshots

@elasticmachine
Copy link

elasticmachine commented Oct 18, 2022 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-02-02T13:55:02.735+0000

  • Duration: 16 min 31 sec

Test stats 🧪

Test Results
Failed 0
Passed 6
Skipped 0
Total 6

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@emnp emnp mentioned this pull request Oct 18, 2022
Closed
7 tasks
@P1llus P1llus self-assigned this Oct 18, 2022
@elasticmachine
Copy link

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

@P1llus
Copy link
Member

P1llus commented Oct 18, 2022

@emnp the license checks is passing now which is great!
I will start the tests and review it tomorrow then, much appreciated!

@emnp
Copy link
Contributor Author

emnp commented Oct 18, 2022

@P1llus , sorry :( I left to update some files.

@P1llus
Copy link
Member

P1llus commented Oct 23, 2022

/test

P1llus
P1llus reviewed Oct 23, 2022
View reviewed changes
Copy link
Member

@P1llus P1llus left a comment

Choose a reason for hiding this comment

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

I added as much as I could for a first review, we might need to require a bit more detailed README, and a few questions we still wonder about, but I am sure we can work it out!

Very much thanks for doing this PR, and I feel it already has pretty much everything we need! :)

packages/trendmicro/_dev/build/build.yml Outdated Show resolved Hide resolved
...es/trendmicro/data_stream/deep_security/_dev/test/pipeline/test-trendmicro.log-expected.json Outdated
"version": "8.4.0"
},
"error": {
"message": "For input string: \\\"\\\""
Copy link
Member

Choose a reason for hiding this comment

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

This parsing seems to have errors?

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 got this when I test the pipeline but it worked well when I did system test and also when I tried to send the log file with elastic agent, it worked well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@emnp the issue seems to be in signature_id which is being set to event.code, but the value of event.code is never populated in the pipeline. This is making signature_id as empty string and causing pipeline test failures

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved by changing pipeline tests log structure as it should be the step after cef_decode processor

packages/trendmicro/_dev/deploy/docker/sample_logs/trendmicro.log Outdated Show resolved Hide resolved
packages/trendmicro/data_stream/deep_security/elasticsearch/ingest_pipeline/default.yml Outdated Show resolved Hide resolved
packages/trendmicro/manifest.yml Outdated
title: Trendmicro Deep Security logs
description: Collect deep security logs
inputs:
- type: logfile
Copy link
Member

Choose a reason for hiding this comment

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

Could you change this from logfile to filestream?

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'm not clear with this one. Need to change logfile to filestream? I set it as logfile currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

@emnp Please refer here for filestream input. The config is very similar to log input but its more efficient.

You will need to specify this in 2 places:
https://github.com/elastic/integrations/pull/4471/files#diff-5a93912aba1067c2a133245e39a2b8c338202478242669fa111607f603449eceR28
and
https://github.com/elastic/integrations/pull/4471/files#diff-79bde44df376a077217c8e85fa3e4e44cbf16aacb73ace2d101e4a7f4eb8d944R4

Refer existing integration:

integrations/packages/hid_bravura_monitor/manifest.yml

Line 46 in bbf0e61

- type: filestream

And this one:

integrations/packages/hid_bravura_monitor/data_stream/log/manifest.yml

Line 4 in bbf0e61

- input: filestream

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved by updating to filestream

packages/trendmicro/manifest.yml Show resolved Hide resolved
packages/trendmicro/manifest.yml Outdated Show resolved Hide resolved
@P1llus
Copy link
Member

P1llus commented Oct 24, 2022

/test

@emnp
Copy link
Contributor Author

emnp commented Oct 31, 2022

Hello @P1llus, sorry for the delay. I've updated some files last night. Could you please help to check?

@andrewkroh
Copy link
Member

/test

@P1llus
Copy link
Member

P1llus commented Nov 24, 2022

/test

@elasticmachine
Copy link

elasticmachine commented Nov 24, 2022 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 22.222% (2/9) 👎 -61.111
Classes 22.222% (2/9) 👎 -61.111
Methods 33.333% (14/42) 👎 -56.14
Lines 23.007% (101/439) 👎 -62.756
Conditionals 100.0% (0/0) 💚

@kcreddy
Copy link
Contributor

kcreddy commented Nov 29, 2022

/test

@ShourieG
Copy link
Contributor

ShourieG commented Dec 5, 2022

@emnp Can you add an entry to .github/CODEOWNERS for this package? CI is failing otherwise.

kcreddy
kcreddy reviewed Dec 27, 2022
View reviewed changes
packages/trendmicro/manifest.yml Outdated
title: Trendmicro Deep Security logs
description: Collect deep security logs
inputs:
- type: logfile
Copy link
Contributor

Choose a reason for hiding this comment

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

@emnp Please refer here for filestream input. The config is very similar to log input but its more efficient.

You will need to specify this in 2 places:
https://github.com/elastic/integrations/pull/4471/files#diff-5a93912aba1067c2a133245e39a2b8c338202478242669fa111607f603449eceR28
and
https://github.com/elastic/integrations/pull/4471/files#diff-79bde44df376a077217c8e85fa3e4e44cbf16aacb73ace2d101e4a7f4eb8d944R4

Refer existing integration:

integrations/packages/hid_bravura_monitor/manifest.yml

Line 46 in bbf0e61

- type: filestream

And this one:

integrations/packages/hid_bravura_monitor/data_stream/log/manifest.yml

Line 4 in bbf0e61

- input: filestream

packages/trendmicro/data_stream/deep_security/_dev/test/pipeline/test-common-config.yml Outdated Show resolved Hide resolved
packages/trendmicro/changelog.yml Outdated Show resolved Hide resolved
...dmicro/data_stream/deep_security/elasticsearch/ingest_pipeline/application-control-event.yml
processor:
set:
field: "{{_ingest._value.to}}"
value: "{{_ingest._value.value}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

add ignore_missing to avoid failure

...dmicro/data_stream/deep_security/elasticsearch/ingest_pipeline/application-control-event.yml Show resolved Hide resolved
packages/trendmicro/data_stream/deep_security/elasticsearch/ingest_pipeline/default.yml Outdated Show resolved Hide resolved
packages/trendmicro/data_stream/deep_security/elasticsearch/ingest_pipeline/default.yml

- remove:
field:
- cef
Copy link
Contributor

Choose a reason for hiding this comment

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

add ignore_missing

packages/trendmicro/data_stream/deep_security/elasticsearch/ingest_pipeline/firewall-event.yml

- remove:
field:
- _tmp_copy
Copy link
Contributor

Choose a reason for hiding this comment

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

add ignore_missing

packages/trendmicro/data_stream/deep_security/elasticsearch/ingest_pipeline/firewall-event.yml Show resolved Hide resolved
packages/trendmicro/data_stream/deep_security/elasticsearch/ingest_pipeline/default.yml Show resolved Hide resolved
@botelastic
Copy link

botelastic bot commented Jan 26, 2023

Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

@botelastic botelastic bot added the Stalled label Jan 26, 2023
@P1llus
Copy link
Member

P1llus commented Feb 2, 2023

@kcreddy i think it might be good if we apply the last changes to the PR of thats okay?

@botelastic botelastic bot removed the Stalled label Feb 2, 2023
@kcreddy
Copy link
Contributor

kcreddy commented Feb 2, 2023

/test

@P1llus
Copy link
Member

P1llus commented Feb 3, 2023

@emnp we have added the last changes needed before a merge, just wanted to check in with you that this is okay?

Sorry for the unusual long waiting time here!

@kcreddy kcreddy added the v8.7.0 label Feb 6, 2023
@kcreddy kcreddy self-assigned this Feb 9, 2023
kcreddy
kcreddy approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

LGTM

@kcreddy kcreddy merged commit 7dfede0 into elastic:main Feb 9, 2023
@kcreddy kcreddy added the Integration:TrendMicro TrendMicro label Feb 9, 2023
@elasticmachine
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@P1llus P1llus P1llus left review comments

@jamiehynds jamiehynds jamiehynds left review comments

@ShourieG ShourieG ShourieG left review comments

@kcreddy kcreddy kcreddy approved these changes

Assignees

@P1llus P1llus

@kcreddy kcreddy

Labels
Integration:TrendMicro TrendMicro New Integration v8.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@emnp @elasticmachine @P1llus @andrewkroh @kcreddy @ShourieG @jamiehynds