Merge branch 'main' into configtls-add-newdefault-funcs

Author: TylerHelmuth

.chloggen/configtls-fix-IncludeSystemCACertsPool-bug.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: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
+component: configtls
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix issue where `IncludeSystemCACertsPool` was not consistently used between `ServerConfig` and `ClientConfig`.
+
+# One or more tracking issues or pull requests related to the change
+issues: [9835]
+
+# (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: []

.github/workflows/build-and-test.yml

@@ -216,7 +216,7 @@ jobs:
       - name: Run Unit Tests With Coverage
         run: make gotest-with-cover
       - name: Upload coverage report
-        uses: Wandalen/wretry.action@1a10d4835a1506f513ad8e7488aeb474ab20055c # v1.4.10
+        uses: Wandalen/wretry.action@f062299947903c6e425ff67d6a93f52066e4ee1c # v2.1.0
         with:
           action: codecov/codecov-action@v3
           with: |

.github/workflows/codeql-analysis.yml

@@ -30,12 +30,12 @@ jobs:
 
       # Initializes the CodeQL tools for scanning.
       - name: Initialize CodeQL
-        uses: github/codeql-action/init@05963f47d870e2cb19a537396c1f668a348c7d8f # v3.24.8
+        uses: github/codeql-action/init@1b1aada464948af03b950897e5eb522f92603cc2 # v3.24.9
         with:
           languages: go
 
       - name: Autobuild
-        uses: github/codeql-action/autobuild@05963f47d870e2cb19a537396c1f668a348c7d8f # v3.24.8
+        uses: github/codeql-action/autobuild@1b1aada464948af03b950897e5eb522f92603cc2 # v3.24.9
 
       - name: Perform CodeQL Analysis
-        uses: github/codeql-action/analyze@05963f47d870e2cb19a537396c1f668a348c7d8f # v3.24.8
+        uses: github/codeql-action/analyze@1b1aada464948af03b950897e5eb522f92603cc2 # v3.24.9

.github/workflows/scorecard.yml

@@ -64,6 +64,6 @@ jobs:
 
       # Upload the results to GitHub's code scanning dashboard.
       - name: "Upload to code-scanning"
-        uses: github/codeql-action/upload-sarif@05963f47d870e2cb19a537396c1f668a348c7d8f # v3.24.8
+        uses: github/codeql-action/upload-sarif@1b1aada464948af03b950897e5eb522f92603cc2 # v3.24.9
         with:
           sarif_file: results.sarif

config/configtls/configtls.go

@@ -193,6 +193,10 @@ func (r *certReloader) GetCertificate() (*tls.Certificate, error) {
 }
 
 func (c TLSSetting) Validate() error {
+	if c.hasCAFile() && c.hasCAPem() {
+		return fmt.Errorf("provide either a CA file or the PEM-encoded string, but not both")
+	}
+
 	minTLS, err := convertVersion(c.MinVersion, defaultMinTLSVersion)
 	if err != nil {
 		return fmt.Errorf("invalid TLS min_version: %w", err)
@@ -309,6 +313,15 @@ func (c Config) loadCertFile(certPath string) (*x509.CertPool, error) {
 
 func (c Config) loadCertPem(certPem []byte) (*x509.CertPool, error) {
 	certPool := x509.NewCertPool()
+	if c.IncludeSystemCACertsPool {
+		scp, err := systemCertPool()
+		if err != nil {
+			return nil, err
+		}
+		if scp != nil {
+			certPool = scp
+		}
+	}
 	if !certPool.AppendCertsFromPEM(certPem) {
 		return nil, fmt.Errorf("failed to parse cert")
 	}

config/configtls/configtls_test.go

@@ -657,30 +657,25 @@ func TestMinMaxTLSVersions(t *testing.T) {
 
 func TestTLSSettingValidate(t *testing.T) {
 	tests := []struct {
-		name       string
-		minVersion string
-		maxVersion string
-		errorTxt   string
+		name      string
+		tlsConfig TLSSetting
+		errorTxt  string
 	}{
-		{name: `TLS Config ["", ""] to be valid`, minVersion: "", maxVersion: ""},
-		{name: `TLS Config ["", "1.3"] to be valid`, minVersion: "", maxVersion: "1.3"},
-		{name: `TLS Config ["1.2", ""] to be valid`, minVersion: "1.2", maxVersion: ""},
-		{name: `TLS Config ["1.3", "1.3"] to be valid`, minVersion: "1.3", maxVersion: "1.3"},
-		{name: `TLS Config ["1.0", "1.1"] to be valid`, minVersion: "1.0", maxVersion: "1.1"},
-		{name: `TLS Config ["asd", ""] to give [Error]`, minVersion: "asd", maxVersion: "", errorTxt: `invalid TLS min_version: unsupported TLS version: "asd"`},
-		{name: `TLS Config ["", "asd"] to give [Error]`, minVersion: "", maxVersion: "asd", errorTxt: `invalid TLS max_version: unsupported TLS version: "asd"`},
-		{name: `TLS Config ["0.4", ""] to give [Error]`, minVersion: "0.4", maxVersion: "", errorTxt: `invalid TLS min_version: unsupported TLS version: "0.4"`},
-		{name: `TLS Config ["1.2", "1.1"] to give [Error]`, minVersion: "1.2", maxVersion: "1.1", errorTxt: `invalid TLS configuration: min_version cannot be greater than max_version`},
+		{name: `TLS Config ["", ""] to be valid`, tlsConfig: Config{MinVersion: "", MaxVersion: ""}},
+		{name: `TLS Config ["", "1.3"] to be valid`, tlsConfig: Config{MinVersion: "", MaxVersion: "1.3"}},
+		{name: `TLS Config ["1.2", ""] to be valid`, tlsConfig: Config{MinVersion: "1.2", MaxVersion: ""}},
+		{name: `TLS Config ["1.3", "1.3"] to be valid`, tlsConfig: Config{MinVersion: "1.3", MaxVersion: "1.3"}},
+		{name: `TLS Config ["1.0", "1.1"] to be valid`, tlsConfig: Config{MinVersion: "1.0", MaxVersion: "1.1"}},
+		{name: `TLS Config ["asd", ""] to give [Error]`, tlsConfig: Config{MinVersion: "asd", MaxVersion: ""}, errorTxt: `invalid TLS min_version: unsupported TLS version: "asd"`},
+		{name: `TLS Config ["", "asd"] to give [Error]`, tlsConfig: Config{MinVersion: "", MaxVersion: "asd"}, errorTxt: `invalid TLS max_version: unsupported TLS version: "asd"`},
+		{name: `TLS Config ["0.4", ""] to give [Error]`, tlsConfig: Config{MinVersion: "0.4", MaxVersion: ""}, errorTxt: `invalid TLS min_version: unsupported TLS version: "0.4"`},
+		{name: `TLS Config ["1.2", "1.1"] to give [Error]`, tlsConfig: Config{MinVersion: "1.2", MaxVersion: "1.1"}, errorTxt: `invalid TLS configuration: min_version cannot be greater than max_version`},
+		{name: `TLS Config with both CA File and PEM`, tlsConfig: Config{CAFile: "test", CAPem: "test"}, errorTxt: `provide either a CA file or the PEM-encoded string, but not both`},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			setting := TLSSetting{
-				MinVersion: test.minVersion,
-				MaxVersion: test.maxVersion,
-			}
-
-			err := setting.Validate()
+			err := test.tlsConfig.Validate()
 
 			if test.errorTxt == "" {
 				assert.Nil(t, err)
@@ -741,6 +736,86 @@ invalid TLS cipher suite: "BAR"`,
 }
 
 func TestSystemCertPool(t *testing.T) {
+	anError := errors.New("my error")
+	tests := []struct {
+		name         string
+		tlsSetting   TLSSetting
+		wantErr      error
+		systemCertFn func() (*x509.CertPool, error)
+	}{
+		{
+			name: "not using system cert pool",
+			tlsSetting: TLSSetting{
+				IncludeSystemCACertsPool: false,
+				CAFile:                   filepath.Join("testdata", "ca-1.crt"),
+			},
+			wantErr:      nil,
+			systemCertFn: x509.SystemCertPool,
+		},
+		{
+			name: "using system cert pool",
+			tlsSetting: TLSSetting{
+				IncludeSystemCACertsPool: true,
+				CAFile:                   filepath.Join("testdata", "ca-1.crt"),
+			},
+			wantErr:      nil,
+			systemCertFn: x509.SystemCertPool,
+		},
+		{
+			name: "error loading system cert pool",
+			tlsSetting: TLSSetting{
+				IncludeSystemCACertsPool: true,
+				CAFile:                   filepath.Join("testdata", "ca-1.crt"),
+			},
+			wantErr: anError,
+			systemCertFn: func() (*x509.CertPool, error) {
+				return nil, anError
+			},
+		},
+		{
+			name: "nil system cert pool",
+			tlsSetting: TLSSetting{
+				IncludeSystemCACertsPool: true,
+				CAFile:                   filepath.Join("testdata", "ca-1.crt"),
+			},
+			wantErr: nil,
+			systemCertFn: func() (*x509.CertPool, error) {
+				return nil, nil
+			},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			oldSystemCertPool := systemCertPool
+			systemCertPool = test.systemCertFn
+			defer func() {
+				systemCertPool = oldSystemCertPool
+			}()
+
+			serverConfig := ServerConfig{
+				TLSSetting: test.tlsSetting,
+			}
+			c, err := serverConfig.LoadTLSConfig()
+			if test.wantErr != nil {
+				assert.ErrorContains(t, err, test.wantErr.Error())
+			} else {
+				assert.NotNil(t, c.RootCAs)
+			}
+
+			clientConfig := ClientConfig{
+				TLSSetting: test.tlsSetting,
+			}
+			c, err = clientConfig.LoadTLSConfig()
+			if test.wantErr != nil {
+				assert.ErrorContains(t, err, test.wantErr.Error())
+			} else {
+				assert.NotNil(t, c.RootCAs)
+			}
+		})
+	}
+}
+
+func TestSystemCertPool_loadCert(t *testing.T) {
 	anError := errors.New("my error")
 	tests := []struct {
 		name         string

confmap/converter/expandconverter/expand.go

@@ -5,51 +5,71 @@ package expandconverter // import "go.opentelemetry.io/collector/confmap/convert
 
 import (
 	"context"
+	"fmt"
 	"os"
+	"regexp"
+
+	"go.uber.org/zap"
 
 	"go.opentelemetry.io/collector/confmap"
 )
 
-type converter struct{}
+type converter struct {
+	logger *zap.Logger
+
+	// Record of which env vars we have logged a warning for
+	loggedDeprecations map[string]struct{}
+}
 
 // New returns a confmap.Converter, that expands all environment variables for a given confmap.Conf.
 //
 // Notice: This API is experimental.
-func New(confmap.ConverterSettings) confmap.Converter {
-	return converter{}
+func New(_ confmap.ConverterSettings) confmap.Converter {
+	return converter{
+		loggedDeprecations: make(map[string]struct{}),
+		logger:             zap.NewNop(), // TODO: pass logger in ConverterSettings
+	}
 }
 
-func (converter) Convert(_ context.Context, conf *confmap.Conf) error {
+func (c converter) Convert(_ context.Context, conf *confmap.Conf) error {
 	out := make(map[string]any)
 	for _, k := range conf.AllKeys() {
-		out[k] = expandStringValues(conf.Get(k))
+		out[k] = c.expandStringValues(conf.Get(k))
 	}
 	return conf.Merge(confmap.NewFromStringMap(out))
 }
 
-func expandStringValues(value any) any {
+func (c converter) expandStringValues(value any) any {
 	switch v := value.(type) {
 	case string:
-		return expandEnv(v)
+		return c.expandEnv(v)
 	case []any:
 		nslice := make([]any, 0, len(v))
 		for _, vint := range v {
-			nslice = append(nslice, expandStringValues(vint))
+			nslice = append(nslice, c.expandStringValues(vint))
 		}
 		return nslice
 	case map[string]any:
 		nmap := map[string]any{}
 		for mk, mv := range v {
-			nmap[mk] = expandStringValues(mv)
+			nmap[mk] = c.expandStringValues(mv)
 		}
 		return nmap
 	default:
 		return v
 	}
 }
 
-func expandEnv(s string) string {
+func (c converter) expandEnv(s string) string {
 	return 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)))
+		if _, exists := c.loggedDeprecations[str]; !exists && regex.MatchString(s) {
+			msg := fmt.Sprintf("Variable substitution using $VAR will be deprecated in favor of ${VAR} and ${env:VAR}, please update $%s", str)
+			c.logger.Warn(msg, zap.String("variable", str))
+			c.loggedDeprecations[str] = struct{}{}
+		}
 		// This allows escaping environment variable substitution via $$, e.g.
 		// - $FOO will be substituted with env var FOO
 		// - $$FOO will be replaced with $FOO