Repository: dexidp/dex (go)

Issue:
SAML connector - nil pointer dereference exception
I'm attempting to set up a SAML2 connector in a k8s cluster for one of our internal environments. While testing this saml setup, the following nil pointer exception happens on every request:

```
2020-07-03 22:28:03.610634 I | http: panic serving 172.16.0.96:60284: runtime error: invalid memory address or nil pointer dereference
goroutine 184 [running]:
net/http.(*conn).serve.func1(0xc000153e00)
        /usr/local/go/src/net/http/server.go:1767 +0x139
panic(0xf0d960, 0x185a270)
        /usr/local/go/src/runtime/panic.go:679 +0x1b2
github.com/beevik/etree.(*Element).dup(0x0, 0x0, 0x9, 0xc000171428)
        /go/pkg/mod/github.com/beevik/etree@v1.1.0/etree.go:965 +0x37
github.com/beevik/etree.(*Element).Copy(...)
        /go/pkg/mod/github.com/beevik/etree@v1.1.0/etree.go:350
github.com/russellhaering/goxmldsig.(*ValidationContext).Validate(0xc0000fccc0, 0x0, 0x1086f79, 0x25, 0x1066d29)
        /go/pkg/mod/github.com/russellhaering/goxmldsig@v0.0.0-20180430223755-7acd5e4a6ef7/validate.go:454 +0x38
github.com/dexidp/dex/connector/saml.verifyResponseSig(0xc0000fccc0, 0xc00018a000, 0xa86, 0xa86, 0xa86, 0xa86, 0x0, 0x0, 0x419eae, 0xc000183000)
        /go/src/github.com/dexidp/dex/connector/saml/saml.go:599 +0x1bb
github.com/dexidp/dex/connector/saml.(*provider).HandlePOST(0xc00008eb60, 0x1060101, 0xc000186000, 0xe08, 0xc000037020, 0x19, 0x0, 0x0, 0x0, 0x0, ...)
        /go/src/github.com/dexidp/dex/connector/saml/saml.go:297 +0x12fb
github.com/dexidp/dex/server.(*Server).handleConnectorCallback(0xc000315600, 0x11d5060, 0xc0002ca8c0, 0xc00049de00)
        /go/src/github.com/dexidp/dex/server/handlers.go:455 +0xe54
github.com/dexidp/dex/server.newServer.func7(0x11d5060, 0xc0002ca8c0, 0xc00049de00)
        /go/src/github.com/dexidp/dex/server/server.go:312 +0x175
net/http.HandlerFunc.ServeHTTP(0xc0001465d0, 0x11d5060, 0xc0002ca8c0, 0xc00049de00)
        /usr/local/go/src/net/http/server.go:2007 +0x44
github.com/gorilla/mux.(*Router).ServeHTTP(0xc0000180c0, 0x11d5060, 0xc0002ca8c0, 0xc00049dc00)
        /go/pkg/mod/github.com/gorilla/mux@v1.7.3/mux.go:212 +0xe2
github.com/dexidp/dex/server.(*Server).ServeHTTP(0xc000315600, 0x11d5060, 0xc0002ca8c0, 0xc00049dc00)
        /go/src/github.com/dexidp/dex/server/server.go:330 +0x58
net/http.serverHandler.ServeHTTP(0xc0004381c0, 0x11d5060, 0xc0002ca8c0, 0xc00049dc00)
        /usr/local/go/src/net/http/server.go:2802 +0xa4
net/http.(*conn).serve(0xc000153e00, 0x11d82e0, 0xc00015df00)
        /usr/local/go/src/net/http/server.go:1890 +0x875
created by net/http.(*Server).Serve
        /usr/local/go/src/net/http/server.go:2928 +0x384
```

The following is our SAML connector config, which is not the only connector we use, but it's the one that's relevant for now:

```yaml
  config.yaml: |-
    issuer: https://dex-ad.<domain>
    storage:
      type: kubernetes
      config:
        inCluster: true
    logger:
      format: json
      level: debug
    web:
      http: 0.0.0.0:8080
    grpc:
      addr: 0.0.0.0:8090
      tlsCert: /etc/dex/tls/grpc/server/tls.crt
      tlsKey: /etc/dex/tls/grpc/server/tls.key
      tlsClientCA: /etc/dex/tls/grpc/ca/tls.crt
    connectors:
    - type: saml
      id: organization
      name: ORGANIZATION
      config:
        ssoURL: https://adfs.<test domain>/adfs/ls/idpinitiatedSignOn.aspx
        redirectURI: https://dex-ad.<domain>/callback
        entityIssuer: https://dex-ad.<domain>/callback
        usernameAttr: name
        emailAttr: email
        caData: <REDACTED>
      insecureSkipSignatureValidation: false

```

When attempting to test the authentication, the following happens:

browse to site login
forwards to dex login
select <LAB> login button
presents credential login page/ credentials entered
wait for about a minutes
presented with 502 gateway error from ingress controller. URL is callback url: `https://dex-ad.<domain>/callback`

dex logs show output from above.

Pull request:
saml connector: fix nil pointer PANIC on validate saml
#### Overview
handle empty data to avoid panics

#### What this PR does / why we need it

"Closes #1757"


#### Special notes for your reviewer
None

Patch:
diff --git a/cmd/dex/config.go b/cmd/dex/config.go
index dd6d2e2a..aa49a181 100644
--- a/cmd/dex/config.go
+++ b/cmd/dex/config.go
@@ -284,6 +284,27 @@ func getORMBasedSQLStorage(normal, entBased StorageConfig) func() StorageConfig
 	}
 }
 
+// Recursively expand environment variables in the map to avoid
+// issues with JSON special characters and escapes
+func expandEnvInMap(m map[string]interface{}) {
+	for k, v := range m {
+		switch vt := v.(type) {
+		case string:
+			m[k] = os.ExpandEnv(vt)
+		case map[string]interface{}:
+			expandEnvInMap(vt)
+		case []interface{}:
+			for i, item := range vt {
+				if itemMap, ok := item.(map[string]interface{}); ok {
+					expandEnvInMap(itemMap)
+				} else if itemString, ok := item.(string); ok {
+					vt[i] = os.ExpandEnv(itemString)
+				}
+			}
+		}
+	}
+}
+
 var storages = map[string]func() StorageConfig{
 	"etcd":       func() StorageConfig { return new(etcd.Etcd) },
 	"kubernetes": func() StorageConfig { return new(kubernetes.Config) },
@@ -312,9 +333,24 @@ func (s *Storage) UnmarshalJSON(b []byte) error {
 	if len(store.Config) != 0 {
 		data := []byte(store.Config)
 		if featureflags.ExpandEnv.Enabled() {
-			// 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)))
+			var rawMap map[string]interface{}
+			if err := json.Unmarshal(store.Config, &rawMap); err != nil {
+				return fmt.Errorf("unmarshal config for env expansion: %v", err)
+			}
+
+			// Recursively expand environment variables in the map to avoid
+			// issues with JSON special characters and escapes
+			expandEnvInMap(rawMap)
+
+			// Marshal the expanded map back to JSON
+			expandedData, err := json.Marshal(rawMap)
+			if err != nil {
+				return fmt.Errorf("marshal expanded config: %v", err)
+			}
+
+			data = expandedData
 		}
+
 		if err := json.Unmarshal(data, storageConfig); err != nil {
 			return fmt.Errorf("parse storage config: %v", err)
 		}
@@ -358,13 +394,29 @@ func (c *Connector) UnmarshalJSON(b []byte) error {
 	if len(conn.Config) != 0 {
 		data := []byte(conn.Config)
 		if featureflags.ExpandEnv.Enabled() {
-			// 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)))
+			var rawMap map[string]interface{}
+			if err := json.Unmarshal(conn.Config, &rawMap); err != nil {
+				return fmt.Errorf("unmarshal config for env expansion: %v", err)
+			}
+
+			// Recursively expand environment variables in the map to avoid
+			// issues with JSON special characters and escapes
+			expandEnvInMap(rawMap)
+
+			// Marshal the expanded map back to JSON
+			expandedData, err := json.Marshal(rawMap)
+			if err != nil {
+				return fmt.Errorf("marshal expanded config: %v", err)
+			}
+
+			data = expandedData
 		}
+
 		if err := json.Unmarshal(data, connConfig); err != nil {
 			return fmt.Errorf("parse connector config: %v", err)
 		}
 	}
+
 	*c = Connector{
 		Type:   conn.Type,
 		Name:   conn.Name,
diff --git a/connector/saml/saml.go b/connector/saml/saml.go
index 1ab8e544..bc8ef726 100644
--- a/connector/saml/saml.go
+++ b/connector/saml/saml.go
@@ -597,6 +597,9 @@ func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signed [
 	}
 
 	response := doc.Root()
+	if response == nil {
+		return nil, false, fmt.Errorf("parse document: empty root")
+	}
 	transformedResponse, err := validator.Validate(response)
 	if err == nil {
 		// Root element is verified, return it.
@@ -609,7 +612,7 @@ func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signed [
 	//
 	// TODO: Only select from child elements of the root.
 	assertion, err := etreeutils.NSSelectOne(response, "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion")
-	if err != nil {
+	if err != nil || assertion == nil {
 		return nil, false, fmt.Errorf("response does not contain an Assertion element")
 	}
 	transformedAssertion, err := validator.Validate(assertion)


Test patch:
diff --git a/cmd/dex/config_test.go b/cmd/dex/config_test.go
index c6d37cb0..68abe1f7 100644
--- a/cmd/dex/config_test.go
+++ b/cmd/dex/config_test.go
@@ -273,7 +273,8 @@ func checkUnmarshalConfigWithEnv(t *testing.T, dexExpandEnv string, wantExpandEn
 	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")
+	os.Setenv("DEX_FOO_POSTGRES_PASSWORD", `psql"test\pass`)
+	os.Setenv("DEX_FOO_OIDC_CLIENT_SECRET", `abc"def\ghi`)
 	if dexExpandEnv != "UNSET" {
 		os.Setenv("DEX_EXPAND_ENV", dexExpandEnv)
 	} else {
@@ -288,6 +289,7 @@ storage:
     # 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'
+    password: '$DEX_FOO_POSTGRES_PASSWORD'
     port: 65432
     maxOpenConns: 5
     maxIdleConns: 3
@@ -350,10 +352,12 @@ logger:
 
 	// This is not a valid hostname. It's only used to check whether os.ExpandEnv was applied or not.
 	wantPostgresHost := "$DEX_FOO_POSTGRES_HOST"
+	wantPostgresPassword := "$DEX_FOO_POSTGRES_PASSWORD"
 	wantOidcClientSecret := "$DEX_FOO_OIDC_CLIENT_SECRET"
 	if wantExpandEnv {
 		wantPostgresHost = "10.0.0.1"
-		wantOidcClientSecret = "bar"
+		wantPostgresPassword = `psql"test\pass`
+		wantOidcClientSecret = `abc"def\ghi`
 	}
 
 	want := Config{
@@ -363,6 +367,7 @@ logger:
 			Config: &sql.Postgres{
 				NetworkDB: sql.NetworkDB{
 					Host:              wantPostgresHost,
+					Password:          wantPostgresPassword,
 					Port:              65432,
 					MaxOpenConns:      5,
 					MaxIdleConns:      3,