Warn if using secrets/env with plugin (#4027) (#4039)

docs/docs/20-usage/51-plugins/51-overview.md

@@ -49,9 +49,10 @@ steps:
 Plugins are just pipeline steps. They share the build workspace, mounted as a volume, and therefore have access to your source tree.
 While normal steps are all about arbitrary code execution, plugins should only allow the functions intended by the plugin author.
 
-So there are a few limitations, like the workspace base is always mounted at `/woodpecker`, but the working directory is dynamically adjusted acordingly. So as user of a plugin you should not have to care about this.
-
-Also instead of using environment variables the plugin should only care about one prefixed with `PLUGIN_` witch are the internaml representation of the **settings** ([read more](./20-creating-plugins.md)).
+That's why there are a few limitations. The workspace base is always mounted at `/woodpecker`, but the working directory is dynamically
+adjusted accordingly, as user of a plugin you should not have to care about this. Also, you cannot use the plugin together with `commands`
+or `entrypoint` which will fail. Using `secrets` or `environment` is possible, but in this case, the plugin is internally not treated as plugin
+anymore. The container then cannot access secrets with plugin filter anymore and the containers won't be privileged without explicit definition.
 
 ## Finding Plugins
 

pipeline/backend/kubernetes/pod_test.go

@@ -65,7 +65,7 @@ func TestStepLabel(t *testing.T) {
 }
 
 func TestTinyPod(t *testing.T) {
-	expected := `
+	const expected = `
 	{
 		"metadata": {
 			"name": "wp-01he8bebctabr3kgk0qj36d2me-0",
@@ -149,7 +149,7 @@ func TestTinyPod(t *testing.T) {
 }
 
 func TestFullPod(t *testing.T) {
-	expected := `
+	const expected = `
 	{
 		"metadata": {
 			"name": "wp-01he8bebctabr3kgk0qj36d2me-0",
@@ -426,7 +426,7 @@ func TestPodPrivilege(t *testing.T) {
 }
 
 func TestScratchPod(t *testing.T) {
-	expected := `
+	const expected = `
 	{
 		"metadata": {
 			"name": "wp-01he8bebctabr3kgk0qj36d2me-0",
@@ -471,7 +471,7 @@ func TestScratchPod(t *testing.T) {
 }
 
 func TestSecrets(t *testing.T) {
-	expected := `
+	const expected = `
 	{
 		"metadata": {
 			"name": "wp-3kgk0qj36d2me01he8bebctabr-0",

pipeline/frontend/yaml/linter/linter.go

@@ -143,7 +143,10 @@ func (l *Linter) lintSettings(config *WorkflowConfig, c *types.Container, field
 		return newLinterError("Cannot configure both entrypoint and settings", config.File, fmt.Sprintf("%s.%s", field, c.Name), false)
 	}
 	if len(c.Environment) != 0 {
-		return newLinterError("Cannot configure both environment and settings", config.File, fmt.Sprintf("%s.%s", field, c.Name), false)
+		return newLinterError("Should not configure both environment and settings", config.File, fmt.Sprintf("%s.%s", field, c.Name), true)
+	}
+	if len(c.Secrets.Secrets) != 0 {
+		return newLinterError("Should not configure both secrets and settings", config.File, fmt.Sprintf("%s.%s", field, c.Name), true)
 	}
 	return nil
 }
@@ -172,7 +175,7 @@ func (l *Linter) lintTrusted(config *WorkflowConfig, c *types.Container, area st
 	if len(c.NetworkMode) != 0 {
 		errors = append(errors, "Insufficient privileges to use network_mode")
 	}
-	if c.Volumes.Volumes != nil && len(c.Volumes.Volumes) != 0 {
+	if len(c.Volumes.Volumes) != 0 {
 		errors = append(errors, "Insufficient privileges to use volumes")
 	}
 	if len(c.Tmpfs) != 0 {

pipeline/frontend/yaml/linter/linter_test.go

@@ -163,7 +163,7 @@ func TestLintErrors(t *testing.T) {
 		},
 		{
 			from: "steps: { build: { image: golang, settings: { test: 'true' }, environment: [ 'TEST=true' ] } }",
-			want: "Cannot configure both environment and settings",
+			want: "Should not configure both environment and settings",
 		},
 	}
 

pipeline/frontend/yaml/linter/schema/schema.json

@@ -511,9 +511,6 @@
         "directory": {
           "$ref": "#/definitions/step_directory"
         },
-        "secrets": {
-          "$ref": "#/definitions/step_secrets"
-        },
         "settings": {
           "$ref": "#/definitions/step_settings"
         },

pipeline/frontend/yaml/types/container.go

@@ -124,7 +124,8 @@ func (c *ContainerList) UnmarshalYAML(value *yaml.Node) error {
 func (c *Container) IsPlugin() bool {
 	return len(c.Commands) == 0 &&
 		len(c.Entrypoint) == 0 &&
-		len(c.Environment) == 0
+		len(c.Environment) == 0 &&
+		len(c.Secrets.Secrets) == 0
 }
 
 func (c *Container) IsTrustedCloneImage() bool {

server/grpc/rpc.go

@@ -138,7 +138,7 @@ func (s *RPC) Update(_ context.Context, strWorkflowID string, state rpc.StepStat
 			Int64("stepPipelineID", step.PipelineID).
 			Int64("currentPipelineID", currentPipeline.ID).
 			Msg(msg)
-		return fmt.Errorf(msg)
+		return errors.New(msg)
 	}
 
 	repo, err := s.store.GetRepo(currentPipeline.RepoID)

server/pipeline/approve.go

@@ -16,6 +16,7 @@ package pipeline
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/rs/zerolog/log"
@@ -37,7 +38,7 @@ func Approve(ctx context.Context, store store.Store, currentPipeline *model.Pipe
 	if err != nil {
 		msg := fmt.Sprintf("failure to load forge for repo '%s'", repo.FullName)
 		log.Error().Err(err).Str("repo", repo.FullName).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	// fetch the pipeline file from the database
@@ -84,7 +85,7 @@ func Approve(ctx context.Context, store store.Store, currentPipeline *model.Pipe
 	if err != nil {
 		msg := fmt.Sprintf("failure to createPipelineItems for %s", repo.FullName)
 		log.Error().Err(err).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	// we have no way to link old workflows and steps in database to new engine generated steps,
@@ -100,7 +101,7 @@ func Approve(ctx context.Context, store store.Store, currentPipeline *model.Pipe
 	if err != nil {
 		msg := fmt.Sprintf("failure to start pipeline for %s: %v", repo.FullName, err)
 		log.Error().Err(err).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	return currentPipeline, nil

server/pipeline/create.go

@@ -38,7 +38,7 @@ func Create(ctx context.Context, _store store.Store, repo *model.Repo, pipeline
 	if err != nil {
 		msg := fmt.Sprintf("failure to find repo owner via id '%d'", repo.UserID)
 		log.Error().Err(err).Str("repo", repo.FullName).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	if pipeline.Event == model.EventPush || pipeline.Event == model.EventPull || pipeline.Event == model.EventPullClosed {
@@ -57,7 +57,7 @@ func Create(ctx context.Context, _store store.Store, repo *model.Repo, pipeline
 	if err != nil {
 		msg := fmt.Sprintf("failure to load forge for repo '%s'", repo.FullName)
 		log.Error().Err(err).Str("repo", repo.FullName).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	// If the forge has a refresh token, the current access token
@@ -117,15 +117,15 @@ func Create(ctx context.Context, _store store.Store, repo *model.Repo, pipeline
 		if err != nil {
 			msg := fmt.Sprintf("failed to find or persist pipeline config for %s", repo.FullName)
 			log.Error().Err(err).Msg(msg)
-			return nil, fmt.Errorf(msg)
+			return nil, errors.New(msg)
 		}
 		configs = append(configs, config)
 	}
 	// link pipeline to persisted configs
 	if err := linkPipelineConfigs(_store, configs, pipeline.ID); err != nil {
 		msg := fmt.Sprintf("failed to find or persist pipeline config for %s", repo.FullName)
 		log.Error().Err(err).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	if err := prepareStart(ctx, _forge, _store, pipeline, repoUser, repo); err != nil {
@@ -145,7 +145,7 @@ func Create(ctx context.Context, _store store.Store, repo *model.Repo, pipeline
 	if err != nil {
 		msg := fmt.Sprintf("failed to start pipeline for %s", repo.FullName)
 		log.Error().Err(err).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	return pipeline, nil

server/pipeline/decline.go

@@ -16,6 +16,7 @@ package pipeline
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/rs/zerolog/log"
@@ -31,7 +32,7 @@ func Decline(ctx context.Context, store store.Store, pipeline *model.Pipeline, u
 	if err != nil {
 		msg := fmt.Sprintf("failure to load forge for repo '%s'", repo.FullName)
 		log.Error().Err(err).Str("repo", repo.FullName).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	if pipeline.Status != model.StatusBlocked {

server/pipeline/restart.go

@@ -33,7 +33,7 @@ func Restart(ctx context.Context, store store.Store, lastPipeline *model.Pipelin
 	if err != nil {
 		msg := fmt.Sprintf("failure to load forge for repo '%s'", repo.FullName)
 		log.Error().Err(err).Str("repo", repo.FullName).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	switch lastPipeline.Status {
@@ -70,7 +70,7 @@ func Restart(ctx context.Context, store store.Store, lastPipeline *model.Pipelin
 	if err != nil {
 		msg := fmt.Sprintf("failure to save pipeline for %s", repo.FullName)
 		log.Error().Err(err).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	if len(configs) == 0 {
@@ -85,27 +85,27 @@ func Restart(ctx context.Context, store store.Store, lastPipeline *model.Pipelin
 	if err := linkPipelineConfigs(store, configs, newPipeline.ID); err != nil {
 		msg := fmt.Sprintf("failure to persist pipeline config for %s.", repo.FullName)
 		log.Error().Err(err).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	newPipeline, pipelineItems, err := createPipelineItems(ctx, forge, store, newPipeline, user, repo, pipelineFiles, envs)
 	if err != nil {
 		msg := fmt.Sprintf("failure to createPipelineItems for %s", repo.FullName)
 		log.Error().Err(err).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	if err := prepareStart(ctx, forge, store, newPipeline, user, repo); err != nil {
 		msg := fmt.Sprintf("failure to prepare pipeline for %s", repo.FullName)
 		log.Error().Err(err).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	newPipeline, err = start(ctx, forge, store, newPipeline, user, repo, pipelineItems)
 	if err != nil {
 		msg := fmt.Sprintf("failure to start pipeline for %s", repo.FullName)
 		log.Error().Err(err).Msg(msg)
-		return nil, fmt.Errorf(msg)
+		return nil, errors.New(msg)
 	}
 
 	return newPipeline, nil