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.

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

[Cisco Secure Endpoint] Parse additional fields to ECS #6258

Merged
merged 16 commits into from
Jun 13, 2023

Conversation

MakoWish
Copy link
Contributor

@MakoWish MakoWish commented May 18, 2023

Type of change

  • Enhancement

What does this PR do?

This PR is to parse out the host.ip and host.mac fields from events' cisco.secure_endpoint.computer.network_addresses array of network MAC and IP addresses, as well as map host.id, group.id, error.code, and error.message from the Cisco Secure Endpoint 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 added an entry to my package's manifest.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • Added painless script for parsing IP addresses to host.ip
  • Added painless script for parsing MAC addresses to host.mac
  • Rename cisco.secure_endpoint.computer.connector_guid --> host.id
  • Rename cisco.secure_endpoint.error.description --> error.message
  • Rename cisco.secure_endpoint.error.error_code --> error.code
  • Rename cisco.secure_endpoint.group_guids --> group.id
  • Corrected some discrepancies in ecs.yml fields file
  • Regenerated expectations

Related issues

@MakoWish MakoWish requested a review from a team as a code owner May 18, 2023 18:08
@MakoWish MakoWish force-pushed the cisco_secure_endpoint_mac_address_fix branch from f2281ea to ccc8f76 Compare May 18, 2023 18:09
@elasticmachine
Copy link

elasticmachine commented May 18, 2023

💚 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-06-13T07:59:06.909+0000

  • Duration: 15 min 32 sec

Test stats 🧪

Test Results
Failed 0
Passed 14
Skipped 0
Total 14

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@MakoWish MakoWish changed the title Cisco secure endpoint mac address fix [Cisco Secure Endpoint] Parse host.ip and host.mac from events May 18, 2023
@MakoWish MakoWish marked this pull request as draft May 18, 2023 20:14
@MakoWish MakoWish changed the title [Cisco Secure Endpoint] Parse host.ip and host.mac from events [Cisco Secure Endpoint] Parse additional fields to ECS May 18, 2023
@MakoWish MakoWish marked this pull request as ready for review May 30, 2023 20:36
@elasticmachine
Copy link

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

@MakoWish
Copy link
Contributor Author

MakoWish commented May 30, 2023

I for some reason cannot set a value for error.message. If I try either a set or rename processor, I get:

FAILURE DETAILS:
cisco_secure_endpoint/event test-cisco-amp1.log:
[0] unexpected pipeline error: Object name not found
[1] unexpected pipeline error: Object path not found
cisco_secure_endpoint/event test-cisco-amp4.log:
[0] unexpected pipeline error: Delete pending
[1] unexpected pipeline error: Object name not found
cisco_secure_endpoint/event test-cisco-amp5.log:
[0] unexpected pipeline error: Cannot delete
[1] unexpected pipeline error: Delete pending
[2] unexpected pipeline error: Object name not found
cisco_secure_endpoint/event test-cisco-amp6.log:
[0] unexpected pipeline error: Cannot delete
[1] unexpected pipeline error: Delete pending
[2] unexpected pipeline error: Object name not found
cisco_secure_endpoint/event test-cisco-amp7.log:
[0] unexpected pipeline error: Delete pending
[1] unexpected pipeline error: Object name not found

When attempting to test. If I use an append processor, I get:

FAILURE DETAILS:
cisco_secure_endpoint/event test-cisco-amp1.log:
[0] unexpected pipeline error (unexpected error.message type []interface {}): [Object name not found]
[1] unexpected pipeline error (unexpected error.message type []interface {}): [Object path not found]
cisco_secure_endpoint/event test-cisco-amp4.log:
[0] unexpected pipeline error (unexpected error.message type []interface {}): [Delete pending]
[1] unexpected pipeline error (unexpected error.message type []interface {}): [Object name not found]
cisco_secure_endpoint/event test-cisco-amp5.log:
[0] unexpected pipeline error (unexpected error.message type []interface {}): [Cannot delete]
[1] unexpected pipeline error (unexpected error.message type []interface {}): [Delete pending]
[2] unexpected pipeline error (unexpected error.message type []interface {}): [Object name not found]
cisco_secure_endpoint/event test-cisco-amp6.log:
[0] unexpected pipeline error (unexpected error.message type []interface {}): [Cannot delete]
[1] unexpected pipeline error (unexpected error.message type []interface {}): [Delete pending]
[2] unexpected pipeline error (unexpected error.message type []interface {}): [Object name not found]
cisco_secure_endpoint/event test-cisco-amp7.log:
[0] unexpected pipeline error (unexpected error.message type []interface {}): [Delete pending]
[1] unexpected pipeline error (unexpected error.message type []interface {}): [Object name not found]

The error when trying to append would suggest error.message cannot be an array (not 100% sure on that), but the error when trying to set or rename doesn't tell me much of anything at all. Not sure how to get around that.

Eric

@efd6
Copy link
Contributor

efd6 commented May 30, 2023

@MakoWish Can you paste the code snippets for the approaches that you tried?

Note that error.message is for errors in the pipeline, rather than errors that are valid for the dataset. So the tests are interpreting the text you place in error.message (valid information for the dataset) as a complaint about the dataset. The "unexpected error.message type" warning is a distraction — that I have to personally apologise for; I'll fix it now — due to how we check the error messages and doesn't change the meaning of what I have above.

@MakoWish
Copy link
Contributor Author

@efd6

Note that error.message is for errors in the pipeline, rather than errors that are valid for the dataset.

Should the ECS Field Reference be updated to reflect that? I have been using error.message whenever the event contains an error message, as suggested in the documentation:

Use them for errors that happen while fetching events or in cases where the event itself contains an error.

Given an event with cisco.secure_endpoint.error.description = "Object name not found", any of these:

  - rename:
      field: cisco.secure_endpoint.error.description
      target_field: error.message
      ignore_missing: true
  - set:
      field: error.message
      value: '{{{cisco.secure_endpoint.error.description}}}'
      if: ctx.cisco?.secure_endpoint?.error?.description != null && ctx.cisco.secure_endpoint.error.description != ''
  - set:
      field: error.message
      copy_from: cisco.secure_endpoint.error.description
      if: ctx.cisco?.secure_endpoint?.error?.description != null && ctx.cisco.secure_endpoint.error.description != ''

causes elastic-package test -v to return:

FAILURE DETAILS:
cisco_secure_endpoint/event test-cisco-amp1.log:
[0] unexpected pipeline error: Object name not found

If I try an append processor:

  - append:
      field: error.message
      value: '{{{cisco.secure_endpoint.error.description}}}'
      if: ctx.cisco?.secure_endpoint?.error?.description != null && ctx.cisco.secure_endpoint.error.description != ''

the test results in:

FAILURE DETAILS:
cisco_secure_endpoint/event test-cisco-amp1.log:
[0] unexpected pipeline error (unexpected error.message type []interface {}): [Object name not found]

In all of these cases, the expected results JSON files do correctly get the error.message field populated, but elastic-package test -v gives the errors regardless.

@efd6
Copy link
Contributor

efd6 commented Jun 1, 2023

Should the ECS Field Reference be updated to reflect that? I have been using error.message whenever the event contains an error message, as suggested in the documentation:

Use them for errors that happen while fetching events or in cases where the event itself contains an error.

I agree, the documentation is unclear.

The part of the error message (unexpected error.message type []interface {}) can be ignored here, it is there to be helpful, but misses the mark in this situation. This PR addresses that and when it's merged will result in the same kind of error message as you see for the set processor setting of error.message. The reason for the parenthetic is that we want to check that people are not putting non-string values into error.message, but the check I put there failed to take into account how Go deserialises arrays of strings without a known type context. The PR above is intended to fix that.

@efd6
Copy link
Contributor

efd6 commented Jun 1, 2023

/test

@elasticmachine
Copy link

elasticmachine commented Jun 1, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 100.0% (1/1) 💚
Classes 100.0% (1/1) 💚
Methods 94.118% (16/17)
Lines 91.055% (509/559)
Conditionals 100.0% (0/0) 💚

@LaZyDK
Copy link
Contributor

LaZyDK commented Jun 1, 2023

Should the ECS Field Reference be updated to reflect that? I have been using error.message whenever the event contains an error message, as suggested in the documentation:

Use them for errors that happen while fetching events or in cases where the event itself contains an error.

I agree, the documentation is unclear.

The part of the error message (unexpected error.message type []interface {}) can be ignored here, it is there to be helpful, but misses the mark in this situation. This PR addresses that and when it's merged will result in the same kind of error message as you see for the set processor setting of error.message. The reason for the parenthetic is that we want to check that people are not putting non-string values into error.message, but the check I put there failed to take into account how Go deserialises arrays of strings without a known type context. The PR above is intended to fix that.

I agree on this one. I have been using error fields for all kinds of errors, just as ECS is stating:

These fields can represent errors of any kind.
Use them for errors that happen while fetching events or in cases where the event itself contains an error.

@efd6
Copy link
Contributor

efd6 commented Jun 1, 2023

It may be that this is elastic-package over-interpreting error.message. Ideally it should check for "event.kind":"pipeline_error", but we don't always set that. I'd like to see what @P1llus thinks.

Co-authored-by: Dan Kortschak <[email protected]>
@MakoWish
Copy link
Contributor Author

MakoWish commented Jun 2, 2023

Until that change happens, are you okay with utilizing error.message and just ignoring the test failures? Again, the "expected results" documents are properly created, and it would be compliant with the ECS documentation; it is just that the Elastic Package test fails (incorrectly IMO).

@efd6
Copy link
Contributor

efd6 commented Jun 4, 2023

Until that change happens, are you okay with utilizing error.message and just ignoring the test failures?

This is not possible; CI will not allow us to merge with failing builds. It may seem reasonable to allow this, but if we do that, now we have to assess whether the failure was a real failure or an unreasonable failure. This is not tenable.

I've raised this internally and the situation is essentially as I describe above; there is a dependency chain on behaviours.

  1. In order to allow relaxation of error.message state checks in testing we need to have an alternative method to detect pipeline error.
  2. This exists in the form of "event.kind":"pipeline_error", but is not uniformly applied, so
  3. We need to have all packages using "event.kind":"pipeline_error" for pipeline errors before we can change EP's behaviour.

This is a reasonable thing to want to do, but it is a non-trivial amount of work given the number of packages and teams that need to engage with it.

@efd6
Copy link
Contributor

efd6 commented Jun 4, 2023

/test

@MakoWish
Copy link
Contributor Author

MakoWish commented Jun 6, 2023

it is a non-trivial amount of work given the number of packages and teams that need to engage with it.

I don't know how your internal team would like to break out that task, but since I would benefit from it, I am willing to help get it done. Let me know, and I could start knocking out some PR's. I am sure you would want some sort of tracking on which have been completed, and which are pending? It would basically just be adding event.kind: pipeline_error like we did in the Arista integration, correct?

@efd6
Copy link
Contributor

efd6 commented Jun 7, 2023

I am willing to help get it done. Let me know, and I could start knocking out some PR's.

Adding the event.kind set to "pipeline_error" certainly would be helpful. We should file an issue; I'll raise it in our meeting.

@efd6
Copy link
Contributor

efd6 commented Jun 7, 2023

/test

@MakoWish
Copy link
Contributor Author

MakoWish commented Jun 8, 2023

@efd6,

Doesn't look like the test kicked off. Can you run another test?

@efd6
Copy link
Contributor

efd6 commented Jun 8, 2023

/test

@LaZyDK
Copy link
Contributor

LaZyDK commented Jun 9, 2023

@MakoWish I think that you will need to run pipeline and system tests again, after I added the new test file.

@LaZyDK
Copy link
Contributor

LaZyDK commented Jun 13, 2023

@efd6 Can you start the test on this one?

@efd6
Copy link
Contributor

efd6 commented Jun 13, 2023

/test

@elasticmachine
Copy link

💔 Build Failed

Failed CI Steps

@LaZyDK
Copy link
Contributor

LaZyDK commented Jun 13, 2023

LGTM

@MakoWish
Copy link
Contributor Author

I have never seen "buildkite" before. What is that? I get a "Page not found" if I click on "Details".

@efd6
Copy link
Contributor

efd6 commented Jun 13, 2023

@MakoWish It's the new build system we are using.

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Thanks

@efd6 efd6 merged commit 13992bf into elastic:main Jun 13, 2023
@MakoWish
Copy link
Contributor Author

MakoWish commented Jun 13, 2023

@MakoWish It's the new build system we are using.

Will non-Elasticians have visibility into failures with that system? I used to be able to see issues in Jenkins when one of you kicked off a test and it failed, but I got a 404 when I tried to see why it failed with buildkite.

@MakoWish MakoWish deleted the cisco_secure_endpoint_mac_address_fix branch June 13, 2023 21:44
@elasticmachine
Copy link

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

sodhikirti07 pushed a commit that referenced this pull request Jun 15, 2023
* Add parsing for `host.ip` and `host.mac`
* Correction to parsing of host.ip
@efd6
Copy link
Contributor

efd6 commented Jun 19, 2023

@MakoWish I have asked internally and will get back to you when I have an answer. Also, WRT your query above #6258 (comment), there is this now #6582.

@MakoWish
Copy link
Contributor Author

@MakoWish I have asked internally and will get back to you when I have an answer.

Looks like they already took action on that ask. I can now see the build statuses in buildkite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cisco Secure Endpoint] Parse additional fields to ECS
5 participants