How to approach (possibly) dangerous functionality?

[zc-devs]

#3655:

EnvVars: []string{"WOODPECKER_BACKEND_K8S_ALLOW_NATIVE_SECRETS"}

#3609:

EnvVars: []string{"WOODPECKER_BACKEND_K8S_POD_LABELS_ALLOW_FROM_STEP"},
EnvVars: []string{"WOODPECKER_BACKEND_K8S_POD_ANNOTATIONS_ALLOW_FROM_STEP"},

@qwerty287 wrote (#3655 (comment)):

I have some kind of issue with the kubernetes approach of options that could be/are dangerous.

We currently have an env var option for everyone of them, just like WOODPECKER_BACKEND_K8S_ALLOW_NATIVE_SECRETS. The docker backend is doing it differently: You can't use the feature as long as the repo isn't trusted. If it's trusted you can use these features.

I prefer the docker approach. It's configurable from ui and can be set per repo. Also, kubernetes and docker backends should work in a similar way and why do we have the trusted option if we don't use, but rather add new env options?

Yes, I know that this is currently not really possible and we would have to do some refactoring for the backends first, but just wanted to point that out.

[zc-devs]

Treat this flags as feature gates.
Yes, it can be used as security valve as "last" resort - disable this functionality on the instance, but this is not the main function.

In regard #3655. Security constraints are not implemented yet. But they will be by Implement access controls and Implement secret masking tasks. So, #3655 functionality is in experimental stage, I don't want to write documentation now, there is no need to announce it. Who want it, will enable the flag. When all tasks are implemented, we can change default value to true. Though, there will be "emergency brake" in case of some regression in this functionality. Note, that admin can decide whether it affects him or not.

In regard #3609. It is just precaution. Let admin decide 😆

In regard #3538. This user=0, group=0 are variation of privileged=true. privileged was implemented via trusted repo's flag. So, it does make sense to implement it like it was. So did I in #3538. Why didn't I make it before - cause I didn't pay much attention to this, I thought we could just patch it later if needed. Feature flag could save us, though ...

kubernetes approach

Kubernetes features - Kubernetes flags, they are in *BACKEND_K8S* namespace ;)

We currently have an env var option...
trusted repo is configurable from ui and can be set per repo

While trusted repo can be set per repo, what if I want to set it per instance? What if I have 100500 repos in public instance?
I really like how Woodpecker configures via env vars. It is really "cloud native" and stateless. In contrast some apps use own config and just override them from envs. But they can support only subset of options via env vars, or set them only first time/run...

You can't use the feature as long as the repo isn't trusted. If it's trusted you can use these features.
why do we have the trusted option if we don't use, but rather add new env options?

It's just one flag. We need go deeper some permission/feature matrix then.
I need some K8s functionality (native secrets, for example). Someone want volumes. We need more granular control, in case of emergency to not disable half of CI functionality. And to follow least privileges principle.

---

Overall, I like feature flags. Though, trusted should be improved.
Feature flags (instance wide) could coexist with feature/permission matrix (per repo). It would be the best, IMO.

[qwerty287]

While trusted repo can be set per repo, what if I want to set it per instance?

It's not a big problem to add a var to enable it by default.

What if I have 100500 repos in public instance?

I don't get that. Why do you want to make repos trusted on a public instance?

Feature flags (instance wide) could coexist with feature/permission matrix (per repo). It would be the best, IMO.

Probably yes, as it looks like the kubernetes backend has a lot more features compared to docker.

But do you mean an "and" or "or" connection?

I.e. you're able to use a feature if it's enabled from env var and the repo is trusted? (That's what I would prefer)

I understand that you want to disable features globally, but still I think you shouldn't be able to use enabled features if the repo isn't trusted.
The kubernetes backend is, compared to docker, currently very unsuitable for public instances if you have a few repos that need additional features. They should work similar though.

Or did you mean by "matrix" that every feature can be enabled/disabled per repo?

[dominic-p]

Just to share my perspective here, my use case is a very private CI environment. Adding manual config in the UI to enable these features would add some friction for me.

Also, I agree with @zc-devs that the env variables can allow for a more fine grained approach than a blanket trusted switch. For example, an instances may need to allow setting security context but may not need secret access. In that case, we can have tighter controls using env vars.

It's not a big problem to add a var to enable it by default.

This ideas concerns me a bit. If we adopt an "and" logic where in order to enable secret access (for example) you need both the env variable AND the repo to be trusted. I will probably wind up needing to make all of my repo's trusted. However, that will likely lead to a situation where I'm granting too many privileges and opening up additional security holes.

In the end, my feelings are that we need more granular control at the repo level (i.e. not just a single trusted switch) if we go with an "and" relationship between env vars and repo level permissions.

[zc-devs]

Did you mean by "matrix" that every feature can be enabled/disabled per repo?

Yes. "My" feature gates are per instance. Matrix is per repo.
Also, feature flags do gate some backend functionality. Therefore it makes sense to do it in Agent as it is now. While "matrix" would be Server-side logic (UI, DB).

It's not a big problem to add a var to enable it by default.

If you were talking about flag per feature it would be current feature flags :) But you are about trusted repo... It is not useful in current state, IMO ⬇️

native secrets, volumes, etc...

Apart from me, it is explained by @dominic-p in #3758 (comment) and @everflux in #3537 (comment).

Why do you want to make repos trusted on a public instance?

I think this is a problem by itself. Why do I have to gain "root" access on the repos, if I want just enable some subset of features?

But do you mean an "and" or "or" connection?
I.e. you're able to use a feature if it's enabled from env var and the repo is trusted?

As I sad I do not consider trusted as a usable option at all...
Server's repo permission should be allowed to set by admin. Agent feature gate... is set by admin. As feature is not some thing to take precedence => I think logic is "or". You enable it per repo in Server UI or per instance via Agent's envs.

Edit: just to be more clear. In my thoughts, trusted transforms to permissions/features "matrix", instance-wide feature flags stays as envs in Agent.

[qwerty287]

While for K8s guaranteed requests.cpu there is no direct alternative (?), but perhaps palying with cpu-period and cpu-quota.

We can also just move those that are not directly supported to backend options.

Anyway, Local doesn't support this (?). It is OK to have different syntax for different backends. Example - image in Local backend.

Yes, but it's better to have as much as possible in the same way.

I know it's not ideal to have it in two different places, but it would be fine for me. Otherwise we probably could also just drop them as they are undocumented and I don't think anyone is using them

Then looks like dns, dns search and extra hosts are not dangerous. I may be wrong, but cannot imagine an example.
Yes, I was also wondering about this, I also don't think they're dangerous. I can check that later again.

Local backend have come up to my mind.
If it is implemented via /etc/resolv.conf and /etc/hosts, then it could just broke the host at least. In the worst case, it could trick > host / other pipelines to connect to a malicious host.

I don't think local should support this in general. You shouldn't run the agent using a privileged user so it shouldn't be possible to edit the files in /etc.

[zc-devs]

I don't think local should support this in general. You shouldn't run the agent using a privileged user so it shouldn't be possible to edit the files in /etc.

Fair enough. Then privileged won't work either. The point is dangerousness of some features differs from backend to backend.

I know it's not ideal to have it in two different places

If they are mutually exclusive like requests.memory, limits.memory, limits.cpu in step, but requests.cpu in backend options, I'm against this.
If duplicate them like requests.memory, limits.memory, limits.cpu in step and requests.memory, limits.memory, limits.cpu, requests.cpu in backend options, it is better, but worth to implement?

it's better to have as much as possible in the same way

This universal options (like memory and CPU resources) would be different from their native options. If I want to use them I have to learn how they are translated into native options (even look into codebase maybe). It isn't convenient to person with Kubernetes background, nor Docker. Using backend options way more simple.
If I was using such options I would probably stick with backend options. But I do not use now, I use namespace's LimitRange. You mentioned, that Docker options are used not so often.

Therefore, question is: does it worth to implement universal resources options?
As CPUQuota CPUShares CPUSet are implemented already, I would move them into backend options and add documentations.

[qwerty287]

This universal options (like memory and CPU resources) would be different from their native options. If I want to use them I have to learn how they are translated into native options (even look into codebase maybe). It isn't convenient to person with Kubernetes background, nor Docker. Using backend options way more simple.

Ah, I thought they would be usable in the same way for docker and kubernetes. In this case it's probably better to move it to backend options.

[zc-devs]

they are undocumented

Apparently, documented now 😕

[qwerty287]

These are not the pipeline features (which you define in yaml). But yes, for the envs we probably need a deprecation first...

[qwerty287]

I started working on this in #4025.

My roadmap:

1. Split trusted setting
2. Allow backends access to trusted configuration
3. Use the trusted setting in backend to check backend options
4. Check feature compatibility of native features and eventually move to docker backend options
5. Eventually deprecate old env vars to disable features