Add access-token in proxy response headers #109

epot · 2018-11-08T13:16:10Z

Problem

I need to do a collateral call to google APIs from my backend to get additional information about the user. For that, I need to have the access token.

Solution

Return the access token in the header key X-Forwarded-AccessToken.

Notes

Let me know if that is a good idea, and I'll make this tests are updated.

shrayolacrayon · 2018-11-08T15:32:33Z

internal/proxy/oauthproxy.go

@@ -1037,6 +1037,7 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er
 	}

 	req.Header.Set("X-Forwarded-User", session.User)
+	req.Header.Set("X-Forwarded-AccessToken", session.AccessToken)


I don't think we want want to pass the Access Token by default in plaintext in the header to the upstream as it seems like it could escalate to be a security vulnerability. Maybe we can figure out a way to make this a configured opt-in header to set.

What do you mean by Maybe we can figure out a way to make this a configured opt-in header to set? I have no idea how secure this has to be, I feared an answer like that :).

Similar to what is done in bitly/oauth2_proxy we could conditionally pass this in if the config file has something set like PassAccessToken. I think that passing the access token should be done with caution though, as an upstream could potentially do something like log it and create a security vulnerability.

Ah nice. Well, I would definitely use such option. If you want me to re-work this PR to offer this feature, let me know ;).

That sounds good to me. Thank you for opening and working on this!

Also Azure has 4kb access tokens. 😝

shrayolacrayon · 2018-11-08T20:42:04Z

@epot one question i have, is what kinds of requests are you trying to make with the google API? Are they queries to get more information about the authenticated user?
One other possible, and maybe more complicated solution that would be to have a set of enrichment APIs that sso can proxy with the google API, which would allow us to not have to pass the access token to the upstream at all.

epot · 2018-11-09T07:56:53Z

Yes that is exactly that: I want to get the user email, full name, profile picture... And possibly team if it's an enterprise account.
I thought about what you suggest, but we would then need to pass those information in all the http headers?

shrayolacrayon · 2018-11-09T21:58:59Z

This would probably be an endpoint on sso-proxy or sso-auth that the upstream would call. For now, since there is precedence for adding the access token in oauth2_proxy we can move forward with the first proposed solution.
I will also open an issue for the preferred abstraction, with the intention having that be the end solution, and eventually deprecating passing the access token in a header.

sporkmonger · 2018-11-10T00:48:10Z

The challenge w/ trying to do it all via extra endpoints rather than passing the token through is that this will vary by provider.

epot · 2018-11-12T14:14:11Z

Ok, let me know what you want me to add on top of what is already there to push this PR forward!

shrayolacrayon · 2018-11-12T18:16:30Z

internal/proxy/options.go

@@ -71,6 +72,8 @@ type Options struct {
 	CookieSecure   bool          `envconfig:"COOKIE_SECURE" default:"true"`
 	CookieHTTPOnly bool          `envconfig:"COOKIE_HTTP_ONLY"`

+	PassAccessToken bool `envconfig:"PASS_ACCESS_TOKEN" default:"false"`


can we add a simple unit test here for the new config value in options_test.go?

And done as well!

shrayolacrayon · 2018-11-12T18:18:34Z

internal/proxy/oauthproxy.go

@@ -1037,6 +1040,11 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er
 	}

 	req.Header.Set("X-Forwarded-User", session.User)
+


can we also add some test in oauthproxy_test.go to test that the right header is being set?

Sure, done!

shrayolacrayon · 2018-11-12T18:42:21Z

@sporkmonger I think you're right that there would need to be separate enrichment APIs for each provider, however I think that that the proposed abstraction is better from a security perspective than passing the access token in plain text to the upstream.

@epot added some requests for tests, then I think we can get this merged!

epot · 2018-11-13T10:20:02Z

Ok @shrayolacrayon here we go!

sporkmonger · 2018-11-21T21:16:16Z

I'm clearly a bit late, but just realized this PR implemented this globally rather than per-upstream. That seems like a mistake. I don't want to ship access tokens to every single application (expecting to be deploying on the order of like 30 services behind the proxy), but I have one or two that certainly would benefit from having it.

mreiferson · 2018-11-26T01:06:31Z

I think there are two ways to look at this:

It's a short-term solution in lieu of the alternative "enrichment" API proposal, in which case a global config might be reasonable

or

We don't actually think that the "enrichment" API proposal is viable, in which case I think a per-upstream config is warranted if we expect this to be a long-term solution

In general, I agree that a per-upstream config is preferable.

WDYT @shrayolacrayon?

Add access-token in proxy response headers

ba24f82

shrayolacrayon reviewed Nov 8, 2018

View reviewed changes

epot force-pushed the add-access-token branch 2 times, most recently from bd46ae2 to 02862ee Compare November 9, 2018 08:54

Add an option to send the access token

f524ed5

epot force-pushed the add-access-token branch from 02862ee to f524ed5 Compare November 9, 2018 08:57

shrayolacrayon mentioned this pull request Nov 9, 2018

*: create set of enrichment APIs for sso to proxy to providers #111

Open

shrayolacrayon reviewed Nov 12, 2018

View reviewed changes

epot force-pushed the add-access-token branch from c37aee9 to 0514292 Compare November 13, 2018 09:55

Add tests for new option

6b9d326

epot force-pushed the add-access-token branch from 0514292 to 6b9d326 Compare November 13, 2018 10:17

Shraya Ramani added 2 commits November 15, 2018 15:40

Merge branch 'master' into add-access-token

d10fb50

Merge branch 'master' into add-access-token

2511e5b

shrayolacrayon approved these changes Nov 16, 2018

View reviewed changes

shrayolacrayon merged commit 6c0f66f into buzzfeed:master Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add access-token in proxy response headers #109

Add access-token in proxy response headers #109

epot commented Nov 8, 2018

shrayolacrayon Nov 8, 2018

epot Nov 8, 2018

shrayolacrayon Nov 8, 2018

epot Nov 8, 2018

shrayolacrayon Nov 8, 2018

sporkmonger Nov 10, 2018

shrayolacrayon commented Nov 8, 2018

epot commented Nov 9, 2018

shrayolacrayon commented Nov 9, 2018

sporkmonger commented Nov 10, 2018

epot commented Nov 12, 2018

shrayolacrayon Nov 12, 2018

epot Nov 13, 2018

shrayolacrayon Nov 12, 2018

epot Nov 13, 2018

shrayolacrayon commented Nov 12, 2018

epot commented Nov 13, 2018

sporkmonger commented Nov 21, 2018

mreiferson commented Nov 26, 2018

		@@ -1037,6 +1040,11 @@ func (p OAuthProxy) Authenticate(rw http.ResponseWriter, req http.Request) (er
		}

		req.Header.Set("X-Forwarded-User", session.User)

Add access-token in proxy response headers #109

Add access-token in proxy response headers #109

Conversation

epot commented Nov 8, 2018

Problem

Solution

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrayolacrayon commented Nov 8, 2018

epot commented Nov 9, 2018

shrayolacrayon commented Nov 9, 2018

sporkmonger commented Nov 10, 2018

epot commented Nov 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrayolacrayon commented Nov 12, 2018

epot commented Nov 13, 2018

sporkmonger commented Nov 21, 2018

mreiferson commented Nov 26, 2018