From 378d7044e78ab7983a08187eea0c16de22ed1b28 Mon Sep 17 00:00:00 2001 From: Alex Boten <223565+codeboten@users.noreply.github.com> Date: Thu, 12 Dec 2024 10:04:46 -0800 Subject: [PATCH] config: fix panic on nil value in headers name/value pair (#6425) Found a bug during testing of v0.3.0 support with the Collector where the value passed in to the headers name/value pair list could be nil, causing a dereferencing error. This fixes that bug. --------- Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com> --- CHANGELOG.md | 1 + config/testdata/invalid_nil_name.json | 49 ++++++++++++++++++++++++++ config/testdata/invalid_nil_name.yaml | 13 +++++++ config/testdata/invalid_nil_value.json | 49 ++++++++++++++++++++++++++ config/testdata/invalid_nil_value.yaml | 13 +++++++ config/v0.3.0/config_json.go | 25 ++++++++----- config/v0.3.0/config_test.go | 22 ++++++++++++ config/v0.3.0/config_yaml.go | 28 +++++++++++++++ 8 files changed, 191 insertions(+), 9 deletions(-) create mode 100644 config/testdata/invalid_nil_name.json create mode 100644 config/testdata/invalid_nil_name.yaml create mode 100644 config/testdata/invalid_nil_value.json create mode 100644 config/testdata/invalid_nil_value.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 2567a9a6d66..024b5fea901 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Removed the redundant handling of panic from the `HTML` function in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#6373) - The `code.function` attribute emitted by `go.opentelemetry.io/contrib/bridges/otelslog` now stores just the function name instead the package path-qualified function name. The `code.namespace` attribute now stores the package path. (#6415) - The `code.function` attribute emitted by `go.opentelemetry.io/contrib/bridges/otelzap` now stores just the function name instead the package path-qualified function name. The `code.namespace` attribute now stores the package path. (#6423) +- Return an error for `nil` values when unmarshaling `NameStringValuePair` in `go.opentelemetry.io/contrib/config`. (#6425) diff --git a/config/testdata/invalid_nil_name.json b/config/testdata/invalid_nil_name.json new file mode 100644 index 00000000000..163aab02d03 --- /dev/null +++ b/config/testdata/invalid_nil_name.json @@ -0,0 +1,49 @@ +{ + "file_format": "0.3", + "disabled": false, + "logger_provider": { + "processors": [ + { + "batch": { + "schedule_delay": 5000, + "export_timeout": 30000, + "max_queue_size": 2048, + "max_export_batch_size": 512, + "exporter": { + "otlp": { + "protocol": "http/protobuf", + "endpoint": "http://localhost:4318/v1/logs", + "certificate": "/app/cert.pem", + "client_key": "/app/cert.pem", + "client_certificate": "/app/cert.pem", + "headers": [ + { + "name": "api-key", + "value": "1234" + }, + { + "value": "nil-name" + } + ], + "headers_list": "api-key=1234", + "compression": "gzip", + "timeout": 10000, + "insecure": false + } + } + } + }, + { + "simple": { + "exporter": { + "console": {} + } + } + } + ], + "limits": { + "attribute_value_length_limit": 4096, + "attribute_count_limit": 128 + } + } +} \ No newline at end of file diff --git a/config/testdata/invalid_nil_name.yaml b/config/testdata/invalid_nil_name.yaml new file mode 100644 index 00000000000..89d197dbb8e --- /dev/null +++ b/config/testdata/invalid_nil_name.yaml @@ -0,0 +1,13 @@ +file_format: "0.3" +disabled: false +logger_provider: + processors: + - batch: + exporter: + otlp: + protocol: http/protobuf + endpoint: http://localhost:4318/v1/logs + headers: + - name: api-key + value: "1234" + - value: nil-name \ No newline at end of file diff --git a/config/testdata/invalid_nil_value.json b/config/testdata/invalid_nil_value.json new file mode 100644 index 00000000000..a6c0c5f856d --- /dev/null +++ b/config/testdata/invalid_nil_value.json @@ -0,0 +1,49 @@ +{ + "file_format": "0.3", + "disabled": false, + "logger_provider": { + "processors": [ + { + "batch": { + "schedule_delay": 5000, + "export_timeout": 30000, + "max_queue_size": 2048, + "max_export_batch_size": 512, + "exporter": { + "otlp": { + "protocol": "http/protobuf", + "endpoint": "http://localhost:4318/v1/logs", + "certificate": "/app/cert.pem", + "client_key": "/app/cert.pem", + "client_certificate": "/app/cert.pem", + "headers": [ + { + "name": "api-key", + "value": "1234" + }, + { + "name": "nil-value" + } + ], + "headers_list": "api-key=1234", + "compression": "gzip", + "timeout": 10000, + "insecure": false + } + } + } + }, + { + "simple": { + "exporter": { + "console": {} + } + } + } + ], + "limits": { + "attribute_value_length_limit": 4096, + "attribute_count_limit": 128 + } + } +} \ No newline at end of file diff --git a/config/testdata/invalid_nil_value.yaml b/config/testdata/invalid_nil_value.yaml new file mode 100644 index 00000000000..8df3f074282 --- /dev/null +++ b/config/testdata/invalid_nil_value.yaml @@ -0,0 +1,13 @@ +file_format: "0.3" +disabled: false +logger_provider: + processors: + - batch: + exporter: + otlp: + protocol: http/protobuf + endpoint: http://localhost:4318/v1/logs + headers: + - name: api-key + value: "1234" + - name: nil-value \ No newline at end of file diff --git a/config/v0.3.0/config_json.go b/config/v0.3.0/config_json.go index f350067280d..2d5a7f33125 100644 --- a/config/v0.3.0/config_json.go +++ b/config/v0.3.0/config_json.go @@ -112,18 +112,25 @@ func (j *NameStringValuePair) UnmarshalJSON(b []byte) error { if err := json.Unmarshal(b, &raw); err != nil { return err } - if _, ok := raw["name"]; raw != nil && !ok { - return errors.New("field name in NameStringValuePair: required") + if _, ok := raw["name"]; !ok { + return errors.New("json: cannot unmarshal field name in NameStringValuePair required") } - if _, ok := raw["value"]; raw != nil && !ok { - return errors.New("field value in NameStringValuePair: required") + if _, ok := raw["value"]; !ok { + return errors.New("json: cannot unmarshal field value in NameStringValuePair required") } - type Plain NameStringValuePair - var plain Plain - if err := json.Unmarshal(b, &plain); err != nil { - return err + var name, value string + var ok bool + if name, ok = raw["name"].(string); !ok { + return errors.New("yaml: cannot unmarshal field name in NameStringValuePair must be string") + } + if value, ok = raw["value"].(string); !ok { + return errors.New("yaml: cannot unmarshal field value in NameStringValuePair must be string") + } + + *j = NameStringValuePair{ + Name: name, + Value: &value, } - *j = NameStringValuePair(plain) return nil } diff --git a/config/v0.3.0/config_test.go b/config/v0.3.0/config_test.go index 0f7822bd8bb..ebe45c95286 100644 --- a/config/v0.3.0/config_test.go +++ b/config/v0.3.0/config_test.go @@ -405,6 +405,16 @@ func TestParseYAML(t *testing.T) { wantErr: errors.New(`yaml: unmarshal errors: line 2: cannot unmarshal !!str ` + "`notabool`" + ` into bool`), }, + { + name: "invalid nil name", + input: "invalid_nil_name.yaml", + wantErr: errors.New(`yaml: cannot unmarshal field name in NameStringValuePair required`), + }, + { + name: "invalid nil value", + input: "invalid_nil_value.yaml", + wantErr: errors.New(`yaml: cannot unmarshal field value in NameStringValuePair required`), + }, { name: "valid v0.2 config", input: "v0.2.yaml", @@ -429,6 +439,7 @@ func TestParseYAML(t *testing.T) { got, err := ParseYAML(b) if tt.wantErr != nil { + require.Error(t, err) require.Equal(t, tt.wantErr.Error(), err.Error()) } else { require.NoError(t, err) @@ -459,6 +470,16 @@ func TestSerializeJSON(t *testing.T) { input: "invalid_bool.json", wantErr: errors.New(`json: cannot unmarshal string into Go struct field Plain.disabled of type bool`), }, + { + name: "invalid nil name", + input: "invalid_nil_name.json", + wantErr: errors.New(`json: cannot unmarshal field name in NameStringValuePair required`), + }, + { + name: "invalid nil value", + input: "invalid_nil_value.json", + wantErr: errors.New(`json: cannot unmarshal field value in NameStringValuePair required`), + }, { name: "valid v0.2 config", input: "v0.2.json", @@ -480,6 +501,7 @@ func TestSerializeJSON(t *testing.T) { err = json.Unmarshal(b, &got) if tt.wantErr != nil { + require.Error(t, err) require.Equal(t, tt.wantErr.Error(), err.Error()) } else { require.NoError(t, err) diff --git a/config/v0.3.0/config_yaml.go b/config/v0.3.0/config_yaml.go index 94af68e6f98..48a8ca640f5 100644 --- a/config/v0.3.0/config_yaml.go +++ b/config/v0.3.0/config_yaml.go @@ -4,6 +4,7 @@ package config // import "go.opentelemetry.io/contrib/config/v0.3.0" import ( + "errors" "fmt" "reflect" ) @@ -30,6 +31,33 @@ func (j *AttributeNameValueType) UnmarshalYAML(unmarshal func(interface{}) error return nil } +// UnmarshalYAML implements yaml.Unmarshaler. +func (j *NameStringValuePair) UnmarshalYAML(unmarshal func(interface{}) error) error { + var raw map[string]interface{} + if err := unmarshal(&raw); err != nil { + return err + } + if _, ok := raw["name"]; !ok { + return errors.New("yaml: cannot unmarshal field name in NameStringValuePair required") + } + if _, ok := raw["value"]; !ok { + return errors.New("yaml: cannot unmarshal field value in NameStringValuePair required") + } + var name, value string + var ok bool + if name, ok = raw["name"].(string); !ok { + return errors.New("yaml: cannot unmarshal field name in NameStringValuePair must be string") + } + if value, ok = raw["value"].(string); !ok { + return errors.New("yaml: cannot unmarshal field value in NameStringValuePair must be string") + } + *j = NameStringValuePair{ + Name: name, + Value: &value, + } + return nil +} + // UnmarshalYAML implements yaml.Unmarshaler. func (j *LanguageSpecificInstrumentation) UnmarshalYAML(unmarshal func(interface{}) error) error { var raw map[string]interface{}