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

Woodpecker 2.4 breaks privileged steps/plugins with Kubernetes backend #3537

Closed
everflux opened this issue Mar 22, 2024 · 16 comments · Fixed by #3711
Closed

Woodpecker 2.4 breaks privileged steps/plugins with Kubernetes backend #3537

everflux opened this issue Mar 22, 2024 · 16 comments · Fixed by #3711
Labels
backend/kubernetes bug Something isn't working

Comments

@everflux
Copy link

everflux commented Mar 22, 2024

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)_

@everflux everflux changed the title Privileged steps/plugins broken Privileged steps/plugins broken with Kubernetes backend Mar 22, 2024
@everflux everflux changed the title Privileged steps/plugins broken with Kubernetes backend Woodpecker 1.4 breaks privileged steps/plugins with Kubernetes backend Mar 22, 2024
@zc-devs
Copy link
Contributor

zc-devs commented Mar 22, 2024

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
Copy link
Author

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.

@everflux everflux changed the title Woodpecker 1.4 breaks privileged steps/plugins with Kubernetes backend Woodpecker 2.4 breaks privileged steps/plugins with Kubernetes backend Mar 23, 2024
@zc-devs
Copy link
Contributor

zc-devs commented Mar 23, 2024

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
Copy link
Member

anbraten commented Mar 23, 2024

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
Copy link
Contributor

zc-devs commented Mar 23, 2024

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

@everflux
Copy link
Author

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
Copy link
Contributor

zc-devs commented Mar 23, 2024

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 ⬇️

Screenshot 2024-03-23 183548

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
Copy link
Author

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
Copy link
Contributor

stevapple commented Mar 24, 2024

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
Copy link
Author

everflux commented Mar 24, 2024

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
Copy link
Member

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
Copy link
Author

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
Copy link
Contributor

zc-devs commented Mar 24, 2024

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
Copy link
Contributor

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
Copy link
Contributor

zc-devs commented Mar 25, 2024

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
Copy link
Member

anbraten commented May 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend/kubernetes bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants