Auth Proxy: Include additional headers as part of the cache key #18298

gotjosh · 2019-07-30T12:19:23Z

What this PR does / why we need it:

Auth proxy has support to send additional user attributes as part of the
authentication flow. These attributes (e.g. Groups) need to be monitored
as part of the process in case of change.

This commit changes the way we compute the cache key to include all of the
attributes sent as part of the authentication request. That way, if we
change any user attributes we'll upsert the user information.

Which issue(s) this PR fixes:

Fixes #18276

Special notes for your reviewer:

There's a tiny refactor of an if/iflese statement to a switch as I feel this reads a bit better - I'm happy to take the commit if others feel is not relevant
I chose to encode the cache key for two main reasons: a) we can't be certain that what we receive is safe to use as a key and b) we're not certain that it is not sensitive data that could be stored as part of a cache log
Choice of base32 is because I wanted something to be filename and URL safe.

Auth proxy has support to send additional user attributes as part of the authentication flow. These attributes (e.g. Groups) need to be monitored as part of the process in case of change. This commit, changes the way we compute the cache key to include all of the attributes sent as part of the authentication request. That way, if we change any user attributes we'll upsert the user information.

gotjosh · 2019-07-30T12:55:38Z

pkg/middleware/auth_proxy/auth_proxy.go

+		key = strings.Join([]string{key, header}, "-") // compose the key with any additional headers
+	})
+
+	hashedKey := base32.StdEncoding.EncodeToString([]byte(key))


The downside to introducing the hashing is that this will force a cache bust for all users. Potentially causing a resource spike within big installations.

gotjosh · 2019-07-30T13:00:13Z

pkg/middleware/auth_proxy/auth_proxy.go

@@ -274,6 +283,21 @@ func (auth *AuthProxy) LoginViaHeader() (int64, error) {
 	return upsert.Result.Id, nil
 }

+// headersIterator iterates over all non-empty supported additional headers
+func (auth *AuthProxy) headersIterator(fn func(field string, header string)) {


I'm happy to hear an opinion on this approach. Is very common within Ruby's enumerable library. Originally, I tried a function that converts the keys to a map and then other functions could just use it but this approach is a tiny bit more efficient (O(n) as opposed O(2n))

xlson

LGTM!

I had originally planned for us to remove the caching for cases when auth proxy wasn't used in conjunction with LDAP. That's what we used to do, the cache was only introduced as a way to protect LDAP from being overloaded with requests. But, this should work just as well as it is based on the incoming data.

gotjosh · 2019-07-31T10:22:14Z

@xlson I thought about that. However, I ended up going this way due to a) the team sync code is non-trivial (it executes at least 3N database queries where N is the number of groups to sync) b) since this is an enterprise-only feature is more likely that this is used with larger installations of Grafana

The removal of the cache is simpler that's for sure. But felt like this approach was simple enough to reap most of the benefits.

* grafana/master: TablePanel: Remove scroll option on TablePanel (#18318) Keyboard Shortcuts: Sign in to enable them (#18271) GitHub Templates: Pull Request Template update (#18300) Auth Proxy: Include additional headers as part of the cache key (#18298)

* grafana/master: (82 commits) TablePanel: Remove scroll option on TablePanel (grafana#18318) Keyboard Shortcuts: Sign in to enable them (grafana#18271) GitHub Templates: Pull Request Template update (grafana#18300) Auth Proxy: Include additional headers as part of the cache key (grafana#18298) grafana/toolkit: support windows paths (grafana#18306) Chore: noImplicitAny Sub 500 errors (grafana#18287) Plugins: return a promise for loadPluginCss (grafana#18273) Utils: avoid calling console.warn() too often for deprecation warnings (grafana#18269) CLI: Allow installing custom binary plugins (grafana#17551) Docs: Update link to example app (grafana#18253) GettingStarted: Skip Query for getting started (grafana#18268) v6.3.0-beta2 is latest testing (grafana#18283) Release: Changelog update with v6.3.0-beta2 (grafana#18281) Chore: Upgrades typescript to version 3.5 (grafana#18263) docs: team sync (grafana#18239) SAML: Only show SAML login button on Enterprise version (grafana#18270) Permissions: Show plugins in nav for non admin users but hide plugin configuration (grafana#18234) CI: Change target branch in CI task trigger-docs-update (grafana#18255) Plugins: Include build number and PR in metadata (grafana#18260) Run End-to-End tests for release builds (grafana#18211) ...

* grafana/master: TablePanel: Remove scroll option on TablePanel (grafana#18318) Keyboard Shortcuts: Sign in to enable them (grafana#18271) GitHub Templates: Pull Request Template update (grafana#18300) Auth Proxy: Include additional headers as part of the cache key (grafana#18298) grafana/toolkit: support windows paths (grafana#18306) Chore: noImplicitAny Sub 500 errors (grafana#18287)

…ana#18298) * Auth Proxy: Include additional headers as part of the cache key Auth proxy has support to send additional user attributes as part of the authentication flow. These attributes (e.g. Groups) need to be monitored as part of the process in case of change. This commit changes the way we compute the cache key to include all of the attributes sent as part of the authentication request. That way, if we change any user attributes we'll upsert the user information. (cherry picked from commit ed8aeb2)

* Auth Proxy: Include additional headers as part of the cache key Auth proxy has support to send additional user attributes as part of the authentication flow. These attributes (e.g. Groups) need to be monitored as part of the process in case of change. This commit changes the way we compute the cache key to include all of the attributes sent as part of the authentication request. That way, if we change any user attributes we'll upsert the user information. (cherry picked from commit ed8aeb2)

Turns out, that behaviour was a bug we introduced as part of the LDAP sync. It was squashed as part of #18298.

gotjosh added 2 commits July 30, 2019 13:04

tiny refactor as I feel this reads a bit better

f670f6a

gotjosh requested a review from xlson July 30, 2019 12:19

Add middleware test

bd122be

gotjosh changed the title ~~Auth proxy cache key~~ Auth Proxy: Include additional headers as part of the cache key Jul 30, 2019

gotjosh added area/auth/authproxy add to changelog cherry-pick needed labels Jul 30, 2019

gotjosh commented Jul 30, 2019

View reviewed changes

xlson approved these changes Jul 31, 2019

View reviewed changes

gotjosh merged commit ed8aeb2 into master Jul 31, 2019

gotjosh deleted the auth-proxy-cache-key branch July 31, 2019 10:23

xlson added this to the 6.3.0-beta4 milestone Aug 2, 2019

xlson removed the cherry-pick needed label Aug 6, 2019

gotjosh mentioned this pull request Aug 7, 2019

Docs: Update Auth Proxy documentation #18444

Merged

gotjosh added a commit that referenced this pull request Aug 7, 2019

Docs: Update Auth Proxy documentation

5549b9f

Turns out, that behaviour was a bug we introduced as part of the LDAP sync. It was squashed as part of #18298.

gotjosh added a commit that referenced this pull request Aug 8, 2019

Docs: Update Auth Proxy documentation (#18444)

6c699c0

Turns out, that behaviour was a bug we introduced as part of the LDAP sync. It was squashed as part of #18298.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auth Proxy: Include additional headers as part of the cache key #18298

Auth Proxy: Include additional headers as part of the cache key #18298

gotjosh commented Jul 30, 2019

gotjosh Jul 30, 2019

gotjosh Jul 30, 2019

xlson left a comment

gotjosh commented Jul 31, 2019

Auth Proxy: Include additional headers as part of the cache key #18298

Auth Proxy: Include additional headers as part of the cache key #18298

Conversation

gotjosh commented Jul 30, 2019

gotjosh Jul 30, 2019

Choose a reason for hiding this comment

gotjosh Jul 30, 2019

Choose a reason for hiding this comment

xlson left a comment

Choose a reason for hiding this comment

gotjosh commented Jul 31, 2019