From 0b5eef7d1ed2969e7c02dd8647ba66d9e85ea641 Mon Sep 17 00:00:00 2001 From: Anbraten Date: Sat, 27 Jan 2024 20:59:44 +0100 Subject: [PATCH] Improve secret availability checks (#3271) --- pipeline/frontend/yaml/compiler/compiler.go | 47 +++++++++++--- .../frontend/yaml/compiler/compiler_test.go | 61 +++++++++++++++--- pipeline/frontend/yaml/compiler/convert.go | 50 +++++++++------ .../frontend/yaml/compiler/settings/params.go | 36 ++++++----- .../yaml/compiler/settings/params_test.go | 62 ++++++++++++++++++- server/model/secret.go | 21 ------- server/model/secret_test.go | 41 ------------ server/pipeline/stepbuilder/stepBuilder.go | 7 ++- 8 files changed, 204 insertions(+), 121 deletions(-) diff --git a/pipeline/frontend/yaml/compiler/compiler.go b/pipeline/frontend/yaml/compiler/compiler.go index 229f399d6..6eab7c471 100644 --- a/pipeline/frontend/yaml/compiler/compiler.go +++ b/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 { - return (len(s.AllowedPlugins) == 0 || utils.MatchImage(container.Image, s.AllowedPlugins...)) && (len(s.AllowedPlugins) == 0 || container.IsPlugin()) +func (s *Secret) Available(event string, container *yaml_types.Container) error { + 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 diff --git a/pipeline/frontend/yaml/compiler/compiler_test.go b/pipeline/frontend/yaml/compiler/compiler_test.go index 329235a17..eca7322f8 100644 --- a/pipeline/frontend/yaml/compiler/compiler_test.go +++ b/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'"}, - })) - assert.False(t, secret.Available(&yaml_types.Container{ + }), "only allowed to be used by plugins by step") + 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)) + }) + } +} diff --git a/pipeline/frontend/yaml/compiler/convert.go b/pipeline/frontend/yaml/compiler/convert.go index 670051f38..b68e80fe4 100644 --- a/pipeline/frontend/yaml/compiler/convert.go +++ b/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 { - pluginSecrets := secretMap{} - for name, secret := range c.secrets { - if secret.Available(container) { - pluginSecrets[name] = secret - } + getSecretValue := func(name string) (string, error) { + name = strings.ToLower(name) + secret, ok := c.secrets[name] + if !ok { + 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 } diff --git a/pipeline/frontend/yaml/compiler/settings/params.go b/pipeline/frontend/yaml/compiler/settings/params.go index 55b3c7995..8dca91e4b 100644 --- a/pipeline/frontend/yaml/compiler/settings/params.go +++ b/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) { - v, err := injectSecretRecursive(v, secrets) +func handleComplex(v any, getSecretValue func(name string) (string, error)) (string, error) { + 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 { - return secret, true, nil + secret, err := getSecretValue(secretName) + 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 } diff --git a/pipeline/frontend/yaml/compiler/settings/params_test.go b/pipeline/frontend/yaml/compiler/settings/params_test.go index 85fbfd729..114ba31b4 100644 --- a/pipeline/frontend/yaml/compiler/settings/params_test.go +++ b/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")) +} diff --git a/server/model/secret.go b/server/model/secret.go index 956e06b0f..4a941ca46 100644 --- a/server/model/secret.go +++ b/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 diff --git a/server/model/secret_test.go b/server/model/secret_test.go index 01d242823..915b432b0 100644 --- a/server/model/secret_test.go +++ b/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() { diff --git a/server/pipeline/stepbuilder/stepBuilder.go b/server/pipeline/stepbuilder/stepBuilder.go index 674f38140..f0bdadb6a 100644 --- a/server/pipeline/stepbuilder/stepBuilder.go +++ b/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) { - continue + events := []string{} + 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, }) }