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: Warn instead of panic when API resource not found or forbidden #73

Merged
merged 10 commits into from
Jul 26, 2023

Conversation

viccuad
Copy link
Member

@viccuad viccuad commented Jul 24, 2023

Description

Fix #68

Test

Added unit tests.

Performed manual integration tests when developing:

  1. Deployed kubewarden helm charts from kubewarden/helm-charts@main:
helm upgrade -i --wait --namespace kubewarden --create-namespace kubewarden-crds ./charts/kubewarden-crds
helm upgrade -i --wait --namespace kubewarden --create-namespace kubewarden-controller ./charts/kubewarden-controller --set auditScanner.cronJob.schedule="*/3 * * * *" --set auditScanner.enable=true
helm upgrade -i --wait --namespace kubewarden --create-namespace kubewarden-defaults ./charts/kubewarden-defaults --set recommendedPolicies.enabled=True --set recommendedPolicies.defaultPolicyMode=monitor
  1. Deployed a labeled namespace (which is clusterwide), and a safe-labels policy that checks pods, namespaces, an unexistent resource, and secrets (forbidden by the audit-scanner ServiceAccount we ship).
apiVersion: v1
kind: Namespace
metadata:
  name: demo2
  labels:
    cost-center: "123"
---
apiVersion: policies.kubewarden.io/v1
kind: ClusterAdmissionPolicy
metadata:
  annotations:
    io.kubewarden.policy.category: Resource validation
    io.kubewarden.policy.severity: low
  name: safe-labels
spec:
  module: registry://ghcr.io/kubewarden/tests/safe-labels:v0.1.13
  settings:
    denied_labels:
      - cost-center
  mode: protect
  rules:
    - apiGroups: [""]
      apiVersions: ["v1"]
      resources: ["pods,namespaces, unexistent, secrets"]
      operations:
        - CREATE
  mutating: false
  backgroundAudit: true
  1. Without the changes, we get a panic on secrets resource as they are forbidden.

  2. With changes applied:
    Deploying the default policies, plus safe-labels policy targetting resources [pods, Pods, namespaces, secrets] (note that Pods is incorrect), we get the following WARN messages, for both Pod (API resource not found) and secrets (API resource forbidden, unknown GVK or ServiceAccount lacks permissions):

audit-scanner-28171311-ls6hw {"level":"info","time":"2023-07-25T09:51:01Z","message":"cluster wide scan started"}
audit-scanner-28171311-ls6hw {"level":"info","dict":{"policies to evaluate":7,"policies skipped":0},"time":"2023-07-25T09:51:01Z","message":"cluster admission policies count"}
audit-scanner-28171311-ls6hw {"level":"warn","resource GVK":"/v1, Resource=Pods","time":"2023-07-25T09:51:01Z","message":"API resource not found"}
audit-scanner-28171311-ls6hw {"level":"info","time":"2023-07-25T09:51:01Z","message":"scan finished"}
audit-scanner-28171311-ls6hw {"level":"info","dict":{"report name":"polr-clusterwide","report ns":"","summary":"{\"pass\":7,\"fail\":1,\"warn\":0,\"error\":0,\"skip\":0}"},"time":"2023-07-25T09:51:01Z","message":"updated ClusterPolicyReport"}
audit-scanner-28171311-ls6hw {"level":"info","time":"2023-07-25T09:51:01Z","message":"all-namespaces scan started"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"default","time":"2023-07-25T09:51:01Z","message":"namespace scan started"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"default","dict":{"policies to evaluate":7,"policies skipped":0},"time":"2023-07-25T09:51:01Z","message":"policy count"}
audit-scanner-28171311-ls6hw {"level":"warn","resource GVK":"/v1, Resource=Pods","time":"2023-07-25T09:51:01Z","message":"API resource not found"}
audit-scanner-28171311-ls6hw {"level":"warn","dict":{"resource GVK":"/v1, Resource=secrets","ns":"default"},"time":"2023-07-25T09:51:01Z","message":"API resource forbidden, unknown GVK or ServiceAccount lacks permissions"}
audit-scanner-28171311-ls6hw {"level":"info","dict":{"report name":"polr-ns-default","report ns":"default","report resourceVersion":"88451","summary":"{\"pass\":6,\"fail\":1,\"warn\":0,\"error\":0,\"skip\":0}"},"time":"2023-07-25T09:51:01Z","message":"updated PolicyReport"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"default","time":"2023-07-25T09:51:01Z","message":"namespace scan finished"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"demo2","time":"2023-07-25T09:51:01Z","message":"namespace scan started"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"demo2","dict":{"policies to evaluate":7,"policies skipped":0},"time":"2023-07-25T09:51:01Z","message":"policy count"}
audit-scanner-28171311-ls6hw {"level":"warn","resource GVK":"/v1, Resource=Pods","time":"2023-07-25T09:51:01Z","message":"API resource not found"}
audit-scanner-28171311-ls6hw {"level":"warn","dict":{"resource GVK":"/v1, Resource=secrets","ns":"demo2"},"time":"2023-07-25T09:51:01Z","message":"API resource forbidden, unknown GVK or ServiceAccount lacks permissions"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"demo2","time":"2023-07-25T09:51:01Z","message":"no pre-existing PolicyReport, will create one at end of the scan if needed"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"demo2","time":"2023-07-25T09:51:01Z","message":"namespace scan finished"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"policy-reporter","time":"2023-07-25T09:51:01Z","message":"namespace scan started"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"policy-reporter","dict":{"policies to evaluate":7,"policies skipped":0},"time":"2023-07-25T09:51:01Z","message":"policy count"}
audit-scanner-28171311-ls6hw {"level":"warn","resource GVK":"/v1, Resource=Pods","time":"2023-07-25T09:51:01Z","message":"API resource not found"}
audit-scanner-28171311-ls6hw {"level":"warn","dict":{"resource GVK":"/v1, Resource=secrets","ns":"policy-reporter"},"time":"2023-07-25T09:51:01Z","message":"API resource forbidden, unknown GVK or ServiceAccount lacks permissions"}
audit-scanner-28171311-ls6hw {"level":"info","dict":{"report name":"polr-ns-policy-reporter","report ns":"policy-reporter","report resourceVersion":"88454","summary":"{\"pass\":6,\"fail\":0,\"warn\":0,\"error\":0,\"skip\":0}"},"time":"2023-07-25T09:51:02Z","message":"updated PolicyReport"}
audit-scanner-28171311-ls6hw {"level":"info","dict":{"report name":"polr-ns-policy-reporter","report ns":"policy-reporter","report resourceVersion":"88456","summary":"{\"pass\":12,\"fail\":0,\"warn\":0,\"error\":0,\"skip\":0}"},"time":"2023-07-25T09:51:02Z","message":"updated PolicyReport"}
audit-scanner-28171311-ls6hw {"level":"info","dict":{"report name":"polr-ns-policy-reporter","report ns":"policy-reporter","report resourceVersion":"88457","summary":"{\"pass\":26,\"fail\":0,\"warn\":0,\"error\":0,\"skip\":0}"},"time":"2023-07-25T09:51:03Z","message":"updated PolicyReport"}
audit-scanner-28171311-ls6hw {"level":"info","namespace":"policy-reporter","time":"2023-07-25T09:51:03Z","message":"namespace scan finished"}
audit-scanner-28171311-ls6hw {"level":"info","time":"2023-07-25T09:51:03Z","message":"all-namespaces scan finished"}
Stream closed EOF for kubewarden/audit-scanner-28171311-ls6hw (audit-scanner)

Additional Information

I recommend review per commit.

Tradeoff

Potential improvement

Refactor the filter packages after first MVP. Opened #74.

@viccuad viccuad self-assigned this Jul 24, 2023
Before this fix, we were doing log.Info().Err(err), which emits a
one-line log that is both info and error:

{"level":"info","error":"resource not found","namespace":"demo2","time":"2023-07-24T15:23:17+02:00","message":"no pre-existing PolicyReport, will create one at the end of the scan"}

Signed-off-by: Víctor Cuadrado Juan <[email protected]>
We are looking for namespaced resources, clusterwide resources get
checked in getClusterWideResourcesForPolicies()

Signed-off-by: Víctor Cuadrado Juan <[email protected]>
When policies are configured with a spec.rules GVK,
if resource doesn't exist because it has an incorrect GVK,
or if because it is a CRD that we don't know about,
emit warning and skip resource from scan.

The same for when policies are configured with a spec.rules GVK that the
audit-scanner lacks permissions for,
emit warning and skip resource from scan.

Signed-off-by: Víctor Cuadrado Juan <[email protected]>
Signed-off-by: Víctor Cuadrado Juan <[email protected]>
Needed, as we need a client when we do `fetcher.isNamespacedResource()`.

Signed-off-by: Víctor Cuadrado Juan <[email protected]>
Signed-off-by: Víctor Cuadrado Juan <[email protected]>
Add a test that checks that we don't error nor panic when trying to
filter resources that are forbidden (because the client lacks
permissions, normally because the ServiceAccount is insufficient).

Signed-off-by: Víctor Cuadrado Juan <[email protected]>
@viccuad viccuad marked this pull request as ready for review July 25, 2023 13:38
@viccuad viccuad requested a review from a team as a code owner July 25, 2023 13:38
@viccuad viccuad removed the kind/bug label Jul 25, 2023
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Nice fixes, thanks!

@viccuad viccuad merged commit ef75cbf into kubewarden:main Jul 26, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Audit-scanner panics when ServiceAccount perms aren't enough
2 participants