Fix keystore secrets parsing when values contain commas. (#50)

Author: cmacknz

.gitignore

@@ -0,0 +1 @@
+build/

go.mod

@@ -5,7 +5,7 @@ go 1.17
 require (
 	github.com/Microsoft/go-winio v0.5.2
 	github.com/elastic/go-structform v0.0.9
-	github.com/elastic/go-ucfg v0.8.4
+	github.com/elastic/go-ucfg v0.8.5
 	github.com/fatih/color v1.13.0
 	github.com/hashicorp/go-multierror v1.1.1
 	github.com/joeshaw/multierror v0.0.0-20140124173710-69b34d4ec901

go.sum

@@ -104,8 +104,8 @@ github.com/elastic/go-structform v0.0.9 h1:HpcS7xljL4kSyUfDJ8cXTJC6rU5ChL1wYb6cx
 github.com/elastic/go-structform v0.0.9/go.mod h1:CZWf9aIRYY5SuKSmOhtXScE5uQiLZNqAFnwKR4OrIM4=
 github.com/elastic/go-sysinfo v1.7.1 h1:Wx4DSARcKLllpKT2TnFVdSUJOsybqMYCNQZq1/wO+s0=
 github.com/elastic/go-sysinfo v1.7.1/go.mod h1:i1ZYdU10oLNfRzq4vq62BEwD2fH8KaWh6eh0ikPT9F0=
-github.com/elastic/go-ucfg v0.8.4 h1:OAHTnubzXKsYYYWVzl8psLcS5mCbNKjXxtMY41itthk=
-github.com/elastic/go-ucfg v0.8.4/go.mod h1:4E8mPOLSUV9hQ7sgLEJ4bvt0KhMuDJa8joDT2QGAEKA=
+github.com/elastic/go-ucfg v0.8.5 h1:4GB/rMpuh7qTcSFaxJUk97a/JyvFzhi6t+kaskTTLdM=
+github.com/elastic/go-ucfg v0.8.5/go.mod h1:4E8mPOLSUV9hQ7sgLEJ4bvt0KhMuDJa8joDT2QGAEKA=
 github.com/elastic/go-windows v1.0.0/go.mod h1:TsU0Nrp7/y3+VwE82FoZF8gC/XFg/Elz6CcloAxnPgU=
 github.com/elastic/go-windows v1.0.1 h1:AlYZOldA+UJ0/2nBuqWdo90GFCgG9xuyw9SYzGUtJm0=
 github.com/elastic/go-windows v1.0.1/go.mod h1:FoVvqWSun28vaDQPbj2Elfc0JahhPB7WQEGa3c814Ss=

keystore/file_keystore_test.go

@@ -31,7 +31,10 @@ import (
 )
 
 var keyValue = "output.elasticsearch.password"
-var secretValue = []byte("secret")
+
+// Include uppercase, lowercase, symbols, and whitespace in the password.
+// Commas in particular have caused parsing issues before: https://github.com/elastic/beats/issues/29789
+var secretValue = []byte(",s3cRet~`! @#$%^&*()_-+={[}]|\\:;\"'<,>.?/")
 
 func TestCanCreateAKeyStore(t *testing.T) {
 	path := GetTemporaryKeystoreFile()
@@ -229,7 +232,7 @@ func TestGetConfig(t *testing.T) {
 
 	secret, err := cfg.String("output.elasticsearch.password", 0)
 	require.NoError(t, err)
-	require.Equal(t, secret, "secret")
+	require.Equal(t, string(secretValue), secret)
 
 	port, err := cfg.String("super.nested", 0)
 	require.NoError(t, err)

keystore/keystore.go

@@ -73,6 +73,9 @@ type ListingKeystore interface {
 	List() ([]string, error)
 }
 
+// Use parse.NoopConfig to disable interpreting all parser characters when loading secrets.
+var parseConfig = parse.NoopConfig
+
 // ResolverWrap wrap a config resolver around an existing keystore.
 func ResolverWrap(keystore Keystore) func(string) (string, parse.Config, error) {
 	return func(keyName string) (string, parse.Config, error) {
@@ -82,17 +85,17 @@ func ResolverWrap(keystore Keystore) func(string) (string, parse.Config, error)
 			// If we cannot find the key, its a non fatal error
 			// and we pass to other resolver.
 			if errors.Is(err, ErrKeyDoesntExists) {
-				return "", parse.DefaultConfig, ucfg.ErrMissing
+				return "", parseConfig, ucfg.ErrMissing
 			}
-			return "", parse.DefaultConfig, err
+			return "", parseConfig, err
 		}
 
 		v, err := key.Get()
 		if err != nil {
-			return "", parse.DefaultConfig, err
+			return "", parseConfig, err
 		}
 
-		return string(v), parse.DefaultConfig, nil
+		return string(v), parseConfig, nil
 	}
 }
 

keystore/keystore_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	ucfg "github.com/elastic/go-ucfg"
 	"github.com/elastic/go-ucfg/parse"
@@ -47,6 +48,18 @@ func TestResolverWhenTheKeyExist(t *testing.T) {
 	resolver := ResolverWrap(keystore)
 	v, pCfg, err := resolver("output.elasticsearch.password")
 	assert.NoError(t, err)
-	assert.Equal(t, pCfg, parse.DefaultConfig)
-	assert.Equal(t, v, "secret")
+	require.NoError(t, err)
+	require.Equal(t, pCfg, parseConfig)
+
+	// Regression test for https://github.com/elastic/beats/issues/29789
+	// Cheat a bit by reproducing part of the go-ucfg dynamic variable resolution process here. The
+	// config returned by the resolver will be used with a call to the go-ucfg parser. The
+	// public entrypoint is the ValueWithConfig function below. Make sure the parsed value is
+	// correct. See https://github.com/elastic/go-ucfg/blob/fc880abbe1f30b653d113da96a4a7e82743c0cc1/types.go#L539
+	iface, err := parse.ValueWithConfig(v, pCfg)
+	require.NoError(t, err)
+
+	secret, ok := iface.(string)
+	require.True(t, ok, "parsed secret is not a string")
+	require.Equal(t, string(secretValue), secret)
 }