Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refresh token expiry ignored #397

Open
dimandzhi opened this issue Sep 6, 2019 · 8 comments
Open

Refresh token expiry ignored #397

dimandzhi opened this issue Sep 6, 2019 · 8 comments

Comments

@dimandzhi
Copy link

dimandzhi commented Sep 6, 2019

What version of Go are you using (go version)?
go version go1.12.7 linux/amd64
What operating system and processor architecture are you using?
Linux version 4.15.0-60-generic (buildd@lgw01-amd64-030) (gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1)) #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019
What did you do?

var conf = oauth2.Config{
	ClientID:     clientID,
	ClientSecret: clientSecret,
	Endpoint: oauth2.Endpoint{
		AuthURL:   endpoint.AuthURL,
		TokenURL:  endpoint.TokenURL,
		AuthStyle: oauth2.AuthStyleInParams,
	},
}
var token, err = conf.PasswordCredentialsToken(ctx, username, password)
var client = conf.Client(ctx, token)
// long-term `client` usage

What did you expect to see?
Usual workflow with http.Client usage and usual http/connection error handling.
What did you see instead?
After 30 minutes got oauth2: cannot fetch token: 400 Bad Request Response: {"error":"invalid_grant","error_description":"Refresh token expired"} error.

Not only access token may have expiration time, but also refresh token itself. Currently, access token is automatically refreshed and its expiration time is exported as field. Unfortunately, there is no such logic for refresh token itself, which makes long-term usage of constructed Token or corresponding http.Client ...complicated.
Simply adding RefreshExpiresIn int32 `json:"refresh_expires_in"` field to tokenJSON and RefreshExpiry field to Token will give programmer ability to handle this scenario.
Also, adding such handler, automated even, to package would be even grater improvement.

truncated .../openid-connect/token response example
{
	"access_token": "...",
	"expires_in": 300,
	"refresh_expires_in": 1800,
	"refresh_token": "...",
	"not-before-policy": 0,
	"session_state": "...",
	"scope": "openid email profile"
}
@cgostuff
Copy link

cgostuff commented Jun 3, 2020

I am encountering this same exact issue now 8 months later. Is there any way to get around this problem?

@jpriebe
Copy link

jpriebe commented Jun 23, 2020

I just encountered this. Our oauth2 client kept trying to use the refresh token for 4 days beyond the expiration, so it never got a valid token once it got into this state (it had run for weeks without encountering this problem).

Strangely, it had used a valid oauth2 token just 1 minute before this happened. Timeline looks like this:

2020-06-18 19:46:57 - successfully uses access token
2020-06-18 19:47:25 - refresh token expires
2020-06-18 19:48:01 - successfully uses access token
... (many successful uses of access token)
2020-06-18 23:54:46 - successfully uses access token
2020-06-18 23:55:49 - gets error trying to refresh the access token (cannot fetch token: 401 Unauthorized / Invalid refresh token (expired))

From what I can tell, the oauth2 module would have never recovered from this state; it would keep presenting an expired refresh token to the server in perpetuity. We had to bounce the application to get it to start over.

Any workaround?

@sneko
Copy link

sneko commented Dec 16, 2021

After investigating to reuse the same TokenSource, it's true that if I cannot be aware of the refresh token being expired, I have no smart way to just ask for a brand new TokenSource.

My workaround: when having an error during a .Token() I consider it may be a refresh token expired, so I override my previous TokenSource with a brand new one. Resulting in having a new refresh token... until it expires again.

The ideal solution: if the library is able to send on upper layers the response (or a part of) of the provider, so we can detect or not if the refreshing has failed due to expired refresh token. But... it would be a real custom solution because each "OAuth2/OIDC providers" has their own logic to notified of this... too specific I agree.

I don't see a perfect solution for now 😞 . Would be interested to your thoughts as main maintainers @ScruffyProdigy @rakyll @bradfitz ?

Thank you,

@Halytskyi
Copy link

The middle of 2022... any solution for that?

@weisdd
Copy link

weisdd commented Jun 17, 2022

From what I see in RFC (https://datatracker.ietf.org/doc/html/rfc6749#section-5.1), refresh token expiration timeout is not shared with a client, so there's no timer you could potentially rely on. expires_in is for an access token.

Also, a refresh token can be revoked due to multiple reasons:

  • it was used more than X times (e.g. Revoke Refresh Token in Keycloak);
  • user session has hit a timeout (e.g. SSO Session Max and SSO Session Idle in Keycloak);
  • and so on.

It makes reliance on an expiration timer not realistic as the validity depends on internal settings of an IDP.

Refresh token may be present in a successful response (again, the section 5.1 of the RFC), and I think if there's a rotation done by an IDP, the change will be reflected by the library:

func (tf *tokenRefresher) Token() (*Token, error) {
	if tf.refreshToken == "" {
		return nil, errors.New("oauth2: token expired and refresh token is not set")
	}

	tk, err := retrieveToken(tf.ctx, tf.conf, url.Values{
		"grant_type":    {"refresh_token"},
		"refresh_token": {tf.refreshToken},
	})

	if err != nil {
		return nil, err
	}
	if tf.refreshToken != tk.RefreshToken {
		tf.refreshToken = tk.RefreshToken
	}
	return tk, err
}

Section 5.2 of the RFC (https://datatracker.ietf.org/doc/html/rfc6749#section-5.2) defines error types, amongst which invalid_grant is what you're looking for:

         invalid_grant
               The provided authorization grant (e.g., authorization
               code, resource owner credentials) or refresh token is
               invalid, expired, revoked, does not match the redirection
               URI used in the authorization request, or was issued to
               another client.

That's the error that the topic starter shared:
oauth2: cannot fetch token: 400 Bad Request Response: {"error":"invalid_grant","error_description":"Refresh token expired"}
You can see it in the test for oauth2:

func TestTokenRetrieveError(t *testing.T) {
	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if r.URL.String() != "/token" {
			t.Errorf("Unexpected token refresh request URL, %v is found.", r.URL)
		}
		w.Header().Set("Content-type", "application/json")
		w.WriteHeader(http.StatusBadRequest)
		w.Write([]byte(`{"error": "invalid_grant"}`))
	}))
	defer ts.Close()
	conf := newConf(ts.URL)
	_, err := conf.Exchange(context.Background(), "exchange-code")
	if err == nil {
		t.Fatalf("got no error, expected one")
	}
	_, ok := err.(*RetrieveError)
	if !ok {
		t.Fatalf("got %T error, expected *RetrieveError; error was: %v", err, err)
	}
	// Test error string for backwards compatibility
	expected := fmt.Sprintf("oauth2: cannot fetch token: %v\nResponse: %s", "400 Bad Request", `{"error": "invalid_grant"}`)
	if errStr := err.Error(); errStr != expected {
		t.Fatalf("got %#v, expected %#v", errStr, expected)
	}
}

Given the fact that Oauth2 defines different flows, and "Authorization Code Grant" flow requires user interaction like on the diagram here: https://datatracker.ietf.org/doc/html/rfc6749#section-4.1, IMO, it would be too much for the library to handle that. Instead, you're expected to inspect the error messages returned by the library, and if you see invalid_grant in the text representation of the error, then it's likely to be the refresh token expiration event. Once you see that, you can restart the authorization flow.
Assuming that you have a short SSO Session Idle timeout (can be named differently in different IDPs), you can just instruct your service to send dummy requests within that timeout, and each time access token is refreshed, you prevent that Idle Timeout from hitting you. Though, SSO Session Max is still going to be hit if configured.

As a final note, please, note that I'm not an Oauth2 expert, my investigation of the issue might not be entirely correct.

@dimandzhi
Copy link
Author

Well, as a workaround I was using a forked version of golang oauth2 package with added and exported refresh token expiry fields. Plus, wrote and used a package for renewing refresh token before expiration.
Initially, this was an issue for me, because I use Keycloak, but since version 12 there was a change in this behavior. The current oauth2 package is sufficient for that as is.
In response to @weisdd notes...
On the one hand, sure, oauth2 errors must not be ignored and used to indicate a need to retrieve a new token if it has been revoked or expired. It is enough to build logic around oauth2 usage.
But, on the other hand, in terms of efficiency and workflow clarity, why not use all available data given by .../openid-connect/token response? Or at least pass it to package users (export those fields that is). Besides, oauth2 package is using expiry information in its logic and not the error from the response.

// expired reports whether the token is expired.
// t must be non-nil.
func (t *Token) expired() bool {
	if t.Expiry.IsZero() {
		return false
	}
	return t.Expiry.Round(0).Add(-expiryDelta).Before(timeNow())
}

// Valid reports whether t is non-nil, has an AccessToken, and is not expired.
func (t *Token) Valid() bool {
	return t != nil && t.AccessToken != "" && !t.expired()
}

And that makes sense: why would I want to make another http(s) request to receive {"error":"invalid_grant","error_description":"Token has been expired or revoked"} if I can tell by expiry that token is definitely invalid and new token must be retrieved? Less traffic, more clarity. The same should be done to refresh token, that's my opinion.

@weisdd
Copy link

weisdd commented Jun 17, 2022

@dimandzhi ah, I wasn't aware that there's refresh_expires_in field in json (found in your commit dimandzhi@f55c7f4) as it's not mentioned in the RFC I was referring to. I guess, it's something implementation-specific, but, based on quick googling, still can be found across many IDPs.
Thanks for that info and for pointing at the Keycloak documentation, this is something new for me as well. :)

@Khoulaiz
Copy link

Two years later again...No work here, Bug is still open....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants