Improve secret availability checks (#3271)

pipeline/frontend/yaml/compiler/compiler.go

@@ -42,22 +42,49 @@ type Secret struct {
 	Name           string
 	Value          string
 	AllowedPlugins []string
+	Events         []string
 }
 
-func (s *Secret) Available(container *yaml_types.Container) bool {
+func (s *Secret) Available(event string, container *yaml_types.Container) error {
-	return (len(s.AllowedPlugins) == 0 || utils.MatchImage(container.Image, s.AllowedPlugins...)) && (len(s.AllowedPlugins) == 0 || container.IsPlugin())
+	onlyAllowSecretForPlugins := len(s.AllowedPlugins) > 0
+	if onlyAllowSecretForPlugins && !container.IsPlugin() {
+		return fmt.Errorf("secret %q only allowed to be used by plugins by step %q", s.Name, container.Name)
+	}
+
+	if onlyAllowSecretForPlugins && !utils.MatchImage(container.Image, s.AllowedPlugins...) {
+		return fmt.Errorf("secret %q is not allowed to be used with image %q by step %q", s.Name, container.Image, container.Name)
+	}
+
+	if !s.Match(event) {
+		return fmt.Errorf("secret %q is not allowed to be used with pipeline event %q", s.Name, event)
+	}
+
+	return nil
+}
+
+// Match returns true if an image and event match the restricted list.
+// Note that EventPullClosed are treated as EventPull.
+func (s *Secret) Match(event string) bool {
+	// if there is no filter set secret matches all webhook events
+	if len(s.Events) == 0 {
+		return true
+	}
+	// tread all pull events the same way
+	if event == "pull_request_closed" {
+		event = "pull_request"
+	}
+	// one match is enough
+	for _, e := range s.Events {
+		if e == event {
+			return true
+		}
+	}
+	// a filter is set but the webhook did not match it
+	return false
 }
 
 type secretMap map[string]Secret
 
-func (sm secretMap) toStringMap() map[string]string {
-	m := make(map[string]string, len(sm))
-	for k, v := range sm {
-		m[k] = v.Value
-	}
-	return m
-}
-
 type ResourceLimit struct {
 	MemSwapLimit int64
 	MemLimit     int64

pipeline/frontend/yaml/compiler/compiler_test.go

@@ -29,28 +29,35 @@ import (
 func TestSecretAvailable(t *testing.T) {
 	secret := Secret{
 		AllowedPlugins: []string{},
+		Events:         []string{"push"},
 	}
-	assert.True(t, secret.Available(&yaml_types.Container{
+	assert.NoError(t, secret.Available("push", &yaml_types.Container{
 		Image:    "golang",
 		Commands: yaml_base_types.StringOrSlice{"echo 'this is not a plugin'"},
 	}))
 
 	// secret only available for "golang" plugin
 	secret = Secret{
+		Name:           "foo",
 		AllowedPlugins: []string{"golang"},
+		Events:         []string{"push"},
 	}
-	assert.True(t, secret.Available(&yaml_types.Container{
+	assert.NoError(t, secret.Available("push", &yaml_types.Container{
+		Name:     "step",
 		Image:    "golang",
 		Commands: yaml_base_types.StringOrSlice{},
 	}))
-	assert.False(t, secret.Available(&yaml_types.Container{
+	assert.ErrorContains(t, secret.Available("push", &yaml_types.Container{
 		Image:    "golang",
 		Commands: yaml_base_types.StringOrSlice{"echo 'this is not a plugin'"},
-	}))
+	}), "only allowed to be used by plugins by step")
-	assert.False(t, secret.Available(&yaml_types.Container{
+	assert.ErrorContains(t, secret.Available("push", &yaml_types.Container{
 		Image:    "not-golang",
 		Commands: yaml_base_types.StringOrSlice{},
-	}))
+	}), "not allowed to be used with image ")
+	assert.ErrorContains(t, secret.Available("pull_request", &yaml_types.Container{
+		Image: "golang",
+	}), "not allowed to be used with pipeline event ")
 }
 
 func TestCompilerCompile(t *testing.T) {
@@ -259,7 +266,7 @@ func TestCompilerCompile(t *testing.T) {
 				Secrets:  yaml_types.Secrets{Secrets: []*yaml_types.Secret{{Source: "missing", Target: "missing"}}},
 			}}}},
 			backConf:    nil,
-			expectedErr: "secret \"missing\" not found or not allowed to be used",
+			expectedErr: "secret \"missing\" not found",
 		},
 		{
 			name: "workflow with broken step dependency",
@@ -295,3 +302,43 @@ func TestCompilerCompile(t *testing.T) {
 		})
 	}
 }
+
+func TestSecretMatch(t *testing.T) {
+	tcl := []*struct {
+		name   string
+		secret Secret
+		event  string
+		match  bool
+	}{
+		{
+			name:   "should match event",
+			secret: Secret{Events: []string{"pull_request"}},
+			event:  "pull_request",
+			match:  true,
+		},
+		{
+			name:   "should not match event",
+			secret: Secret{Events: []string{"pull_request"}},
+			event:  "push",
+			match:  false,
+		},
+		{
+			name:   "should match when no event filters defined",
+			secret: Secret{},
+			event:  "pull_request",
+			match:  true,
+		},
+		{
+			name:   "pull close should match pull",
+			secret: Secret{Events: []string{"pull_request"}},
+			event:  "pull_request_closed",
+			match:  true,
+		},
+	}
+
+	for _, tc := range tcl {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.match, tc.secret.Match(tc.event))
+		})
+	}
+}

pipeline/frontend/yaml/compiler/convert.go

@@ -35,7 +35,7 @@ func (c *Compiler) createProcess(container *yaml_types.Container, stepType backe
 		uuid = ulid.Make()
 
 		detached   bool
-		workingdir string
+		workingDir string
 
 		workspace   = fmt.Sprintf("%s_default:%s", c.prefix, c.base)
 		privileged  = container.Privileged
@@ -86,22 +86,41 @@ func (c *Compiler) createProcess(container *yaml_types.Container, stepType backe
 	}
 
 	if !detached || len(container.Commands) != 0 {
-		workingdir = c.stepWorkdir(container)
+		workingDir = c.stepWorkingDir(container)
 	}
 
-	if !detached {
+	getSecretValue := func(name string) (string, error) {
-		pluginSecrets := secretMap{}
+		name = strings.ToLower(name)
-		for name, secret := range c.secrets {
+		secret, ok := c.secrets[name]
-			if secret.Available(container) {
+		if !ok {
-				pluginSecrets[name] = secret
+			return "", fmt.Errorf("secret %q not found", name)
-			}
 		}
 
-		if err := settings.ParamsToEnv(container.Settings, environment, pluginSecrets.toStringMap()); err != nil {
+		event := c.metadata.Curr.Event
+		err := secret.Available(event, container)
+		if err != nil {
+			return "", err
+		}
+
+		return secret.Value, nil
+	}
+
+	// TODO: why don't we pass secrets to detached steps?
+	if !detached {
+		if err := settings.ParamsToEnv(container.Settings, environment, getSecretValue); err != nil {
 			return nil, err
 		}
 	}
 
+	for _, requested := range container.Secrets.Secrets {
+		secretValue, err := getSecretValue(requested.Source)
+		if err != nil {
+			return nil, err
+		}
+
+		environment[strings.ToUpper(requested.Target)] = secretValue
+	}
+
 	if utils.MatchImage(container.Image, c.escalated...) && container.IsPlugin() {
 		privileged = true
 	}
@@ -115,15 +134,6 @@ func (c *Compiler) createProcess(container *yaml_types.Container, stepType backe
 		}
 	}
 
-	for _, requested := range container.Secrets.Secrets {
-		secret, ok := c.secrets[strings.ToLower(requested.Source)]
-		if ok && secret.Available(container) {
-			environment[strings.ToUpper(requested.Target)] = secret.Value
-		} else {
-			return nil, fmt.Errorf("secret %q not found or not allowed to be used", requested.Source)
-		}
-	}
-
 	// Advanced backend settings
 	backendOptions := backend_types.BackendOptions{
 		Kubernetes: convertKubernetesBackendOptions(&container.BackendOptions.Kubernetes),
@@ -181,7 +191,7 @@ func (c *Compiler) createProcess(container *yaml_types.Container, stepType backe
 		Pull:           container.Pull,
 		Detached:       detached,
 		Privileged:     privileged,
-		WorkingDir:     workingdir,
+		WorkingDir:     workingDir,
 		Environment:    environment,
 		Commands:       container.Commands,
 		Entrypoint:     container.Entrypoint,
@@ -208,7 +218,7 @@ func (c *Compiler) createProcess(container *yaml_types.Container, stepType backe
 	}, nil
 }
 
-func (c *Compiler) stepWorkdir(container *yaml_types.Container) string {
+func (c *Compiler) stepWorkingDir(container *yaml_types.Container) string {
 	if path.IsAbs(container.Directory) {
 		return container.Directory
 	}

pipeline/frontend/yaml/compiler/settings/params.go

@@ -26,7 +26,7 @@ import (
 
 // ParamsToEnv uses reflection to convert a map[string]interface to a list
 // of environment variables.
-func ParamsToEnv(from map[string]any, to, secrets map[string]string) (err error) {
+func ParamsToEnv(from map[string]any, to map[string]string, getSecretValue func(name string) (string, error)) (err error) {
 	if to == nil {
 		return fmt.Errorf("no map to write to")
 	}
@@ -34,7 +34,7 @@ func ParamsToEnv(from map[string]any, to, secrets map[string]string) (err error)
 		if v == nil || len(k) == 0 {
 			continue
 		}
-		to[sanitizeParamKey(k)], err = sanitizeParamValue(v, secrets)
+		to[sanitizeParamKey(k)], err = sanitizeParamValue(v, getSecretValue)
 		if err != nil {
 			return err
 		}
@@ -62,7 +62,7 @@ func isComplex(t reflect.Kind) bool {
 }
 
 // sanitizeParamValue returns the value of a setting as string prepared to be injected as environment variable
-func sanitizeParamValue(v any, secrets map[string]string) (string, error) {
+func sanitizeParamValue(v any, getSecretValue func(name string) (string, error)) (string, error) {
 	t := reflect.TypeOf(v)
 	vv := reflect.ValueOf(v)
 
@@ -84,7 +84,7 @@ func sanitizeParamValue(v any, secrets map[string]string) (string, error) {
 		// gopkg.in/yaml.v3 only emits this map interface
 		case map[string]any:
 			// check if it's a secret and return value if it's the case
-			value, isSecret, err := injectSecret(v, secrets)
+			value, isSecret, err := injectSecret(v, getSecretValue)
 			if err != nil {
 				return "", err
 			} else if isSecret {
@@ -94,7 +94,7 @@ func sanitizeParamValue(v any, secrets map[string]string) (string, error) {
 			return "", fmt.Errorf("could not handle: %#v", v)
 		}
 
-		return handleComplex(vv.Interface(), secrets)
+		return handleComplex(vv.Interface(), getSecretValue)
 
 	case reflect.Slice, reflect.Array:
 		if vv.Len() == 0 {
@@ -123,7 +123,7 @@ func sanitizeParamValue(v any, secrets map[string]string) (string, error) {
 				}
 
 				var err error
-				if in[i], err = sanitizeParamValue(v, secrets); err != nil {
+				if in[i], err = sanitizeParamValue(v, getSecretValue); err != nil {
 					return "", err
 				}
 			}
@@ -135,12 +135,12 @@ func sanitizeParamValue(v any, secrets map[string]string) (string, error) {
 	}
 
 	// handle all elements which are not primitives, string-maps containing secrets or arrays
-	return handleComplex(vv.Interface(), secrets)
+	return handleComplex(vv.Interface(), getSecretValue)
 }
 
 // handleComplex uses yaml2json to get json strings as values for environment variables
-func handleComplex(v any, secrets map[string]string) (string, error) {
+func handleComplex(v any, getSecretValue func(name string) (string, error)) (string, error) {
-	v, err := injectSecretRecursive(v, secrets)
+	v, err := injectSecretRecursive(v, getSecretValue)
 	if err != nil {
 		return "", err
 	}
@@ -159,13 +159,15 @@ func handleComplex(v any, secrets map[string]string) (string, error) {
 // injectSecret probes if a map is a from_secret request.
 // If it's a from_secret request it either  returns the secret value or an error if the secret was not found
 // else it just indicates to progress normally using the provided map as is
-func injectSecret(v map[string]any, secrets map[string]string) (string, bool, error) {
+func injectSecret(v map[string]any, getSecretValue func(name string) (string, error)) (string, bool, error) {
 	if secretNameI, ok := v["from_secret"]; ok {
 		if secretName, ok := secretNameI.(string); ok {
-			if secret, ok := secrets[strings.ToLower(secretName)]; ok {
+			secret, err := getSecretValue(secretName)
-				return secret, true, nil
+			if err != nil {
+				return "", false, err
 			}
-			return "", false, fmt.Errorf("no secret found for %q", secretName)
+
+			return secret, true, nil
 		}
 		return "", false, fmt.Errorf("from_secret has to be a string")
 	}
@@ -174,7 +176,7 @@ func injectSecret(v map[string]any, secrets map[string]string) (string, bool, er
 
 // injectSecretRecursive iterates over all types and if they contain elements
 // it iterates recursively over them too, using injectSecret internally
-func injectSecretRecursive(v any, secrets map[string]string) (any, error) {
+func injectSecretRecursive(v any, getSecretValue func(name string) (string, error)) (any, error) {
 	t := reflect.TypeOf(v)
 
 	if !isComplex(t.Kind()) {
@@ -187,7 +189,7 @@ func injectSecretRecursive(v any, secrets map[string]string) (any, error) {
 		// gopkg.in/yaml.v3 only emits this map interface
 		case map[string]any:
 			// handle secrets
-			value, isSecret, err := injectSecret(v, secrets)
+			value, isSecret, err := injectSecret(v, getSecretValue)
 			if err != nil {
 				return nil, err
 			} else if isSecret {
@@ -195,7 +197,7 @@ func injectSecretRecursive(v any, secrets map[string]string) (any, error) {
 			}
 
 			for key, val := range v {
-				v[key], err = injectSecretRecursive(val, secrets)
+				v[key], err = injectSecretRecursive(val, getSecretValue)
 				if err != nil {
 					return nil, err
 				}
@@ -210,7 +212,7 @@ func injectSecretRecursive(v any, secrets map[string]string) (any, error) {
 		vl := make([]any, vv.Len())
 
 		for i := 0; i < vv.Len(); i++ {
-			v, err := injectSecretRecursive(vv.Index(i).Interface(), secrets)
+			v, err := injectSecretRecursive(vv.Index(i).Interface(), getSecretValue)
 			if err != nil {
 				return nil, err
 			}

pipeline/frontend/yaml/compiler/settings/params_test.go

@@ -15,6 +15,8 @@
 package settings
 
 import (
+	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -57,7 +59,17 @@ func TestParamsToEnv(t *testing.T) {
 		"secret_token": "FooBar",
 	}
 	got := map[string]string{}
-	assert.NoError(t, ParamsToEnv(from, got, secrets))
+	getSecretValue := func(name string) (string, error) {
+		name = strings.ToLower(name)
+		secret, ok := secrets[name]
+		if ok {
+			return secret, nil
+		}
+
+		return "", fmt.Errorf("secret %q not found or not allowed to be used", name)
+	}
+
+	assert.NoError(t, ParamsToEnv(from, got, getSecretValue))
 	assert.EqualValues(t, want, got, "Problem converting plugin parameters to environment variables")
 
 	// handle edge cases (#1609)
@@ -114,7 +126,17 @@ list.map:
 		"cb_password":  "geheim",
 	}
 	got := map[string]string{}
-	assert.NoError(t, ParamsToEnv(from, got, secrets))
+	getSecretValue := func(name string) (string, error) {
+		name = strings.ToLower(name)
+		secret, ok := secrets[name]
+		if ok {
+			return secret, nil
+		}
+
+		return "", fmt.Errorf("secret %q not found or not allowed to be used", name)
+	}
+
+	assert.NoError(t, ParamsToEnv(from, got, getSecretValue))
 	assert.EqualValues(t, want, got, "Problem converting plugin parameters to environment variables")
 }
 
@@ -128,7 +150,17 @@ func TestYAMLToParamsToEnvError(t *testing.T) {
 	secrets := map[string]string{
 		"secret_token": "FooBar",
 	}
-	assert.Error(t, ParamsToEnv(from, make(map[string]string), secrets))
+	getSecretValue := func(name string) (string, error) {
+		name = strings.ToLower(name)
+		secret, ok := secrets[name]
+		if ok {
+			return secret, nil
+		}
+
+		return "", fmt.Errorf("secret %q not found or not allowed to be used", name)
+	}
+
+	assert.Error(t, ParamsToEnv(from, make(map[string]string), getSecretValue))
 }
 
 func stringsToInterface(val ...string) []any {
@@ -138,3 +170,27 @@ func stringsToInterface(val ...string) []any {
 	}
 	return res
 }
+
+func TestSecretNotFound(t *testing.T) {
+	from := map[string]any{
+		"map": map[string]any{"secret": map[string]any{"from_secret": "secret_token"}},
+	}
+
+	secrets := map[string]string{
+		"a_different_password": "secret",
+	}
+	getSecretValue := func(name string) (string, error) {
+		name = strings.ToLower(name)
+		secret, ok := secrets[name]
+		if ok {
+			return secret, nil
+		}
+
+		return "", fmt.Errorf("secret %q not found or not allowed to be used", name)
+	}
+	got := map[string]string{}
+
+	assert.ErrorContains(t,
+		ParamsToEnv(from, got, getSecretValue),
+		fmt.Sprintf("secret %q not found or not allowed to be used", "secret_token"))
+}

server/model/secret.go

@@ -102,27 +102,6 @@ func (s Secret) IsRepository() bool {
 	return s.RepoID != 0 && s.OrgID == 0
 }
 
-// Match returns true if an image and event match the restricted list.
-// Note that EventPullClosed are treated as EventPull.
-func (s *Secret) Match(event WebhookEvent) bool {
-	// if there is no filter set secret matches all webhook events
-	if len(s.Events) == 0 {
-		return true
-	}
-	// tread all pull events the same way
-	if event == EventPullClosed {
-		event = EventPull
-	}
-	// one match is enough
-	for _, e := range s.Events {
-		if e == event {
-			return true
-		}
-	}
-	// a filter is set but the webhook did not match it
-	return false
-}
-
 var validDockerImageString = regexp.MustCompile(
 	`^(` +
 		`[\w\d\-_\.]+` + // hostname

server/model/secret_test.go

@@ -18,49 +18,8 @@ import (
 	"testing"
 
 	"github.com/franela/goblin"
-	"github.com/stretchr/testify/assert"
 )
 
-func TestSecretMatch(t *testing.T) {
-	tcl := []*struct {
-		name   string
-		secret Secret
-		event  WebhookEvent
-		match  bool
-	}{
-		{
-			name:   "should match event",
-			secret: Secret{Events: []WebhookEvent{"pull_request"}},
-			event:  EventPull,
-			match:  true,
-		},
-		{
-			name:   "should not match event",
-			secret: Secret{Events: []WebhookEvent{"pull_request"}},
-			event:  EventPush,
-			match:  false,
-		},
-		{
-			name:   "should match when no event filters defined",
-			secret: Secret{},
-			event:  EventPull,
-			match:  true,
-		},
-		{
-			name:   "pull close should match pull",
-			secret: Secret{Events: []WebhookEvent{"pull_request"}},
-			event:  EventPullClosed,
-			match:  true,
-		},
-	}
-
-	for _, tc := range tcl {
-		t.Run(tc.name, func(t *testing.T) {
-			assert.Equal(t, tc.match, tc.secret.Match(tc.event))
-		})
-	}
-}
-
 func TestSecretValidate(t *testing.T) {
 	g := goblin.Goblin(t)
 	g.Describe("Secret", func() {

server/pipeline/stepbuilder/stepBuilder.go

@@ -240,13 +240,16 @@ func (b *StepBuilder) environmentVariables(metadata metadata.Metadata, axis matr
 func (b *StepBuilder) toInternalRepresentation(parsed *yaml_types.Workflow, environ map[string]string, metadata metadata.Metadata, stepID int64) (*backend_types.Config, error) {
 	var secrets []compiler.Secret
 	for _, sec := range b.Secs {
-		if !sec.Match(b.Curr.Event) {
+		events := []string{}
-			continue
+		for _, event := range sec.Events {
+			events = append(events, string(event))
 		}
+
 		secrets = append(secrets, compiler.Secret{
 			Name:           sec.Name,
 			Value:          sec.Value,
 			AllowedPlugins: sec.Images,
+			Events:         events,
 		})
 	}