fix: Restore credentials after config env substitution, fix #102

ParseConfig applies environment substitution on the config file being parsed. This interferes with
credential parsing, which may contain valid `$` symbols, and messes up credentials otherwise
perfectly valid credentials (we should base-64 encode credentials at some point). This fix restores
the credentials after env substitution to the original ones.

config_test.go

@@ -501,6 +501,48 @@ func TestStunnerURIParser(t *testing.T) {
 	}
 }
 
+// make sure credentials are excempt from env-substitution in ParseConfig
+func TestCredentialParser(t *testing.T) {
+	lim := test.TimeOut(time.Second * 30)
+	defer lim.Stop()
+
+	report := test.CheckRoutines(t)
+	defer report()
+
+	loggerFactory := logger.NewLoggerFactory(stunnerTestLoglevel)
+	log := loggerFactory.NewLogger("test")
+
+	for _, testConf := range []struct {
+		name               string
+		config             []byte
+		user, pass, secret string
+	}{
+		{"plain", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"password":"pass","username":"user"}}}`), "user", "pass", ""},
+		// user name with $
+		{"username_with_leading_$", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"password":"pass","username":"$user"}}}`), "$user", "pass", ""},
+		{"username_with_trailing_$", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"password":"pass","username":"user$"}}}`), "user$", "pass", ""},
+		{"username_with_$", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"password":"pass","username":"us$er"}}}`), "us$er", "pass", ""},
+		// passwd with $
+		{"passwd_with_leading_$", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"password":"$pass","username":"user"}}}`), "user", "$pass", ""},
+		{"passwd_with_trailing_$", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"password":"pass$","username":"user"}}}`), "user", "pass$", ""},
+		{"passwd_with_$", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"password":"pa$ss","username":"user"}}}`), "user", "pa$ss", ""},
+		// secret with $
+		{"secret_with_leading_$", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"secret":"$secret","username":"user"}}}`), "user", "", "$secret"},
+		{"secret_with_trailing_$", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"secret":"secret$","username":"user"}}}`), "user", "", "secret$"},
+		{"secret_with_$", []byte(`{"version":"v1","admin":{"name":"ns1/tester"},"auth":{"type":"static","credentials":{"secret":"sec$ret","username":"user"}}}`), "user", "", "sec$ret"},
+	} {
+		testName := fmt.Sprintf("TestCredentialParser:%s", testConf.name)
+		t.Run(testName, func(t *testing.T) {
+			log.Debugf("-------------- Running test: %s -------------", testName)
+			c, err := cdsclient.ParseConfig(testConf.config)
+			assert.NoError(t, err, "parser")
+			assert.Equal(t, testConf.user, c.Auth.Credentials["username"], "username")
+			assert.Equal(t, testConf.pass, c.Auth.Credentials["password"], "password")
+			assert.Equal(t, testConf.secret, c.Auth.Credentials["secret"], "secret")
+		})
+	}
+}
+
 func checkDefaultConfig(t *testing.T, c *stnrv1.StunnerConfig, proto string) {
 	assert.Equal(t, "static", c.Auth.Type, "auth-type")
 	assert.Equal(t, "user1", c.Auth.Credentials["username"], "username")

pkg/config/client/config.go

@@ -3,6 +3,7 @@ package client
 import (
 	"encoding/json"
 	"fmt"
+	"maps"
 	"os"
 	"regexp"
 	"strconv"
@@ -54,12 +55,36 @@ func ParseConfig(c []byte) (*stnrv1.StunnerConfig, error) {
 		os.Setenv("STUNNER_PORT", fmt.Sprintf("%d", publicPort))
 	}
 
+	// make sure credentials are not affected by environment substitution
+
+	// parse up before env substitution is applied
+	confRaw, err := parseRaw(c)
+	if err != nil {
+		return nil, err
+	}
+
+	// save credentials
+	credRaw := make(map[string]string)
+	maps.Copy(credRaw, confRaw.Auth.Credentials)
+
+	// apply env substitution and parse again
 	e := os.ExpandEnv(string(c))
+	confExp, err := parseRaw([]byte(e))
+	if err != nil {
+		return nil, err
+	}
+
+	// restore credentials
+	maps.Copy(confExp.Auth.Credentials, credRaw)
+
+	return confExp, nil
+}
 
+func parseRaw(c []byte) (*stnrv1.StunnerConfig, error) {
 	// try to parse only the config version first
 	k := ConfigSkeleton{}
-	if err := yaml.Unmarshal([]byte(e), &k); err != nil {
-		if errJ := json.Unmarshal([]byte(e), &k); err != nil {
+	if err := yaml.Unmarshal([]byte(c), &k); err != nil {
+		if errJ := json.Unmarshal([]byte(c), &k); err != nil {
 			return nil, fmt.Errorf("could not parse config file API version: "+
 				"YAML parse error: %s, JSON parse error: %s\n",
 				err.Error(), errJ.Error())
@@ -70,17 +95,17 @@ func ParseConfig(c []byte) (*stnrv1.StunnerConfig, error) {
 
 	switch k.ApiVersion {
 	case stnrv1.ApiVersion:
-		if err := yaml.Unmarshal([]byte(e), &s); err != nil {
-			if errJ := json.Unmarshal([]byte(e), &s); err != nil {
+		if err := yaml.Unmarshal([]byte(c), &s); err != nil {
+			if errJ := json.Unmarshal([]byte(c), &s); errJ != nil {
 				return nil, fmt.Errorf("could not parse config file: "+
 					"YAML parse error: %s, JSON parse error: %s\n",
 					err.Error(), errJ.Error())
 			}
 		}
 	case stnrv1a1.ApiVersion:
 		a := stnrv1a1.StunnerConfig{}
-		if err := yaml.Unmarshal([]byte(e), &a); err != nil {
-			if errJ := json.Unmarshal([]byte(e), &a); err != nil {
+		if err := yaml.Unmarshal([]byte(c), &a); err != nil {
+			if errJ := json.Unmarshal([]byte(c), &a); errJ != nil {
 				return nil, fmt.Errorf("could not parse config file: "+
 					"YAML parse error: %s, JSON parse error: %s\n",
 					err.Error(), errJ.Error())