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

[Security solution][Endpoint] Removes zip compression when creating artifacts #101379

Conversation

dasansol92
Copy link
Contributor

@dasansol92 dasansol92 commented Jun 4, 2021

Summary

Description:

As a result of moving Artifact storage to fleet (v7.13) the endpoint ManifestManager process should be refactored to no longer compress (zip) the artifacts content

Also:

  • It fixes related unit tests.
  • Removes old unused code.

artifacts without compression

For maintainers

@dasansol92 dasansol92 added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v7.14.0 v8.0.0 labels Jun 4, 2021
@dasansol92 dasansol92 marked this pull request as ready for review June 4, 2021 14:59
@dasansol92 dasansol92 requested a review from a team as a code owner June 4, 2021 14:59
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

@dasansol92
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

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

🚀 LGTM.

InternalArtifactCompleteSchema,
} from './saved_objects';

const compressArtifact = async (artifact: InternalArtifactCompleteSchema) => {
Copy link
Member

Choose a reason for hiding this comment

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

nice 🎉

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Just some questions for now, but looks good. Going to run a test locally now and will approve after that

const createdArtifact = await this.fleetArtifacts.createArtifact({
content: artifactContent.toString(),
content: Buffer.from(artifact.body, 'base64').toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the part I was questioning in my mind as to how we would handle it. I think the returned records from fleetArtifacts.createArtifact() needs to be "merged" in with the interal record manifest manager keeps, so that the copreesed data is added in. maybe? 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.

Not sure about it, I just removed the zip compression step, so not needed to uncompress the content here, just decode from base64

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good.

(FYI: Now I'm also thinking in the future, we might want to remove this redundant base64 encoding 🤔 )

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Did a local test (without Endpoint) and looks like we're populating the Policy manifest with information about artifacts that does not include the compression info. (which still happens, but is done by the Fleet artifct exposed client).

You can see this by looking at the Policy record (endpoint integration policy) and noticed that the compression_algorithm' is set to none` and that body decoded and encoded data is the same

image

I think the challange here will be to figure out how we can get the data returned by from fleet artifact client when the artifact is created back into the internal manifest entry that Manifest Manager keeps. I belive the internal record is written/saved before the artifact is actuallly created, so we might have to chagne the flow there.

…965' of github.com:dasansol92/kibana into feature/olm-no_compress_artifacts_during_it_processing-965
@dasansol92
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

I did not check out the code locally this time, but changes looks good. Maybe just add a screen capture to the Description of the PR that shows the Policy manifest entries (json), so that we can see that zlib is still being set for compression and that the size/hashes are also set accordingly.

Other than that - 🚢

Thanks for taking this on

@dasansol92
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dasansol92 dasansol92 merged commit ec2ec6a into elastic:master Jun 14, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 14, 2021
…rtifacts (elastic#101379)

* Removes zlib compression when creating artifacts. Also fixes related unit tests and removes old code

* Replaces artifact in new manifest using the ones from fleet client with zlip compression

* Fixes create_policy_artifact_manifest pushArtifacts missing new manifest. Also fixes unit tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 14, 2021
* master:
  [Security solution][Endpoint] Removes zip compression when creating artifacts (elastic#101379)
  [Dashboard]: Fixes disabled viz filter is applied (elastic#101859)
  [Discover] Deangularization of search embeddable (elastic#100552)
kibanamachine added a commit that referenced this pull request Jun 14, 2021
…rtifacts (#101379) (#102033)

* Removes zlib compression when creating artifacts. Also fixes related unit tests and removes old code

* Replaces artifact in new manifest using the ones from fleet client with zlip compression

* Fixes create_policy_artifact_manifest pushArtifacts missing new manifest. Also fixes unit tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: David Sánchez <davidsansol92@gmail.com>
cuff-links pushed a commit to cuff-links/kibana that referenced this pull request Jun 15, 2021
…rtifacts (elastic#101379)

* Removes zlib compression when creating artifacts. Also fixes related unit tests and removes old code

* Replaces artifact in new manifest using the ones from fleet client with zlip compression

* Fixes create_policy_artifact_manifest pushArtifacts missing new manifest. Also fixes unit tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants