Add access-token in proxy response headers#109
Add access-token in proxy response headers#109shrayolacrayon merged 5 commits intobuzzfeed:masterfrom
Conversation
internal/proxy/oauthproxy.go
Outdated
| } | ||
|
|
||
| req.Header.Set("X-Forwarded-User", session.User) | ||
| req.Header.Set("X-Forwarded-AccessToken", session.AccessToken) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;).
There was a problem hiding this comment.
That sounds good to me. Thank you for opening and working on this!
There was a problem hiding this comment.
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? |
|
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. |
epot
force-pushed
the
add-access-token
branch
2 times, most recently
from
bd46ae2 to
02862ee
Compare
epot
force-pushed
the
add-access-token
branch
from
02862ee to
f524ed5
Compare
|
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 |
|
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"` |
There was a problem hiding this comment.
can we add a simple unit test here for the new config value in options_test.go?
| } | ||
|
|
||
| req.Header.Set("X-Forwarded-User", session.User) | ||
|
|
There was a problem hiding this comment.
can we also add some test in oauthproxy_test.go to test that the right header is being set?
|
@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
force-pushed
the
add-access-token
branch
from
c37aee9 to
0514292
Compare
epot
force-pushed
the
add-access-token
branch
from
0514292 to
6b9d326
Compare
|
Ok @shrayolacrayon here we go! |
|
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:
or
In general, I agree that a per-upstream config is preferable. WDYT @shrayolacrayon? |
4 participants
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.