Skip to content
This repository has been archived by the owner on Jan 23, 2025. It is now read-only.

Fix: correct search for control plane containers #1269

Merged
merged 3 commits into from
Apr 12, 2023
Merged

Fix: correct search for control plane containers #1269

merged 3 commits into from
Apr 12, 2023

Conversation

alex123012
Copy link
Contributor

Now in functions is_apiserver, is_etcd, is_controllermananager and is_scheduler regexes for first command entry are not "strict", so when there is more than one container in control plane pod with first command entries, for example, kube-apiserver and kube-apiserver-healthcheck - checks for control plane containers would be applied also for another (non control plane) container. This produces false negative check results.

In this PR I proposed solution for fixing this behavior.

@simar7 simar7 requested a review from chen-keinan April 6, 2023 23:06
simar7
simar7 previously approved these changes Apr 6, 2023
Copy link
Member

@simar7 simar7 left a comment

Choose a reason for hiding this comment

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

could you give an example of how this would fail today (false positive)?

@simar7
Copy link
Member

simar7 commented Apr 6, 2023

adding @chen-keinan to take a second look

@alex123012
Copy link
Contributor Author

@simar7 Hi!
So, we are trying to run CIS benchmark with trivy-operator and receive fail for all kube-apiserver checks. Our kube-apiserver pod looks like:

apiVersion: v1
kind: Pod
metadata:
  name: kube-apiserver
  namespace: kube-system
spec:
  containers:
  - command:
    - kube-apiserver
    - "..."
    name: kube-apiserver
  - command:
    - /usr/local/bin/kube-apiserver-healthcheck
    name: healthcheck

With old regex it will check both containers in kube-apiserver pod:
image
(look at dev tools)

If we use regex from this PR - all works as expected (only kube-apiserver container is checked):
image

As you can see, examples differ only by regex in is_apiserver func

playground

@simar7
Copy link
Member

simar7 commented Apr 12, 2023

I see - thanks that makes sense now. In that case, could you also update the unit tests for this check to make sure that we can capture this? The test file lives here https://github.com/aquasecurity/defsec/blob/master/rules/kubernetes/policies/cisbenchmarks/apiserver/audit_log_path_test.rego

You could add the input you provided in the rego playground and modify accordingly to have multiple containers as input, but only one of them triggers the check.

@alex123012
Copy link
Contributor Author

@simar7 And hi here:)
I've added tests not to cis benchmark rules, but for lib itself.
Also i've fixed regex to proper validate abs paths for control plane binaries in pod command (refer to tests)

@alex123012 alex123012 requested a review from simar7 April 12, 2023 11:14
alexey.makhonin and others added 3 commits April 12, 2023 14:49
Signed-off-by: alexey.makhonin <[email protected]>
Signed-off-by: Simar <[email protected]>
@simar7
Copy link
Member

simar7 commented Apr 12, 2023

@simar7 And hi here:)
I've added tests not to cis benchmark rules, but for lib itself.
Also i've fixed regex to proper validate abs paths for control plane binaries in pod command (refer to tests)

awesome, thanks!

@simar7 simar7 merged commit 07617fa into aquasecurity:master Apr 12, 2023
@alex123012 alex123012 deleted the fix-kubernetes-lib branch April 12, 2023 23:10
aisha-als pushed a commit to aisha-als/defsec that referenced this pull request Apr 17, 2023
* fix

Signed-off-by: alexey.makhonin <[email protected]>

* Fix regex and add tests for lib

* opa fmt

Signed-off-by: Simar <[email protected]>

---------

Signed-off-by: alexey.makhonin <[email protected]>
Signed-off-by: Simar <[email protected]>
Co-authored-by: Simar <[email protected]>
aisha-als pushed a commit to aisha-als/defsec that referenced this pull request Apr 17, 2023
* fix

Signed-off-by: alexey.makhonin <[email protected]>

* Fix regex and add tests for lib

* opa fmt

Signed-off-by: Simar <[email protected]>

---------

Signed-off-by: alexey.makhonin <[email protected]>
Signed-off-by: Simar <[email protected]>
Co-authored-by: Simar <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants