Woodpecker 2.4 breaks privileged steps/plugins with Kubernetes backend

[everflux]

This worked with WP 2.3 and Kubernetes backend:

  publish:
    image: woodpeckerci/plugin-docker-buildx
    settings:
      repo: *repo
      tags: 8h

Using WP 2.4 the docker daemon does not start.

Debugging the Pod manifest I see an empty security context and the docker daemon does not start.

This works with WP 2.4.1 but is not so user friendly:

  publish:
    image: woodpeckerci/plugin-docker-buildx
    privileged: true
    backend_options:
      kubernetes:
        securityContext:
          privileged: true
    settings:
      repo: *repo
      tags: 8h
      daemon.debug: "true"

See also: #3482 (comment)_

[zc-devs]

Comment is almost useless, but...

1. I think you are talking about 2.3.x, 2.4.x versions.
2. One of the options should be enough. Pipeline's privileged takes precedence over backend's option, as I remember.

    privileged: true
    backend_options:
      kubernetes:
        securityContext:
          privileged: true

Edit 1.
Otherwise - backend's option took precedence over step's one:

        if sc != nil && sc.Privileged != nil && *sc.Privileged {
		privileged = sc.Privileged // true
	} else if stepPrivileged {
		privileged = &stepPrivileged // true
	}

before #3482

[everflux]

You are correct. I actually meant version 2.4, I linked the comment as I suspect this commit might be the cause.

For me it did not work to set privileged only on pipeline level.
With the old woodpecker version 2.3, it was not necessary to set the option at all.
I am quite sure this is because this image is recognized against list of hardcoded images to be run in privileged mode when used as plugin.

[zc-devs]

I am quite sure this is because this image is recognized against list of hardcoded images to be run in privileged mode when used as plugin.

Seems, it sets up privileged on step level.

With the old woodpecker version 2.3, it was not necessary to set the option at all.

Backend option was checked firstly. If was not set, step-level option was checked (was set by trusted plugin logic) => security context was set.

For me it did not work to set privileged only on pipeline level.

Now, it is and logic: security context sets from backend option if step option is set too.
So, as pipeline-level option is set by plugin logic, you have to set backend one as well.

cc @anbraten

Related #3511

[anbraten]

Haven't had a closer look into this issue yet, but it could definitely be related to #3482. We changed the logic in there as we got security request checking this area again as it seemed to be possible to run privileged pods in Kubernetes.

[zc-devs]

Then ideally, all checks on pipeline's stepPrivileged should check trusted repository's flag instead, I think.

[everflux]

Now, it is and logic: security context sets from backend option if step option is set too.
So, as pipeline-level option is set by plugin logic, you have to set backend one as well.

I understand the motivation, please don’t get the following feedback wrong: I am in the situation to prepare an elaborated lab setup for 30+ participants and choose woodpecker for a number of reasons over drone. During rehearsal this change caught me unprepared and lead to quite some anxiety and stress as I had to pinpoint the problem after everything broke and I had to question wether I did something wrong or had undocumented steps.
I would appreciate if such breaking changes would lead to a major semver increase to be aware of it.

On the implementation: Perhaps it would make sense to distinguish between trusted (volumes) and privileged repositories? Just a thought.
The current implementation requiring backend specific configuration options feels like boilerplate and bad user experience to me. (The hard coded list of plugins do not make much sense with this as well imho )

[zc-devs]

Understandable.

I would appreciate if such breaking changes would lead to a major semver increase to be aware of it.

Choice Semver is an illusion 😄 And I feel that an even version is an RC for odd one... Looking forward to stable 2.5 :)
Apart from above, it would be great having some explanation of security changes like impact, severity. So, I can decide to upgrade or not.

Perhaps it would make sense to distinguish between trusted (volumes) and privileged repositories?

Perhaps you meant this ⬇️

At least it should be better worded. Mounting the volumes is not the main purpose of it, to me.

boilerplate and bad user experience

Yeah...

---

Then ideally, all checks on pipeline's stepPrivileged should check trusted repository's flag instead, I think.

And mentioned list of trusted plugins, seems. Maybe, there combined flag already exists: trusted repository or trusted plugin.

[everflux]

Regarding the repo setting: Thanks, I was aware of the 'trusted' setting. What I wanted to bring up as a possible improvement was to have two settings: One for 'allow volumes' and a different one for 'allow privileged containers'. I always though that mixing these aspects felt a little off as I much more often want to allow volumes than priviliged containers. Kubernetes differentiates these aspects as well.

[stevapple]

IMHO, this might be a somewhat more ideal model:

• If a plugin is trusted, then it can always run privileged unless the user explicitly sets the security context, especially by marking the step or the backend as privileged: false;
• If a repository is not trusted, then the user cannot mark a step or a backend to be privileged: true or to run as root, which means it should trigger a validation error;
• If a repository is trusted but the step is not privileged, then the user cannot mark the backend to be privileged: true or to run as root, which means it should trigger a validation error;
• If a repository is trusted, and the user marks the step as privileged: true, then the pod is implicitly privileged: true, unless being opted out by privileged: false in the backend settings.

And to achieve such model (not familiar with implementation detail so I'll go with Javascript-like pseudo code):

function getSecurityContext(repo, plugin, step) {
  function _getSecurityContext(plugin, step) {
    if (plugin.trusted) {
      return [step.privileged ?? plugin.privileged, {...plugin.securityContext, ...step.securityContext}]
    } else {
      return [step.privileged, {privileged: step.privileged, ...step.securityContext}]
    }
  }
  function _validateSecurityContext(repo, privileged, securityContext) {
    if (!repo.trusted && privileged) {
      throw new ValidationError("Cannot run a privileged step in an untrusted repository")
    }
    if (!privileged && (securityContext.privileged || securityContext.runAsUser === 0 || securityContext.runAsGroup === 0) {
      throw new ValidationError("Cannot elevate a Pod in an unprivileged step")
    }
  }
  const [privileged, securityContext] = _getSecurityContext(plugin, step)
  _validateSecurityContext(repo, privileged, securityContext)
  return [privileged ?? false, securityContext]
}

[everflux]

I like the general idea.
Just two additions. I really like the idea to have plugins that can run privileged if the repository is trusted, for example the buildx requires it. So in addition to a user that marks a repository as trusted I suggest that trusted plugins can by default run as privileged (but not without marking the repository trusted). It might be a nice thing to display a hint about this behavior as well.

The other aspect is about volumes: I would like to be able to mount a volume but not elevate the privileges automatically. They are different aspects anyway, but if a step is privileged, volumes could be included. So something like trust levels (ux: slider?) might be worth thinking about.

Repository trust: "none", "volumes", "privileged"

[anbraten]

The other aspect is about volumes: I would like to be able to mount a volume but not elevate the privileges automatically. They are different aspects anyway, but if a step is privileged, volumes could be included. So something like trust levels (ux: slider?) might be worth thinking about.

We also have to support this for other backends and as for example docker would allow mounting the docker.sock again, you could reach root access by using volumes. #2272 might be an option.

[everflux]

Valid point, how about mounting predefined volumes only? Docker supports named volumes as well. Temporary volumes for a single instance of a workflow feels a little short lived to me. (I have a kaniko volume for base images that I would like to preserve across multiple invocations of a workflow.)

[zc-devs]

If a repository is trusted but the step is not privileged, then the user cannot mark the backend to be privileged: true or to run as root

If you can set privileged on step level (if trusted repo), why you can't via backend options? What is the difference?
Other points are implemented as you wrote, except validation error.

I really like the idea...

It is controlled by some logic, that sets up step-level privileged flag in core. Should work.

[stevapple]

If you can set privileged on step level (if trusted repo), why you can't via backend options? What is the difference?

Because I feel like the privileged step not necessarily means a privileged container. It’s just a signal that the step can have additional security privileges over regular ones, and that can be overridden by privileged: false in the backend options. On the other hand, an unprivileged step should not have such privileges.

Much like “hey I just want to give some of you the privileges to do whatever you want, but you can always deny if you don’t like it”.

[zc-devs]

It’s just a signal that the step can have additional security privileges over regular ones
I just want to give some of you the privileges to do whatever you want, but you can always deny if you don’t like it

That is what trusted repo for.

I feel like the privileged step not necessarily means a privileged container

But that is exactly how it was implemented and it is now. When I implemented security context, I added just yet another source of this field. If you look in kube docs, privileged is container's security context flag.

The --privileged flag gives all capabilities to the container. All capabilities. The same with Kubernetes.

If you want to change semantics of the pipeline's (whether step's or backend option) privileged flag, then I would suggest to create separate discussion/issue. It will breaking change, at least.

The same with volumes stuff, I think. When issue is closed, everflux's proposal will get lost.

---

kaniko volume for base images

Try pull through cache.

[anbraten]

Did some testing and think #3711 should recover the expected behavior.