Fix netrc injection -> allow injection into untrusted containers #2585

pat-s · 2023-10-14T20:19:20Z

And improve settings documentation.

codecov-commenter · 2023-10-14T20:33:09Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (22bb924) 33.96% compared to head (5606833) 33.95%.
Report is 3 commits behind head on main.

❗ Current head 5606833 differs from pull request most recent head 82b9b43. Consider uploading reports for the commit 82b9b43 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2585      +/-   ##
==========================================
- Coverage   33.96%   33.95%   -0.02%     
==========================================
  Files         203      203              
  Lines       13016    13016              
==========================================
- Hits         4421     4419       -2     
- Misses       8243     8245       +2     
  Partials      352      352

Files	Coverage Δ
pipeline/frontend/yaml/compiler/compiler.go	`45.81% <50.00%> (+0.55%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

woodpecker-bot · 2023-10-14T21:24:58Z

Deployment of preview was torn down

pat-s · 2023-10-14T21:45:10Z

Tested that the fixes work - credentials are now injected as expected. The change in line 256 was the important one.

qwerty287

We need more discussion on this. There's also #2214 removing it completely.
On the other side, I see this is a bugfix so we can move the discussion to another PR later.

anbraten · 2023-10-15T06:11:03Z

I am fine with fixing it first.

anbraten · 2023-10-15T06:17:49Z

However AFAIK this is a feature not a fix, right? I think it was on purpose that only the clone image would receive netrc credentials when used in the steps.

pipeline/frontend/yaml/compiler/compiler.go

anbraten · 2023-10-15T06:26:14Z

pipeline/frontend/yaml/compiler/compiler.go

@@ -253,7 +253,7 @@ func (c *Compiler) Compile(conf *yaml_types.Workflow) (*backend_types.Config, er
 		step := c.createProcess(name, container, stepType)

 		// inject netrc if it's a trusted repo or a trusted clone-plugin
-		if c.trustedPipeline || (container.IsPlugin() && container.IsTrustedCloneImage()) {
+		if !c.netrcOnlyTrusted || c.trustedPipeline || (c.netrcOnlyTrusted && container.IsPlugin() && container.IsTrustedCloneImage()) {


Thinking about it, it could be best to have following cases:

if trusted repo (needs admin access)

if plugin and trusted clone image (clone image should not steal /leak the credentials)

New case:

if netrcOnlyTrusted is active only allow plugins as the admin could trust them, so we might need a list of passNetrcToImages instead

Main scenarios how WP is used (from what I know):

single admin using it as "hobbist" => could simply use trusted option if wanted

instance with multiple users (company, multiple friends, community like codeberg) => not everyone was made an admin somehow (probably they do not 100% trust everybody), so to prevent miss-configuration it could be best to still be pretty strict about options like passing netrc

pat-s · 2023-10-15T11:17:02Z

However AFAIK this is a feature not a fix, right? I think it was on purpose that only the clone image would receive netrc credentials when used in the steps.

It sounds like nobody really knows 😅 To me, reading the option description, it sounds like this is a "bugfix" as I would have expected that unchecking this option injects netrc into all containers - as the option did not say that it's limited to the clone step.
Yes, the docs give a hint about that but that can be overlooked easily.

WRT to security: I fully understand the hesitation and why it should not be injected into all containers by default. Yet I wonder if there's really a different in limiting it to the clone step only or not - if there is malicious motivation, the step does not really matter. Or am I overlooking something? If this is the case, then the PR can be merged.

instance with multiple users (company, multiple friends, community like codeberg) => not everyone was made an admin somehow (probably they do not 100% trust everybody), so to prevent miss-configuration it could be best to still be pretty strict about options like passing netrc

This is my use case (at least in a specific env). Not everybody is admin (as this is not good per se and not everyone knows what's possible etc.). But everybody should have the option to decide for their own repos whether they want to inject netrc - at least that's my view.

I see the point when an admin wants to prevent this entirely. So maybe an additional admin option is needed which allows/forbids changing the repo setting in the first place?

anbraten · 2023-10-17T06:33:29Z

Although it was document in a bad way / the option implied sth else, no one was currently able to use it in steps therefore we should carefully thing how we provide this power to users as it would be breaking if we remove it later on again.

Regarding the security aspect I currently think it would be best to replace the current option with a comma separated list of images. As netrc credentials are somehow a secret as well this would match the logic we use for the secret image list. The credentials should therefore only be passed to plugin of that list. If empty they should not be passed to anything else then the official plugin. It would probably the best if we make this a breaking change with 2.0 now and require the user to add custom clone images to the list as well.

qwerty287 · 2023-11-02T09:34:28Z

@pat-s I think we can close this in favor of #2601?

pat-s · 2023-11-02T16:30:08Z

I am not sure because AFAICS #2601 only tackles the plugin case but doesn't allow arbitrary images to use the netrc creds (which should also be an option for repo admins IMO). Besides the one to give netrc to plugins. But I didn't had time to dive deep into this topic in the last days.

I don't think we can finish this for 2.0, so let's at least move it out of this milestone.

xoxys · 2024-11-07T15:47:40Z

@woodpecker-ci/maintainers Can we fix conflicts and merge this fix as is? Right now, the option Only inject netrc credentials into trusted containers is just broken, as unchecking it doesn't work.

If we decided to drop this feature completely, fine for me, but right now the checkbox exist in the latest version of wp and as long as it exists it should at least work. I cannot believe that we have not fixed this issue since more than a year just because we might or might not want to drop it (which also doesn't happen till now).

xoxys · 2024-11-07T16:58:17Z

@6543 this is definitely not an enhancement. This checkbox exists today, and unchecking the checkbox has no effect. That's what I would call a bug.

qwerty287 · 2024-11-09T06:40:01Z

Did not test that, but from looking at the code, this should work as intended. However, we probably talk about different things here: I think the intended way of the feature is to never inject the creds into normal containers, only in clone containers. Tbh I'm not a big fan of allowing this, I think you should not use these creds in your own steps, only for cloning.

nemunaire · 2024-11-09T07:15:34Z

We agreed with that: the default behavior should be to not share credentials with containers except clone. The fact there is a dedicated option is to let the user choose if it requires a less secure alternative in all conscience.
For example, this can help to git push.

Although I migrated to WP a long time ago, I'm still required to run Drone for repositories making git push...

This doesn't involve to stop thinking to a more secure way to do that, but ATM there is only this option ... that doesn't work.

pat-s · 2024-11-09T09:16:06Z

While I understand all discussion around this feature in general, as @xoxys said: the checkbox exists currently and is broken. This PR only fixes that.

Everything else (whether to restrict the option to admins-only or other ideas) should be done in a separate discussion and PR then IMO.

I am gonna resolve conflicts and then leave it to somebody else to possibly approve or deny this.

anbraten · 2024-11-09T09:50:08Z

@pat-s It actually works fine (just checked the code and function again). You can use a custom clone plugin and set this option to still allow the netrc injection. It was not intended to be used for normal steps as this PR is doing it. Therefore this PR is rather a new feature and should probably start migrating to sth like #2601. We could also change the description of the current option to sth like Only inject netrc credentials into trusted clone step for the beginning.

pat-s · 2024-11-09T09:56:39Z

I understand (now) better. Yes, I think updating the wording here is very important to avoid these misconceptions.

I think also the description should be much more verbose here to explain the setting. Currently it is just repeating the title of the setting, which doesn't help at all.

pat-s · 2024-11-09T09:57:00Z

Closing as due to the latest comment by @anbraten.

xoxys · 2024-11-09T09:57:28Z

Well if that is really the intended way then the description is completely misleading. So what was the intention to write such a description then back in the days where this setting was introduced?

pat-s added 2 commits October 14, 2023 22:17

update if condition

20c69ed

fix settings description

fddb791

pat-s added bug Something isn't working server labels Oct 14, 2023

pat-s added 3 commits October 14, 2023 23:18

fix if condition

d278dfd

update docs

3372846

doc

5606833

pat-s marked this pull request as ready for review October 14, 2023 21:45

pat-s added this to the 2.0.0 milestone Oct 14, 2023

pat-s requested a review from a team October 14, 2023 21:46

qwerty287 requested changes Oct 15, 2023

View reviewed changes

qwerty287 approved these changes Oct 15, 2023

View reviewed changes

anbraten reviewed Oct 15, 2023

View reviewed changes

pipeline/frontend/yaml/compiler/compiler.go Outdated Show resolved Hide resolved

anbraten reviewed Oct 15, 2023

View reviewed changes

remove unnecessary code

82b9b43

anbraten mentioned this pull request Oct 17, 2023

Replace NetrcOnlyTrusted with list of trusted plugins for netrc #2601

Closed

pat-s modified the milestones: 2.0.0, 3.x.x Nov 2, 2023

6543 added enhancement improve existing features and removed bug Something isn't working labels Nov 7, 2024

xoxys added bug Something isn't working and removed server labels Nov 7, 2024

xoxys modified the milestones: 3.x.x, 2.8.0 Nov 7, 2024

xoxys added server and removed enhancement improve existing features labels Nov 7, 2024

pat-s added 2 commits November 9, 2024 10:25

chore: merge

4407286

Merge branch 'main' into relax-netrc-injection

2612285

pat-s closed this Nov 9, 2024

pat-s mentioned this pull request Nov 9, 2024

Update netrc option description #4342

Merged

anbraten mentioned this pull request Nov 18, 2024

Allow to set custom trusted clone plugins #4352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix netrc injection -> allow injection into untrusted containers #2585

Fix netrc injection -> allow injection into untrusted containers #2585

pat-s commented Oct 14, 2023

codecov-commenter commented Oct 14, 2023 •

edited

Loading

woodpecker-bot commented Oct 14, 2023 •

edited

Loading

pat-s commented Oct 14, 2023

qwerty287 left a comment •

edited

Loading

anbraten commented Oct 15, 2023 •

edited

Loading

anbraten commented Oct 15, 2023 •

edited

Loading

anbraten Oct 15, 2023 •

edited

Loading

pat-s commented Oct 15, 2023

anbraten commented Oct 17, 2023 •

edited

Loading

qwerty287 commented Nov 2, 2023

pat-s commented Nov 2, 2023

xoxys commented Nov 7, 2024

xoxys commented Nov 7, 2024 •

edited

Loading

qwerty287 commented Nov 9, 2024

nemunaire commented Nov 9, 2024 •

edited

Loading

pat-s commented Nov 9, 2024

anbraten commented Nov 9, 2024

pat-s commented Nov 9, 2024

pat-s commented Nov 9, 2024

xoxys commented Nov 9, 2024 •

edited

Loading

Fix netrc injection -> allow injection into untrusted containers #2585

Fix netrc injection -> allow injection into untrusted containers #2585

Conversation

pat-s commented Oct 14, 2023

codecov-commenter commented Oct 14, 2023 • edited Loading

Codecov Report

woodpecker-bot commented Oct 14, 2023 • edited Loading

pat-s commented Oct 14, 2023

qwerty287 left a comment • edited Loading

Choose a reason for hiding this comment

anbraten commented Oct 15, 2023 • edited Loading

anbraten commented Oct 15, 2023 • edited Loading

anbraten Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

pat-s commented Oct 15, 2023

anbraten commented Oct 17, 2023 • edited Loading

qwerty287 commented Nov 2, 2023

pat-s commented Nov 2, 2023

xoxys commented Nov 7, 2024

xoxys commented Nov 7, 2024 • edited Loading

qwerty287 commented Nov 9, 2024

nemunaire commented Nov 9, 2024 • edited Loading

pat-s commented Nov 9, 2024

anbraten commented Nov 9, 2024

pat-s commented Nov 9, 2024

pat-s commented Nov 9, 2024

xoxys commented Nov 9, 2024 • edited Loading

codecov-commenter commented Oct 14, 2023 •

edited

Loading

woodpecker-bot commented Oct 14, 2023 •

edited

Loading

qwerty287 left a comment •

edited

Loading

anbraten commented Oct 15, 2023 •

edited

Loading

anbraten commented Oct 15, 2023 •

edited

Loading

anbraten Oct 15, 2023 •

edited

Loading

anbraten commented Oct 17, 2023 •

edited

Loading

xoxys commented Nov 7, 2024 •

edited

Loading

nemunaire commented Nov 9, 2024 •

edited

Loading

xoxys commented Nov 9, 2024 •

edited

Loading