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

Prevent CVE-2024-3094 #1010

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

kurktchiev
Copy link

@kurktchiev kurktchiev commented May 14, 2024

Description

Prevent CVE-2024-3094

Checklist

  • I have read the policy contribution guidelines.
  • [] I have added test manifests and resources covering both positive and negative tests that prove this policy works as intended.
  • I have added the artifacthub-pkg.yml file and have verified it is complete and correct.

kurktchiev and others added 6 commits May 14, 2024 19:11
Signed-off-by: Boris 'B' Kurktchiev <boris.kurktchiev@nirmata.com>
Signed-off-by: Boris 'B' Kurktchiev <boris.kurktchiev@nirmata.com>
Signed-off-by: Boris 'B' Kurktchiev <boris.kurktchiev@nirmata.com>
Copy link
Author

@kurktchiev kurktchiev left a comment

Choose a reason for hiding this comment

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

I am not exactly sure how to provide tests for this since it requires both a bad image to test against and specific credentials

@kurktchiev kurktchiev marked this pull request as ready for review July 29, 2024 19:57
Signed-off-by: Boris 'B' Kurktchiev <boris.kurktchiev@nirmata.com>
Copy link
Contributor

@chipzoller chipzoller left a comment

Choose a reason for hiding this comment

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

Have you been able to identify some public image out there which we could use for a test of this, keeping in mind it needs a CycloneDX SBOM?

@kurktchiev
Copy link
Author

No so that would be kinda unethical, pointing people to known vulnerable images doesn’t pass muster.

@chipzoller
Copy link
Contributor

We wouldn't be "pointing people" to it; it would only be used for test purposes, inside an isolated CI/CD environment, not as part of the policy definition itself.

@kurktchiev
Copy link
Author

The test cases are public correct? I would have to include a pointer to the image in them? Along with the ci/cd run itself is public?

@chipzoller
Copy link
Contributor

Yes. We have a number of images we use which fall into the same category. Their presence doesn't serve as an endorsement for others, only inputs for policy validation.

@kurktchiev
Copy link
Author

I do not believe that makes the practice ok. I am welcome to have other maintainers chime in, but providing traces of vulnerable code, no matter the intent, is a security no no, and considering the project falls under the security umbrella I do not see this as a good practice.

@chipzoller
Copy link
Contributor

I get your intentions but I generally disagree. Regardless, how do we verify your policy works as intended? We obviously need an image with an SBOM with the affected packages at least forged in the SBOM. We do not accept policies in the library which cannot be independently verified, even if they are not or cannot be tested during CI.

@realshuting
Copy link
Member

Typically we have tests covered for policies with new functionalities, which should be tested thoroughly in kyverno/kyverno. And I agree that the sample policies provided in the Kyverno official pool should have tests covered for all scenarios. However this makes a contribution become complex.

Is it a good idea to create another pool to include example policies contributed by Kyverno users? While Kyverno offers an unofficial policies pool, it does not assure that all created policies will function as intended, they are used only as references, and users should thoroughly test and evaluate their policies.

@chipzoller
Copy link
Contributor

Supplying automated tests along with a policy is not a hard requirement, it is just strongly encouraged. But what is a hard requirement is that someone (a project maintainer) other than the policy author be able to verify the policy does what it claims to do.

@kurktchiev
Copy link
Author

I do not think this is the correct avenue for the discussion. However, I will say that I simply used the following already merged and accepted policy as the basis for this one: https://kyverno.io/policies/other/verify-image-cve-2022-42889/verify-image-cve-2022-42889/. Its codebase is in the repo here: https://github.com/kyverno/policies/tree/main/other/verify-image-cve-2022-42889. I do not see associated test cases with it.

@chipzoller
Copy link
Contributor

That policy was created two years ago when we did not have any CI tests in place, but the policy was tested (by me) to ensure that it worked as written. Whether the reviewer (Jim) tested it himself I do not know.

kurktchiev and others added 3 commits July 31, 2024 10:26
Signed-off-by: Boris 'B' Kurktchiev <boris.kurktchiev@nirmata.com>
Signed-off-by: Boris 'B' Kurktchiev <boris.kurktchiev@nirmata.com>
@kurktchiev kurktchiev marked this pull request as draft August 6, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants