Warn if using secrets/env with plugin

[qwerty287]

address #3959

If secrets is set, it's not a plugin anymore.

However, only warn about such cases, but do not fail.

This PR should be backported.

[lafriks]

Can we use settings with non-plugins?

[qwerty287]

Yeah that's a discussion point. I'd just allow it (but warn) as they're just env vars in the end.

[zc-devs]

Can we use settings with non-plugins?

I vote for No.
Settings are for plugins; envs and secrets are prohibited.
Envs (and secrets*) are for general steps; (secrets*) settings are prohibited. * depends on #4015 resolution.

Edit
So, how it works now:

skip_clone: true
steps:
  test:
    image: alpine
    settings:
      token: qweasdzxc
    commands:
      - echo $$PLUGIN_TOKEN  | sed -e 's/\(.\)/\1\ /g'

[qwerty287]

envs and secrets are prohibited.

This pr changes it, they're no longer prohibited, only a warning is printed.
This warning is also there for settings. So you can use it, but get a warning.

[zc-devs]

Title

Disallow secrets for plugins

for plugins envs and secrets are prohibited

This pr changes it, they're no longer prohibited

I'm lost.

[qwerty287]

Ah sorry 😂

I'll try to describe it better:

• current errors (settings with environment, commands etc.) are changed to warnings and therefore don't block anymore
• secrets are added to this list. If you try to use settings and secrets you'll get a warning

If you have a better title, just comment.

[zc-devs]

So, now using settings with environment, secrets, commands, entrypoint in plugins would rise warnings instead of errors and won't fail pipeline anymore, right? It is like deprecation? The reason behind this (changing to warnings) is we don't want to introduce breaking changes in pipeline syntax in upcoming 3.0, right?

If yes, I would rename to Deprecate general step syntax in plugins with according record in the docs.

[zc-devs]

I would switch this PR to release/v2.7 branch.

And for 3.0 I would keep errors and disallow secrets (hard-way with pipeline fail). Although it breaks pipeline syntax, security should get priority (definitely for major releases, for minor and patch versions it is an open question). Otherwise we would live with not fixed security issue yet badge for a year.

[qwerty287]

It is like deprecation?

No. It's more like the "bad habit" errors. I think it should stay like this and warn about it, but don't fail.

Although it breaks pipeline syntax, security should get priority (definitely for major releases, for minor and patch versions it is an open question). Otherwise we would live with not fixed security issue yet badge for a year.

If you set commands, environment etc. on a plugin, the step is not treated as plugin internally anymore and will therefore not be given additional privileges without explicit definition (which requires the repo to be trusted). It will just be like any other step, so there isn't any security problem here.

[lafriks]

If you set commands, environment etc. on a plugin, the step is not treated as plugin internally anymore and will therefore not be given additional privileges without explicit definition (which requires the repo to be trusted). It will just be like any other step, so there isn't any security problem here.

Also secrets that are allowed only for specified images won't be allowed?

[qwerty287]

Sure, and they won't be available if you add secrets or environment as this only works if it's a plugin

[zc-devs]

I understand that it works, and why. Thank you for explanation.

The reason behind this (changing to warnings) is we don't want to introduce breaking changes in pipeline syntax in upcoming 3.0, right?

Is this a temporary solution? Do you try to trick the system (semver)?

[qwerty287]

No, I wouldn't make this a temporary solution, I'd keep it.

[zc-devs]

I like fail-fast, strict approach, which was before.

[qwerty287]

In many other cases, I agree, but at least environment and secrets should be allowed. I think there are many users using them, but don't care about the limitation that it's not a plugin. Failing at commands and entrypoint is probably better, I can change that.

[zc-devs]

I feel I make too much efforts to defend something, that I do not really use. It makes me tired. Probably, you too.

Failing at commands and entrypoint is probably better

I think so.

In the end, I just wanna highlight another thing. It is just to think (or not :) about, no need to reply.
#3959 (comment)
Did the example work as plugin? I assume, yes.
Now, does it execut as plugin? I assume, no. Can it lead to some plugin unexpected behavior from a user point of view. How janydoe should have known about it? Can it lead to endless questions / bug reports?
That is how magic works. Schrödinger's plugin 😆

[qwerty287]

Can it lead to some plugin unexpected behavior from a user point of view

Of course - secrets filter for example won't work. Yes, we need to have a clear documentation about this to help users understanding why this doesn't work.

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 26.88%. Comparing base (0d9e57d) to head (d8049f7).
Report is 62 commits behind head on main.

Files with missing lines	Patch %	Lines
pipeline/frontend/yaml/linter/linter.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4027      +/-   ##
==========================================
+ Coverage   26.84%   26.88%   +0.03%     
==========================================
  Files         396      396              
  Lines       27497    27465      -32     
==========================================
+ Hits         7382     7384       +2     
+ Misses      19411    19376      -35     
- Partials      704      705       +1     

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

[woodpecker-bot]

Deployment of preview was torn down