Skip to content

Conversation

@vinit-chauhan
Copy link
Contributor

Type of change

  • Enhancement

What does this PR do?

Make enhancement in Cisco ISE connector with listed best practices.

Sr. No. Best Practices
1 Verify best practices test cases are executed
2 Verify all automated tests
3 Verify default values of port for TCP, UDP or HTTP Endpoint Input
4 Verify that the data is ingested in kibana for in interval
5 Verify all fields are correctly mapped for all Data streams
6 Verify related.ip field in datastream
7 Verify related.hosts field in datastream
8 Verify related.user field in datastream
9 Verify network.protocol is in lowercase form
10 Verify log.level is in lowercase form
11 Verify very long syslog message
12 Verify MAC address should be separated by dash and value should be an array and MAC address should be valid. MAC address is in uppercase form
13 Verify format of title of visualizations
14 Verify format of title of dashboard
15 Verify Datastream Dashboard
16 Verify filters in visualizations
17 Verify visualizations in different browser
18 Verify README file

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.

How to test this PR locally

  • Clone integrations repo.
  • Install elastic package locally.
  • Start elastic stack using elastic-package.
  • Move to integrations/packages/cisco_ise directory.
  • Run the following command to run tests.

elastic-package test

Related issues

Screenshots

image
image
image

@vinit-chauhan vinit-chauhan requested a review from a team as a code owner November 22, 2022 12:50
@elasticmachine
Copy link

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

@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elasticmachine
Copy link

elasticmachine commented Nov 22, 2022

💚 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: 2022-11-28T11:51:43.277+0000

  • Duration: 16 min 1 sec

Test stats 🧪

Test Results
Failed 0
Passed 37
Skipped 0
Total 37

🤖 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
Copy link

elasticmachine commented Nov 22, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 100.0% (17/17) 💚
Classes 100.0% (17/17) 💚
Methods 100.0% (176/176) 💚 1.961
Lines 99.003% (4566/4612) 👍 3.217
Conditionals 100.0% (0/0) 💚

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.

Some great enhancements here, some small nitpicks, mostly the same thing over and over again that might need to change a bit.

Comment on lines 11 to 13
- trim:
field: event.original
ignore_failure: true
Copy link
Member

Choose a reason for hiding this comment

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

Event original fields should never have any changes done to it, it should always be the raw field, please remove the processor.

ctx.event.category = eventCategory;
- grok:
field: cisco_ise.log.message.description
if: ctx.cisco_ise?.log?.message?.description != null && ctx.cisco_ise.log.message.description != ''
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have to be grok? Can we not simply rename description to event.action if the field is not null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @P1llus, There are two sections of the message, that are separated by a colon. We have extracted only the value before the colon in the message, as the first section of the message gives a high-level overview of the action performed. Moreover, the message can be very long and we don't think keeping the whole message as event.action is appropriate. Therefore, we opted to extract only the part before the : to be the event.action.
ex: message --> "Administrator-Login: Administrator authentication succeeded" and event.action --> "administrator-login"

field: error.message
value: '{{{_ingest.on_failure_message}}}'
- grok:
field: cisco_ise.log.message.description
Copy link
Member

Choose a reason for hiding this comment

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

Same here

value: '{{{_ingest.on_failure_message}}}'
- grok:
field: cisco_ise.log.message.description
if: ctx.cisco_ise?.log?.message?.description != null && ctx.cisco_ise.log.message.description != ''
Copy link
Member

Choose a reason for hiding this comment

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

Same here

ctx.event.action = ctx?.cisco_ise?.log?.message?.description?.splitOnToken(":")[0]?.toLowerCase();
- grok:
field: cisco_ise.log.message.description
if: ctx.cisco_ise?.log?.message?.description != null && ctx.cisco_ise.log.message.description != ''
Copy link
Member

Choose a reason for hiding this comment

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

Same here

field: cisco_ise.log.message.description
value: "RADIUS: An Access-Request MUST contain at least a NAS-IP-Address NAS-IPv6-Address, or a NAS-Identifier; Continue processing"
if: ctx?.cisco_ise?.log?.message?.code == "11015"
value: 'RADIUS: An Access-Request MUST contain at least a NAS-IP-Address NAS-IPv6-Address, or a NAS-Identifier; Continue processing'
Copy link
Member

Choose a reason for hiding this comment

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

Why would we need to set the description manually here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We manually set the message for this specific message code, "11015," because it has multiple commas: "RADIUS: An Access-Request MUST contain at least a NAS-IP-Address, NAS-IPv6-Address, or a NAS-Identifier; Continue processing." If we do not set it manually, it will result in an inappropriate mapping in the KV processor, because in all the other messages, we get an end of the message with " ," . Moreover, we split the message using it. Therefore, we skip that message in the grok and manually set the value of the message field.

- grok:
field: cisco_ise.log.message.description
if: ctx.cisco_ise?.log?.message?.description != null && ctx.cisco_ise.log.message.description != ''
patterns:
Copy link
Member

Choose a reason for hiding this comment

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

Same here

- grok:
field: cisco_ise.log.message.description
if: ctx.cisco_ise?.log?.message?.description != null && ctx.cisco_ise.log.message.description != ''
patterns:
Copy link
Member

Choose a reason for hiding this comment

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

.

- name: Cisco ISE
title: Cisco_ISE logs
description: Collect cisco_ise logs
description: Collect cisco_ise logs.
Copy link
Member

Choose a reason for hiding this comment

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

Should be Collect Cisco ISE logs.

@vinit-chauhan vinit-chauhan requested a review from P1llus November 29, 2022 10:23
@P1llus P1llus merged commit 41f06b7 into elastic:main Nov 29, 2022
@elasticmachine
Copy link

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

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

Labels

enhancement New feature or request Integration:cisco_ise Cisco ISE

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants