Restrict characters in environment variable names (open-telemetry#10183)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Restricts Environment Variable names according to the
[restrictions](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/file-configuration.md#environment-variable-substitution)
published by the OpenTelemetry Configuration Working Group. Changes both
the expand converter and the env provider. Defines one regex to be used
for both of these.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes
open-telemetry#9531

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit tests

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Pablo Baeyens <[email protected]>

Author: 2 people

.chloggen/env-var-name-verify.yaml

@@ -0,0 +1,25 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: breaking
+
+# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
+component: envprovider
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Restricts Environment Variable names.  Environment variable names must now be ASCII only and start with a letter or an underscore, and can only contain underscores, letters, or numbers.
+
+# One or more tracking issues or pull requests related to the change
+issues: [9531]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
+
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: ['user']

confmap/converter/expandconverter/expand.go

@@ -12,6 +12,7 @@ import (
 	"go.uber.org/zap"
 
 	"go.opentelemetry.io/collector/confmap"
+	"go.opentelemetry.io/collector/confmap/internal/envvar"
 )
 
 type converter struct {
@@ -35,36 +36,50 @@ func newConverter(set confmap.ConverterSettings) confmap.Converter {
 }
 
 func (c converter) Convert(_ context.Context, conf *confmap.Conf) error {
+	var err error
 	out := make(map[string]any)
 	for _, k := range conf.AllKeys() {
-		out[k] = c.expandStringValues(conf.Get(k))
+		out[k], err = c.expandStringValues(conf.Get(k))
+		if err != nil {
+			return err
+		}
 	}
 	return conf.Merge(confmap.NewFromStringMap(out))
 }
 
-func (c converter) expandStringValues(value any) any {
+func (c converter) expandStringValues(value any) (any, error) {
+	var err error
 	switch v := value.(type) {
 	case string:
 		return c.expandEnv(v)
 	case []any:
 		nslice := make([]any, 0, len(v))
 		for _, vint := range v {
-			nslice = append(nslice, c.expandStringValues(vint))
+			var nv any
+			nv, err = c.expandStringValues(vint)
+			if err != nil {
+				return nil, err
+			}
+			nslice = append(nslice, nv)
 		}
-		return nslice
+		return nslice, nil
 	case map[string]any:
 		nmap := map[string]any{}
 		for mk, mv := range v {
-			nmap[mk] = c.expandStringValues(mv)
+			nmap[mk], err = c.expandStringValues(mv)
+			if err != nil {
+				return nil, err
+			}
 		}
-		return nmap
+		return nmap, nil
 	default:
-		return v
+		return v, nil
 	}
 }
 
-func (c converter) expandEnv(s string) string {
-	return os.Expand(s, func(str string) string {
+func (c converter) expandEnv(s string) (string, error) {
+	var err error
+	res := os.Expand(s, func(str string) string {
 		// Matches on $VAR style environment variables
 		// in order to make sure we don't log a warning for ${VAR}
 		var regex = regexp.MustCompile(fmt.Sprintf(`\$%s`, regexp.QuoteMeta(str)))
@@ -80,6 +95,13 @@ func (c converter) expandEnv(s string) string {
 		if str == "$" {
 			return "$"
 		}
+		// For $ENV style environment variables os.Expand returns once it hits a character that isn't an underscore or
+		// an alphanumeric character - so we cannot detect those malformed environment variables.
+		// For ${ENV} style variables we can detect those kinds of env var names!
+		if !envvar.ValidationRegexp.MatchString(str) {
+			err = fmt.Errorf("environment variable %q has invalid name: must match regex %s", str, envvar.ValidationPattern)
+			return ""
+		}
 		val, exists := os.LookupEnv(str)
 		if !exists {
 			c.logger.Warn("Configuration references unset environment variable", zap.String("name", str))
@@ -88,4 +110,5 @@ func (c converter) expandEnv(s string) string {
 		}
 		return val
 	})
+	return res, err
 }

confmap/converter/expandconverter/expand_test.go

@@ -17,6 +17,7 @@ import (
 
 	"go.opentelemetry.io/collector/confmap"
 	"go.opentelemetry.io/collector/confmap/confmaptest"
+	"go.opentelemetry.io/collector/confmap/internal/envvar"
 )
 
 func TestNewExpandConverter(t *testing.T) {
@@ -176,13 +177,16 @@ func TestDeprecatedWarning(t *testing.T) {
 	t.Setenv("PORT", "4317")
 
 	t.Setenv("HOST_NAME", "127.0.0.2")
-	t.Setenv("HOST.NAME", "127.0.0.3")
+	t.Setenv("HOSTNAME", "127.0.0.3")
+
+	t.Setenv("BAD!HOST", "127.0.0.2")
 
 	var testCases = []struct {
 		name             string
 		input            map[string]any
 		expectedOutput   map[string]any
 		expectedWarnings []string
+		expectedError    error
 	}{
 		{
 			name: "no warning",
@@ -193,6 +197,7 @@ func TestDeprecatedWarning(t *testing.T) {
 				"test": "127.0.0.1:4317",
 			},
 			expectedWarnings: []string{},
+			expectedError:    nil,
 		},
 		{
 			name: "one deprecated var",
@@ -203,6 +208,7 @@ func TestDeprecatedWarning(t *testing.T) {
 				"test": "127.0.0.1:4317",
 			},
 			expectedWarnings: []string{"PORT"},
+			expectedError:    nil,
 		},
 		{
 			name: "two deprecated vars",
@@ -213,6 +219,7 @@ func TestDeprecatedWarning(t *testing.T) {
 				"test": "127.0.0.1:4317",
 			},
 			expectedWarnings: []string{"HOST", "PORT"},
+			expectedError:    nil,
 		},
 		{
 			name: "one depracated serveral times",
@@ -225,25 +232,63 @@ func TestDeprecatedWarning(t *testing.T) {
 				"test2": "127.0.0.1",
 			},
 			expectedWarnings: []string{"HOST"},
+			expectedError:    nil,
 		},
 		{
 			name: "one warning",
 			input: map[string]any{
-				"test": "$HOST_NAME,${HOST.NAME}",
+				"test": "$HOST_NAME,${HOSTNAME}",
 			},
 			expectedOutput: map[string]any{
 				"test": "127.0.0.2,127.0.0.3",
 			},
 			expectedWarnings: []string{"HOST_NAME"},
+			expectedError:    nil,
+		},
+		{
+			name: "malformed environment variable",
+			input: map[string]any{
+				"test": "${BAD!HOST}",
+			},
+			expectedOutput: map[string]any{
+				"test": "blah",
+			},
+			expectedWarnings: []string{},
+			expectedError:    fmt.Errorf("environment variable \"BAD!HOST\" has invalid name: must match regex %s", envvar.ValidationRegexp),
+		},
+		{
+			name: "malformed environment variable number",
+			input: map[string]any{
+				"test": "${2BADHOST}",
+			},
+			expectedOutput: map[string]any{
+				"test": "blah",
+			},
+			expectedWarnings: []string{},
+			expectedError:    fmt.Errorf("environment variable \"2BADHOST\" has invalid name: must match regex %s", envvar.ValidationRegexp),
+		},
+		{
+			name: "malformed environment variable unicode",
+			input: map[string]any{
+				"test": "${😊BADHOST}",
+			},
+			expectedOutput: map[string]any{
+				"test": "blah",
+			},
+			expectedWarnings: []string{},
+			expectedError:    fmt.Errorf("environment variable \"😊BADHOST\" has invalid name: must match regex %s", envvar.ValidationRegexp),
 		},
 	}
+
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
 			conf := confmap.NewFromStringMap(tt.input)
 			conv, logs := NewTestConverter()
-			require.NoError(t, conv.Convert(context.Background(), conf))
-
-			assert.Equal(t, tt.expectedOutput, conf.ToStringMap())
+			err := conv.Convert(context.Background(), conf)
+			assert.Equal(t, tt.expectedError, err)
+			if tt.expectedError == nil {
+				assert.Equal(t, tt.expectedOutput, conf.ToStringMap())
+			}
 			assert.Equal(t, len(tt.expectedWarnings), len(logs.All()))
 			for i, variable := range tt.expectedWarnings {
 				errorMsg := fmt.Sprintf(msgTemplate, variable)
@@ -253,6 +298,38 @@ func TestDeprecatedWarning(t *testing.T) {
 	}
 }
 
+func TestNewExpandConverterWithErrors(t *testing.T) {
+	var testCases = []struct {
+		name          string // test case name (also file name containing config yaml)
+		expectedError error
+	}{
+		{
+			name:          "expand-list-error.yaml",
+			expectedError: fmt.Errorf("environment variable \"EXTRA_LIST_^VALUE_2\" has invalid name: must match regex %s", envvar.ValidationRegexp),
+		},
+		{
+			name:          "expand-list-map-error.yaml",
+			expectedError: fmt.Errorf("environment variable \"EXTRA_LIST_MAP_V#ALUE_2\" has invalid name: must match regex %s", envvar.ValidationRegexp),
+		},
+		{
+			name:          "expand-map-error.yaml",
+			expectedError: fmt.Errorf("environment variable \"EX#TRA\" has invalid name: must match regex %s", envvar.ValidationRegexp),
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			conf, err := confmaptest.LoadConf(filepath.Join("testdata", "errors", test.name))
+			require.NoError(t, err, "Unable to get config")
+
+			// Test that expanded configs are the same with the simple config with no env vars.
+			err = createConverter().Convert(context.Background(), conf)
+
+			assert.Equal(t, test.expectedError, err)
+		})
+	}
+}
+
 func createConverter() confmap.Converter {
 	return NewFactory().Create(confmap.ConverterSettings{Logger: zap.NewNop()})
 }

confmap/converter/expandconverter/testdata/errors/expand-list-error.yaml

@@ -0,0 +1,4 @@
+test_map:
+  extra_list:
+    - "some list value_1"
+    - "${EXTRA_LIST_^VALUE_2}"

confmap/converter/expandconverter/testdata/errors/expand-list-map-error.yaml

@@ -0,0 +1,3 @@
+test_map:
+  extra_list_map:
+    - { recv.1: "some list map value_1",recv.2: "${EXTRA_LIST_MAP_V#ALUE_2}" }

confmap/converter/expandconverter/testdata/errors/expand-map-error.yaml

@@ -0,0 +1,2 @@
+test_map:
+  extra: "${EX#TRA}"

confmap/internal/envvar/pattern.go

@@ -0,0 +1,10 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package envvar // import "go.opentelemetry.io/collector/confmap/internal/envvar"
+
+import "regexp"
+
+const ValidationPattern = `^[a-zA-Z_][a-zA-Z0-9_]*$`
+
+var ValidationRegexp = regexp.MustCompile(ValidationPattern)

confmap/provider/envprovider/provider.go

@@ -12,10 +12,13 @@ import (
 	"go.uber.org/zap"
 
 	"go.opentelemetry.io/collector/confmap"
+	"go.opentelemetry.io/collector/confmap/internal/envvar"
 	"go.opentelemetry.io/collector/confmap/provider/internal"
 )
 
-const schemeName = "env"
+const (
+	schemeName = "env"
+)
 
 type provider struct {
 	logger *zap.Logger
@@ -40,6 +43,10 @@ func (emp *provider) Retrieve(_ context.Context, uri string, _ confmap.WatcherFu
 		return nil, fmt.Errorf("%q uri is not supported by %q provider", uri, schemeName)
 	}
 	envVarName := uri[len(schemeName)+1:]
+	if !envvar.ValidationRegexp.MatchString(envVarName) {
+		return nil, fmt.Errorf("environment variable %q has invalid name: must match regex %s", envVarName, envvar.ValidationPattern)
+
+	}
 	val, exists := os.LookupEnv(envVarName)
 	if !exists {
 		emp.logger.Warn("Configuration references unset environment variable", zap.String("name", envVarName))

confmap/provider/envprovider/provider_test.go

@@ -5,6 +5,7 @@ package envprovider
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -14,6 +15,7 @@ import (
 
 	"go.opentelemetry.io/collector/confmap"
 	"go.opentelemetry.io/collector/confmap/confmaptest"
+	"go.opentelemetry.io/collector/confmap/internal/envvar"
 )
 
 const envSchemePrefix = schemeName + ":"
@@ -54,7 +56,7 @@ func TestInvalidYAML(t *testing.T) {
 }
 
 func TestEnv(t *testing.T) {
-	const envName = "default-config"
+	const envName = "default_config"
 	t.Setenv(envName, validYAML)
 
 	env := createProvider()
@@ -72,7 +74,7 @@ func TestEnv(t *testing.T) {
 }
 
 func TestEnvWithLogger(t *testing.T) {
-	const envName = "default-config"
+	const envName = "default_config"
 	t.Setenv(envName, validYAML)
 	core, ol := observer.New(zap.WarnLevel)
 	logger := zap.New(core)
@@ -93,7 +95,7 @@ func TestEnvWithLogger(t *testing.T) {
 }
 
 func TestUnsetEnvWithLoggerWarn(t *testing.T) {
-	const envName = "default-config"
+	const envName = "default_config"
 	core, ol := observer.New(zap.WarnLevel)
 	logger := zap.New(core)
 
@@ -114,8 +116,19 @@ func TestUnsetEnvWithLoggerWarn(t *testing.T) {
 	assert.Equal(t, envName, logLine.Context[0].String)
 }
 
+func TestEnvVarNameRestriction(t *testing.T) {
+	const envName = "default%config"
+	t.Setenv(envName, validYAML)
+
+	env := createProvider()
+	ret, err := env.Retrieve(context.Background(), envSchemePrefix+envName, nil)
+	assert.Equal(t, err, fmt.Errorf("environment variable \"default%%config\" has invalid name: must match regex %s", envvar.ValidationRegexp))
+	assert.NoError(t, env.Shutdown(context.Background()))
+	assert.Nil(t, ret)
+}
+
 func TestEmptyEnvWithLoggerWarn(t *testing.T) {
-	const envName = "default-config"
+	const envName = "default_config"
 	t.Setenv(envName, "")
 
 	core, ol := observer.New(zap.InfoLevel)