From 83ae9bd0e338d6899a61d270ac398e51e154f009 Mon Sep 17 00:00:00 2001 From: Zhihan Li <54661071+zhihali@users.noreply.github.com> Date: Wed, 21 Aug 2024 15:53:24 +0100 Subject: [PATCH] Bugfix: OTLP exporters should not percent decode the key when parsing HEADERS env vars (#5705) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugfix #5623 As stated in the issue, we need to avoid parsing the key and instead implement a validation check for it. I've added some unit tests to verify this fix. However, I noticed a comment at the top of this file: ``` // Code created by gotmpl. DO NOT MODIFY. // source: internal/shared/otlp/envconfig/envconfig.go.tmpl ``` It seems that `internal/shared/otlp/envconfig/envconfig.go.tmpl` is the source template for this file. Since this template matches `exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go`, I updated the template to maintain consistency. I’m not entirely sure if this approach is correct, so please confirm if this is the right course of action. --------- Co-authored-by: Fools <54661071+Charlie-lizhihan@users.noreply.github.com> Co-authored-by: Damien Mathieu <42@dmathieu.com> Co-authored-by: Robert Pająk Co-authored-by: Chester Cheung Co-authored-by: Tyler Yahn --- CHANGELOG.md | 2 + .../internal/envconfig/envconfig.go | 32 +++++++++++-- .../internal/envconfig/envconfig_test.go | 48 +++++++++++++++++++ .../internal/envconfig/envconfig.go | 32 +++++++++++-- .../internal/envconfig/envconfig_test.go | 48 +++++++++++++++++++ .../internal/envconfig/envconfig.go | 32 +++++++++++-- .../internal/envconfig/envconfig_test.go | 48 +++++++++++++++++++ .../internal/envconfig/envconfig.go | 32 +++++++++++-- .../internal/envconfig/envconfig_test.go | 48 +++++++++++++++++++ .../shared/otlp/envconfig/envconfig.go.tmpl | 34 +++++++++++-- .../otlp/envconfig/envconfig_test.go.tmpl | 48 +++++++++++++++++++ 11 files changed, 383 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fd8f44fea2..e490614e868 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,8 @@ The next release will require at least [Go 1.22]. ### Fixed +- Stop percent encoding header environment variables in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`, `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`, `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` (#5705) +- Remove invalid environment variable header keys in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`, `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`, `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` (#5705) - Correct comments for the priority of the `WithEndpoint` and `WithEndpointURL` options and their corresponding environment variables in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`. (#5584) - Correct the `Tracer`, `Meter`, and `Logger` names used in `go.opentelemetry.io/otel/example/dice`. (#5612) - Correct the `Tracer` names used in `go.opentelemetry.io/otel/example/namedtracer`. (#5612) diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go index b2735ba923c..261f5502682 100644 --- a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go +++ b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go @@ -15,6 +15,7 @@ import ( "strconv" "strings" "time" + "unicode" "go.opentelemetry.io/otel/internal/global" ) @@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.PathUnescape(n) - if err != nil { - global.Error(err, "escape header key", "key", n) + + trimmedName := strings.TrimSpace(n) + + // Validate the key. + if !isValidHeaderKey(trimmedName) { + global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName) continue } - trimmedName := strings.TrimSpace(name) + + // Only decode the value. value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) @@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) { } return cp, nil } + +func isValidHeaderKey(key string) bool { + if key == "" { + return false + } + for _, c := range key { + if !isTokenChar(c) { + return false + } + } + return true +} + +func isTokenChar(c rune) bool { + return c <= unicode.MaxASCII && (unicode.IsLetter(c) || + unicode.IsDigit(c) || + c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' || + c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~') +} diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go index 14543ea11eb..53cb0fededd 100644 --- a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go +++ b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go @@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) { }, }, }, + { + name: "with percent-encoded headers", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "user%2Did=42,user%20name=alice%20smith" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "user%2Did": "42", + "user%20name": "alice smith", + }, + }, + }, + }, + { + name: "with invalid header key", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "valid-key=value,invalid key=value" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "valid-key": "value", + }, + }, + }, + }, { name: "with URL", reader: EnvOptionsReader{ @@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) { name: "invalid key", value: "%XX=missing,userId=alice", want: map[string]string{ + "%XX": "missing", "userId": "alice", }, }, diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go index 35885ba8a72..7ac42759f6c 100644 --- a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go +++ b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go @@ -15,6 +15,7 @@ import ( "strconv" "strings" "time" + "unicode" "go.opentelemetry.io/otel/internal/global" ) @@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.PathUnescape(n) - if err != nil { - global.Error(err, "escape header key", "key", n) + + trimmedName := strings.TrimSpace(n) + + // Validate the key. + if !isValidHeaderKey(trimmedName) { + global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName) continue } - trimmedName := strings.TrimSpace(name) + + // Only decode the value. value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) @@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) { } return cp, nil } + +func isValidHeaderKey(key string) bool { + if key == "" { + return false + } + for _, c := range key { + if !isTokenChar(c) { + return false + } + } + return true +} + +func isTokenChar(c rune) bool { + return c <= unicode.MaxASCII && (unicode.IsLetter(c) || + unicode.IsDigit(c) || + c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' || + c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~') +} diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go index 14543ea11eb..53cb0fededd 100644 --- a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go +++ b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go @@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) { }, }, }, + { + name: "with percent-encoded headers", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "user%2Did=42,user%20name=alice%20smith" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "user%2Did": "42", + "user%20name": "alice smith", + }, + }, + }, + }, + { + name: "with invalid header key", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "valid-key=value,invalid key=value" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "valid-key": "value", + }, + }, + }, + }, { name: "with URL", reader: EnvOptionsReader{ @@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) { name: "invalid key", value: "%XX=missing,userId=alice", want: map[string]string{ + "%XX": "missing", "userId": "alice", }, }, diff --git a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go index 9513c0a57ca..4abf48d1f62 100644 --- a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go +++ b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go @@ -15,6 +15,7 @@ import ( "strconv" "strings" "time" + "unicode" "go.opentelemetry.io/otel/internal/global" ) @@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.PathUnescape(n) - if err != nil { - global.Error(err, "escape header key", "key", n) + + trimmedName := strings.TrimSpace(n) + + // Validate the key. + if !isValidHeaderKey(trimmedName) { + global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName) continue } - trimmedName := strings.TrimSpace(name) + + // Only decode the value. value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) @@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) { } return cp, nil } + +func isValidHeaderKey(key string) bool { + if key == "" { + return false + } + for _, c := range key { + if !isTokenChar(c) { + return false + } + } + return true +} + +func isTokenChar(c rune) bool { + return c <= unicode.MaxASCII && (unicode.IsLetter(c) || + unicode.IsDigit(c) || + c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' || + c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~') +} diff --git a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go index 14543ea11eb..53cb0fededd 100644 --- a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go +++ b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go @@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) { }, }, }, + { + name: "with percent-encoded headers", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "user%2Did=42,user%20name=alice%20smith" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "user%2Did": "42", + "user%20name": "alice smith", + }, + }, + }, + }, + { + name: "with invalid header key", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "valid-key=value,invalid key=value" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "valid-key": "value", + }, + }, + }, + }, { name: "with URL", reader: EnvOptionsReader{ @@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) { name: "invalid key", value: "%XX=missing,userId=alice", want: map[string]string{ + "%XX": "missing", "userId": "alice", }, }, diff --git a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go index 26a316d003d..f30bb66aeda 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go +++ b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go @@ -15,6 +15,7 @@ import ( "strconv" "strings" "time" + "unicode" "go.opentelemetry.io/otel/internal/global" ) @@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.PathUnescape(n) - if err != nil { - global.Error(err, "escape header key", "key", n) + + trimmedName := strings.TrimSpace(n) + + // Validate the key. + if !isValidHeaderKey(trimmedName) { + global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName) continue } - trimmedName := strings.TrimSpace(name) + + // Only decode the value. value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) @@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) { } return cp, nil } + +func isValidHeaderKey(key string) bool { + if key == "" { + return false + } + for _, c := range key { + if !isTokenChar(c) { + return false + } + } + return true +} + +func isTokenChar(c rune) bool { + return c <= unicode.MaxASCII && (unicode.IsLetter(c) || + unicode.IsDigit(c) || + c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' || + c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~') +} diff --git a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go index 14543ea11eb..53cb0fededd 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go +++ b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go @@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) { }, }, }, + { + name: "with percent-encoded headers", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "user%2Did=42,user%20name=alice%20smith" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "user%2Did": "42", + "user%20name": "alice smith", + }, + }, + }, + }, + { + name: "with invalid header key", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "valid-key=value,invalid key=value" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "valid-key": "value", + }, + }, + }, + }, { name: "with URL", reader: EnvOptionsReader{ @@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) { name: "invalid key", value: "%XX=missing,userId=alice", want: map[string]string{ + "%XX": "missing", "userId": "alice", }, }, diff --git a/internal/shared/otlp/envconfig/envconfig.go.tmpl b/internal/shared/otlp/envconfig/envconfig.go.tmpl index 6e0110b3344..4abf48d1f62 100644 --- a/internal/shared/otlp/envconfig/envconfig.go.tmpl +++ b/internal/shared/otlp/envconfig/envconfig.go.tmpl @@ -4,7 +4,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -package envconfig +package envconfig // import "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig" import ( "crypto/tls" @@ -15,6 +15,7 @@ import ( "strconv" "strings" "time" + "unicode" "go.opentelemetry.io/otel/internal/global" ) @@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string { global.Error(errors.New("missing '="), "parse headers", "input", header) continue } - name, err := url.PathUnescape(n) - if err != nil { - global.Error(err, "escape header key", "key", n) + + trimmedName := strings.TrimSpace(n) + + // Validate the key. + if !isValidHeaderKey(trimmedName) { + global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName) continue } - trimmedName := strings.TrimSpace(name) + + // Only decode the value. value, err := url.PathUnescape(v) if err != nil { global.Error(err, "escape header value", "value", v) @@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) { } return cp, nil } + +func isValidHeaderKey(key string) bool { + if key == "" { + return false + } + for _, c := range key { + if !isTokenChar(c) { + return false + } + } + return true +} + +func isTokenChar(c rune) bool { + return c <= unicode.MaxASCII && (unicode.IsLetter(c) || + unicode.IsDigit(c) || + c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' || + c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~') +} diff --git a/internal/shared/otlp/envconfig/envconfig_test.go.tmpl b/internal/shared/otlp/envconfig/envconfig_test.go.tmpl index 14543ea11eb..53cb0fededd 100644 --- a/internal/shared/otlp/envconfig/envconfig_test.go.tmpl +++ b/internal/shared/otlp/envconfig/envconfig_test.go.tmpl @@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) { }, }, }, + { + name: "with percent-encoded headers", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "user%2Did=42,user%20name=alice%20smith" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "user%2Did": "42", + "user%20name": "alice smith", + }, + }, + }, + }, + { + name: "with invalid header key", + reader: EnvOptionsReader{ + GetEnv: func(n string) string { + if n == "HELLO" { + return "valid-key=value,invalid key=value" + } + return "" + }, + }, + configs: []ConfigFn{ + WithHeaders("HELLO", func(v map[string]string) { + options = append(options, testOption{TestHeaders: v}) + }), + }, + expectedOptions: []testOption{ + { + TestHeaders: map[string]string{ + "valid-key": "value", + }, + }, + }, + }, { name: "with URL", reader: EnvOptionsReader{ @@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) { name: "invalid key", value: "%XX=missing,userId=alice", want: map[string]string{ + "%XX": "missing", "userId": "alice", }, },