-
Notifications
You must be signed in to change notification settings - Fork 511
qualys_vmdr: Add latest transform for Asset Host Detections #13455
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: Add latest transform for Asset Host Detections #13455
Conversation
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that having these in separate files reflects the situation in the data stream's defs, but it makes me sad. I the rationale for this diff collision avoidance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fields in these files package.yml, resource.yml, vulnerability.yml are meant for extended ECS fields that are not in ECS definitions, but are used by the Elastic workflows.
Here both package.name, package.version are present in ECS: https://www.elastic.co/guide/en/ecs/current/ecs-package.html. But field package.fixed_version isn't. Hence it was separated.
It could also help checking diff between source fields and destination fields: https://github.com/elastic/integrations/blob/main/packages/qualys_vmdr/data_stream/asset_host_detection/fields/package.yml as they are now identical.
|
The PR will be moved to |
still waiting for credentials to test it. |
| move_on_creation: true | ||
| latest: | ||
| unique_key: | ||
| - vulnerability.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might be causing duplication of the documents (we see duplication in the test env). I remember we had this conversation before, but I need to find out the outcome. If having fields with multiple values as unique_key causing duplication we might need to find another way, eg. use QID+resource.id+namespace as a key. We need to understand better our options here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kcreddy @maxcold IMO we should use event.id+resource.id+namespace since we set event.id based on unique_vuln_id in host detection, or use qualys_vmdr.knowledge_base.qid field in knowledge_base ingest pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion @alexreal1314.
I have implemented it here: 591a0f0. Removed vulnerability.package.name and vulnerability.package.version as well.
| retention_policy: | ||
| time: | ||
| field: "@timestamp" | ||
| max_age: 4h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kcreddy am I correct that Qualys integration implemented in a way that it ingests the whole data every cycle?
If yes, we need to think about the value here. 4h is the default value but also configurable. For example InfoSec team set it to 12h now. this means that after 4h the findings will be gone for the next 8h. Until we implement the connection between the params and this value in the transform, setting higher value might be needed. Say 24h as a good middle ground or maybe even more (native CNVM has 72h for example). In this case for some time there might be some old vulnerabilities lingering around in the env, but it's better than having no findings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am I correct that Qualys integration implemented in a way that it ingests the whole data every cycle?
Yes it's correct, it's the default behaviour. However, there is the option to enable an incremental scan.
If yes, we need to think about the value here. 4h is the default value but also configurable. For example InfoSec team set it to 12h now. this means that after 4h the findings will be gone for the next 8h. Until we implement the connection between the params and this value in the transform, setting higher value might be needed. Say 24h as a good middle ground or maybe even more (native CNVM has 72h for example). In this case for some time there might be some old vulnerabilities lingering around in the env, but it's better than having no findings
I agree with your analysis. I would like to add details that will likely influence the value:
- If Qualys is configured with authenticated scan (either through the Qualys agent, either through network scan), Qualys will see if the vulnerability is fixed and will change the status as fixed.
- If Qualys is configured to scan with the Qualys agent, the minimal scanning interval is 4h and the default is also 4h
To sum up, we'll see old vulnerabilities lingering around only on non-authenticated network scans. As it's not a recommended practice to run unauthenticated scans, we should be fine most of the time. EDIT: I think it's inaccurate unless the CNVM functionality is filtering fixed or ignored vulnerabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion @maxcold and @clement-fouque
I updated the retention to 24h here: 591a0f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clement-fouque thanks for sharing more context! We will work toward connecting the data retention param on the transform with the params of the integration, but for now, I think we should proceed with the longer retention period
🚀 Benchmarks reportTo see the full report comment with |
|
@kcreddy we probably need to update |
@maxcold, Okay. Do you think merging this PR closer to |
|
@nick-alayil can you provide your input on
This would mean that the Qualys data won't be available in Serverless before 8.19/9.1 release in June. Are we ok with that? do we have customers in Serverless we would like to enable with this feature? |
Thanks for the input @nick-alayil and @maxcold. |
|
The CI error should be fixed by elastic/elasticsearch#126417. |
|
Running this locally on v8.19.0-SNAPSHOT, I have minimised this to This ceases to fail when Using the dev tools gives an indication why this might be: gives while when the document is If I run the same failing simulate request on v8.17.3, I get no failure. The image for the v8.19.0-SNAPSHOT is 3c3225efef7e8285bb2c5de5cf0b15fe4747677fc84473a2b3db53fe2391212e, which reports |
|
Thanks @efd6 for the analysis. It is indeed a variation of elastic/elasticsearch#126417 but for lists. @joegallo made the fix in elastic/elasticsearch#127006 and should be available in |
|
/test |
2 similar comments
|
/test |
|
/test |
💚 Build Succeeded
History
cc @kcreddy |
|
|
Hi @kcreddy @maxcold I've tested latest changes on my env https://alex-dev91.kb.us-west2.gcp.elastic-cloud.com/ and seems that after we changed the unique key of the transform there are no duplicates. this is the query i ran: so from my side we are good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for pushing this forward @kcreddy !
|
Package qualys_vmdr - 6.6.0 containing this change is available at https://epr.elastic.co/package/qualys_vmdr/6.6.0/ |



Proposed commit message
Checklist
changelog.ymlfile.How to test this PR locally
Related issues