Fix privileged steps in kubernetes

[anbraten]

closes #3537

[qwerty287]

Shouldn't it only possible to use this when the repo is trusted? Where is this checked?

[xoxys]

Yep, it looks like it ignores the entire discussion from the linked PR.

[anbraten]

Step privileged is only set in case its a plugin and in the list of privileged plugins:

woodpecker/pipeline/frontend/yaml/compiler/convert.go

Line 130 in 75803db

privileged = true

Or if the privileged: true option is set.

In case a user tries to set that option without having a trusted repo he will get a linter error from:

woodpecker/pipeline/frontend/yaml/linter/linter.go

Line 115 in b2cfa37

if !l.trusted {

woodpecker/pipeline/frontend/yaml/linter/linter.go

Line 153 in b2cfa37

err = "Insufficient privileges to use privileged mode"

[qwerty287]

Yes, but there's no linter for securityContext option. You can set that on any step and make the step privileged.

[anbraten]

Setting the security-context / user / group to 0 (root) or fs-group etc is checked here and will only work for privileged steps which can only be allowed by trusted plugins or by setting privileged: true which can only be done if the repo is trusted:

woodpecker/pipeline/backend/kubernetes/pod.go

Line 391 in 9cc5d08

user = sc.RunAsUser

woodpecker/pipeline/backend/kubernetes/pod.go

Line 445 in 9cc5d08

return nil

[anbraten]

Yep, it looks like it ignores the entire discussion from the linked PR.

It solves the issue that the woodpeckerci/plugin-docker-buildx plugin can only be used if a securityContext is explicitly configured. For already privileged steps (trusted plugins) this wont be necessary anymore now.

[qwerty287]

And this is the only option that could be dangerous?

[anbraten]

At least those are the options we have and the checks we do:

woodpecker/pipeline/backend/kubernetes/pod.go

Lines 389 to 409 in b08133b

	// only allow to set user if its not root or step is privileged
	if sc.RunAsUser != nil && (*sc.RunAsUser != 0 || stepPrivileged) {
	user = sc.RunAsUser
	}
	// only allow to set group if its not root or step is privileged
	if sc.RunAsGroup != nil && (*sc.RunAsGroup != 0 || stepPrivileged) {
	group = sc.RunAsGroup
	}
	// only allow to set fsGroup if its not root or step is privileged
	if sc.FSGroup != nil && (*sc.FSGroup != 0 || stepPrivileged) {
	fsGroup = sc.FSGroup
	}
	// only allow to set nonRoot if it's not set globally already
	if nonRoot == nil && sc.RunAsNonRoot != nil {
	nonRoot = sc.RunAsNonRoot
	}
	seccomp = seccompProfile(sc.SeccompProfile)

[anbraten]

My goal was to simply fix the actual issue #3537 first 🤷🏾 . Changing the logic to require a trusted repo for the buildx plugin is somehow a different change IMO (at least that's how the docker backend works and the docs explain it to the users) .

Requiring a trusted repo to manually set the securityContext could be done separately.

[qwerty287]

Thanks, yes, in general it should work like docker. However, users should not be allowed to set dangerous options without the repo being trusted.
Maybe we can merge this as a hotfix so we can finally release 2.5.0 and revisit this whole topic again?
I'm not really in the discussion of this issue so I can't really tell you whether this solution was discussed already and rejected or whatever. Maybe @xoxys you can tell us more about k8s and this discussion I think?

[xoxys]

I would suggest to involve the people that have created, contributed and tested the original PR. This entire discussion should have been done in the existing PR instead of just starting a new one...

[qwerty287]

What about this now? Would it be fine to get it merged as hotfix and then rework the whole thing again?
I really would like to get 2.5.0 out.

[HannesDampft]

I would like to briefly share my perspective as a user of the Kubernetes backend. Recently, I had to downgrade to version 2.3 because it is not feasible to ask every user to adapt to a bug in Woodpecker that will eventually be reverted.

If this PR is skipped in the release, most Kubernetes users would likely have to skip this release, which would be unfortunate.

I sincerely appreciate the inclusion of this PR in the next release, as I expect most of us users would.

[qwerty287]

I'll just approve this now to get 2.5.0 ready.

Please use a new discussion to revert these changes and make it how it should work ideally, but the bug should be fixed asap.

[anbraten]

Thanks for approving.

Please use a new discussion to revert these changes and make it how it should work ideally, but the bug should be fixed asap.

We can just move on with #3538 as this is part of it.