woodpecker-ci / woodpecker

Woodpecker is a simple, yet powerful CI/CD engine with great extensibility.
https://woodpecker-ci.org
Apache License 2.0
4.11k stars 354 forks source link

Replace `NetrcOnlyTrusted` with list of trusted plugins for netrc #2601

Open anbraten opened 11 months ago

anbraten commented 11 months ago

As noticed in several PRs the current option NetrcOnlyTrusted is limiting users, but on the other hand not really protecting them as well as netrc credentials could be stolen by a custom clone step with custom commands.

To prevent this a pass-netrc-to-plugins option should replace the NetrcOnlyTrusted option. This option would contain list of images which will receive the netrc credentials if they are used as plugins. If the image however uses custom commands it wont get the credentials as those commands could be changed by others than the admin.

This will allow all steps and the clone step to use netrc credentials. It will however be breaking as it wont be possible anymore to use clone steps with custom commands.

pat-s commented 11 months ago

If the image however uses custom commands it wont get the credentials

Then we're still missing an option that allows the use of credentials in custom steps/images. If the repo admin wants to allow it (e.g. in private repos). Currently this is only possible on the instance level by setting the repo to "trusted" but not on the repo level.

So why not structure it like this:

  1. Option to allow injecting netrc into plugins/trusted images
  2. Option to allow injecting netrc into any image

The use case I had was a step executing arbitrary git commands, among them git push, to push back to the repo. Many people like this style instead of having to use a plugin as it "feels like freedom" ;)

6543 commented 9 months ago

is this not solved by simply make

https://github.com/woodpecker-ci/woodpecker/blob/26b77ed677bb6121f35cdba2731692d26bd5b3de/shared/constant/constant.go#L39-L42

as an configuration option?

qwerty287 commented 9 months ago

Yes, but then it's only a global configuration and not per-repo.

6543 commented 9 months ago

an option only instance admins should change in any case

qwerty287 commented 9 months ago

I don't think so. What about codeberg ci? The users wouldn't be able to use this option. Also, if you can set this for every single repo, you can make it more fine-granular: If you would like to use the netrc creds in an image that's not allowed to use them by default, and you need to enable it globally, it will be available to all usages of the image across all repos. If only setting for one repo, it's more limited.

6543 commented 9 months ago

well that would allow any one to steal other users credentials at codeberg:

  1. propose for ci acces
  2. create own repo (with custom clone to extract netrc)
  3. let other users creat pulls
  4. you got all there oauth2 tokens :tada:
qwerty287 commented 9 months ago

Is cloning using the tokens of the pipeline authors? Shouldn't it use the repo users/owners?

Anyways, I see your point. Maybe we should limit this to plugins?

anbraten commented 9 months ago

you got all there oauth2 tokens 🎉

I am also not for exposing those secrets to easily (as commented before), but your point seems to be wrong.

Is cloning using the tokens of the pipeline authors? Shouldn't it use the repo users/owners?

It uses the oauth token of the first repo admin (might be updated to another repo admin on sync later not 100% sure).

Maybe we should limit this to plugins?

An image filter would be for plugins only as otherwise it would not really help (see my description & similar to #2213).

qwerty287 commented 9 months ago

I also think in general we shouldn't expose these internal secrets. You can always add them manually.

If I think about it, an image for plugins only in this case is kinda useless, so I'm not sure if we should implement this. (Using global config as suggested by @6543 could be an option but I'm not sure if it solves the underlying issue.)

anbraten commented 9 months ago

Speaking of the use-cases (AFAIK from reading the request / comments) the idea would be that a user could use:

Using the credentials to clone somewhere in the steps again using the official clone plugin is already supported.

runephilosof-karnovgroup commented 9 months ago

For github forge, it would make the credentials non-sensitive, if it is converted to a github app, using short lived and restricted access tokens. Is something similar possible on the other forges?

pat-s commented 9 months ago

Do we know how other CI providers handle this? We should also take into account that this is a complicated situation for users if "too much" magic is happening behind the scenes or certain things only apply in some special situations. Also WRT to documentation.

A situation which is popular in the instances I managed (and which worked/still works in drone) is to use arbitrary git commands to apply some git modifications and then push back to the cloned repo. For this a git plugin would only partially help (unless it has a setting where one could input arbitrary git commands of infinite length).

qwerty287 commented 9 months ago

GH Actions have https://docs.github.com/en/actions/security-guides/automatic-token-authentication which is a token for a special user/app AFAIK (https://github.com/apps/github-actions). I don't think though we should do something similar. You can still add secrets containing the credentials, but the netrc vars should be internal vars I think.

A situation which is popular in the instances I managed (and which worked/still works in drone) is to use arbitrary git commands to apply some git modifications and then push back to the cloned repo. For this a git plugin would only partially help (unless it has a setting where one could input arbitrary git commands of infinite length).

How does drone handle this? It's a really critical security issue if you just expose some random oauth tokens. Also, we can't say to just use the token of the pipeline author (e.g. PR author), because the pipeline author is not necessarily registered so we don't have a token.

pat-s commented 9 months ago

How does drone handle this? It's a really critical security issue if you just expose some random oauth tokens. Also, we can't say to just use the token of the pipeline author (e.g. PR author), because the pipeline author is not necessarily registered so we don't have a token.

I agree, not saying we should do the same. Just reporting how it works. What I got as a reply from users when telling them to migrate their workflows was "Ugh, I don't like this, this is a step backwards. Can we stay with drone?" 🥴️ -> which is why I opened the discussion in the first place as I want them to use/be happy with WP of course 😅

qwerty287 commented 9 months ago

Hmm looking at drone's docs I can't find an env var holding a token. There's nothing listed containing token, netrc or clone in https://docs.drone.io/pipeline/environment/reference/. Do you know how it's called?

pat-s commented 9 months ago

It's also not clear to me, unfortunately. The pipeline I am talking about does the following

kind: pipeline
type: kubernetes
name: CI

steps:
  - name: CI
    image: <some image with git>
    commands:
      - git fetch origin json
      [...]
      - git status
      - git diff --numstat
      - git remote -v
      - git add .
      - git commit --message "Automated import"
      - git push

and the git push just works. No manual secret added. I've checked the drone config itself and everything around (set up by myself) but I couldn't yet infer why this actually works. I know there is https://github.com/appleboy/drone-git-push for such use cases as it shouldn't work OOB. Maybe it's related to the drone kube runner which handles auth things differently compared to running drone on a single VM with the docker backend?

qwerty287 commented 9 months ago

It even works without changing author/email config? Interesting. Does somebody has access to a drone instance so we can find out more?

anbraten commented 9 months ago

https://docs.drone.io/runner/docker/configuration/reference/drone-netrc-clone-only/

I am not 100% sure, but I think it is mounting /root/.netrc to steps.

anbraten commented 9 months ago

This code shows how they create the netrc file: https://github.com/drone-runners/drone-runner-docker/blob/bb7abc32a5b79f7f03fae2bb398366b1ef94e0ff/engine/compiler/shell/shell.go#L38

pat-s commented 9 months ago

Ah yeah, that might be it. Also

Please note that Drone injects clone credentials into all pipeline steps if the repository is private or requires authentication to clone; Drone never injects credentials into pipeline steps if the repository is public.

WRT to my example above, the repo is "private".