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

feat(executor): set seccomp profile to runtimedefault #12984

Merged
merged 1 commit into from May 11, 2024

Conversation

lukashankeln
Copy link
Contributor

@lukashankeln lukashankeln commented Apr 26, 2024

Motivation

We are using Kyverno Policies to enforce different security settings within our cluster. For example running containers as non Root.

But also to explicitly have set the SeccompProfile to RuntimeDefault. See the official Documentation

Currently the artifact GC is beeing blocked by our Kyverno policy. On my search for values to change the default (e.g. via Helm) i found the code I changed in this PR.

Modifications

Explicitly Setting the Seccomp Profile to RuntimeDefault when creating the artifact GC Pod.

Verification

The E2E tests should catch any issues, if you want me to test this locally let me know

@lukashankeln lukashankeln marked this pull request as ready for review April 26, 2024 07:28
@agilgur5
Copy link
Member

You might notice podSpecPatch a few lines below -- you can set that to change anything on the Pod

@agilgur5 agilgur5 added area/artifacts S3/GCP/OSS/Git/HDFS etc area/gc Garbage collection, such as TTLs, retentionPolicy, delays, and more type/security Security related labels Apr 26, 2024
@agilgur5 agilgur5 added the solution/workaround There's a workaround, might not be great, but exists label Apr 29, 2024
@agilgur5 agilgur5 self-assigned this May 1, 2024
@lukashankeln lukashankeln force-pushed the feat/artifact-gc-seccomp branch 2 times, most recently from 35e9798 to f850887 Compare May 6, 2024 19:09
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

have some naming suggestions below

workflow/common/security_context.go Outdated Show resolved Hide resolved
workflow/common/security_context.go Outdated Show resolved Hide resolved
workflow/controller/artifact_gc.go Outdated Show resolved Hide resolved
@agilgur5 agilgur5 changed the title feat: set the seccomp profile for artifact gc to runtimedefault feat(executor): set seccomp profile to runtimedefault May 11, 2024
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

1 change needed to the resource container, otherwise LGTM

workflow/controller/workflowpod.go Show resolved Hide resolved
@agilgur5
Copy link
Member

Oh and the other remaining test failure needs a change in TestExecutorPluginsSuite/TestTemplateExecutor to handle the now set SC fields (it was expecting them to be nil before):

executor_plugins_test.go:41: 
        	Error Trace:	/home/runner/work/argo-workflows/argo-workflows/test/e2e/executor_plugins_test.go:41
        	            				/home/runner/work/argo-workflows/argo-workflows/test/e2e/fixtures/then.go:262
        	            				/home/runner/work/argo-workflows/argo-workflows/test/e2e/executor_plugins_test.go:36
        	Error:      	Not equal: 
        	            	expected: &v1.PodSecurityContext{SELinuxOptions:(*v1.SELinuxOptions)(nil), WindowsOptions:(*v1.WindowsSecurityContextOptions)(nil), RunAsUser:(*int64)(0xc000107280), RunAsGroup:(*int64)(nil), RunAsNonRoot:(*bool)(0xc000107288), SupplementalGroups:[]int64(nil), FSGroup:(*int64)(nil), Sysctls:[]v1.Sysctl(nil), FSGroupChangePolicy:(*v1.PodFSGroupChangePolicy)(nil), SeccompProfile:(*v1.SeccompProfile)(nil)}
        	            	actual  : &v1.PodSecurityContext{SELinuxOptions:(*v1.SELinuxOptions)(nil), WindowsOptions:(*v1.WindowsSecurityContextOptions)(nil), RunAsUser:(*int64)(0xc000106f58), RunAsGroup:(*int64)(nil), RunAsNonRoot:(*bool)(0xc000106fe4), SupplementalGroups:[]int64(nil), FSGroup:(*int64)(nil), Sysctls:[]v1.Sysctl(nil), FSGroupChangePolicy:(*v1.PodFSGroupChangePolicy)(nil), SeccompProfile:(*v1.SeccompProfile)(0xc00050d290)}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -10,3 +10,6 @@
        	            	  FSGroupChangePolicy: (*v1.PodFSGroupChangePolicy)(<nil>),
        	            	- SeccompProfile: (*v1.SeccompProfile)(<nil>)
        	            	+ SeccompProfile: (*v1.SeccompProfile)({
        	            	+  Type: (v1.SeccompProfileType) (len=14) "RuntimeDefault",
        	            	+  LocalhostProfile: (*string)(<nil>)
        	            	+ })
        	            	 })
        	Test:       	TestExecutorPluginsSuite/TestTemplateExecutor
    executor_plugins_test.go:69: 
        	Error Trace:	/home/runner/work/argo-workflows/argo-workflows/test/e2e/executor_plugins_test.go:69
        	            				/home/runner/work/argo-workflows/argo-workflows/test/e2e/fixtures/then.go:262
        	            				/home/runner/work/argo-workflows/argo-workflows/test/e2e/executor_plugins_test.go:[36](https://github.com/argoproj/argo-workflows/actions/runs/9028328717/job/24808748392?pr=12984#step:18:37)
        	Error:      	Not equal: 
        	            	expected: &v1.SecurityContext{Capabilities:(*v1.Capabilities)(0xc0005a6360), Privileged:(*bool)(nil), SELinuxOptions:(*v1.SELinuxOptions)(nil), WindowsOptions:(*v1.WindowsSecurityContextOptions)(nil), RunAsUser:(*int64)(0xc000728e18), RunAsGroup:(*int64)(nil), RunAsNonRoot:(*bool)(0xc000728e20), ReadOnlyRootFilesystem:(*bool)(0xc000728e22), AllowPrivilegeEscalation:(*bool)(0xc000728e21), ProcMount:(*v1.ProcMountType)(nil), SeccompProfile:(*v1.SeccompProfile)(nil)}
        	            	actual  : &v1.SecurityContext{Capabilities:(*v1.Capabilities)(0xc0001271a0), Privileged:(*bool)(0xc000106dbf), SELinuxOptions:(*v1.SELinuxOptions)(nil), WindowsOptions:(*v1.WindowsSecurityContextOptions)(nil), RunAsUser:(*int64)(0xc000106dc0), RunAsGroup:(*int64)(nil), RunAsNonRoot:(*bool)(0xc000106dcc), ReadOnlyRootFilesystem:(*bool)(0xc000106dcd), AllowPrivilegeEscalation:(*bool)(0xc000106dce), ProcMount:(*v1.ProcMountType)(nil), SeccompProfile:(*v1.SeccompProfile)(0xc000[50](https://github.com/argoproj/argo-workflows/actions/runs/9028328717/job/24808748392?pr=12984#step:18:51)d278)}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -7,3 +7,3 @@
        	            	  }),
        	            	- Privileged: (*bool)(<nil>),
        	            	+ Privileged: (*bool)(false),
        	            	  SELinuxOptions: (*v1.SELinuxOptions)(<nil>),
        	            	@@ -16,3 +16,6 @@
        	            	  ProcMount: (*v1.ProcMountType)(<nil>),
        	            	- SeccompProfile: (*v1.SeccompProfile)(<nil>)
        	            	+ SeccompProfile: (*v1.SeccompProfile)({
        	            	+  Type: (v1.SeccompProfileType) (len=14) "RuntimeDefault",
        	            	+  LocalhostProfile: (*string)(<nil>)
        	            	+ })
        	            	 })
        	Test:       	TestExecutorPluginsSuite/TestTemplateExecutor
=== FAIL: ExecutorPluginsSuite/TestTemplateExecutor

@agilgur5 agilgur5 added area/executor area/templates/resource area/agent Argo Agent that runs for HTTP and Plugin templates labels May 11, 2024
@lukashankeln lukashankeln force-pushed the feat/artifact-gc-seccomp branch 3 times, most recently from bf144fb to c2a5458 Compare May 11, 2024 16:00
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for iterating on this and improving the default security contexts!

@agilgur5 agilgur5 enabled auto-merge (squash) May 11, 2024 16:07
@agilgur5 agilgur5 added this to the v3.6.0 milestone May 11, 2024
Signed-off-by: Lukas Hankeln <lukashankeln@googlemail.com>
auto-merge was automatically disabled May 11, 2024 16:17

Head branch was pushed to by a user without write access

@lukashankeln
Copy link
Contributor Author

LGTM, thanks for iterating on this and improving the default security contexts!

Thank you for helping me out. One test was still failing, that should pass now.

@agilgur5 agilgur5 enabled auto-merge (squash) May 11, 2024 16:25
@agilgur5 agilgur5 merged commit f4419d0 into argoproj:main May 11, 2024
27 of 28 checks passed
@lukashankeln lukashankeln deleted the feat/artifact-gc-seccomp branch May 11, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent Argo Agent that runs for HTTP and Plugin templates area/artifacts S3/GCP/OSS/Git/HDFS etc area/executor area/gc Garbage collection, such as TTLs, retentionPolicy, delays, and more area/templates/resource solution/workaround There's a workaround, might not be great, but exists type/security Security related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants