From 66b0c3c4fd569c0b90be371a5909b4a616335bde Mon Sep 17 00:00:00 2001 From: "John L. Peterson (Jack)" Date: Wed, 4 Dec 2024 04:42:14 -0500 Subject: [PATCH] [exporter/datadog] add basic API key validation on startup (#36510) #### Description adds basic hexadecimal character validation to Datadog API key on startup #### Link to tracking issue Fixes #36509 #### Testing new "invalid API Key" test #### Documentation changelog file --------- Co-authored-by: Pablo Baeyens --- .chloggen/dd-config-api-key-fix.yaml | 27 +++++++++++++ connector/datadogconnector/example_test.go | 2 +- .../examples/k8s-chart/configmap.yaml | 2 +- exporter/datadogexporter/examples_test.go | 2 +- exporter/datadogexporter/factory_test.go | 2 +- .../integration_test_config.yaml | 2 +- ...egration_test_internal_metrics_config.yaml | 2 +- .../integration_test_logs_config.yaml | 2 +- .../integration_test_toplevel_config.yaml | 2 +- .../datadogexporter/metrics_exporter_test.go | 4 +- pkg/datadog/config/config.go | 14 ++++++- pkg/datadog/config/config_test.go | 39 +++++++++++-------- pkg/datadog/config/testdata/config.yaml | 2 +- 13 files changed, 74 insertions(+), 28 deletions(-) create mode 100644 .chloggen/dd-config-api-key-fix.yaml diff --git a/.chloggen/dd-config-api-key-fix.yaml b/.chloggen/dd-config-api-key-fix.yaml new file mode 100644 index 000000000000..e91e23d13036 --- /dev/null +++ b/.chloggen/dd-config-api-key-fix.yaml @@ -0,0 +1,27 @@ +# 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. filelogreceiver) +component: connector/datadog, exporter/datadog, pkg/datadog + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: throw error if datadog API key contains invalid characters + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [36509] + +# (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: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# 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] diff --git a/connector/datadogconnector/example_test.go b/connector/datadogconnector/example_test.go index 444ead06b3ec..b9c297f5c559 100644 --- a/connector/datadogconnector/example_test.go +++ b/connector/datadogconnector/example_test.go @@ -22,7 +22,7 @@ import ( ) func TestExamples(t *testing.T) { - t.Setenv("DD_API_KEY", "testvalue") + t.Setenv("DD_API_KEY", "aaaaaaaaa") factories := newTestComponents(t) const configFile = "./examples/config.yaml" // https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/33594 diff --git a/exporter/datadogexporter/examples/k8s-chart/configmap.yaml b/exporter/datadogexporter/examples/k8s-chart/configmap.yaml index facfdf4b6690..a601ee7b87ab 100644 --- a/exporter/datadogexporter/examples/k8s-chart/configmap.yaml +++ b/exporter/datadogexporter/examples/k8s-chart/configmap.yaml @@ -56,7 +56,7 @@ data: exporters: datadog: api: - key: + key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" # Change this to your Datadog API key site: datadoghq.com # Change this to your site if not using the default processors: resourcedetection: diff --git a/exporter/datadogexporter/examples_test.go b/exporter/datadogexporter/examples_test.go index 64484e953f78..bcfb3ebc7f8b 100644 --- a/exporter/datadogexporter/examples_test.go +++ b/exporter/datadogexporter/examples_test.go @@ -53,7 +53,7 @@ func TestExamples(t *testing.T) { continue } t.Run(filepath.Base(f.Name()), func(t *testing.T) { - t.Setenv("DD_API_KEY", "testvalue") + t.Setenv("DD_API_KEY", "aaaaaaaaa") name := filepath.Join(folder, f.Name()) // https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/33594 // nolint:staticcheck diff --git a/exporter/datadogexporter/factory_test.go b/exporter/datadogexporter/factory_test.go index 8fefd791fa63..f13ed2fa4fe0 100644 --- a/exporter/datadogexporter/factory_test.go +++ b/exporter/datadogexporter/factory_test.go @@ -300,7 +300,7 @@ func TestOnlyMetadata(t *testing.T) { BackOffConfig: configretry.NewDefaultBackOffConfig(), QueueSettings: exporterhelper.NewDefaultQueueConfig(), - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, Metrics: MetricsConfig{TCPAddrConfig: confignet.TCPAddrConfig{Endpoint: server.URL}}, Traces: TracesConfig{TCPAddrConfig: confignet.TCPAddrConfig{Endpoint: server.URL}}, OnlyMetadata: true, diff --git a/exporter/datadogexporter/integrationtest/integration_test_config.yaml b/exporter/datadogexporter/integrationtest/integration_test_config.yaml index 19aa31595cf8..dff2f35340a9 100644 --- a/exporter/datadogexporter/integrationtest/integration_test_config.yaml +++ b/exporter/datadogexporter/integrationtest/integration_test_config.yaml @@ -30,7 +30,7 @@ exporters: verbosity: detailed datadog: api: - key: "key" + key: "aaa" tls: insecure_skip_verify: true host_metadata: diff --git a/exporter/datadogexporter/integrationtest/integration_test_internal_metrics_config.yaml b/exporter/datadogexporter/integrationtest/integration_test_internal_metrics_config.yaml index 6887f11d6627..9100aecf8cad 100644 --- a/exporter/datadogexporter/integrationtest/integration_test_internal_metrics_config.yaml +++ b/exporter/datadogexporter/integrationtest/integration_test_internal_metrics_config.yaml @@ -17,7 +17,7 @@ receivers: exporters: datadog: api: - key: "key" + key: "aaa" tls: insecure_skip_verify: true host_metadata: diff --git a/exporter/datadogexporter/integrationtest/integration_test_logs_config.yaml b/exporter/datadogexporter/integrationtest/integration_test_logs_config.yaml index 3082caf83b80..f02d45dafc16 100644 --- a/exporter/datadogexporter/integrationtest/integration_test_logs_config.yaml +++ b/exporter/datadogexporter/integrationtest/integration_test_logs_config.yaml @@ -21,7 +21,7 @@ receivers: exporters: datadog: api: - key: "key" + key: "aaa" tls: insecure_skip_verify: true host_metadata: diff --git a/exporter/datadogexporter/integrationtest/integration_test_toplevel_config.yaml b/exporter/datadogexporter/integrationtest/integration_test_toplevel_config.yaml index f0fac370e413..e542b95d3626 100644 --- a/exporter/datadogexporter/integrationtest/integration_test_toplevel_config.yaml +++ b/exporter/datadogexporter/integrationtest/integration_test_toplevel_config.yaml @@ -17,7 +17,7 @@ exporters: verbosity: detailed datadog: api: - key: "key" + key: "aaa" tls: insecure_skip_verify: true host_metadata: diff --git a/exporter/datadogexporter/metrics_exporter_test.go b/exporter/datadogexporter/metrics_exporter_test.go index 379e25842208..7d973171c850 100644 --- a/exporter/datadogexporter/metrics_exporter_test.go +++ b/exporter/datadogexporter/metrics_exporter_test.go @@ -43,7 +43,7 @@ func TestNewExporter(t *testing.T) { cfg := &Config{ API: APIConfig{ - Key: "ddog_32_characters_long_api_key1", + Key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", }, Metrics: MetricsConfig{ TCPAddrConfig: confignet.TCPAddrConfig{ @@ -424,7 +424,7 @@ func TestNewExporter_Zorkian(t *testing.T) { cfg := &Config{ API: APIConfig{ - Key: "ddog_32_characters_long_api_key1", + Key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", }, Metrics: MetricsConfig{ TCPAddrConfig: confignet.TCPAddrConfig{ diff --git a/pkg/datadog/config/config.go b/pkg/datadog/config/config.go index c1b40bd7cef0..65133187c5b3 100644 --- a/pkg/datadog/config/config.go +++ b/pkg/datadog/config/config.go @@ -6,6 +6,7 @@ package config // import "github.com/open-telemetry/opentelemetry-collector-cont import ( "errors" "fmt" + "regexp" "strings" "time" @@ -27,11 +28,17 @@ var ( ErrNoMetadata = errors.New("only_metadata can't be enabled when host_metadata::enabled = false or host_metadata::hostname_source != first_resource") // ErrInvalidHostname is returned when the hostname is invalid. ErrEmptyEndpoint = errors.New("endpoint cannot be empty") + // ErrAPIKeyFormat is returned if API key contains invalid characters + ErrAPIKeyFormat = errors.New("api::key contains invalid characters") + // NonHexRegex is a regex of characters that are always invalid in a Datadog API key + NonHexRegex = regexp.MustCompile(NonHexChars) ) const ( // DefaultSite is the default site of the Datadog intake to send data to DefaultSite = "datadoghq.com" + // NonHexChars is a regex of characters that are always invalid in a Datadog API key + NonHexChars = "[^0-9a-fA-F]" ) // APIConfig defines the API configuration options @@ -120,10 +127,15 @@ func (c *Config) Validate() error { return fmt.Errorf("hostname field is invalid: %w", err) } - if c.API.Key == "" { + if string(c.API.Key) == "" { return ErrUnsetAPIKey } + invalidAPIKeyChars := NonHexRegex.FindAllString(string(c.API.Key), -1) + if len(invalidAPIKeyChars) > 0 { + return fmt.Errorf("%w: invalid characters: %s", ErrAPIKeyFormat, strings.Join(invalidAPIKeyChars, ", ")) + } + if err := c.Traces.Validate(); err != nil { return err } diff --git a/pkg/datadog/config/config_test.go b/pkg/datadog/config/config_test.go index a76b2b5a5b9b..11c0d2db2bb2 100644 --- a/pkg/datadog/config/config_test.go +++ b/pkg/datadog/config/config_test.go @@ -44,10 +44,17 @@ func TestValidate(t *testing.T) { }, err: ErrUnsetAPIKey.Error(), }, + { + name: "invalid format api::key", + cfg: &Config{ + API: APIConfig{Key: "'aaaaaaa"}, + }, + err: ErrAPIKeyFormat.Error(), + }, { name: "invalid hostname", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, TagsConfig: TagsConfig{Hostname: "invalid_host"}, HostMetadata: HostMetadataConfig{Enabled: true, ReporterPeriod: 10 * time.Minute}, }, @@ -56,7 +63,7 @@ func TestValidate(t *testing.T) { { name: "no metadata", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, OnlyMetadata: true, HostMetadata: HostMetadataConfig{Enabled: true, ReporterPeriod: 10 * time.Minute}, }, @@ -65,7 +72,7 @@ func TestValidate(t *testing.T) { { name: "span name remapping valid", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, Traces: TracesExporterConfig{TracesConfig: TracesConfig{SpanNameRemappings: map[string]string{"old.opentelemetryspan.name": "updated.name"}}}, HostMetadata: HostMetadataConfig{Enabled: true, ReporterPeriod: 10 * time.Minute}, }, @@ -73,7 +80,7 @@ func TestValidate(t *testing.T) { { name: "span name remapping empty val", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, Traces: TracesExporterConfig{TracesConfig: TracesConfig{SpanNameRemappings: map[string]string{"oldname": ""}}}, HostMetadata: HostMetadataConfig{Enabled: true, ReporterPeriod: 10 * time.Minute}, }, @@ -82,7 +89,7 @@ func TestValidate(t *testing.T) { { name: "span name remapping empty key", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, Traces: TracesExporterConfig{TracesConfig: TracesConfig{SpanNameRemappings: map[string]string{"": "newname"}}}, HostMetadata: HostMetadataConfig{Enabled: true, ReporterPeriod: 10 * time.Minute}, }, @@ -91,7 +98,7 @@ func TestValidate(t *testing.T) { { name: "ignore resources valid", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, Traces: TracesExporterConfig{TracesConfig: TracesConfig{IgnoreResources: []string{"[123]"}}}, HostMetadata: HostMetadataConfig{Enabled: true, ReporterPeriod: 10 * time.Minute}, }, @@ -99,7 +106,7 @@ func TestValidate(t *testing.T) { { name: "ignore resources missing bracket", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, Traces: TracesExporterConfig{TracesConfig: TracesConfig{IgnoreResources: []string{"[123"}}}, HostMetadata: HostMetadataConfig{Enabled: true, ReporterPeriod: 10 * time.Minute}, }, @@ -108,7 +115,7 @@ func TestValidate(t *testing.T) { { name: "invalid histogram settings", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, Metrics: MetricsConfig{ HistConfig: HistogramConfig{ Mode: HistogramModeNoBuckets, @@ -122,7 +129,7 @@ func TestValidate(t *testing.T) { { name: "TLS settings are valid", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, ClientConfig: confighttp.ClientConfig{ TLSSetting: configtls.ClientConfig{ InsecureSkipVerify: true, @@ -134,7 +141,7 @@ func TestValidate(t *testing.T) { { name: "With trace_buffer", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, Traces: TracesExporterConfig{TraceBuffer: 10}, HostMetadata: HostMetadataConfig{Enabled: true, ReporterPeriod: 10 * time.Minute}, }, @@ -142,7 +149,7 @@ func TestValidate(t *testing.T) { { name: "With peer_tags", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, Traces: TracesExporterConfig{ TracesConfig: TracesConfig{ PeerTags: []string{"tag1", "tag2"}, @@ -154,7 +161,7 @@ func TestValidate(t *testing.T) { { name: "With confighttp client configs", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, ClientConfig: confighttp.ClientConfig{ ReadBufferSize: 100, WriteBufferSize: 200, @@ -173,7 +180,7 @@ func TestValidate(t *testing.T) { { name: "unsupported confighttp client configs", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "aaaaaaa"}, ClientConfig: confighttp.ClientConfig{ Endpoint: "endpoint", Compression: "gzip", @@ -189,7 +196,7 @@ func TestValidate(t *testing.T) { { name: "Invalid reporter_period", cfg: &Config{ - API: APIConfig{Key: "notnull"}, + API: APIConfig{Key: "abcdef0"}, HostMetadata: HostMetadataConfig{Enabled: true, ReporterPeriod: 4 * time.Minute}, }, err: "reporter_period must be 5 minutes or higher", @@ -199,7 +206,7 @@ func TestValidate(t *testing.T) { t.Run(testInstance.name, func(t *testing.T) { err := testInstance.cfg.Validate() if testInstance.err != "" { - assert.EqualError(t, err, testInstance.err) + assert.ErrorContains(t, err, testInstance.err) } else { assert.NoError(t, err) } @@ -649,7 +656,7 @@ func TestLoadConfig(t *testing.T) { BackOffConfig: configretry.NewDefaultBackOffConfig(), QueueSettings: exporterhelper.NewDefaultQueueConfig(), API: APIConfig{ - Key: "key", + Key: "abc", Site: "datadoghq.com", FailOnInvalidKey: false, }, diff --git a/pkg/datadog/config/testdata/config.yaml b/pkg/datadog/config/testdata/config.yaml index e1002550cab9..0dbfc0a4782a 100644 --- a/pkg/datadog/config/testdata/config.yaml +++ b/pkg/datadog/config/testdata/config.yaml @@ -40,7 +40,7 @@ datadog/default: datadog/customReporterPeriod: api: - key: key + key: abc host_metadata: enabled: true reporter_period: 10m