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

qualys_vmdr: expand documents to map each vulnerability per host #9293

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Mar 7, 2024

Proposed commit message

See title.

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

  • [ ]

How to test this PR locally

Related issues

Screenshots

@efd6 efd6 added enhancement New feature or request Integration: Qualys VMDR Team:Security-Service Integrations Security Service Integrations Team labels Mar 7, 2024
@efd6 efd6 self-assigned this Mar 7, 2024
Comment on lines +39 to +43
"events": body.doc.HOST_LIST_VM_DETECTION_OUTPUT.RESPONSE.HOST_LIST.HOST.map(h,
h.DETECTION_LIST.DETECTION.map(v, {
"message": h.with({"DETECTION_LIST": v}).encode_json(),
})
).flatten(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A less invasive change that does not require any of the ingest pipeline changes, and is not a breaking change, but does not match the issue is

Suggested change
"events": body.doc.HOST_LIST_VM_DETECTION_OUTPUT.RESPONSE.HOST_LIST.HOST.map(h,
h.DETECTION_LIST.DETECTION.map(v, {
"message": h.with({"DETECTION_LIST": v}).encode_json(),
})
).flatten(),
"events": body.doc.HOST_LIST_VM_DETECTION_OUTPUT.RESPONSE.HOST_LIST.HOST.map(h,
h.DETECTION_LIST.DETECTION.map(v, {
"message": h.with({"DETECTION_LIST": {"DETECTION": [v]}}).encode_json(),
})
).flatten(),

This would still map each vuln of each host to a separate document, but would leave the detection in an array of one with the old name. This would mean that it does not break any existing users, but as noted in the issue, "With the current integration, we can't easily create dashboards and enrich vulnerability data (not sure it's possible in an array)." Also the name would now be misleading since it's only an array in shape.

Copy link

@clement-fouque clement-fouque Mar 7, 2024

Choose a reason for hiding this comment

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

I don't think there are many people using it in Production at the moment. And I'm not sure we can enrich from the Qualys Knowledge Base from an array (even with 1 value).

I'm in favour to have something cleaner and not store the vulnerability in an array of one value (not sure if it'll be the case or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I agree, but wanted to make sure.

@elasticmachine
Copy link

elasticmachine commented Mar 7, 2024

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@efd6 efd6 marked this pull request as ready for review March 7, 2024 08:23
@efd6 efd6 requested a review from a team as a code owner March 7, 2024 08:23
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@jamiehynds
Copy link

fyi @clement-fouque

Copy link

@clement-fouque clement-fouque left a comment

Choose a reason for hiding this comment

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

It looks good to me. I'm wondering if we should name the parent field qualys_vmdr.vulnerability or something else. But I don't think about something better.

target_field: qualys_vmdr.asset_host_detection.vulnerability
ignore_missing: true
- rename:
if: ctx.qualys_vmdr?.asset_host_detection?.vulnerability != null
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this null check on root field indicate the inner field qualys_vmdr.asset_host_detection.vulnerability.UNIQUE_VULN_ID to be non-null? Just making sure because its same for other processors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Checking the XSD, they are all minOccurs='0' maxOccurs='1'. Will fix.

Comment on lines +982 to +985
on_failure:
- append:
field: error.message
value: 'Processor {{{_ingest.on_failure_processor_type}}} with tag {{{_ingest.on_failure_processor_tag}}} in pipeline {{{_ingest.pipeline}}} failed with message: {{{_ingest.on_failure_message}}}'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also capture failures for all above date processors to improve telemetry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add.

@efd6 efd6 requested a review from kcreddy March 13, 2024 07:51
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 👍🏼

@efd6 efd6 enabled auto-merge (squash) March 13, 2024 07:57
@elasticmachine
Copy link

💚 Build Succeeded

History

cc @efd6

Copy link

@efd6 efd6 merged commit 143d85d into elastic:main Mar 13, 2024
5 checks passed
@elasticmachine
Copy link

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

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: Qualys VMDR Team:Security-Service Integrations Security Service Integrations Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Qualys VMDR integration to one doc per vulnerability
5 participants