diff --git a/docs/howto/hooks/webhooks.md b/docs/howto/hooks/webhooks.md index 60536564bc2..f706dba5b2a 100644 --- a/docs/howto/hooks/webhooks.md +++ b/docs/howto/hooks/webhooks.md @@ -34,6 +34,9 @@ _See the [Action configuration](./index.md#action-file) for overall configuratio lakeFS Actions supports secrets by using environment variables. The format `{% raw %}{{{% endraw %} ENV.SOME_ENV_VAR {% raw %}}}{% endraw %}` will be replaced with the value of `$SOME_ENV_VAR` during the execution of the action. If that environment variable doesn't exist in the lakeFS server environment, the action run will fail. + +For security purposes, any environment variable whose name begins with "LAKEFS" will be blocked. In this case, the variable will be evaluated to an empty string, effectively making it inaccessible. +Additionally, the `actions.env.enabled` configuration parameter can be set to `false` to block access to all environment variables. {: .note } Example: diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index b82df994baf..1481e536563 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -33,6 +33,8 @@ This reference uses `.` to denote the nesting of values. * `logging.files_keep` `(int : 0)` - Number of log files to keep, default is all. * `actions.enabled` `(bool : true)` - Setting this to false will block hooks from being executed. * `actions.lua.net_http_enabled` `(bool : false)` - Setting this to true will load the `net/http` package. +* `actions.env.enabled` `(bool : true)` - Environment variables accessible by hooks, disabled values evaluated to empty strings +* `actions.env.prefix` `(string : "LAKEFSACTION_")` - Access to environment variables is restricted to those with the prefix. When environment access is enabled and no prefix is provided, all variables are accessible. **Note:** Deprecated - See `database` section {: .note } diff --git a/pkg/actions/airflow.go b/pkg/actions/airflow.go index ebf3dbd0457..d0d427e1678 100644 --- a/pkg/actions/airflow.go +++ b/pkg/actions/airflow.go @@ -56,7 +56,7 @@ var ( errAirflowHookDAGFailed = errors.New("airflow hook DAG failed") ) -func NewAirflowHook(h ActionHook, action *Action, cfg Config, endpoint *http.Server, collector stats.Collector) (Hook, error) { +func NewAirflowHook(h ActionHook, action *Action, cfg Config, endpoint *http.Server, _ stats.Collector) (Hook, error) { airflowHook := Airflow{ HookBase: HookBase{ ID: h.ID, @@ -86,7 +86,8 @@ func NewAirflowHook(h ActionHook, action *Action, cfg Config, endpoint *http.Ser if err != nil { return nil, fmt.Errorf("airflow hook password property: %w", err) } - airflowHook.Password, err = NewSecureString(rawPass) + envGetter := NewEnvironmentVariableGetter(cfg.Env.Enabled, cfg.Env.Prefix) + airflowHook.Password, err = NewSecureString(rawPass, envGetter) if err != nil { return nil, fmt.Errorf("airflow hook password property: %w", err) } diff --git a/pkg/actions/lua.go b/pkg/actions/lua.go index 52de8790aa1..3a371f7456d 100644 --- a/pkg/actions/lua.go +++ b/pkg/actions/lua.go @@ -12,6 +12,7 @@ import ( "time" "github.com/Shopify/go-lua" + "github.com/spf13/viper" lualibs "github.com/treeverse/lakefs/pkg/actions/lua" "github.com/treeverse/lakefs/pkg/actions/lua/lakefs" luautil "github.com/treeverse/lakefs/pkg/actions/lua/util" @@ -148,12 +149,12 @@ func (h *LuaHook) collectMetrics(l *lua.State) { l.Pop(1) // Pop the _LOADED table from the stack } -func DescendArgs(args interface{}) (interface{}, error) { +func DescendArgs(args interface{}, getter EnvGetter) (interface{}, error) { var err error switch t := args.(type) { case Properties: for k, v := range t { - t[k], err = DescendArgs(v) + t[k], err = DescendArgs(v, getter) if err != nil { return nil, err } @@ -161,14 +162,14 @@ func DescendArgs(args interface{}) (interface{}, error) { return t, nil case map[string]interface{}: for k, v := range t { - t[k], err = DescendArgs(v) + t[k], err = DescendArgs(v, getter) if err != nil { return nil, err } } return t, nil case string: - secure, secureErr := NewSecureString(t) + secure, secureErr := NewSecureString(t, getter) if secureErr != nil { return t, secureErr } @@ -176,13 +177,13 @@ func DescendArgs(args interface{}) (interface{}, error) { case []string: stuff := make([]interface{}, len(t)) for i, e := range t { - stuff[i], err = DescendArgs(e) + stuff[i], err = DescendArgs(e, getter) } return stuff, err case []interface{}: stuff := make([]interface{}, len(t)) for i, e := range t { - stuff[i], err = DescendArgs(e) + stuff[i], err = DescendArgs(e, getter) } return stuff, err default: @@ -206,7 +207,10 @@ func NewLuaHook(h ActionHook, action *Action, cfg Config, e *http.Server, collec return nil, fmt.Errorf("'args' should be a map: %w", errWrongValueType) } } - parsedArgs, err := DescendArgs(args) + parsedArgs, err := DescendArgs(args, &EnvironmentVariableGetter{ + Enabled: cfg.Env.Enabled, + Prefix: viper.GetEnvPrefix(), + }) if err != nil { return &LuaHook{}, fmt.Errorf("error parsing args: %w", err) } diff --git a/pkg/actions/lua/storage/aws/glue.go b/pkg/actions/lua/storage/aws/glue.go index 640d7995941..6b4b893b8bd 100644 --- a/pkg/actions/lua/storage/aws/glue.go +++ b/pkg/actions/lua/storage/aws/glue.go @@ -91,7 +91,6 @@ func deleteTable(c *GlueClient) lua.Function { Name: aws.String(tableName), CatalogId: catalogID, }) - if err != nil { lua.Errorf(l, err.Error()) panic("unreachable") @@ -100,6 +99,7 @@ func deleteTable(c *GlueClient) lua.Function { return 0 } } + func updateTable(c *GlueClient) lua.Function { return func(l *lua.State) int { client := c.client() @@ -148,6 +148,7 @@ func updateTable(c *GlueClient) lua.Function { return 0 } } + func createTable(c *GlueClient) lua.Function { return func(l *lua.State) int { client := c.client() diff --git a/pkg/actions/lua_test.go b/pkg/actions/lua_test.go index 48ac17c6ed9..1f84d722fa6 100644 --- a/pkg/actions/lua_test.go +++ b/pkg/actions/lua_test.go @@ -462,7 +462,8 @@ func TestDescendArgs(t *testing.T) { "e": []interface{}{"a", 1, false, "{{ ENV.magic_environ123123 }}"}, }, } - out, err := actions.DescendArgs(v) + envGetter := actions.NewEnvironmentVariableGetter(true, "") + out, err := actions.DescendArgs(v, envGetter) if err != nil { t.Fatalf("unexpected err: %s", err) } @@ -501,9 +502,71 @@ func TestDescendArgs(t *testing.T) { "e": []interface{}{"a", 1, false, "{{ ENV.magic_environ123123456 }}"}, // <- shouldn't exist? }, } - _, err := actions.DescendArgs(v) + + envGetter := actions.NewEnvironmentVariableGetter(true, "") + _, err := actions.DescendArgs(v, envGetter) if err == nil { t.Fatalf("expected error!") } }) + + t.Run("env_disabled", func(t *testing.T) { + testutil.WithEnvironmentVariable(t, "magic_environ123123", "magic_environ_value") + v := map[string]interface{}{ + "key": "value", + "secure_key": "value with {{ ENV.magic_environ123123 }}", + } + envGetter := actions.NewEnvironmentVariableGetter(false, "") + args, err := actions.DescendArgs(v, envGetter) + if err != nil { + t.Fatalf("DescendArgs failed: %s", err) + } + argsMap, ok := args.(map[string]interface{}) + if !ok { + t.Fatalf("expected map[string]interface{}, got a %T", argsMap) + } + secureString, ok := argsMap["secure_key"].(string) + if !ok { + t.Fatalf("expected a string, got a %T", argsMap["secure_key"]) + } + const expectedValue = "value with " + if secureString != expectedValue { + t.Fatalf("expected '%s' as value from env when env is disabled, got '%s'", expectedValue, secureString) + } + }) + + t.Run("env_prefix", func(t *testing.T) { + testutil.WithEnvironmentVariable(t, "magic_environ123123", "magic_environ_value") + v := map[string]interface{}{ + "key": "value{{ ENV.no_magic_environ123123 }}", + "secure_key": "value with {{ ENV.magic_environ123123 }}", + } + envGetter := actions.NewEnvironmentVariableGetter(true, "magic_") + args, err := actions.DescendArgs(v, envGetter) + if err != nil { + t.Fatalf("DescendArgs failed: %s", err) + } + argsMap, ok := args.(map[string]interface{}) + if !ok { + t.Fatalf("expected map[string]interface{}, got a %T", argsMap) + } + + // verify that we have value access to keys with the prefix + secureString, ok := argsMap["secure_key"].(string) + if !ok { + t.Fatalf("expected a string, got a %T", argsMap["secure_key"]) + } + if secureString != "value with magic_environ_value" { + t.Fatalf("expected magic environ value, got '%s'", secureString) + } + + // verify that we don't have value access to keys without the prefix + secureString, ok = argsMap["key"].(string) + if !ok { + t.Fatalf("expected a string, got a %T", argsMap["key"]) + } + if secureString != "value" { + t.Fatalf("expected just value for 'key', got '%s'", secureString) + } + }) } diff --git a/pkg/actions/secure_string.go b/pkg/actions/secure_string.go index 1f9de434a40..2254d5bb5bc 100644 --- a/pkg/actions/secure_string.go +++ b/pkg/actions/secure_string.go @@ -26,7 +26,12 @@ func (s SecureString) String() string { var envVarRegex = regexp.MustCompile(`{{ ?ENV\..*? ?}}`) // NewSecureString creates a new SecureString, reading env var if needed. -func NewSecureString(s string) (SecureString, error) { +// If the string is not of the form {{ ENV.EXAMPLE_VARIABLE }}, the value is not considered a secret. +// If the string is of the form {{ ENV.EXAMPLE_VARIABLE }}, the value is populated from EXAMPLE_VARIABLE and +// is considered a secret. +// If the environment variable is not found, an error is returned. +// IF envEnabled is false, the value we evaluate is an empty string. +func NewSecureString(s string, envGetter EnvGetter) (SecureString, error) { matches := 0 var err error ret := envVarRegex.ReplaceAllStringFunc(s, func(origin string) string { @@ -35,13 +40,12 @@ func NewSecureString(s string) (SecureString, error) { } matches++ raw := strings.Trim(origin, "{} ") - parts := strings.SplitN(raw, ".", 2) //nolint: gomnd - if len(parts) != 2 || parts[0] != "ENV" { + source, envVarName, ok := strings.Cut(raw, ".") + if !ok || source != "ENV" { return origin } - envVarName := parts[1] - val, ok := os.LookupEnv(envVarName) + val, ok := envGetter.Lookup(envVarName) if !ok { err = fmt.Errorf("%s not found: %w", envVarName, errMissingEnvVar) return "" @@ -57,3 +61,36 @@ func NewSecureString(s string) (SecureString, error) { return SecureString{val: ret, secret: true}, nil } + +type EnvGetter interface { + Lookup(name string) (string, bool) +} + +type EnvironmentVariableGetter struct { + Enabled bool + Prefix string +} + +// NewEnvironmentVariableGetter creates a new EnvironmentVariableGetter. +// If envEnabled is false, the value we evaluate is an empty string. +// If filterPrefix is not empty, we only evaluate environment variables that start with this prefix. +func NewEnvironmentVariableGetter(envEnabled bool, prefix string) *EnvironmentVariableGetter { + return &EnvironmentVariableGetter{ + Enabled: envEnabled, + Prefix: prefix, + } +} + +// Lookup retrieves the value of the environment variable named +// by the key. If the variable is present in the environment, the +// value (which may be empty) is returned and the boolean is true. +// This function doesn't provide a way to extract variables that can be used by viper. +func (o *EnvironmentVariableGetter) Lookup(name string) (string, bool) { + if !o.Enabled { + return "", true + } + if o.Prefix != "" && !strings.HasPrefix(name, o.Prefix) { + return "", true + } + return os.LookupEnv(name) +} diff --git a/pkg/actions/secure_string_test.go b/pkg/actions/secure_string_test.go index e01fe583ed4..a78bd5824b1 100644 --- a/pkg/actions/secure_string_test.go +++ b/pkg/actions/secure_string_test.go @@ -10,54 +10,86 @@ import ( func TestSecureString(t *testing.T) { tests := []struct { - name string - input string - err error - envVarToSet map[string]string - expectedOut SecureString + name string + input string + err error + envVarToSet map[string]string + envEnabled bool + envFilterPrefix string + expectedOut SecureString }{ { name: "No env var", input: "this is just a string", + envEnabled: true, expectedOut: SecureString{val: "this is just a string", secret: false}, }, { name: "simple env var", input: "{{ ENV.SIMPLE_FIRST }}", envVarToSet: map[string]string{"SIMPLE_FIRST": "some value"}, + envEnabled: true, expectedOut: SecureString{val: "some value", secret: true}, }, { name: "no spaces env var", input: "{{ENV.NO_SPACES_FIRST}}", envVarToSet: map[string]string{"NO_SPACES_FIRST": "this is some value"}, + envEnabled: true, expectedOut: SecureString{val: "this is some value", secret: true}, }, { name: "wrapped with text", input: "this {{ ENV.WRAPPED_FIRST }} value", envVarToSet: map[string]string{"WRAPPED_FIRST": "is another"}, + envEnabled: true, expectedOut: SecureString{val: "this is another value", secret: true}, }, { name: "multiple vars and text", input: "let me count: {{ ENV.MULTIPLE_FIRST }}, {{ENV.MULTIPLE_SECOND}}, {{ ENV.MULTIPLE_THIRD }}", envVarToSet: map[string]string{"MULTIPLE_FIRST": "one", "MULTIPLE_SECOND": "two", "MULTIPLE_THIRD": "three"}, + envEnabled: true, expectedOut: SecureString{val: "let me count: one, two, three", secret: true}, }, { name: "not an env var", input: "{{ NV.NOT_AN_ENV_VAR }}", envVarToSet: map[string]string{"NOT_AN_ENV_VAR": "one"}, + envEnabled: true, expectedOut: SecureString{val: "{{ NV.NOT_AN_ENV_VAR }}", secret: false}, }, { name: "missing env var", input: "{{ ENV.MISSING_FIRST }}", envVarToSet: map[string]string{"SIMPLE_FIRST": "some value"}, + envEnabled: true, expectedOut: SecureString{}, err: errMissingEnvVar, }, + { + name: "env disabled", + input: "{{ ENV.SIMPLE_FIRST }}", + envVarToSet: map[string]string{"SIMPLE_FIRST": "some value"}, + envEnabled: false, + expectedOut: SecureString{val: "", secret: true}, + }, + { + name: "filter env by prefix no match", + input: "{{ ENV.SIMPLE_FIRST }}", + envVarToSet: map[string]string{"SIMPLE_FIRST": "some value", "COMPLEX_SECOND": "another value"}, + envEnabled: true, + envFilterPrefix: "COMPLEX", + expectedOut: SecureString{val: "", secret: true}, + }, + { + name: "filter env by prefix match", + input: "{{ ENV.SIMPLE_FIRST }}", + envVarToSet: map[string]string{"SIMPLE_FIRST": "some value", "COMPLEX_SECOND": "another value"}, + envEnabled: true, + envFilterPrefix: "SIMPLE", + expectedOut: SecureString{val: "some value", secret: true}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -65,7 +97,8 @@ func TestSecureString(t *testing.T) { require.NoError(t, os.Setenv(k, v)) } - out, err := NewSecureString(tt.input) + envGetter := NewEnvironmentVariableGetter(tt.envEnabled, tt.envFilterPrefix) + out, err := NewSecureString(tt.input, envGetter) if tt.err == nil { require.Nil(t, err) } else { diff --git a/pkg/actions/service.go b/pkg/actions/service.go index ffb3fc5fade..5210a110f53 100644 --- a/pkg/actions/service.go +++ b/pkg/actions/service.go @@ -42,6 +42,10 @@ type Config struct { Lua struct { NetHTTPEnabled bool } + Env struct { + Enabled bool + Prefix string + } } // StoreService is an implementation of actions.Service that saves diff --git a/pkg/actions/service_test.go b/pkg/actions/service_test.go index c89f2d554a8..221035907f4 100644 --- a/pkg/actions/service_test.go +++ b/pkg/actions/service_test.go @@ -63,6 +63,7 @@ func GetKVService(t *testing.T, ctx context.Context, source actions.Source, writ kvStore := kvtest.GetStore(ctx, t) cfg := actions.Config{Enabled: runHooks} cfg.Lua.NetHTTPEnabled = true + cfg.Env.Enabled = true return actions.NewService(ctx, actions.NewActionsKVStore(kvStore), source, writer, &actions.DecreasingIDGenerator{}, stats, cfg) } diff --git a/pkg/actions/webhook.go b/pkg/actions/webhook.go index 3aa1ec577b6..14691bd95a9 100644 --- a/pkg/actions/webhook.go +++ b/pkg/actions/webhook.go @@ -36,7 +36,7 @@ var ( errWebhookWrongFormat = errors.New("webhook wrong format") ) -func NewWebhook(h ActionHook, action *Action, cfg Config, e *http.Server, collector stats.Collector) (Hook, error) { +func NewWebhook(h ActionHook, action *Action, cfg Config, e *http.Server, _ stats.Collector) (Hook, error) { url, ok := h.Properties[webhookURLPropertyKey] if !ok { return nil, fmt.Errorf("missing url: %w", errWebhookWrongFormat) @@ -46,12 +46,13 @@ func NewWebhook(h ActionHook, action *Action, cfg Config, e *http.Server, collec return nil, fmt.Errorf("webhook url must be string: %w", errWebhookWrongFormat) } - queryParams, err := extractQueryParams(h.Properties) + envGetter := NewEnvironmentVariableGetter(cfg.Env.Enabled, cfg.Env.Prefix) + queryParams, err := extractQueryParams(h.Properties, envGetter) if err != nil { return nil, fmt.Errorf("extracting query params: %w", err) } - headers, err := extractHeaders(h.Properties) + headers, err := extractHeaders(h.Properties, envGetter) if err != nil { return nil, fmt.Errorf("extracting headers: %w", err) } @@ -171,7 +172,7 @@ func doHTTPRequestResponseWithLog(ctx context.Context, req *http.Request, respJS return resp.StatusCode, nil } -func extractQueryParams(props map[string]interface{}) (map[string][]SecureString, error) { +func extractQueryParams(props map[string]interface{}, envGetter EnvGetter) (map[string][]SecureString, error) { params, ok := props[queryParamsPropertyKey] if !ok { return nil, nil @@ -190,8 +191,7 @@ func extractQueryParams(props map[string]interface{}) (map[string][]SecureString if !ok { return nil, fmt.Errorf("query params array should contains only strings: %w", errWebhookWrongFormat) } - - avs, err := NewSecureString(av) + avs, err := NewSecureString(av, envGetter) if err != nil { return nil, fmt.Errorf("reading query param: %w", err) } @@ -204,7 +204,7 @@ func extractQueryParams(props map[string]interface{}) (map[string][]SecureString return nil, fmt.Errorf("query params single value should be of type string: %w", errWebhookWrongFormat) } - avs, err := NewSecureString(av) + avs, err := NewSecureString(av, envGetter) if err != nil { return nil, fmt.Errorf("reading query param: %w", err) } @@ -214,7 +214,7 @@ func extractQueryParams(props map[string]interface{}) (map[string][]SecureString return res, nil } -func extractHeaders(props map[string]interface{}) (map[string]SecureString, error) { +func extractHeaders(props map[string]interface{}, envGetter EnvGetter) (map[string]SecureString, error) { params, ok := props[HeadersPropertyKey] if !ok { return map[string]SecureString{}, nil @@ -232,7 +232,7 @@ func extractHeaders(props map[string]interface{}) (map[string]SecureString, erro return nil, fmt.Errorf("headers array should contains only strings: %w", errWebhookWrongFormat) } - vss, err := NewSecureString(vs) + vss, err := NewSecureString(vs, envGetter) if err != nil { return nil, fmt.Errorf("reading header: %w", err) } diff --git a/pkg/config/config.go b/pkg/config/config.go index f6e7ef5a4b3..309c358583d 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -104,7 +104,11 @@ type Config struct { Lua struct { NetHTTPEnabled bool `mapstructure:"net_http_enabled"` } `mapstructure:"lua"` - } + Env struct { + Enabled bool `mapstructure:"enabled"` + Prefix string `mapstructure:"prefix"` + } `mapstructure:"env"` + } `mapstructure:"actions"` Logging struct { Format string `mapstructure:"format"` diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 17ef70a7c4a..f699bd908a5 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -45,6 +45,8 @@ func setDefaults(cfgType string) { viper.SetDefault("logging.file_max_size_mb", (1<<10)*100) // 100MiB viper.SetDefault("actions.enabled", true) + viper.SetDefault("actions.env.enabled", true) + viper.SetDefault("actions.env.prefix", "LAKEFSACTION_") viper.SetDefault("auth.cache.enabled", true) viper.SetDefault("auth.cache.size", 1024) diff --git a/pkg/config/types_test.go b/pkg/config/types_test.go index b71a72f2ece..36e41dc75c6 100644 --- a/pkg/config/types_test.go +++ b/pkg/config/types_test.go @@ -1,14 +1,14 @@ package config_test import ( + "errors" + "strings" + "testing" + "github.com/go-test/deep" "github.com/mitchellh/mapstructure" "github.com/treeverse/lakefs/pkg/config" "github.com/treeverse/lakefs/pkg/testutil" - - "errors" - "strings" - "testing" ) type StringsStruct struct {