feat: setup securityContext on plugins #362

nerzhul · 2022-07-19T15:40:06Z

At Veepee we only permit pods to run as non root. Plugin works properly as non root, add variables to setup it as non root and permit to customize UID

Description

Ref #121

Checklist

I've read the guidelines for contributing to this repository.
I've added tests that prove my fix is effective or that my feature works.
I've updated the documentation with the relevant information (if needed).
I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

CLAassistant · 2022-07-19T15:40:11Z

All committers have signed the CLA.

nerzhul · 2022-07-20T07:41:41Z

I just fixed tests, it's now mergeable, if you can review and release when you can, we'd glad to do more test on our 200 nodes cluster :)

erikgb · 2022-07-20T11:43:10Z

docs/operator/configuration.md

@@ -50,6 +50,7 @@ To change the target namespace from all namespaces to the `default` namespace ed
 | `scanJob.tolerations`| N/A| JSON representation of the [tolerations] to be applied to the scanner pods so that they can run on nodes with matching taints. Example: `'[{"key":"key1", "operator":"Equal", "value":"value1", "effect":"NoSchedule"}]'`|
 | `scanJob.annotations`| N/A| One-line comma-separated representation of the annotations which the user wants the scanner pods to be annotated with. Example: `foo=bar,env=stage` will annotate the scanner pods with the annotations `foo: bar` and `env: stage` |
 | `scanJob.templateLabel`| N/A| One-line comma-separated representation of the template labels which the user wants the scanner pods to be labeled with. Example: `foo=bar,env=stage` will labeled the scanner pods with the labels `foo: bar` and `env: stage`|
+| `scanJob.podSecurityContext`| N/A| One-line JSON representation of the template securityContext which the user wants the scanner pods to be secured with. Example: `{"RunAsUser": 1000, "RunAsGroup": 1000, "RunAsNonRoot": true}`|


Will only "One-line JSON representation" be supported by this? I think it would be nice to support YAML-format - if possible, as this will be a better UX IMO. But probably not possible?

you mean people must setup a multine YAML instead of a single line JSON in the string variable passed in the config ?

Isn't this configured from a configmap? But I see that the similar property scanJob.templateLabel (also a typo; should be Labels) is using a simple string. I'll leave this decision up to the maintainers....

it's the configmap key exact, i use JSON because GetScanJobTolerations uses also a JSON to be consistent

pkg/vulnerabilityreport/builder_test.go

pkg/trivyoperator/config.go

erikgb · 2022-07-20T12:12:00Z

@nerzhul Thanks for this PR! I have added a few comments. We run on Openshift, and it is really picky about securityContext. It has it's SCC that want to control all that.

nerzhul · 2022-07-20T12:25:01Z

@erikgb i addressed your concerns, except the one on the format, let's wait for maintainers, but it kept consistency with tolerations, both are JSON unmarshaled.

For Openshift, you have specific things. On our side we use PSP and PodSecurities on all namespaces, then it's not possible to run root pods. With this patch pods runs perfectly as UID > 1000 users and creates reports, on a vanilla Kubernetes with just PSP/PS

erikgb · 2022-07-20T12:30:01Z

For Openshift, you have specific things.

I am not against adding this new feature, but I don't think it should break the existing operator. :-)

nerzhul · 2022-07-20T12:35:08Z

no problem @erikgb , now it's more close to the original one, regarding the tests on other parts :)
Tests on our dev cluster shows it's fine.
I just amened the helm chart to make helm chart use YAML syntax and let it transform to JSON like tolerations

nerzhul · 2022-07-20T12:37:56Z

@chen-keinan any opinion about this very attended PR (on our use case :) ?

pkg/trivyoperator/config.go

At Veepee we only permit pods to run as non root. Plugin works properly as non root, add variables to setup it as non root and permit to customize UID

erikgb · 2022-07-20T13:32:44Z

This LGTM now, but I think the maintainers should take a decision regarding the config UX. At some point we should probably review the "plugin" architecture for this. We currently have just a single plugin in this project, and I am not sure if the plugin approach gives more benefits than harm. But that will be breaking tho.

nerzhul · 2022-07-20T13:46:14Z

Okay i hope we can merge this before you change the whole architecture :) who are maintainers ? in the changelog it seems you are 2 currently active people

erikgb · 2022-07-20T14:00:03Z

Okay i hope we can merge this before you change the whole architecture :) who are maintainers ? in the changelog it seems you are 2 currently active people

@chen-keinan is the maintainer here. I have brought in some ideas from our custom in-house image scanner lately. 😉 Looking to replace it with trivy-operator.

josedonizetti · 2022-07-20T15:02:23Z

@nerzhul Thanks for the PR, didn't have time to look/test this yet, but I think your change here is addressing what was asked here? #121

chen-keinan · 2022-07-20T15:19:20Z

@nerzhul thanks you for contributing , I'm fine with keeping the podSecurityContext in json format to be consist with this type of data.
I'll approve it later after I'll test it and make sure it works fine.

chen-keinan · 2022-07-20T17:38:40Z

I assume it will solve the most cases where it is required in pod level however not if it is required for a specific init container

josedonizetti

Looks good! But I do have a comment/question. Considering trivy will scan and report misconfiguration for pods not having a security context with runAsUser, runAsGroup and runAsNonRoot.

https://avd.aquasec.com/misconfig/kubernetes/general/avd-ksv-0012/
https://avd.aquasec.com/misconfig/kubernetes/general/avd-ksv-0020/
https://avd.aquasec.com/misconfig/kubernetes/general/avd-ksv-0021/

Why don't we have a default value for podTemplateSecurityContext considering the AVDs above?

josedonizetti

I'll create an issue to discuss further if we should have default or not. :)

nerzhul · 2022-07-21T10:08:22Z

@josedonizetti it's a very good comment, i think we should have best security options by default, i just kept compat with existing when writing this pr :)

erikgb · 2022-07-21T10:20:58Z

@josedonizetti it's a very good comment, i think we should have best security options by default, i just kept compat with existing when writing this pr :)

It will be a breaking change for users on Openshift, but I do not oppose adding a default according to K8s security best practices. In this case, I think Openshift can improve. 😉 But it should be mentioned in the release notes as a breaking change IMO.

nerzhul force-pushed the feat/trivy_scan_securitycontext branch 2 times, most recently from c35ff36 to ca40568 Compare July 19, 2022 16:18

nerzhul marked this pull request as ready for review July 19, 2022 16:19

nerzhul force-pushed the feat/trivy_scan_securitycontext branch from ca40568 to 4d80194 Compare July 20, 2022 07:41

chen-keinan self-requested a review July 20, 2022 08:49

erikgb reviewed Jul 20, 2022

View reviewed changes

pkg/trivyoperator/config.go Outdated Show resolved Hide resolved

erikgb reviewed Jul 20, 2022

View reviewed changes

pkg/trivyoperator/config.go Outdated Show resolved Hide resolved

nerzhul force-pushed the feat/trivy_scan_securitycontext branch from 4d80194 to a63979e Compare July 20, 2022 12:21

nerzhul force-pushed the feat/trivy_scan_securitycontext branch from a63979e to 49fdc70 Compare July 20, 2022 12:33

nerzhul force-pushed the feat/trivy_scan_securitycontext branch from 49fdc70 to 8f82e2a Compare July 20, 2022 12:36

erikgb reviewed Jul 20, 2022

View reviewed changes

pkg/trivyoperator/config.go Outdated Show resolved Hide resolved

feat: setup securityContext on plugins

3b57fdf

At Veepee we only permit pods to run as non root. Plugin works properly as non root, add variables to setup it as non root and permit to customize UID

nerzhul force-pushed the feat/trivy_scan_securitycontext branch from 8f82e2a to 3b57fdf Compare July 20, 2022 13:13

chen-keinan approved these changes Jul 20, 2022

View reviewed changes

chen-keinan mentioned this pull request Jul 20, 2022

more granular way to set up security context in helm chart #121

Closed

chen-keinan requested a review from josedonizetti July 20, 2022 17:58

josedonizetti reviewed Jul 20, 2022

View reviewed changes

josedonizetti approved these changes Jul 20, 2022

View reviewed changes

josedonizetti merged commit 6916d7b into aquasecurity:main Jul 20, 2022

nerzhul deleted the feat/trivy_scan_securitycontext branch July 21, 2022 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: setup securityContext on plugins #362

feat: setup securityContext on plugins #362

nerzhul commented Jul 19, 2022 •

edited by chen-keinan

CLAassistant commented Jul 19, 2022 •

edited

nerzhul commented Jul 20, 2022

erikgb Jul 20, 2022

nerzhul Jul 20, 2022

erikgb Jul 20, 2022

nerzhul Jul 20, 2022

erikgb commented Jul 20, 2022

nerzhul commented Jul 20, 2022

erikgb commented Jul 20, 2022

nerzhul commented Jul 20, 2022

nerzhul commented Jul 20, 2022

erikgb commented Jul 20, 2022

nerzhul commented Jul 20, 2022

erikgb commented Jul 20, 2022

josedonizetti commented Jul 20, 2022

chen-keinan commented Jul 20, 2022 •

edited

chen-keinan commented Jul 20, 2022

josedonizetti left a comment

josedonizetti left a comment

nerzhul commented Jul 21, 2022

erikgb commented Jul 21, 2022

feat: setup securityContext on plugins #362

feat: setup securityContext on plugins #362

Conversation

nerzhul commented Jul 19, 2022 • edited by chen-keinan

Description

Checklist

CLAassistant commented Jul 19, 2022 • edited

nerzhul commented Jul 20, 2022

erikgb Jul 20, 2022

Choose a reason for hiding this comment

nerzhul Jul 20, 2022

Choose a reason for hiding this comment

erikgb Jul 20, 2022

Choose a reason for hiding this comment

nerzhul Jul 20, 2022

Choose a reason for hiding this comment

erikgb commented Jul 20, 2022

nerzhul commented Jul 20, 2022

erikgb commented Jul 20, 2022

nerzhul commented Jul 20, 2022

nerzhul commented Jul 20, 2022

erikgb commented Jul 20, 2022

nerzhul commented Jul 20, 2022

erikgb commented Jul 20, 2022

josedonizetti commented Jul 20, 2022

chen-keinan commented Jul 20, 2022 • edited

chen-keinan commented Jul 20, 2022

josedonizetti left a comment

Choose a reason for hiding this comment

josedonizetti left a comment

Choose a reason for hiding this comment

nerzhul commented Jul 21, 2022

erikgb commented Jul 21, 2022

nerzhul commented Jul 19, 2022 •

edited by chen-keinan

CLAassistant commented Jul 19, 2022 •

edited

chen-keinan commented Jul 20, 2022 •

edited