Add access-token in proxy response headers by epot · Pull Request #109

Add access-token in proxy response headers by epot · Pull Request #109 · buzzfeed/sso

Conversation

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.

		}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also Azure has 4kb access tokens. 😝

@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.

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?

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.

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

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

		CookieSecure bool `envconfig:"COOKIE_SECURE" default:"true"`
		CookieHTTPOnly bool `envconfig:"COOKIE_HTTP_ONLY"`

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And done as well!

		}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done!

@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!

Shraya Ramani added 2 commits

November 15, 2018 15:40

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.

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

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?