Merge pull request #1902 from faro-oss/feature/no-expand-env

Allow to disable os.ExpandEnv for storage + connector configs by env variable DEX_EXPAND_ENV = false

.github/workflows/ci.yml

@@ -57,7 +57,6 @@ jobs:
       - name: Run tests
         run: make testall
         env:
-          DEX_FOO_USER_PASSWORD: $2a$10$33EMT0cVYVlPy6WAMCLsceLYjWhuHpbz5yuZxu/GAFj03J9Lytjuy
           DEX_MYSQL_DATABASE: dex
           DEX_MYSQL_USER: root
           DEX_MYSQL_PASSWORD: root

cmd/dex/config.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"strconv"
 	"strings"
 
 	"golang.org/x/crypto/bcrypt"
@@ -181,6 +182,19 @@ var storages = map[string]func() StorageConfig{
 	"mysql":      func() StorageConfig { return new(sql.MySQL) },
 }
 
+// isExpandEnvEnabled returns if os.ExpandEnv should be used for each storage and connector config.
+// Disabling this feature avoids surprises e.g. if the LDAP bind password contains a dollar character.
+// Returns false if the env variable "DEX_EXPAND_ENV" is a falsy string, e.g. "false".
+// Returns true if the env variable is unset or a truthy string, e.g. "true", or can't be parsed as bool.
+func isExpandEnvEnabled() bool {
+	enabled, err := strconv.ParseBool(os.Getenv("DEX_EXPAND_ENV"))
+	if err != nil {
+		// Unset, empty string or can't be parsed as bool: Default = true.
+		return true
+	}
+	return enabled
+}
+
 // UnmarshalJSON allows Storage to implement the unmarshaler interface to
 // dynamically determine the type of the storage config.
 func (s *Storage) UnmarshalJSON(b []byte) error {
@@ -198,7 +212,11 @@ func (s *Storage) UnmarshalJSON(b []byte) error {
 
 	storageConfig := f()
 	if len(store.Config) != 0 {
-		data := []byte(os.ExpandEnv(string(store.Config)))
+		data := []byte(store.Config)
+		if isExpandEnvEnabled() {
+			// Caution, we're expanding in the raw JSON/YAML source. This may not be what the admin expects.
+			data = []byte(os.ExpandEnv(string(store.Config)))
+		}
 		if err := json.Unmarshal(data, storageConfig); err != nil {
 			return fmt.Errorf("parse storage config: %v", err)
 		}
@@ -240,7 +258,11 @@ func (c *Connector) UnmarshalJSON(b []byte) error {
 
 	connConfig := f()
 	if len(conn.Config) != 0 {
-		data := []byte(os.ExpandEnv(string(conn.Config)))
+		data := []byte(conn.Config)
+		if isExpandEnvEnabled() {
+			// Caution, we're expanding in the raw JSON/YAML source. This may not be what the admin expects.
+			data = []byte(os.ExpandEnv(string(conn.Config)))
+		}
 		if err := json.Unmarshal(data, connConfig); err != nil {
 			return fmt.Errorf("parse connector config: %v", err)
 		}

cmd/dex/config_test.go

@@ -16,8 +16,6 @@ import (
 
 var _ = yaml.YAMLToJSON
 
-const testHashStaticPasswordEnv = "DEX_FOO_USER_PASSWORD"
-
 func TestValidConfiguration(t *testing.T) {
 	configuration := Config{
 		Issuer: "http://127.0.0.1:5556/dex",
@@ -110,7 +108,7 @@ staticPasswords:
   hash: "$2a$10$33EMT0cVYVlPy6WAMCLsceLYjWhuHpbz5yuZxu/GAFj03J9Lytjuy"
   username: "admin"
   userID: "08a8684b-db88-4b73-90a9-3cd1661f5466"
-- email: "foo@example.com"  
+- email: "foo@example.com"
   # base64'd value of the same bcrypt hash above. We want to be able to parse both of these
   hash: "JDJhJDEwJDMzRU1UMGNWWVZsUHk2V0FNQ0xzY2VMWWpXaHVIcGJ6NXl1Wnh1L0dBRmowM0o5THl0anV5"
   username: "foo"
@@ -219,17 +217,54 @@ logger:
 	}
 }
 
-func TestUnmarshalConfigWithEnv(t *testing.T) {
-	staticPasswordEnv := os.Getenv(testHashStaticPasswordEnv)
-	if staticPasswordEnv == "" {
-		t.Skipf("test environment variable %q not set, skipping", testHashStaticPasswordEnv)
+func TestUnmarshalConfigWithEnvNoExpand(t *testing.T) {
+	// If the env variable DEX_EXPAND_ENV is set and has a "falsy" value, os.ExpandEnv is disabled.
+	// ParseBool: "It accepts 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False."
+	checkUnmarshalConfigWithEnv(t, "0", false)
+	checkUnmarshalConfigWithEnv(t, "f", false)
+	checkUnmarshalConfigWithEnv(t, "F", false)
+	checkUnmarshalConfigWithEnv(t, "FALSE", false)
+	checkUnmarshalConfigWithEnv(t, "false", false)
+	checkUnmarshalConfigWithEnv(t, "False", false)
+	os.Unsetenv("DEX_EXPAND_ENV")
+}
+
+func TestUnmarshalConfigWithEnvExpand(t *testing.T) {
+	// If the env variable DEX_EXPAND_ENV is unset or has a "truthy" or unknown value, os.ExpandEnv is enabled.
+	// ParseBool: "It accepts 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False."
+	checkUnmarshalConfigWithEnv(t, "1", true)
+	checkUnmarshalConfigWithEnv(t, "t", true)
+	checkUnmarshalConfigWithEnv(t, "T", true)
+	checkUnmarshalConfigWithEnv(t, "TRUE", true)
+	checkUnmarshalConfigWithEnv(t, "true", true)
+	checkUnmarshalConfigWithEnv(t, "True", true)
+	// Values that can't be parsed as bool:
+	checkUnmarshalConfigWithEnv(t, "UNSET", true)
+	checkUnmarshalConfigWithEnv(t, "", true)
+	checkUnmarshalConfigWithEnv(t, "whatever - true is default", true)
+	os.Unsetenv("DEX_EXPAND_ENV")
+}
+
+func checkUnmarshalConfigWithEnv(t *testing.T, dexExpandEnv string, wantExpandEnv bool) {
+	// For hashFromEnv:
+	os.Setenv("DEX_FOO_USER_PASSWORD", "$2a$10$33EMT0cVYVlPy6WAMCLsceLYjWhuHpbz5yuZxu/GAFj03J9Lytjuy")
+	// For os.ExpandEnv ($VAR -> value_of_VAR):
+	os.Setenv("DEX_FOO_POSTGRES_HOST", "10.0.0.1")
+	os.Setenv("DEX_FOO_OIDC_CLIENT_SECRET", "bar")
+	if dexExpandEnv != "UNSET" {
+		os.Setenv("DEX_EXPAND_ENV", dexExpandEnv)
+	} else {
+		os.Unsetenv("DEX_EXPAND_ENV")
 	}
+
 	rawConfig := []byte(`
 issuer: http://127.0.0.1:5556/dex
 storage:
   type: postgres
   config:
-    host: 10.0.0.1
+    # Env variables are expanded in raw YAML source.
+    # Single quotes work fine, as long as the env variable doesn't contain any.
+    host: '$DEX_FOO_POSTGRES_HOST'
     port: 65432
     maxOpenConns: 5
     maxIdleConns: 3
@@ -263,7 +298,9 @@ connectors:
   config:
     issuer: https://accounts.google.com
     clientID: foo
-    clientSecret: bar
+    # Env variables are expanded in raw YAML source.
+    # Single quotes work fine, as long as the env variable doesn't contain any.
+    clientSecret: '$DEX_FOO_OIDC_CLIENT_SECRET'
     redirectURI: http://127.0.0.1:5556/dex/callback/google
 
 enablePasswordDB: true
@@ -288,13 +325,21 @@ logger:
   format: "json"
 `)
 
+	// This is not a valid hostname. It's only used to check whether os.ExpandEnv was applied or not.
+	wantPostgresHost := "$DEX_FOO_POSTGRES_HOST"
+	wantOidcClientSecret := "$DEX_FOO_OIDC_CLIENT_SECRET"
+	if wantExpandEnv {
+		wantPostgresHost = "10.0.0.1"
+		wantOidcClientSecret = "bar"
+	}
+
 	want := Config{
 		Issuer: "http://127.0.0.1:5556/dex",
 		Storage: Storage{
 			Type: "postgres",
 			Config: &sql.Postgres{
 				NetworkDB: sql.NetworkDB{
-					Host:              "10.0.0.1",
+					Host:              wantPostgresHost,
 					Port:              65432,
 					MaxOpenConns:      5,
 					MaxIdleConns:      3,
@@ -339,7 +384,7 @@ logger:
 				Config: &oidc.Config{
 					Issuer:       "https://accounts.google.com",
 					ClientID:     "foo",
-					ClientSecret: "bar",
+					ClientSecret: wantOidcClientSecret,
 					RedirectURI:  "http://127.0.0.1:5556/dex/callback/google",
 				},
 			},