Closed JoelSpeed closed 6 years ago
I'm testing this now in a build with #534 and #464 applied as well, container here: viafoura/oauth2-proxy:oidc-1
So far so good, waiting for my token to time out so I can see it refresh. :)
Probably should have mentioned that I also built an image 😅
We have a branch at Pusher (https://github.com/pusher/oauth2_proxy/tree/kubernetes) that I'm maintaining which has all of my PRs in (#464, #534, #620) and I've published an image on quay: quay.io/joelspeed/oauth2_proxy:kubernetes-2
We are now running all three PRs in production and I've been testing locally with Dex/OAuth Proxy with 1-minute tokens.
A regular call to ouath2/auth
takes about 10ms and I'm seeing the refresh calls take around 100ms, if anyone is interested
Nice. I checked my merged branch against yours and see some minor variation, not sure if there's a change missing somewhere. For some reason I can't get a good diff view in github, so I'm uploading a patch here, just wanted to run by you in case it means something is missing from one of the PRs?
@jhohertz I added a commit to the end of #464 to add some documentation which you are missing,
You're also using the copy of this branch pusher/oidc-refresh-bitly
which won't work in conjunction with #534, my plan was to wait and see which of this and 534 gets merged first and fix the other one up appropriately, you need the commits from pusher/oidc-refresh
And the final change is a line I never meant to commit that I... err... erased from history (I'm bad, I know 😆)
If I were you I would just take the pusher/kubernetes
branch and push it over the top of yours
@JoelSpeed : Thanks for taking the time to explain. Now on a build from your kubernetes branch, will let you know if I run into any issues.
@JoelSpeed I seem to be having issues with refresh, though I suspect it may be a bad configuration on my part vs. a problem w/ the code. From the logs (w/ token/email obfuscated):
2018/06/20 18:26:00 oauthproxy.go:727: 100.100.0.5:33598 ("10.2.8.52") refreshing 1h18m1s old session cookie for Session{email:joe@domain.com user:joe token:true id_token:true expires:2018-06-21 17:07:58 +0000 UTC refresh_token:true} (refresh after 1h0m0s)
2018/06/20 18:26:00 internal_util.go:60: GET ?access_token=abcdefghijkl...
2018/06/20 18:26:00 internal_util.go:61: token validation request failed: Get ?access_token=abcdefghijklmnopqrstuvwxy: unsupported protocol scheme ""
I have dex (with your google PRs) running as the oidc provider.
My guess is one of the url params of the proxy I am not setting is required for this? (redeem? unsure what the right endpoint on dex for this would be)
@jhohertz just to double check, do you have offline_access
set as part of your scope
???
And also what timings have you got for the cookie_refresh
setting on the oauth proxy and the token expiry on dex?
I do have offline _access
in my scope, here's the flags I set for oauth2-proxy:
- --cookie-secure=true
- --cookie-expire=168h0m
- --cookie-refresh=60m
- --cookie-domain=.{{ cluster_name }}
- --whitelist-domain=.{{ cluster_name }}
- --email-domain={{ gapps_domain_name }}
- --set-authorization-header=true
- --http-address=0.0.0.0:4180
- --provider=oidc
- --client-id=kubernetes
- --client-secret={{ dex_oauth2_client_secret }}
- --oidc-issuer-url=https://dex.{{ cluster_name }}
- --scope=openid email profile groups offline_access
- --skip-provider-button=true
For Dex... it looks like I must be using defaults, as I am not setting any timeout values in my config. I will have a look.
@jhohertz I've been having a look at this and have found the problem.
If you don't actually need to refresh, the RefreshSessionIfNeeded
returns false, nil
, but this then triggers a ValidateSessionState
which the OIDC provider doesn't override.
The default ValidateSessionState
won't work with OIDC and so causes a failure (there is no validate URL for OIDC)
To fix this (properly) I could override the ValidateSessionState
for the OIDC provider, but to make that work properly, I need the changes from #534 since you need to have the id_token
to verify that session properly.
The alternative that I have come up with is a bit of a cheeky hack,
Replacing the start of RefreshSessionIfNeeded
with:
// Can't refresh without the refresh token
if s == nil || s.RefreshToken == "" {
return false, nil
}
// If id_token hasn't expired, no need to refresh, just verify
if s.ExpiresOn.After(time.Now()) {
ctx := context.Background()
_, err := p.Verifier.Verify(ctx, s.IdToken)
if err != nil {
return false, fmt.Errorf("unable to verify id_token: %v", err)
}
return true, nil
}
But not without the verification step if not using #534
Any maintainers around to weigh in on this?
I think I might create a branch that does this properly, but I may have to merge #534 and #620 into a bigger OIDC rewrite PR
An alternative quick fix is to make sure your cookie_refresh
== Dex token lifetime (by default 24h
)
Closing this in favour of #621
Thanks @jhohertz for pointing out the refresh bug here
Allows tokens to be refreshed within the OIDC provider.
This means that users will no longer have to re-authenticate when the ID Token expires but instead the OAuth proxy will refresh automatically in the background.
Make sure to add
offline_access
to the requestscope
when using this (else you won't request a refresh token from the upstream provider).I'd also recommend setting the
cookie_refresh
parameter to the same duration as your token lifetime.Can be used in conjunction with #534 using the branch https://github.com/pusher/oauth2_proxy/tree/oidc-refresh