Using repository trust in security contexts #3538

zc-devs · 2024-03-23T17:55:25Z

Addresses #3537 (comment)

zc-devs · 2024-03-23T18:03:03Z

As it uses step's privileged, should close #3537.

6543 · 2024-03-23T21:23:58Z

@zc-devs bugfix or enhancement :)

zc-devs · 2024-03-23T22:21:05Z

bugfix or enhancement

As a user I would call #3537 a bug => bugfix.

I also should have notified, that I did not test it. If code looks good, then I would request image to test by me and probably @everflux.

BTW, I noticed the same error as I almost always face. This occurs not only with fr.js. Does someone know what is it and how to get rid of? Now, I'm restarting a pipeline until it works.

everflux · 2024-03-24T08:08:52Z

Happy to test it out, do you have a pullable image?

xoxys · 2024-03-24T13:01:17Z

pipeline/backend/kubernetes/pod.go

@@ -464,6 +480,15 @@ func mapToEnvVars(m map[string]string) []v1.EnvVar {
 	return ev
 }

+func isRepoTrusted(step *types.Step) bool {
+	// TODO: probably, should be stored in the model (pipeline/backend/types/config.go)
+	trustedRepo, err := strconv.ParseBool(step.Environment["CI_REPO_TRUSTED"])


Is it safe to read it from the env? It can't be overwritten by users?

Is it safe to read it from the env?

¯_(ツ)_/¯
Elaborate, please.
Do you suggest to use dedicated variable in pipeline/backend/types/config.go?
What is the difference between env var (one map's entry) vs dedicated variable?

It can't be overwritten by users?

Seems, not for now, at least: #3164.

Elaborate, please.
Do you suggest to use dedicated variable in pipeline/backend/types/config.go?

Right now I have not suggested anything :) I just asked if it is safe to read a security-related setting from an env var. If only the server can set it, and it can't be overwritten by the pipeline config, I'm fine with it, but it might increase the possibilities to become an issue in the future.

Yes, might be an issue. Depends on #3164 implementation.
So, what are we gonna do?

IMO, the clean solution would be to pass down the repo settings to the backend similar to Environment e.g.
Step.RepoSettings.

zc-devs · 2024-03-24T14:57:29Z

do you have a pullable image?

There.
@6543, thanks for a label.
@everflux, I've tested ⬇️ trusted/untrusted logic. Could you test if plugin works?

zc-devs · 2024-03-24T15:18:06Z

Pipeline:

skip_clone: true
steps:
  server:
    image: alpine
    commands:
      - echo Hello
      - sleep 60
    backend_options:
      kubernetes:
        securityContext:
          privileged: true
          runAsUser: 0
          runAsGroup: 0

woodpecker-agent.log
pod-untrusted.yaml
pod-trusted.yaml.txt

everflux · 2024-03-24T22:12:44Z

Thanks for looking into it this quickly.
I have currently only arm machine at my disposal I plan to verify tomorrow evening.

everflux · 2024-03-25T18:41:31Z

Successfully tested the prebuilt image with buildx plugin. Only setting "trusted repository" and no additional "privileged" setting on the workflow or workflow.backend level. Nice!

stevapple · 2024-03-25T19:47:43Z

Only setting "trusted repository" and no additional "privileged" setting on the workflow or workflow.backend level.

IIRC running a trusted plugin (with a privileged container) shouldn't require the repository to be trusted?

everflux · 2024-03-26T20:43:52Z

I would be really happy if this could be included in 2.5.0 and any further changes could be discussed separately. (For example should escalated plugins be treated as trusted implicitly, would handling of volumes differently from privileged make sense etc)

zc-devs · 2024-03-26T22:34:42Z

Only setting "trusted repository"

Now, Buildx plugin should work without "trusted repository" as it was before, on 2.3.x.

everflux · 2024-04-02T07:50:27Z

Any chance this could be merged into a 2.4.x release?

qwerty287 · 2024-04-02T07:57:00Z

Probably not, but if this is merged you can use next. 2.5.0 should be released on Apr 17 at the latest.

everflux · 2024-04-02T09:04:21Z

Too bad, I have a training planned on April 9th...

xoxys · 2024-04-02T09:05:43Z

~~We can add a label to build an image for this PR and you can use that one for your training.~~

That exists already https://hub.docker.com/layers/woodpeckerci/woodpecker-server/pull_3538-alpine/images/sha256-ed7b0918be40787a5d4293a372195b3dedab165b63d2189a0585c2e64efaf20b?context=explore

everflux · 2024-04-02T10:19:03Z

Thanks for the proposal, but that won't suffice (mixed user base with apple arm64 and amd64 architectures). No worries, though, I can use drone instead, the concepts are similar enough. And I don't want to cause additional effort in any way, I was just hoping for a point release before 2.5.

pat-s · 2024-04-08T15:54:50Z

I would be ok merging this to allow @everflux to possible use WP on April 9th (if not too late). Besides, I am seeing the same issues on our PROD instance with more than 50+ files I would need to adapt on a mixed cluster, hence I am also having a strong need for a fix that restores 2.3.x functionality.

If not everything is working percectly yet we can still iterate until 2.5.x but in the meantime allow for a workaround via next for mixed cluster users.

@zc-devs Merging main caused some build failures now. In addition, cspell.json needs a fix. I just a few minutes right now but can look into it later this evening.

xoxys · 2024-04-08T16:28:50Z

Not perfectly working and introducing another security issue again is a difference 🙂 if we are sure reading the repo trust setting from the env var and it cant be manipulated by end users feel free to go on.

# Conflicts: # pipeline/backend/kubernetes/pod.go

zc-devs · 2024-04-08T17:15:11Z

Merging main caused some build failures now

Fixed.

reading the repo trust setting from the env var...

the clean solution would be to pass down the repo settings to the backend similar to Environment e.g.
Step.RepoSettings

TBH, I don't want to put my dirty hands in the "core". And definitely won't before majority agreed on implementation or at least said something on topic.
Looks like a separate PR to me.

xoxys · 2024-04-08T17:19:53Z

Fully agree on that but the PR should not be merged untill the majority agreed than as well.

pat-s · 2024-04-16T09:16:00Z

@zc-devs @xoxys It seems one conversation from you is still open (see above). How can we make progress there?

If this doesn't make it into 2.5.0 (release date tomorrow), 2.5.0 will also be unusable as 2.4.x for folks coming from 2.3.x without touching many workflow files.

The current situation is therefore a bit suboptimal. If we can't find an agreement, I'd also consider reverting the backend change in 2.4 and restore the 2.3 behavior and include the proposed changes of this PR in a new/updated approach?

zc-devs · 2024-04-16T10:20:58Z

I think, Xoxys and I have met agreement. Moreover, I'm on his side in general (otherwise didn't leave the TODO).

How can we make progress there?

the majority agreed

@6543, @anbraten, @qwerty287

release date tomorrow

It can be merged as is by deadline. At least I cannot develop in next few days.

reverting the backend change in 2.4

You cannot.

pat-s · 2024-04-16T10:36:10Z

You cannot #3537 (comment).

I know about the issue but that doesn't mean it is not revertable at all. Anyone can still run and install any WP version (even the ones including the mentioned flaws). But anyway, as this is not the main goal here, let's focus on getting this one here merged and move onward.

everflux · 2024-04-17T12:21:16Z

Perhaps the release could wait for resolving this issue?

Using repository trust in security contexts

e76f370

zc-devs marked this pull request as ready for review March 23, 2024 18:03

6543 added the backend/kubernetes label Mar 23, 2024

linter

c6f6352

6543 added build_pr_images If set, the CI will build images for this PR and push to Dockerhub bug Something isn't working labels Mar 24, 2024

Merge branch 'main' into 3537-secctx

598fb0a

xoxys reviewed Mar 24, 2024

View reviewed changes

zc-devs mentioned this pull request Mar 24, 2024

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

Open

Merge branch 'main' into 3537-secctx

b69bb22

qwerty287 added this to the 2.5.0 milestone Mar 29, 2024

Merge remote-tracking branch 'upstream/main' into 3537-secctx

7b3b3a8

# Conflicts: # pipeline/backend/kubernetes/pod.go

zc-devs force-pushed the 3537-secctx branch from 72678e8 to 7b3b3a8 Compare April 8, 2024 16:56

anbraten mentioned this pull request May 15, 2024

Fix privileged steps in kubernetes #3711

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using repository trust in security contexts #3538

Using repository trust in security contexts #3538

zc-devs commented Mar 23, 2024

zc-devs commented Mar 23, 2024

6543 commented Mar 23, 2024

zc-devs commented Mar 23, 2024 •

edited

everflux commented Mar 24, 2024

xoxys Mar 24, 2024

zc-devs Mar 24, 2024

xoxys Mar 24, 2024 •

edited

zc-devs Mar 24, 2024

xoxys Apr 2, 2024

zc-devs commented Mar 24, 2024 •

edited

zc-devs commented Mar 24, 2024

everflux commented Mar 24, 2024

everflux commented Mar 25, 2024

stevapple commented Mar 25, 2024

everflux commented Mar 26, 2024

zc-devs commented Mar 26, 2024

everflux commented Apr 2, 2024

qwerty287 commented Apr 2, 2024

everflux commented Apr 2, 2024

xoxys commented Apr 2, 2024 •

edited

everflux commented Apr 2, 2024

pat-s commented Apr 8, 2024

xoxys commented Apr 8, 2024

zc-devs commented Apr 8, 2024 •

edited

xoxys commented Apr 8, 2024 •

edited

pat-s commented Apr 16, 2024

zc-devs commented Apr 16, 2024 •

edited

pat-s commented Apr 16, 2024 •

edited

everflux commented Apr 17, 2024

Using repository trust in security contexts #3538

Are you sure you want to change the base?

Using repository trust in security contexts #3538

Conversation

zc-devs commented Mar 23, 2024

zc-devs commented Mar 23, 2024

6543 commented Mar 23, 2024

zc-devs commented Mar 23, 2024 • edited

everflux commented Mar 24, 2024

xoxys Mar 24, 2024

Choose a reason for hiding this comment

zc-devs Mar 24, 2024

Choose a reason for hiding this comment

xoxys Mar 24, 2024 • edited

Choose a reason for hiding this comment

zc-devs Mar 24, 2024

Choose a reason for hiding this comment

xoxys Apr 2, 2024

Choose a reason for hiding this comment

zc-devs commented Mar 24, 2024 • edited

zc-devs commented Mar 24, 2024

everflux commented Mar 24, 2024

everflux commented Mar 25, 2024

stevapple commented Mar 25, 2024

everflux commented Mar 26, 2024

zc-devs commented Mar 26, 2024

everflux commented Apr 2, 2024

qwerty287 commented Apr 2, 2024

everflux commented Apr 2, 2024

xoxys commented Apr 2, 2024 • edited

everflux commented Apr 2, 2024

pat-s commented Apr 8, 2024

xoxys commented Apr 8, 2024

zc-devs commented Apr 8, 2024 • edited

xoxys commented Apr 8, 2024 • edited

pat-s commented Apr 16, 2024

zc-devs commented Apr 16, 2024 • edited

pat-s commented Apr 16, 2024 • edited

everflux commented Apr 17, 2024

zc-devs commented Mar 23, 2024 •

edited

xoxys Mar 24, 2024 •

edited

zc-devs commented Mar 24, 2024 •

edited

xoxys commented Apr 2, 2024 •

edited

zc-devs commented Apr 8, 2024 •

edited

xoxys commented Apr 8, 2024 •

edited

zc-devs commented Apr 16, 2024 •

edited

pat-s commented Apr 16, 2024 •

edited