Add IDToken for Azure provider #258
Conversation
…n to ensure that the refresh token and expires on date are both being set
There was a problem hiding this comment.
Code changes look good!
Please add a note to the changelog if possible and for the cookie splitting issue, i would recommend using the new Redis based session storage backend, it resolves the issue of having to fit within the 4kb cookie limit. Could potentially add a note to the documentation about this, WDYT?
…xy into feature/azure-idtoken
…ith the size of the cookie session storage
|
I've added a note to both the documentation and the changelog |
|
We just tested this branch and ran into an issue with As you say in the conversation/documentation the cookie has been split into _oauth2_proxy_0, _oauth2_proxy_1, _oauth2_proxy_2. We are using the auth-request mode which seems to be the most common. Nginx has been configured with increased proxy buffer size. Where do we need to make changes now to make this work? This is how we configure our ingress: |
|
@iremmats What do the logs for the ingress controller say? The log from oauth2-proxy says it's not getting the cookie and the reason for that lies somewhere in nginx so those logs should hold the answer. |
|
@iremmats Split cookies with auth request mode can cause issues. Ensuring Nginx properly copies the set-cookie headers across has had many different solutions posted in issues on this repo for a while but it has not really been solved. Instead, the recommendation is to use redis for session storage, which solves the problem of large cookies by just issuing a session ticket rather than the full session, meaning no more split cookies |
Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>
|
Any idea when this will be merged? I'm looking forward to testing it out! Thanks. |
|
If we can get the conflicts resolved I think this will be good to go. |
|
@syscll I've resolved the conflicts if you're able to review it once more. Thanks. |
| @@ -5,6 +5,8 @@ | |||
| - [#227](https://github.com/pusher/oauth2_proxy/pull/227) Add Keycloak provider (@Ofinka) | |||
| - [#273](https://github.com/pusher/oauth2_proxy/pull/273) Support Go 1.13 (@dio) | |||
| - [#275](https://github.com/pusher/oauth2_proxy/pull/275) docker: build from debian buster (@syscll) | |||
| - [#258](https://github.com/pusher/oauth2_proxy/pull/258) Add IDToken for Azure provider | |||
| - This PR adds the IDToken into the session for the Azure provider allowing requests to a backend to be identified as a specific user. As a consequence, if you are using a cookie to store the session the cookie will now exceed the 4kb size limit and be split into multiple cookies. This can cause problems when using nginx as a proxy, resulting in no cookie being passed at all. Either increase the proxy_buffer_size in nginx or implement the redis session storage (see https://pusher.github.io/oauth2_proxy/configuration#redis-storage) | |||
There was a problem hiding this comment.
I feel like this note is definitely relevant but maybe the changelog is the wrong place? WDYT @JoelSpeed @steakunderscore?
There was a problem hiding this comment.
It's also been added to the Azure section of the Authentication documentation in this pull request. Obviously though this is very much worth highlighting to people as I'd imagine it will break a lot of configurations so if there is some where else we can put it to draw peoples attention to it let me know.
There was a problem hiding this comment.
I am happy to have a note like this in the changelog, it's important that people are aware of changes like this that may break their setups.
It's the sort of thing I'll copy out to highlight it in the release highlights when I cut the next release
There was a problem hiding this comment.
Totally happy for it to stay in the changelog 🙂
oauth2-proxy wasn't working with oauth2-proy 5.x.x until I tried with 4.0.0, this smells like related to a ~recent AD api change. NOTE that kubeprod is still using old AD api, see [2] below. Refs: - [1] "Add IDToken for Azure provider #258" oauth2-proxy/oauth2-proxy#258 - [2] "Switch Azure AD graph API to Microsoft Graph API" oauth2-proxy/oauth2-proxy@7f72a22
Description
This adds the IDToken to the session for the Azure provider.
Motivation and Context
This allows it to be passed as a bearer token in the Authorization header to an upstream such as the Kubernetes dashboard. It solves the issue in #131
How Has This Been Tested?
This has been tested against Azure AD to check that it returns the IDToken. It was tested with nginx in auth proxy mode to check that this token could be used to authenticate to the kubernetes dashboard.
As a result of the change the cookie being returned is larger and has to be split. This can cause an issue if using nginx to proxy requiring proxy buffer size be increased (see https://andrewlock.net/fixing-nginx-upstream-sent-too-big-header-error-when-running-an-ingress-controller-in-kubernetes/). I'm not sure therefore if something should be noted in the changelog regarding this.
Checklist: