profiler: add API key check at initialization of Profiler object #718

sanchda · 2020-09-02T19:30:01Z

This change adds a new check which verifies the sanity of the user provided API key and returns an error if the key is invalid.

ref: PROF-1571

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.

Moved the API key check to profiler/profiler.go. An API key with an invalid format will now cause newProfiler to throw an error.

My mistake, didn't know we had gofmt as part of our ci.

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.

gbbr

Thanks for the work. Left a few pointers.

profiler/options.go

profiler/profiler.go

gbbr

Since this is an open-source project, please provide a proper description of the work done, the motivations, etc. in the description of the PR. An internal ticket number has very little meaning to an external contributor.

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

Instead, just don't print the API key.

gbbr

This is looking good now! Can we please also have tests for isAPIKeyValid? Table-tests is what you want here, it will make it easy. See https://github.com/golang/go/wiki/TableDrivenTests

We should also test that the error is returned, as it should, when trying to create a profiler (newProfiler) with a bad key.

gbbr · 2020-09-04T13:10:12Z

profiler/options_test.go

@@ -37,18 +37,20 @@ func TestOptions(t *testing.T) {
 	})

 	t.Run("WithAPIKey", func(t *testing.T) {
+		var validKey = "12345678901234567890123456789012"


Suggested change

var validKey = "12345678901234567890123456789012"

validKey := "12345678901234567890123456789012"

Also, maybe we can have just one global constant testAPIKey

Great suggestions.

Writing the tests revealed an issue with non-ASCII glyphs returning true by unicode.IsLower(), so this already paid off.

Also added a simple positive and negative check for Profiler instantiation.

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.

Just a few tests to cover some obvious edge-cases. There may be many more.

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.

gbbr

Fantastic 👍

profiler/profiler.go

@gbbr

As per @gbbr's suggestion Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>

gbbr · 2020-09-10T12:23:52Z

Thanks!

sanchda · 2020-09-10T12:26:34Z

Woohoo! Many thanks for the help and guidance on this one.

sanchda added 2 commits September 2, 2020 17:36

Add format-type API key check to profiler config

86bccc7

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: API key check throws error

adaf3f5

Moved the API key check to profiler/profiler.go. An API key with an invalid format will now cause newProfiler to throw an error.

sanchda requested a review from AlexJF September 2, 2020 19:30

sanchda added 3 commits September 2, 2020 19:33

profiler: stylistic changes to please gofmt

50dd443

My mistake, didn't know we had gofmt as part of our ci.

profiler: fix additional linter complaints

fac9de7

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: get local tests running for api check

3af39db

gbbr reviewed Sep 3, 2020

View reviewed changes

profiler/options.go Outdated Show resolved Hide resolved

profiler/options.go Outdated Show resolved Hide resolved

profiler/options.go Outdated Show resolved Hide resolved

profiler/options.go Outdated Show resolved Hide resolved

profiler/profiler.go Outdated Show resolved Hide resolved

gbbr suggested changes Sep 3, 2020

View reviewed changes

sanchda added this to the Unplanned milestone Sep 3, 2020

gbbr reviewed Sep 4, 2020

View reviewed changes

profiler/options.go Outdated Show resolved Hide resolved

profiler: remove API key sanitization

23e4c86

Instead, just don't print the API key.

gbbr reviewed Sep 4, 2020

View reviewed changes

sanchda added 3 commits September 9, 2020 13:36

profiler: update isAPIKeyValid to fail on extended ASCII input

4bf0ca5

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: add tests for isAPIKeyValid

2e3c600

Just a few tests to cover some obvious edge-cases. There may be many more.

profiler: add Profiler startup tests for API key

32b4d37

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.

gbbr previously approved these changes Sep 10, 2020

View reviewed changes

gbbr reviewed Sep 10, 2020

View reviewed changes

profiler/profiler.go Outdated Show resolved Hide resolved

profiler: streamline non-parametrized error

fd07880

As per @gbbr's suggestion Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>

sanchda dismissed gbbr’s stale review via fd07880 September 10, 2020 12:05

sanchda removed the request for review from AlexJF September 10, 2020 12:08

profiler: fix import issue from previous commit

c124f1e

gbbr modified the milestones: Triage, 1.27.0 Sep 10, 2020

gbbr added the profiler label Sep 10, 2020

gbbr approved these changes Sep 10, 2020

View reviewed changes

gbbr merged commit 7e9fc2b into v1 Sep 10, 2020

gbbr deleted the sanchda/profiling-apicheck branch September 10, 2020 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

profiler: add API key check at initialization of Profiler object #718

profiler: add API key check at initialization of Profiler object #718

sanchda commented Sep 2, 2020 •

edited by gbbr

Loading

gbbr left a comment

gbbr left a comment

gbbr left a comment •

edited

Loading

gbbr Sep 4, 2020

gbbr Sep 4, 2020

sanchda Sep 9, 2020

gbbr left a comment

gbbr commented Sep 10, 2020

sanchda commented Sep 10, 2020

	var validKey = "12345678901234567890123456789012"
	validKey := "12345678901234567890123456789012"

profiler: add API key check at initialization of Profiler object #718

profiler: add API key check at initialization of Profiler object #718

Conversation

sanchda commented Sep 2, 2020 • edited by gbbr Loading

gbbr left a comment

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

gbbr left a comment • edited Loading

Choose a reason for hiding this comment

gbbr Sep 4, 2020

Choose a reason for hiding this comment

gbbr Sep 4, 2020

Choose a reason for hiding this comment

sanchda Sep 9, 2020

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

gbbr commented Sep 10, 2020

sanchda commented Sep 10, 2020

sanchda commented Sep 2, 2020 •

edited by gbbr

Loading

gbbr left a comment •

edited

Loading