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 CVE list for each vulnerability #9375

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Mar 17, 2024

Proposed commit message

Reference for the DTD: https://cdn2.qualys.com/docs/qualys-api-vmpc-xml-dtd-reference.pdf

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

@elasticmachine
Copy link

elasticmachine commented Mar 17, 2024

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

This is the equivalent change to the asset_host_detection change for
v2.0.0

Reference for the DTD: https://cdn2.qualys.com/docs/qualys-api-vmpc-xml-dtd-reference.pdf
@clement-fouque
Copy link
Contributor

I think we can keep it to something simpler like an array of all CVEs. If we have one document per CVE per QID, it'll make enrichment harder (as it'll match multiple documents). By keeping an array of CVEs, we can easily enrich on any CVE in the array.

I think the final document should be similar to the following:

[
    {
        "qualys_vmdr": {
            "knowledge_base": {
                "title": "Amazon Linux Security Advisory for Hypertext Preprocessor (PHP) : ALAS2PHP8.0-2023-005",
                "qid": "356044",
                "cve_list": [
                    "CVE-2022-31629",
                    "CVE-2022-31628"
                ]
            }
        }
    },
    {
        "qualys_vmdr": {
            "knowledge_base": {
                "title": "Another vulnerability",
                "qid": "111111",
                "cve_list": [
                    "CVE-2024-00001",
                    "CVE-2024-00002"
                ]
            }
        }
    }
]

@efd6
Copy link
Contributor Author

efd6 commented Mar 18, 2024

@clement-fouque So, you basically just want to drop the URLs and remove the then-redundant object?

@clement-fouque
Copy link
Contributor

@clement-fouque So, you basically just want to drop the URLs and remove the then-redundant object?

Yes, correct (if by then-redundant object you mean one document per CVE per QID).

@efd6
Copy link
Contributor Author

efd6 commented Mar 18, 2024

if by then-redundant object you mean one document per CVE per QID

Not quite; from cve_list: [{id, url}, ...] to cve_list: [id, ...].

@efd6
Copy link
Contributor Author

efd6 commented Mar 18, 2024

Also, I'm trying to understand how this would be consistent with ECS. As noted in the issue this would result in an array in vulnerability.id, but this is not noted to be an array.

@efd6
Copy link
Contributor Author

efd6 commented Mar 18, 2024

This is arguably now not a breaking change. The field definitions are different to how they were before, but they did not work previously.

@elasticmachine
Copy link

💚 Build Succeeded

History

Copy link

@efd6 efd6 marked this pull request as ready for review March 18, 2024 21:33
@efd6 efd6 requested a review from a team as a code owner March 18, 2024 21:33
@efd6 efd6 self-assigned this Mar 18, 2024
@efd6 efd6 added enhancement New feature or request Integration:qualys_vmdr Qualys VMDR Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations] labels Mar 18, 2024
@elasticmachine
Copy link

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

@clement-fouque
Copy link
Contributor

Also, I'm trying to understand how this would be consistent with ECS. As noted #9364 (comment) this would result in an array in vulnerability.id, but this is not noted to be an array.

I'm not sure I understand why it'll result in an array in vulnerability.id (I got the array part but not why vulnerability.id will be filled in). For the Knowledge Base datastream, I don't think we need (and we have) to copy any value into vulnerability.id.

I think it's good to only have the array field qualys_vmdr.knowledge_base.cve_list.

The only element that I think of to have one document per CVE per QID is if we want to enrich other sources (e.g. Snyk, Rapid7) with the Qualys Knowledge Base. It is unlikely, but still I think it would work if we define the match field on the multi-value field qualys_vmdr.knowledge_base.cve_list.

@efd6
Copy link
Contributor Author

efd6 commented Mar 18, 2024

The previous code attempted to set vulnerability.id from the qualys_vmdr.knowledge_base.cve_list. If this is not wanted, it can be remove and would solve the issue. However, the description of vulnerability.id does suggest that the CVE ID should go there.

@clement-fouque
Copy link
Contributor

I think it's better to comply with ECS and don't copy CVE IDs into the vulnerability.id field. Users will have the possibility to do it on their own.

@kcreddy
Copy link
Contributor

kcreddy commented Mar 22, 2024

We do have few packages that are using vulnerability.id and vulnerability.reference as an array even though ECS doesn't state them explicitly.
Examples;
https://github.com/elastic/integrations/blob/main/packages/tenable_io/data_stream/plugin/_dev/test/pipeline/test-plugin.log-expected.json#L124-L130
https://github.com/elastic/integrations/blob/main/packages/amazon_security_lake/data_stream/event/_dev/test/pipeline/test-findings.log-expected.json#L204-L210

Since Elasticsearch supports array out-of-the-box, maybe we could add multiple CVEs into vulnerability.id.

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 merged commit 25033a3 into elastic:main Mar 29, 2024
5 checks passed
@elasticmachine
Copy link

Package qualys_vmdr - 3.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 Qualys VMDR Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing CVE list in the Knowledge Base integration
4 participants