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

Replace NetrcOnlyTrusted with list of trusted plugins for netrc #2601

Closed
anbraten opened this issue Oct 17, 2023 · 20 comments · Fixed by #4352
Closed

Replace NetrcOnlyTrusted with list of trusted plugins for netrc #2601

anbraten opened this issue Oct 17, 2023 · 20 comments · Fixed by #4352
Labels
breaking will break existing installations if no manual action happens feature add new functionality security
Milestone

Comments

@anbraten
Copy link
Member

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.

@anbraten anbraten added feature add new functionality breaking will break existing installations if no manual action happens security labels Oct 17, 2023
@pat-s
Copy link
Contributor

pat-s commented Oct 18, 2023

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

6543 commented Nov 26, 2023

is this not solved by simply make

var TrustedCloneImages = []string{
DefaultCloneImage,
"quay.io/woodpeckerci/plugin-git",
}

as an configuration option?

@qwerty287
Copy link
Contributor

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

@6543
Copy link
Member

6543 commented Nov 26, 2023

an option only instance admins should change in any case

@qwerty287
Copy link
Contributor

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

6543 commented Nov 26, 2023

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 🎉

@qwerty287
Copy link
Contributor

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

anbraten commented Nov 26, 2023

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

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

anbraten commented Nov 26, 2023

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

  • a "special" clone image as plugin (question would be which feature is missing in the default clone image)
  • the netrc credentials to commit & push changes using sth like a git-push plugin (it would use the repo-admins user to push the changes not sure if this is even desired)

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

@runephilosof-karnovgroup
Copy link
Contributor

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

pat-s commented Nov 27, 2023

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

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

pat-s commented Nov 27, 2023

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

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

pat-s commented Nov 27, 2023

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

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

@anbraten
Copy link
Member Author

@anbraten
Copy link
Member Author

@pat-s
Copy link
Contributor

pat-s commented Nov 27, 2023

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking will break existing installations if no manual action happens feature add new functionality security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants