golang / oauth2

Go OAuth2
https://golang.org/x/oauth2
BSD 3-Clause "New" or "Revised" License
5.4k stars 991 forks source link

Refresh token expiry ignored #397

Open dimandzhi opened 5 years ago

dimandzhi commented 5 years ago

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 commented 4 years ago

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

jpriebe commented 4 years ago

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 commented 2 years ago

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 commented 2 years ago

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

weisdd commented 2 years ago

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 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 commented 2 years ago

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 commented 2 years ago

@dimandzhi ah, I wasn't aware that there's refresh_expires_in field in json (found in your commit https://github.com/dimandzhi/oauth2/commit/f55c7f4f378bf61ed922665a5455459ad06f01f8) 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 commented 7 months ago

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