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

Allow to set custom trusted clone plugins #4352

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

qwerty287
Copy link
Contributor

closes #2601

@qwerty287 qwerty287 added enhancement improve existing features breaking will break existing installations if no manual action happens labels Nov 11, 2024
@qwerty287 qwerty287 added this to the 3.0.0 milestone Nov 11, 2024
@qwerty287 qwerty287 requested a review from a team November 11, 2024 07:12
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Nov 11, 2024

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-4352.surge.sh

@pat-s
Copy link
Contributor

pat-s commented Nov 11, 2024

How is that different to WOODPECKER_PLUGINS_TRUSTED_CLONE?

@xoxys
Copy link
Member

xoxys commented Nov 11, 2024

You can now set it per repo as well.

@6543
Copy link
Member

6543 commented Nov 11, 2024

You can now set it per repo as well.

did not checked the code jet - but ony an instance admin should be able to change it

@6543
Copy link
Member

6543 commented Nov 11, 2024

one question (idea) that comes into my mind: why not add the config into TrustedConfiguration ?

server/model/repo.go Outdated Show resolved Hide resolved
@xoxys
Copy link
Member

xoxys commented Nov 11, 2024

did not checked the code jet - but ony an instance admin should be able to change it

Repeating it doesnt help. Can we keep the discussion in the issue? You never responded to #2601 (comment)

@qwerty287
Copy link
Contributor Author

Why the instance admin? This is about per-repo/per-user credentials, so the repo admins should decide how they are used.

Comment on lines -256 to -260
if c.securityTrustedPipeline || (container.IsPlugin() && container.IsTrustedCloneImage(c.trustedClonePlugins)) {
for k, v := range c.cloneEnv {
step.Environment[k] = v
}
}
Copy link
Member

@anbraten anbraten Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will prevent users from cloning / fetching tags etc afterwards which is currently a feature we support.

In general if we go this path we should consider allowing to pass the netrc credentials to all plugins that are listed in NetRCTrusted / NetRCTrustedPlugins IMO. Related to security I can't see an issue as the repo admins sets this for their own credentials. Worst case they could only mess with their own credentials.

Use cases:

  • clone using custom plugin in clone section
  • clone later on
    • used skip_clone as user had to do other tasks before cloning
    • wants to clone again to get other parts afterwards (fetch tags, sub-modules)
  • wants to push changes

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 enhancement improve existing features security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace NetrcOnlyTrusted with list of trusted plugins for netrc
7 participants