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

feat: load rego embedded libs by default #926

Merged

Conversation

chen-keinan
Copy link
Contributor

Signed-off-by: chenk [email protected]
Close: #925

@chen-keinan chen-keinan force-pushed the feat/load-k8s-embedded-libraries branch from 94ac4e1 to a2d27e8 Compare September 6, 2022 10:29
@chen-keinan chen-keinan changed the title feat: load k8s embedded lib feat: load k8s embedded libs by default Sep 6, 2022
@chen-keinan chen-keinan changed the title feat: load k8s embedded libs by default feat: load rego embedded libs by default Sep 6, 2022
@chen-keinan chen-keinan force-pushed the feat/load-k8s-embedded-libraries branch 2 times, most recently from 8bd0c83 to 6356b74 Compare September 8, 2022 07:57
@chen-keinan chen-keinan force-pushed the feat/load-k8s-embedded-libraries branch from 6356b74 to 92585fc Compare September 11, 2022 12:21
@owenrumney owenrumney merged commit 01c35b4 into aquasecurity:master Sep 13, 2022
@simar7
Copy link
Member

simar7 commented Oct 14, 2022

hi @chen-keinan - we're trying to bring back policy fetching into trivy if a new bundle is found upstream. This is so that we can decouple trivy releases from policy updates.

tldr: Currently this behaviour doesn't allow trivy to download a bundle (with both policies and libraries in it) as libraries are unconditionally embedded into the trivy binary, causing rego namespace collisions. Bundle will include both policies and libraries. Context is here: d128e52#r86794387

I understand you had made this change to solve https://github.com/aquasecurity/defsec/issues/925 - is it possible we (for the users of trivy operator) can provide the libraries in another way? Maybe they could be stored as configmaps within the operator deployment charts? Just a thought.

Ideally, it'd be best to keep policies and libraries together behind the same flag. In other words, if loadEmbedded { should gate both the policies and libraries as decoupling the two introduces situations where you can have potentially have a mismatch (old libraries, new policies) or rego namespace collisions (this scenario).

Thoughts?

@simar7
Copy link
Member

simar7 commented Oct 14, 2022

cc @liamg @itaysk

simar7 added a commit to simar7/defsec that referenced this pull request Oct 15, 2022
@simar7
Copy link
Member

simar7 commented Oct 15, 2022

Created #1012 to address this.

@chen-keinan
Copy link
Contributor Author

hi @chen-keinan - we're trying to bring back policy fetching into trivy if a new bundle is found upstream. This is so that we can decouple trivy releases from policy updates.

tldr: Currently this behaviour doesn't allow trivy to download a bundle (with both policies and libraries in it) as libraries are unconditionally embedded into the trivy binary, causing rego namespace collisions. Bundle will include both policies and libraries. Context is here: d128e52#r86794387

I understand you had made this change to solve #925 - is it possible we (for the users of trivy operator) can provide the libraries in another way? Maybe they could be stored as configmaps within the operator deployment charts? Just a thought.

Ideally, it'd be best to keep policies and libraries together behind the same flag. In other words, if loadEmbedded { should gate both the policies and libraries as decoupling the two introduces situations where you can have potentially have a mismatch (old libraries, new policies) or rego namespace collisions (this scenario).

Thoughts?

sure , lets discuss the options

simar7 added a commit to simar7/defsec that referenced this pull request Oct 17, 2022
@simar7
Copy link
Member

simar7 commented Oct 18, 2022

hi @chen-keinan - we're trying to bring back policy fetching into trivy if a new bundle is found upstream. This is so that we can decouple trivy releases from policy updates.
tldr: Currently this behaviour doesn't allow trivy to download a bundle (with both policies and libraries in it) as libraries are unconditionally embedded into the trivy binary, causing rego namespace collisions. Bundle will include both policies and libraries. Context is here: d128e52#r86794387
I understand you had made this change to solve #925 - is it possible we (for the users of trivy operator) can provide the libraries in another way? Maybe they could be stored as configmaps within the operator deployment charts? Just a thought.
Ideally, it'd be best to keep policies and libraries together behind the same flag. In other words, if loadEmbedded { should gate both the policies and libraries as decoupling the two introduces situations where you can have potentially have a mismatch (old libraries, new policies) or rego namespace collisions (this scenario).
Thoughts?

sure , lets discuss the options

Previously, in the case of Trivy for using custom policies the users have to supply their own libraries in case they need them. Having said that, maybe we can instead create a reference deployment in Trivy Operator (example helm chart, for instance) that includes some scaffolding to help them do so?

Long term, decoupling policies from Trivy binary helps us as policies get updated often compared to Trivy itself. This is the same model that Trivy DB follows for updating vulnerability definitions.

What do you think? Maybe we can jump on a call and sort it out.

@chen-keinan
Copy link
Contributor Author

chen-keinan commented Oct 18, 2022

What do you think? Maybe we can jump on a call and sort it out.

@simar7 sure I'll be happy to have a call to discuss it

simar7 added a commit to simar7/defsec that referenced this pull request Oct 19, 2022
simar7 added a commit to simar7/defsec that referenced this pull request Oct 19, 2022
liamg pushed a commit that referenced this pull request Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Load the k8s Embedded Libraries by default
3 participants