From 86bccc7a9b7b00a9192f68e429464a904d476a79 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 2 Sep 2020 17:36:12 +0000 Subject: [PATCH 01/12] Add format-type API key check to profiler config Validation only triggers when the user specifies a key using WithAPIKey. No other actions are taken, no error is returned, and the specified key will be used. --- profiler/options.go | 20 ++++++++++++++++++++ profiler/options_test.go | 15 +++++++++------ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/profiler/options.go b/profiler/options.go index fba5afebe6..a78947fb4d 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -45,6 +45,9 @@ const ( defaultEnv = "none" ) +// Length of valid API key +const optionApiKeyLength = 32 + var defaultProfileTypes = []ProfileType{CPUProfile, HeapProfile} type config struct { @@ -71,6 +74,19 @@ func urlForSite(site string) (string, error) { return u, err } +// Check for API key constraints: +func apiKeyCheck(key string) bool { + if optionApiKeyLength != len(key) { + return false + } + for _, c := range key { + if (!((c >= 'a' && c <= 'z') || (c >= '0' && c <= '9'))) { + return false + } + } + return true +} + func (c *config) addProfileType(t ProfileType) { if c.types == nil { c.types = make(map[ProfileType]struct{}) @@ -152,8 +168,12 @@ func WithAgentAddr(hostport string) Option { } // WithAPIKey specifies the API key to use when connecting to the Datadog API directly, skipping the agent. +// If the API key might be invalid, emit a warning but otherwise do nothing. func WithAPIKey(key string) Option { return func(cfg *config) { + if !apiKeyCheck(key) { + log.Error("profiler: invalid API key provided, will try to use $DD_API_KEY.") + } cfg.apiKey = key } } diff --git a/profiler/options_test.go b/profiler/options_test.go index bcbdda4d8f..1cfc7da7b6 100644 --- a/profiler/options_test.go +++ b/profiler/options_test.go @@ -37,18 +37,20 @@ func TestOptions(t *testing.T) { }) t.Run("WithAPIKey", func(t *testing.T) { + var validKey = "12345678901234567890123456789012" var cfg config - WithAPIKey("123")(&cfg) - assert.Equal(t, "123", cfg.apiKey) + WithAPIKey(validKey)(&cfg) + assert.Equal(t, validKey, cfg.apiKey) assert.Equal(t, cfg.apiURL, cfg.targetURL) }) t.Run("WithAPIKey/override", func(t *testing.T) { os.Setenv("DD_API_KEY", "apikey") defer os.Unsetenv("DD_API_KEY") + var validKey = "12345678901234567890123456789012" var cfg config - WithAPIKey("123")(&cfg) - assert.Equal(t, "123", cfg.apiKey) + WithAPIKey(validKey)(&cfg) + assert.Equal(t, validKey, cfg.apiKey) }) t.Run("WithURL", func(t *testing.T) { @@ -179,10 +181,11 @@ func TestEnvVars(t *testing.T) { }) t.Run("DD_API_KEY", func(t *testing.T) { - os.Setenv("DD_API_KEY", "123") + var validKey = "12345678901234567890123456789012" + os.Setenv("DD_API_KEY", validKey) defer os.Unsetenv("DD_API_KEY") cfg := defaultConfig() - assert.Equal(t, "123", cfg.apiKey) + assert.Equal(t, validKey, cfg.apiKey) }) t.Run("DD_SITE", func(t *testing.T) { From adaf3f5243c9ba5fd161cc1b1675c69ac8c1718a Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 2 Sep 2020 19:05:42 +0000 Subject: [PATCH 02/12] profiler: API key check throws error Moved the API key check to profiler/profiler.go. An API key with an invalid format will now cause newProfiler to throw an error. --- profiler/options.go | 6 +----- profiler/profiler.go | 3 +++ 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/profiler/options.go b/profiler/options.go index a78947fb4d..e144d30f63 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -46,7 +46,7 @@ const ( ) // Length of valid API key -const optionApiKeyLength = 32 +const optionApiKeyLength = 32 var defaultProfileTypes = []ProfileType{CPUProfile, HeapProfile} @@ -168,12 +168,8 @@ func WithAgentAddr(hostport string) Option { } // WithAPIKey specifies the API key to use when connecting to the Datadog API directly, skipping the agent. -// If the API key might be invalid, emit a warning but otherwise do nothing. func WithAPIKey(key string) Option { return func(cfg *config) { - if !apiKeyCheck(key) { - log.Error("profiler: invalid API key provided, will try to use $DD_API_KEY.") - } cfg.apiKey = key } } diff --git a/profiler/profiler.go b/profiler/profiler.go index 9fb3ef1b59..870cc00944 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -70,6 +70,9 @@ func newProfiler(opts ...Option) (*profiler, error) { opt(cfg) } if cfg.apiKey != "" { + if !apiKeyCheck(cfg.apiKey) { + return nil, fmt.Errorf("API key has incorrect form (must be 32-character lowercase alphanumeric string). Check calls to profiler.WithAPIKey or the DD_API_KEY environment variable. API key is not required in many configurations.") + } cfg.targetURL = cfg.apiURL } else { cfg.targetURL = cfg.agentURL From 50dd443f823493d086c28fef88915dc9c7b2797e Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 2 Sep 2020 19:33:45 +0000 Subject: [PATCH 03/12] profiler: stylistic changes to please gofmt My mistake, didn't know we had gofmt as part of our ci. --- profiler/options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiler/options.go b/profiler/options.go index e144d30f63..d9645aefbe 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -80,7 +80,7 @@ func apiKeyCheck(key string) bool { return false } for _, c := range key { - if (!((c >= 'a' && c <= 'z') || (c >= '0' && c <= '9'))) { + if !((c >= 'a' && c <= 'z') || (c >= '0' && c <= '9')) { return false } } From fac9de731100ae5967f300d31916e6da66295173 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 2 Sep 2020 19:38:57 +0000 Subject: [PATCH 04/12] profiler: fix additional linter complaints Refactored the error message as well, which was too specific; hopefully users onboarding into a configuration where an API key is necessary either don't exist or can refer to documentation. --- profiler/options.go | 2 +- profiler/profiler.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/profiler/options.go b/profiler/options.go index d9645aefbe..bb789c1ba6 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -46,7 +46,7 @@ const ( ) // Length of valid API key -const optionApiKeyLength = 32 +const optionAPIKeyLength = 32 var defaultProfileTypes = []ProfileType{CPUProfile, HeapProfile} diff --git a/profiler/profiler.go b/profiler/profiler.go index 870cc00944..db2c605619 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -71,7 +71,7 @@ func newProfiler(opts ...Option) (*profiler, error) { } if cfg.apiKey != "" { if !apiKeyCheck(cfg.apiKey) { - return nil, fmt.Errorf("API key has incorrect form (must be 32-character lowercase alphanumeric string). Check calls to profiler.WithAPIKey or the DD_API_KEY environment variable. API key is not required in many configurations.") + return nil, fmt.Errorf("API key has incorrect format; try checking your DD_API_KEY environment variable if submitting an API key is necessary for your configuration") } cfg.targetURL = cfg.apiURL } else { From 3af39db0686711495563109032c58aca4737f09e Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 2 Sep 2020 19:53:52 +0000 Subject: [PATCH 05/12] profiler: get local tests running for api check --- profiler/options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiler/options.go b/profiler/options.go index bb789c1ba6..e1332cf846 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -76,7 +76,7 @@ func urlForSite(site string) (string, error) { // Check for API key constraints: func apiKeyCheck(key string) bool { - if optionApiKeyLength != len(key) { + if optionAPIKeyLength != len(key) { return false } for _, c := range key { From ef1b11d939e812ae4e921a28cd0c6d14bcb3ff16 Mon Sep 17 00:00:00 2001 From: sanchda Date: Thu, 3 Sep 2020 15:54:37 +0000 Subject: [PATCH 06/12] profiler: sanitize API keys, address concerns Added a preliminary attempt at sanitizing API keys. This supposes that API keys are secret, and even invalid API keys may contain a valid API key and must be de-identified a little bit before display. None of this is relevant if we don't care... Address a few other concerns from #718 review --- profiler/options.go | 22 +++++++++++++++------- profiler/profiler.go | 4 ++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/profiler/options.go b/profiler/options.go index e1332cf846..0b3cc3833d 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -14,6 +14,7 @@ import ( "runtime" "strings" "time" + "unicode" "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" @@ -45,9 +46,6 @@ const ( defaultEnv = "none" ) -// Length of valid API key -const optionAPIKeyLength = 32 - var defaultProfileTypes = []ProfileType{CPUProfile, HeapProfile} type config struct { @@ -74,19 +72,29 @@ func urlForSite(site string) (string, error) { return u, err } -// Check for API key constraints: -func apiKeyCheck(key string) bool { - if optionAPIKeyLength != len(key) { +// isAPIKeyValid reports whether the given string is a structurally valid API key +func isAPIKeyValid(key string) bool { + if len(key) != 32 { return false } for _, c := range key { - if !((c >= 'a' && c <= 'z') || (c >= '0' && c <= '9')) { + if !unicode.IsLetter(c) && !unicode.IsLower(c) && !unicode.IsNumber(c) { return false } } return true } +// sanitizeAPIKey prepares an API key for display, by censoring everything +// beyond the first 8 characters of the input +func sanitizeAPIKey(key string) string { + ret := []byte(key) + for i := 8; i < len(ret); i++ { + ret[i] = '*' + } + return string(ret) +} + func (c *config) addProfileType(t ProfileType) { if c.types == nil { c.types = make(map[ProfileType]struct{}) diff --git a/profiler/profiler.go b/profiler/profiler.go index db2c605619..88403ced8d 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -70,8 +70,8 @@ func newProfiler(opts ...Option) (*profiler, error) { opt(cfg) } if cfg.apiKey != "" { - if !apiKeyCheck(cfg.apiKey) { - return nil, fmt.Errorf("API key has incorrect format; try checking your DD_API_KEY environment variable if submitting an API key is necessary for your configuration") + if !isAPIKeyValid(cfg.apiKey) { + return nil, fmt.Errorf("API key has incorrect format: %s", sanitizeAPIKey(cfg.apiKey)) } cfg.targetURL = cfg.apiURL } else { From 23e4c869d861503aeeeac1ea9a26ea72b2c69411 Mon Sep 17 00:00:00 2001 From: sanchda Date: Fri, 4 Sep 2020 12:37:16 +0000 Subject: [PATCH 07/12] profiler: remove API key sanitization Instead, just don't print the API key. --- profiler/options.go | 10 ---------- profiler/profiler.go | 2 +- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/profiler/options.go b/profiler/options.go index 0b3cc3833d..b498fa0ff0 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -85,16 +85,6 @@ func isAPIKeyValid(key string) bool { return true } -// sanitizeAPIKey prepares an API key for display, by censoring everything -// beyond the first 8 characters of the input -func sanitizeAPIKey(key string) string { - ret := []byte(key) - for i := 8; i < len(ret); i++ { - ret[i] = '*' - } - return string(ret) -} - func (c *config) addProfileType(t ProfileType) { if c.types == nil { c.types = make(map[ProfileType]struct{}) diff --git a/profiler/profiler.go b/profiler/profiler.go index 88403ced8d..15767ccdce 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -71,7 +71,7 @@ func newProfiler(opts ...Option) (*profiler, error) { } if cfg.apiKey != "" { if !isAPIKeyValid(cfg.apiKey) { - return nil, fmt.Errorf("API key has incorrect format: %s", sanitizeAPIKey(cfg.apiKey)) + return nil, fmt.Errorf("API key has incorrect format") } cfg.targetURL = cfg.apiURL } else { From 4bf0ca513227be2b1632eabad6b28ebac5813ce7 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 9 Sep 2020 13:36:46 +0000 Subject: [PATCH 08/12] profiler: update isAPIKeyValid to fail on extended ASCII input Erroneous passes were possible with inputs from the extended ASCII space (and beyond), when those glyphs were characterized (ha) as lowercase by unicode.IsLower. This patch explicitly filters for the ASCII range. --- profiler/options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiler/options.go b/profiler/options.go index b498fa0ff0..f63eea7ae1 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -78,7 +78,7 @@ func isAPIKeyValid(key string) bool { return false } for _, c := range key { - if !unicode.IsLetter(c) && !unicode.IsLower(c) && !unicode.IsNumber(c) { + if c > unicode.MaxASCII || (!unicode.IsLower(c) && !unicode.IsNumber(c)) { return false } } From 2e3c60074f34c6c804406d147a8a0594e2329234 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 9 Sep 2020 13:38:31 +0000 Subject: [PATCH 09/12] profiler: add tests for isAPIKeyValid Just a few tests to cover some obvious edge-cases. There may be many more. --- profiler/options_test.go | 42 +++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/profiler/options_test.go b/profiler/options_test.go index 1cfc7da7b6..caa75a574a 100644 --- a/profiler/options_test.go +++ b/profiler/options_test.go @@ -9,6 +9,7 @@ import ( "net" "os" "path/filepath" + "strconv" "testing" "time" @@ -17,7 +18,32 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" ) +// testAPIKey is an example API key for validation purposes +const testAPIKey = "12345678901234567890123456789012" + func TestOptions(t *testing.T) { + t.Run("APIKeyChecks", func(t *testing.T) { + var apikeytests = []struct { + in string + out bool + }{ + {"", false}, // Fail, empty string + {"1234567890123456789012345678901", false}, // Fail, too short + {"123456789012345678901234567890123", false}, // Fail, too long + {"12345678901234567890123456789012", true}, // Pass, numeric only + {"abcdefabcdabcdefabcdefabcdefabcd", true}, // Pass, alpha only + {"abcdefabcdabcdef7890abcdef789012", true}, // Pass, alphanumeric + {"abcdefabcdabcdef7890Abcdef789012", false}, // Fail, contains an uppercase + {"abcdefabcdabcdef7890@bcdef789012", false}, // Fail, contains an ASCII symbol + {"abcdefabcdabcdef7890ábcdef789012", false}, // Fail, lowercase extended ASCII + {"abcdefabcdabcdef7890ábcdef78901", false}, // Fail, lowercase extended ASCII, conservative + } + + for i, tt := range apikeytests { + assert.Equal(t, tt.out, isAPIKeyValid(tt.in), strconv.Itoa(i)+" : "+tt.in) + } + }) + t.Run("WithAgentAddr", func(t *testing.T) { var cfg config WithAgentAddr("test:123")(&cfg) @@ -37,20 +63,19 @@ func TestOptions(t *testing.T) { }) t.Run("WithAPIKey", func(t *testing.T) { - var validKey = "12345678901234567890123456789012" var cfg config - WithAPIKey(validKey)(&cfg) - assert.Equal(t, validKey, cfg.apiKey) + WithAPIKey(testAPIKey)(&cfg) + assert.Equal(t, testAPIKey, cfg.apiKey) assert.Equal(t, cfg.apiURL, cfg.targetURL) }) t.Run("WithAPIKey/override", func(t *testing.T) { os.Setenv("DD_API_KEY", "apikey") defer os.Unsetenv("DD_API_KEY") - var validKey = "12345678901234567890123456789012" + var testAPIKey = "12345678901234567890123456789012" var cfg config - WithAPIKey(validKey)(&cfg) - assert.Equal(t, validKey, cfg.apiKey) + WithAPIKey(testAPIKey)(&cfg) + assert.Equal(t, testAPIKey, cfg.apiKey) }) t.Run("WithURL", func(t *testing.T) { @@ -181,11 +206,10 @@ func TestEnvVars(t *testing.T) { }) t.Run("DD_API_KEY", func(t *testing.T) { - var validKey = "12345678901234567890123456789012" - os.Setenv("DD_API_KEY", validKey) + os.Setenv("DD_API_KEY", testAPIKey) defer os.Unsetenv("DD_API_KEY") cfg := defaultConfig() - assert.Equal(t, validKey, cfg.apiKey) + assert.Equal(t, testAPIKey, cfg.apiKey) }) t.Run("DD_SITE", func(t *testing.T) { From 32b4d37cd75540ba58e6610309b9c4e666e42132 Mon Sep 17 00:00:00 2001 From: sanchda Date: Wed, 9 Sep 2020 14:22:46 +0000 Subject: [PATCH 10/12] profiler: add Profiler startup tests for API key Creating a profiler should fail if the API key validity check fails, and it should succeed if it succeeds (equivalently, if the API key isn't needed). Added simple tests for these conditions. --- profiler/profiler_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/profiler/profiler_test.go b/profiler/profiler_test.go index 863bb97514..d0bfdcf7ed 100644 --- a/profiler/profiler_test.go +++ b/profiler/profiler_test.go @@ -58,6 +58,16 @@ func TestStart(t *testing.T) { assert.NotEmpty(t, activeProfiler.cfg.hostname) mu.Unlock() }) + + t.Run("options/GoodAPIKey", func(t *testing.T) { + _, err := newProfiler(WithAPIKey("12345678901234567890123456789012")) + assert.Nil(t, err) + }) + + t.Run("options/BadAPIKey", func(t *testing.T) { + _, err := newProfiler(WithAPIKey("aaaa")) + assert.NotNil(t, err) + }) } func TestStartStopIdempotency(t *testing.T) { From fd07880eb946e9c2dbfbfa12e5f67a3f76464193 Mon Sep 17 00:00:00 2001 From: David Sanchez <838104+sanchda@users.noreply.github.com> Date: Thu, 10 Sep 2020 07:05:12 -0500 Subject: [PATCH 11/12] profiler: streamline non-parametrized error As per @gbbr's suggestion Co-authored-by: Gabriel Aszalos --- profiler/profiler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiler/profiler.go b/profiler/profiler.go index 15767ccdce..7dd8f6ccbb 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -71,7 +71,7 @@ func newProfiler(opts ...Option) (*profiler, error) { } if cfg.apiKey != "" { if !isAPIKeyValid(cfg.apiKey) { - return nil, fmt.Errorf("API key has incorrect format") + return nil, errors.New("API key has incorrect format") } cfg.targetURL = cfg.apiURL } else { From c124f1e658391405ec9f1f945b10b45ca4e8f8e2 Mon Sep 17 00:00:00 2001 From: sanchda Date: Thu, 10 Sep 2020 12:14:21 +0000 Subject: [PATCH 12/12] profiler: fix import issue from previous commit --- profiler/profiler.go | 1 + 1 file changed, 1 insertion(+) diff --git a/profiler/profiler.go b/profiler/profiler.go index 7dd8f6ccbb..332cfa96d1 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -6,6 +6,7 @@ package profiler import ( + "errors" "fmt" "os" "runtime"