Enhances environment variable expansion in configuration files. (#1389)

- Variable expressions cannot be split across newlines.
- Unterminated variable expressions cause errors rather than silently swallowing characters.
- Added an escape sequence for using a literal '${' value.

Follow up to issue #1304

CHANGELOG.asciidoc

@@ -36,6 +36,7 @@ https://github.com/elastic/beats/compare/v5.0.0-alpha1...master[Check the HEAD d
 
 *Affecting all Beats*
 - Drain response buffers when pipelining is used by redis output. {pull}1353[1353]
+- Unterminated environment variable expressions in config files will now cause an error {pull}1389[1389]
 
 *Packetbeat*
 
@@ -52,6 +53,7 @@ https://github.com/elastic/beats/compare/v5.0.0-alpha1...master[Check the HEAD d
 - Add SOCKS5 proxy support to redis output. {pull}1353[1353]
 - Failover and load balancing support in redis output. {pull}1353[1353]
 - Multiple-worker per host support for redis output. {pull}1353[1353]
+- Added ability to escape `${x}` in config files to avoid environment variable expansion {pull}1389[1389]
 
 *Packetbeat*
 

libbeat/cfgfile/cfgfile.go

@@ -6,10 +6,8 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"github.com/elastic/beats/libbeat/common"
-	"github.com/elastic/beats/libbeat/logp"
 )
 
 // Command line flags.
@@ -67,7 +65,10 @@ func Load(path string) (*common.Config, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failed to read %s: %v", path, err)
 	}
-	fileContent = expandEnv(fileContent)
+	fileContent, err = expandEnv(filepath.Base(path), fileContent)
+	if err != nil {
+		return nil, err
+	}
 
 	config, err := common.NewConfigWithYAML(fileContent, path)
 	if err != nil {
@@ -81,89 +82,3 @@ func Load(path string) (*common.Config, error) {
 func IsTestConfig() bool {
 	return *testConfig
 }
-
-// expandEnv replaces ${var} in config according to the values of the current
-// environment variables. The replacement is case-sensitive. References to
-// undefined variables are replaced by the empty string. A default value can be
-// given by using the form ${var:default value}.
-func expandEnv(config []byte) []byte {
-	return []byte(expand(string(config), func(key string) string {
-		keyAndDefault := strings.SplitN(key, ":", 2)
-		key = keyAndDefault[0]
-
-		v := os.Getenv(key)
-		if v == "" && len(keyAndDefault) == 2 {
-			// Set value to the default.
-			v = keyAndDefault[1]
-			logp.Info("Replacing config environment variable '${%s}' with "+
-				"default '%s'", key, keyAndDefault[1])
-		} else {
-			logp.Info("Replacing config environment variable '${%s}' with '%s'",
-				key, v)
-		}
-
-		return v
-	}))
-}
-
-// The following methods were copied from the os package of the stdlib. The
-// expand method was modified to only expand variables defined with braces and
-// ignore $var.
-
-// Expand replaces ${var} in the string based on the mapping function.
-func expand(s string, mapping func(string) string) string {
-	buf := make([]byte, 0, 2*len(s))
-	// ${} is all ASCII, so bytes are fine for this operation.
-	i := 0
-	for j := 0; j < len(s); j++ {
-		if s[j] == '$' && j+2 < len(s) && s[j+1] == '{' {
-			buf = append(buf, s[i:j]...)
-			name, w := getShellName(s[j+1:])
-			buf = append(buf, mapping(name)...)
-			j += w
-			i = j + 1
-		}
-	}
-	return string(buf) + s[i:]
-}
-
-// isShellSpecialVar reports whether the character identifies a special
-// shell variable such as $*.
-func isShellSpecialVar(c uint8) bool {
-	switch c {
-	case '*', '#', '$', '@', '!', '?', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9':
-		return true
-	}
-	return false
-}
-
-// isAlphaNum reports whether the byte is an ASCII letter, number, or underscore
-func isAlphaNum(c uint8) bool {
-	return c == '_' || '0' <= c && c <= '9' || 'a' <= c && c <= 'z' || 'A' <= c && c <= 'Z'
-}
-
-// getShellName returns the name that begins the string and the number of bytes
-// consumed to extract it.  If the name is enclosed in {}, it's part of a ${}
-// expansion and two more bytes are needed than the length of the name.
-func getShellName(s string) (string, int) {
-	switch {
-	case s[0] == '{':
-		if len(s) > 2 && isShellSpecialVar(s[1]) && s[2] == '}' {
-			return s[1:2], 3
-		}
-		// Scan to closing brace
-		for i := 1; i < len(s); i++ {
-			if s[i] == '}' {
-				return s[1:i], i + 1
-			}
-		}
-		return "", 1 // Bad syntax; just eat the brace.
-	case isShellSpecialVar(s[0]):
-		return s[0:1], 1
-	}
-	// Scan alphanumerics.
-	var i int
-	for i = 0; i < len(s) && isAlphaNum(s[i]); i++ {
-	}
-	return s[:i], i
-}

libbeat/cfgfile/cfgfile_test.go

@@ -45,37 +45,61 @@ func TestExpandEnv(t *testing.T) {
 	var tests = []struct {
 		in  string
 		out string
+		err string
 	}{
 		// Environment variables can be specified as ${env} only.
-		{"${y}", "y"},
-		{"$y", "$y"},
+		{"${y}", "y", ""},
+		{"$y", "$y", ""},
 
 		// Environment variables are case-sensitive.
-		{"${Y}", ""},
+		{"${Y}", "", ""},
 
 		// Defaults can be specified.
-		{"x${Z:D}", "xD"},
-		{"x${Z:A B C D}", "xA B C D"}, // Spaces are allowed in the default.
-		{"x${Z:}", "x"},
+		{"x${Z:D}", "xD", ""},
+		{"x${Z:A B C D}", "xA B C D", ""}, // Spaces are allowed in the default.
+		{"x${Z:}", "x", ""},
 
-		// Un-matched braces are swallowed by the Go os.Expand function.
-		{"x${Y ${Z:Z}", "xZ"},
+		// Un-matched braces cause an error.
+		{"x${Y ${Z:Z}", "", "unexpected character in variable expression: " +
+			"U+0020 ' ', expected a default value or closing brace"},
 
 		// Special environment variables are not replaced.
-		{"$*", "$*"},
-		{"${*}", ""},
-		{"$@", "$@"},
-		{"${@}", ""},
-		{"$1", "$1"},
-		{"${1}", ""},
-
-		{"", ""},
-		{"$$", "$$"},
+		{"$*", "$*", ""},
+		{"${*}", "", "shell variable cannot start with U+002A '*'"},
+		{"$@", "$@", ""},
+		{"${@}", "", "shell variable cannot start with U+0040 '@'"},
+		{"$1", "$1", ""},
+		{"${1}", "", "shell variable cannot start with U+0031 '1'"},
+
+		{"", "", ""},
+		{"$$", "$$", ""},
+
+		{"${a_b}", "", ""}, // Underscores are allowed in variable names.
+
+		// ${} cannot be split across newlines.
+		{"hello ${name: world\n}", "", "unterminated brace"},
+
+		// To use a literal '${' you write '$${'.
+		{`password: "abc$${!"`, `password: "abc${!"`, ""},
+
+		// The full error contains the line number.
+		{"shipper:\n  name: ${var", "", "failure while expanding environment " +
+			"variables in config.yml at line=2, unterminated brace"},
 	}
 
 	for _, test := range tests {
 		os.Setenv("y", "y")
-		output := expandEnv([]byte(test.in))
-		assert.Equal(t, test.out, string(output), "Input: %s", test.in)
+		output, err := expandEnv("config.yml", []byte(test.in))
+
+		switch {
+		case test.err != "" && err == nil:
+			t.Errorf("Expected an error for test case %+v", test)
+		case test.err == "" && err != nil:
+			t.Errorf("Unexpected error for test case %+v, %v", test, err)
+		case err != nil:
+			assert.Contains(t, err.Error(), test.err)
+		default:
+			assert.Equal(t, test.out, string(output), "Input: %s", test.in)
+		}
 	}
 }