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 image-sha check #540

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

VedantMahabaleshwarkar
Copy link

Implementation for #539

Code is pretty similar to the latest-tag check as the intent behind the checks are pretty similar. The image-sha check implements an AllowList regex instead of a blocklist that verifies if the images have an sha tag

@VedantMahabaleshwarkar
Copy link
Author

I'm not sure what the test failure here is. Perhaps @janisz can tell me what I'm missing

@janisz
Copy link
Collaborator

janisz commented Apr 12, 2023

You need to regenerate files

make generated-srcs

@janisz
Copy link
Collaborator

janisz commented Apr 18, 2023

@VedantMahabaleshwarkar test is failing

@janisz
Copy link
Collaborator

janisz commented Apr 28, 2023

@VedantMahabaleshwarkar ping

@VedantMahabaleshwarkar
Copy link
Author

@janisz I'm seeing a failure in the Run Bats Tests test on the PR. Logs below.

*** /tmp/kubelinter/02-05-2023-18-07/kubelinterchecks.log	2023-05-02 18:07:41.103073179 +0000
--- /tmp/kubelinter/02-05-2023-18-07/batstests.log	2023-05-02 18:07:41.063072533 +0000
***************
*** 18,26 ****
  host-network
  host-pid
  hpa-minimum-three-replicas
- image-sha
  invalid-target-ports
  latest-tag
  minimum-three-replicas
  mismatching-selector
  no-anti-affinity
--- 18,26 ----
  host-network
  host-pid
  hpa-minimum-three-replicas
  invalid-target-ports
  latest-tag
+ image-sha
  minimum-three-replicas
  mismatching-selector
  no-anti-affinity
ERROR: The output of '/home/runner/work/kube-linter/kube-linter/.gobin/kube-linter checks list' differs from the tests in 'e2etests/bats-tests.sh'. See above diff.

However, on my local machine I do see the check if I run ./kube-linter checks list --format json | jq -r '.[].name', I see

access-to-create-pods
access-to-secrets
cluster-admin-role-binding
dangling-horizontalpodautoscaler
dangling-ingress
dangling-networkpolicy
dangling-networkpolicypeer-podselector
dangling-service
default-service-account
deprecated-service-account-field
dnsconfig-options
docker-sock
drop-net-raw-capability
duplicate-env-var
env-var-secret
exposed-services
host-ipc
host-network
host-pid
hpa-minimum-three-replicas
image-sha
invalid-target-ports
latest-tag
minimum-three-replicas
mismatching-selector
no-anti-affinity
no-extensions-v1beta
no-liveness-probe
no-node-affinity
no-read-only-root-fs
no-readiness-probe
no-rolling-update-strategy
non-existent-service-account
non-isolated-pod
pdb-max-unavailable
pdb-min-available
privilege-escalation-container
privileged-container
privileged-ports
read-secret-from-env-var
required-annotation-email
required-label-owner
run-as-non-root
sensitive-host-mounts
ssh-port
unsafe-proc-mount
unsafe-sysctls
unset-cpu-requirements
unset-memory-requirements
use-namespace
wildcard-in-rules
writable-host-mount

I'm having trouble understanding the reason behind the error, I ran make generated srcs before pushing. Can you help?

@janisz
Copy link
Collaborator

janisz commented May 2, 2023

The problem is in order of tests. Bats tests should be sorted so you need to move your test after hpa-minimum-three-replicas.

I'll think how to make error message better as I also needed some time to understand what's wrong. Maybe before making diff we should check order.

@VedantMahabaleshwarkar
Copy link
Author

@janisz Thanks for the help! It worked, all tests are passing now

@VedantMahabaleshwarkar
Copy link
Author

@janisz pinging again for a review on this PR

@janisz
Copy link
Collaborator

janisz commented May 10, 2023

I'm on PTO. I'll take a look next week.

@janisz
Copy link
Collaborator

janisz commented May 15, 2023

@VedantMahabaleshwarkar Please regenerate files

Copy link
Collaborator

@janisz janisz left a comment

Choose a reason for hiding this comment

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

This PR should only add a new check using existing template latest tag

@VedantMahabaleshwarkar
Copy link
Author

@janisz hey sorry for the delay. I changed the test to accept all filetypes instead of just deploymentlike and I have updated the docs as well. I think the tests are passing now.

@VedantMahabaleshwarkar
Copy link
Author

@janisz Can this PR be merged?

scope:
objectKinds:
- "Any"
template: "image-sha"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
template: "image-sha"
template: "latest-tag"

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could reuse the latest-tag template. probably we should change it's name

Copy link
Collaborator

@janisz janisz left a comment

Choose a reason for hiding this comment

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

Please reuse existing latest-tag template and not copy it.

- pkg/templates/imageshatag/internal/params/gen-params.go
- pkg/templates/imageshatag/internal/params/params.go
- pkg/templates/imageshatag/template.go
- pkg/templates/imageshatag/template_test.go

diff --git a/e2etests/bats-tests.sh b/e2etests/bats-tests.sh
index 37e0491..d074400 100755
--- a/e2etests/bats-tests.sh
+++ b/e2etests/bats-tests.sh
@@ -359,8 +359,8 @@ get_value_from() {
   message2=$(get_value_from "${lines[0]}" '.Reports[1].Object.K8sObject.GroupVersionKind.Kind + ": " + .Reports[1].Diagnostic.Message')
   count=$(get_value_from "${lines[0]}" '.Reports | length')
 
-  [[ "${message1}" == "Deployment: The container \"app\" is using an invalid container image, \"app:v1\". Please reference the image using a SHA256 tag." ]]
-  [[ "${message2}" == "DeploymentConfig: The container \"app\" is using an invalid container image, \"app:v1\". Please reference the image using a SHA256 tag." ]]
+  [[ "${message1}" == 'Deployment: The container "app" is using an invalid container image, "app:v1". Please use images that satisfies the `AllowList` criteria : [".*:[a-fA-F0-9]{64}$"]' ]]
+  [[ "${message2}" == 'DeploymentConfig: The container "app" is using an invalid container image, "app:v1". Please use images that satisfies the `AllowList` criteria : [".*:[a-fA-F0-9]{64}$"]' ]]
   [[ "${count}" == "2" ]]  
 }
 
diff --git a/pkg/builtinchecks/yamls/image-sha.yaml b/pkg/builtinchecks/yamls/image-sha.yaml
index dd5eeb8..3aa99a5 100644
--- a/pkg/builtinchecks/yamls/image-sha.yaml
+++ b/pkg/builtinchecks/yamls/image-sha.yaml
@@ -4,6 +4,6 @@ remediation: "Reference all images using their sha256 tags."
 scope:
   objectKinds:
     - "Any"
-template: "image-sha"
+template: "latest-tag"
 params:
   AllowList: [".*:[a-fA-F0-9]{64}$"] 
diff --git a/pkg/templates/all/all.go b/pkg/templates/all/all.go
index 9e0056d..db88467 100644
--- a/pkg/templates/all/all.go
+++ b/pkg/templates/all/all.go
@@ -24,7 +24,6 @@ import (
        _ "golang.stackrox.io/kube-linter/pkg/templates/hostpid"
        _ "golang.stackrox.io/kube-linter/pkg/templates/hpareplicas"
        _ "golang.stackrox.io/kube-linter/pkg/templates/imagepullpolicy"
-       _ "golang.stackrox.io/kube-linter/pkg/templates/imageshatag"
        _ "golang.stackrox.io/kube-linter/pkg/templates/latesttag"
        _ "golang.stackrox.io/kube-linter/pkg/templates/livenessprobe"
        _ "golang.stackrox.io/kube-linter/pkg/templates/memoryrequirements"

spec:
containers:
- name: app
image: app:75bf9b911b6481dcf29f7942240d1555adaa607eec7fc61bedb7f624f87c36d4
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a valid format? I thought it should have sha256 prefix

- "Any"
template: "image-sha"
params:
AllowList: [".*:[a-fA-F0-9]{64}$"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
AllowList: [".*:[a-fA-F0-9]{64}$"]
AllowList: [".*@sha256:[a-fA-F0-9]{64}$"]

@janisz janisz self-requested a review December 21, 2023 17:01
Copy link
Collaborator

@janisz janisz left a comment

Choose a reason for hiding this comment

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

ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants