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

fix(query): covered additional deprecated API versions in k8s rule #4830

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

Churro
Copy link
Contributor

@Churro Churro commented Feb 13, 2022

Problem

  • Ingress in extensions/v1beta1 recommends apps/v1 but correct would be networking.k8s.io/v1
  • negative.yaml includes an Ingress with networking.k8s.io/v1beta1 but this API version is also deprecated
  • All findings of a particular type are shown only once in results, even if they appear multiple times in a scanned *.yaml file
  • The existing checks do not cover deprecations in K8s > v1.16

Proposed Changes

  • Improvements for all items mentioned above
  • Additional checks for commonly defined resource APIs that were deprecated with K8s v1.22

I submit this contribution under the Apache-2.0 license.

@kicsbot
Copy link
Contributor

kicsbot commented Feb 13, 2022

Scan submitted to Checkmarx

@kicsbot
Copy link
Contributor

kicsbot commented Feb 13, 2022

Logo
Checkmarx SAST - Scan Summary & Details

Cx-SAST Summary

Total of 5 vulnerabilities
High 0 High
Medium 0 Medium
Low 5 Low
Info 0 Info

Violation Summary

No policy violation found

Copy link
Contributor

@rafaela-soares rafaela-soares left a comment

Choose a reason for hiding this comment

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

Hello, @Churro 🙂

Your observations are always on point 🚀 Thank you so much for sharing them and contributing to KICS!

I would suggest including even more deprecated API versions If you do not mind. The comment below addresses my suggestion. Let me know what you think about it, please.

"ClusterRoleBinding": "rbac.authorization.k8s.io/v1",
"Role": "rbac.authorization.k8s.io/v1",
"RoleBinding": "rbac.authorization.k8s.io/v1",
},
Copy link
Contributor

@rafaela-soares rafaela-soares Feb 14, 2022

Choose a reason for hiding this comment

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

Suggested change
},
"batch/v1beta1": {
"CronJob": "batch/v1",
},
"policy/v1beta1": {
"PodDisruptionBudget": "policy/v1",
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good suggestion, thank you! I also thought about these two additions but hesitated due to the fact that the substitutes for these deprecations were only introduced with Kubernetes v1.21. In other words, someone still running v1.20 would not be able to apply the recommended version without running into unforeseen issues. It seems that the 1.20 branch is also still maintained until 2022-02-28.

I'd therefore suggest to add the two deprecations you suggested only when 1.25 is released. Would you prefer having them added already now or would it be better to wait?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right. It would be better to add it when 1.25 is released! Thank you so much 😊

Copy link
Contributor

@rafaela-soares rafaela-soares left a comment

Choose a reason for hiding this comment

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

LGTM

@joaoReigota1 joaoReigota1 merged commit 488a42e into Checkmarx:master Feb 15, 2022
@rafaela-soares rafaela-soares added the community Community contribution label Mar 16, 2022
@rafaela-soares rafaela-soares added the query New query feature label Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution query New query feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants