Read action's secrets from env vars and avoid logging them

CHANGELOG.md

@@ -4,6 +4,7 @@
 ## Unreleased - XXXX-XX-XX
  - Fix blank screen bug in UI (#1908)
  - Support multiple AWS regions for underlying buckets (#2245, #2325, #2326)
+ - Actions secrets support with env vars (#2333)
 
 ## v0.47.0 - 2021-07-28
 

nessie/hooks_test.go

@@ -41,8 +41,8 @@ hooks:
     description: Check webhooks for pre-commit works
     properties:
       url: "{{.URL}}/pre-commit"
-	  query_params:
-        check_env_vars: {{"{{"}} ENV.ACTIONS_VAR {{"}}"}}
+      query_params:
+        check_env_vars: "{{"{{ ENV.ACTIONS_VAR }}"}}"
 `
 
 const actionPostCommitYaml = `

nessie/ops/docker-compose.yaml

@@ -18,6 +18,7 @@ services:
       - LAKEFS_STATS_ENABLED
       - LAKEFS_BLOCKSTORE_AZURE_STORAGE_ACCOUNT
       - LAKEFS_BLOCKSTORE_AZURE_STORAGE_ACCESS_KEY
+      - ACTIONS_VAR=this_is_actions_var
     entrypoint: ["/app/wait-for", "postgres:5432", "--", "/app/lakefs", "run"]
   postgres:
     image: "postgres:11"
@@ -42,7 +43,6 @@ services:
       - NESSIE_AWS_ACCESS_KEY_ID
       - NESSIE_AWS_SECRET_ACCESS_KEY
       - NESSIE_ENDPOINT_URL=http://lakefs:8000
-      - ACTIONS_VAR="this_is_actions_var"
     working_dir: /lakefs
     entrypoint: ["go", "test", "-v", "./nessie", "--system-tests"]
     volumes:

pkg/actions/airflow.go

@@ -61,7 +61,7 @@ func NewAirflowHook(h ActionHook, action *Action) (Hook, error) {
 	if err != nil {
 		return nil, fmt.Errorf("airflow hook password property: %w", err)
 	}
-	airflowHook.Password, err = NewFromString(rawPass)
+	airflowHook.Password, err = NewSecureString(rawPass)
 	if err != nil {
 		return nil, fmt.Errorf("airflow hook password property: %w", err)
 	}

pkg/actions/secure_string.go

@@ -1,13 +1,16 @@
 package actions
 
 import (
+	"fmt"
 	"os"
+	"strings"
+
 	"regexp"
 )
 
-// SecureString is a string tha may or may not contain a secret.
-// Secret is considered any value that was read from an environment variable, in the following syntax:
-// "{{ ENV.VAR_KEY }}"
+// SecureString is a string that may be populated from an environment variable.
+// If constructed with a string of the form {{ ENV.EXAMPLE_VARIABLE }}, the value is populated from EXAMPLE_VARIABLE and
+// is considered a secret. Otherwise the value is taken from the string as-is, and is not considered a secret.
 type SecureString struct {
 	val    string
 	secret bool
@@ -21,19 +24,37 @@ func (s SecureString) String() string {
 	return s.val
 }
 
-var envVarRegex = regexp.MustCompile(`{{ ENV\.(.*) }}`)
+var envVarRegex = regexp.MustCompile(`{{ ?ENV\..*? ?}}`)
 
 // Creates a new SecureString, reading env var if needed.
-func NewFromString(s string) (SecureString, error) {
-	matches := envVarRegex.FindStringSubmatch(s)
-	if len(matches) == 0 {
-		return SecureString{val: s, secret: false}, nil
+func NewSecureString(s string) (SecureString, error) {
+	matches := 0
+	var err error
+	ret := envVarRegex.ReplaceAllStringFunc(s, func(origin string) string {
+		if err != nil {
+			return ""
+		}
+		matches++
+		raw := strings.Trim(origin, "{} ")
+		parts := strings.SplitN(raw, ".", 2)
+		if len(parts) != 2 || parts[0] != "ENV" {
+			return s
+		}
+
+		envVarName := parts[1]
+		val, ok := os.LookupEnv(envVarName)
+		if !ok {
+			err = fmt.Errorf("%s not found: %w", envVarName, errMissingEnvVar)
+			return ""
+		}
+		return val
+	})
+	if err != nil {
+		return SecureString{}, err
 	}
-	envVarName := s[len("{{ ENV.") : len(s)-len(" }}")]
-	val, ok := os.LookupEnv(envVarName)
-	if !ok {
-		return SecureString{}, errMissingEnvVar
+	if matches == 0 {
+		return SecureString{val: s, secret: false}, nil
 	}
 
-	return SecureString{val: val, secret: true}, nil
+	return SecureString{val: ret, secret: true}, nil
 }

pkg/actions/secure_string_test.go

@@ -0,0 +1,70 @@
+package actions
+
+import (
+	"errors"
+	"github.com/stretchr/testify/require"
+	"os"
+	"testing"
+)
+
+func TestSecureString(t *testing.T) {
+	tests := []struct {
+		name        string
+		input       string
+		err         error
+		envVarToSet map[string]string
+		expectedOut SecureString
+	}{
+		{
+			name:        "No env var",
+			input:       "this is just a string",
+			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"},
+			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"},
+			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"},
+			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"},
+			expectedOut: SecureString{val: "let me count: one, two, three", secret: true},
+		},
+		{
+			name:        "missing env var",
+			input:       "{{ ENV.MISSING_FIRST }}",
+			envVarToSet: map[string]string{"SIMPLE_FIRST": "some value"},
+			expectedOut: SecureString{},
+			err:         errMissingEnvVar,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			for k, v := range tt.envVarToSet {
+				require.NoError(t, os.Setenv(k, v))
+			}
+
+			out, err := NewSecureString(tt.input)
+			if tt.err == nil {
+				require.Nil(t, err)
+			} else {
+				require.True(t, errors.Is(err, tt.err))
+			}
+			require.Equal(t, tt.expectedOut, out)
+		})
+	}
+}

pkg/actions/testdata/action_secrets.yaml

@@ -21,4 +21,4 @@ hooks:
     type: webhook
     description: check production is not in dev freeze
     properties:
-      url: "https://api.lakefs.io/webhook2?t=1za2PbkZK1bd4prMuTDr6BeEQwWYcX2R"
+      url: "https://api.lakefs.io/webhook2?t=1za2PbkZK1bd4prMuTDr6BeEQwWYcX2R"

pkg/actions/webhook.go

@@ -177,7 +177,7 @@ func extractQueryParams(props map[string]interface{}) (map[string][]SecureString
 					return nil, fmt.Errorf("query params array should contains only strings: %w", errWebhookWrongFormat)
 				}
 
-				avs, err := NewFromString(av)
+				avs, err := NewSecureString(av)
 				if err != nil {
 					return nil, fmt.Errorf("reading query param: %w", err)
 				}
@@ -190,7 +190,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 := NewFromString(av)
+		avs, err := NewSecureString(av)
 		if err != nil {
 			return nil, fmt.Errorf("reading query param: %w", err)
 		}
@@ -218,7 +218,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 := NewFromString(vs)
+		vss, err := NewSecureString(vs)
 		if err != nil {
 			return nil, fmt.Errorf("reading header: %w", err)
 		}