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

Add Latest Transform - Wiz Vulnerabilities #10895

Merged
merged 28 commits into from
Sep 12, 2024

Conversation

CohenIdo
Copy link
Contributor

@CohenIdo CohenIdo commented Aug 27, 2024

solves:

Summary

  • Create a latest Transform for the Wiz vulnerabilities data stream to support CDR.

@andrewkroh andrewkroh added the enhancement New feature or request label Aug 28, 2024
@CohenIdo CohenIdo marked this pull request as ready for review August 28, 2024 11:54
@CohenIdo CohenIdo requested a review from a team as a code owner August 28, 2024 11:54
@andrewkroh andrewkroh added the Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations] label Aug 28, 2024
@elasticmachine
Copy link

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

@kcreddy
Copy link
Contributor

kcreddy commented Aug 29, 2024

@CohenIdo, the CI is failing with error:

error running package asset tests: could not complete test run: can't install the package: there was an apply error: installation failed: can't install the package: could not zip-install package; API status code = 500; response body = {"statusCode":500,"error":"Internal Server Error","message":"security_exception\n\tRoot causes:\n\t\tsecurity_exception: action [indices:admin/delete] is unauthorized for service account [elastic/kibana] on indices [security_solution-wiz.vulnerability_latest-v1], this action is granted by the index privileges [delete_index,manage,all]"}

If you added the necessary permissions into Elasticsearch which will be merged into, say 8.15.2 version, then the manifest.yml should be updated with same version so that CI picks up the version you added permissions in:

conditions:
  kibana:
    version: "^8.15.2"

@elastic elastic deleted a comment from maxcold Sep 2, 2024
@@ -1,17 +1,17 @@
format_version: 3.0.2
name: wiz
title: Wiz
version: "1.7.2"
version: '1.8.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed with @CohenIdo we might want to make this change under preview version first as the UX is not yet ready, plus as the change is quite big and requires more e2e testing

Copy link
Contributor

Choose a reason for hiding this comment

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

lets put this on preview guys
I don't think we should release this change without a full QA cycle
wdyt?

Copy link
Contributor Author

@CohenIdo CohenIdo Sep 4, 2024

Choose a reason for hiding this comment

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

@kfirpeled yes, me and @maxcold were agreed on that and put it on today sync agenda sync to hear other thoughts. So it seems we all agree. will do

@CohenIdo CohenIdo requested a review from a team as a code owner September 4, 2024 07:40
@elasticmachine
Copy link

elasticmachine commented Sep 4, 2024

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

.vscode/settings.json Outdated Show resolved Hide resolved
packages/wiz/manifest.yml Outdated Show resolved Hide resolved
@@ -1,4 +1,9 @@
# newer versions go on top
- version: "1.8.0-preview01"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not just a release?

Copy link
Contributor

Choose a reason for hiding this comment

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

as the change introduces the transform we want to test the approach extensively before opening it up for users. There are also some UX adjustments we might need to do based on the real data coming from Wiz. Making the integration a preview first, will allow the team better DX for testing the features. This approach works well for us in the cloud_security_posture integration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make it clearer, any bug fix can be merged under 1.7.x and any changes that are for 8.16 will be under 1.8.0-preview0x

Copy link
Contributor

Choose a reason for hiding this comment

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

If bugfixes were continued to be merged under 1.7.x, I think the users will not be seeing them because the mainfest's kibana.version has moved to 8.16.0. The users need to be >= package's kibana.version to get the fixes/upgrades.

Comment on lines +1 to +18
- name: cloud.account.id
external: ecs
- name: cloud.region
external: ecs
- name: package.name
external: ecs
- name: package.version
external: ecs
- name: vulnerability.description
external: ecs
- name: vulnerability.id
external: ecs
- name: vulnerability.score.base
external: ecs
- name: vulnerability.score.version
external: ecs
- name: vulnerability.severity
external: ecs
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the transform destination index is not predefined in Elasticsearch (unlike the logs-* and metrics-* indices), so the ECS fields are not inherited automatically and must be explicitly defined.

description: Collect logs from Wiz with Elastic Agent.
type: integration
categories:
- security
- cloudsecurity_cdr
conditions:
kibana:
version: "^8.13.0"
version: "^8.16.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the requirement, this should probably be a draft PR until we get there. If we merge this now, all the subsequent changes will either require 8.16 or have to have back-ports.

Copy link
Contributor

Choose a reason for hiding this comment

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

the package will be available in Serverless as the stack version in serverless is 8.16 already. Together with the preview version, this should give the team including our product an easier way to test the new approach we are trying out on Wiz, see https://docs.google.com/document/d/1govaQj_8LwUOLtk3zrcjCEvRgyKWRuC-TUihCrk04ao/edit for more details
Are there changes planned for Wiz integration where backporting a concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there changes planned for Wiz integration where backporting a concern?

It's difficult to predict bugs.

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 unexpected bug fixes can be backported to 1.7, I assume the backporting flow exists for this reason. The bumping kibana stack version while using preview versions works quite well for cloud_security_posture, more so with the introduction of serverless. Any concern with this approach for Wiz except the potential backports?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what the feature is present in 8.16 that is not present in 8.13? If there is none, what is the reason for wanting to bump this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what the feature is present in 8.16 that is not present in 8.13? If there is none, what is the reason for wanting to bump this?

This is the change/feature related to 8.16 thats needed: elastic/elasticsearch#112192.
The transform in this PR needs permissions to create and maintain the latest indices. These permissions are targeted for v8.16.0 in Elasticsearch.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @kcreddy , that's correct. Plus we release UX features that would leverage the latest transform results, without them having transform would be just a waste of resources. and yes, won't event work properly due to missing privileges

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to remove my block, but despite the documented backport release workflow, in practice, making this change will in all likelihood result in reduction in fixes being released for pre-8.16-dependent versions of the package. This is just an observation on normal dev responses to additional friction from this kind of thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@efd6 thanks for providing more context for your concerns. If I understand correctly, the concerns are not about fixing specific Wiz bugs, but rather about making overall improvements to multiple integrations which is hard then to backport to each of these integrations one by one. I understand this concern, and I think it is a very valid one. But still, I think it's important to have a way not to keep the prs open for months. With serverless and with increased gap between versions it becomes a must in my opinion.

@CohenIdo CohenIdo requested a review from efd6 September 8, 2024 06:25
efd6
efd6 previously requested changes Sep 9, 2024
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.

Hold purely to avoid a merge before the version issue is discussed within the team.

@CohenIdo CohenIdo requested a review from efd6 September 10, 2024 09:00
@CohenIdo
Copy link
Contributor Author

Hold purely to avoid a merge before the version issue is discussed within the team.

There is a simple way to backport for the pervious stack version.
Adding @maxcold's investigations findings:
"all the docs I could find is this issue elastic/package-spec#225 (linked in the package-spec) To my understanding, our use of the pre-release version is totally valid. I also don't see a problem with backport flow after I did backport for csp integration. It involves a bit more work (create backport branch via CI and remember to update the changelog in main manually) but I don't see it as a big blocker"

@kcreddy
Copy link
Contributor

kcreddy commented Sep 11, 2024

More details on supporting bugfixes on older package versions: https://www.elastic.co/guide/en/integrations-developer/current/developer-workflow-support-old-package.html

@elasticmachine
Copy link

💚 Build Succeeded

History

Copy link

Quality Gate failed Quality Gate failed

Failed conditions
5.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

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

@CohenIdo CohenIdo merged commit b659fac into elastic:main Sep 12, 2024
4 of 5 checks passed
@elasticmachine
Copy link

Package wiz - 1.9.0-preview01 containing this change is available at https://epr.elastic.co/search?package=wiz

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:wiz Wiz 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.

7 participants