Use GitLab's refresh_token during Refresh.

[dhaus67]

Overview

Use GitLab's refresh_token instead of acces_token during connector.Refresh when offline_access is specified.

What this PR does / why we need it

Closes #2316

Special notes for your reviewer

Does this PR introduce a user-facing change?

Fix GitLab connector to use refresh_tokens with `offline_access`

[nabokihms]

The code looks good, thank you.

I have only one concern about migration. What will happen for users with offline sessions with access tokens? Whether this PR invalidates all currently issued refresh tokens? It seems like a severe breaking change to me.

If possible, I would like to offer support for both access and refresh tokens in the connector data to make the migration smoother.

[dhaus67]

@nabokihms sorry for being AFK so long. I’ve updated the code with the following changes:

• Store access_token as well as refresh_token in the connector data.
• When refreshing, first try the access_token (supporting old versions of GitLab + users who still choose non-expiring access_tokens)
• If an issue occurred related to unauthorized, try again by redeeming the refresh_token.

This should make things hopefully smoother for existing users.

[nabokihms]

@dhaus67 thanks. Code scanning errors do look irrelevant to this PR. I am going to test the code manually. If everything is ok, I will merge it.

[nabokihms]

After manually checking this PR, I have a couple of concerns left. We need to discuss them before merging.

[dhaus67]

Addressed your comments, if you have the time PTAL @nabokihms

[nabokihms]

The code looks good, thank you.

While testing this PR, I have found a bug. Dex fails to refresh the token on concurrent requests.

Dex logs:

{"level":"error","msg":"failed to refresh identity: gitlab: failed to get refresh token: oauth2: cannot fetch token: 400 Bad Request\nResponse: {\"error\":\"invalid_grant\",\"error_description\":\"The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client.\"}","time":"2022-03-05T04:13:54Z"}
{"level":"error","msg":"failed to refresh identity: gitlab: failed to get refresh token: oauth2: cannot fetch token: 400 Bad Request\nResponse: {\"error\":\"invalid_grant\",\"error_description\":\"The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client.\"}","time":"2022-03-05T04:13:54Z"}
{"level":"error","msg":"failed to refresh identity: gitlab: failed to get refresh token: oauth2: cannot fetch token: 400 Bad Request\nResponse: {\"error\":\"invalid_grant\",\"error_description\":\"The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client.\"}","time":"2022-03-05T04:13:54Z"}
{"level":"error","msg":"failed to refresh identity: gitlab: failed to get refresh token: oauth2: cannot fetch token: 400 Bad Request\nResponse: {\"error\":\"invalid_grant\",\"error_description\":\"The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client.\"}","time":"2022-03-05T04:13:54Z"}

Gitlab logs:

Started POST "/oauth/token" for **** at 2022-03-05 07:13:54 +0300
Started POST "/oauth/token" for **** at 2022-03-05 07:13:54 +0300
Started POST "/oauth/token" for **** at 2022-03-05 07:13:54 +0300
Started POST "/oauth/token" for **** at 2022-03-05 07:13:54 +0300
Processing by Oauth::TokensController#create as HTML
  Parameters: {"grant_type"=>"refresh_token", "refresh_token"=>"[FILTERED]"}
Processing by Oauth::TokensController#create as HTML
  Parameters: {"grant_type"=>"refresh_token", "refresh_token"=>"[FILTERED]"}
Processing by Oauth::TokensController#create as HTML
  Parameters: {"grant_type"=>"refresh_token", "refresh_token"=>"[FILTERED]"}
Processing by Oauth::TokensController#create as HTML
  Parameters: {"grant_type"=>"refresh_token", "refresh_token"=>"[FILTERED]"}
Started POST "/oauth/token" for 95.217.82.131 at 2022-03-05 07:13:54 +0300
Processing by Oauth::TokensController#create as HTML
  Parameters: {"grant_type"=>"refresh_token", "refresh_token"=>"[FILTERED]"}
Completed 400 Bad Request in 36ms (ActiveRecord: 14.7ms | Elasticsearch: 0.0ms | Allocations: 2815)
Completed 400 Bad Request in 35ms (ActiveRecord: 15.0ms | Elasticsearch: 0.0ms | Allocations: 3238)
Completed 400 Bad Request in 57ms (ActiveRecord: 29.3ms | Elasticsearch: 0.0ms | Allocations: 9069)
Completed 200 OK in 61ms (Views: 0.3ms | ActiveRecord: 12.8ms | Elasticsearch: 0.0ms | Allocations: 11997)
Completed 400 Bad Request in 47ms (Views: 0.3ms | ActiveRecord: 21.4ms | Elasticsearch: 0.0ms | Allocations: 8327)

I assume it is because Gitlab uses refresh token rotation - it returns a new refresh token on each refresh. The solution to this problem is not apparent, and it should not be specific for each connector.

Let's hold on a little with this PR until we figure out how to fix it.

[nabokihms]

After careful consideration, I agree to merge this PR.

My only concern is that because Gitlab rotates refresh tokens, Dex has to send a refreshing request to the external connector only once. However, Dex sends as many as it receives concurrent requests, leading to errors and forcing users to authenticate again.

This is not a problem with the Gitlab connector. Other callback-based connectors, e.g., generic OIDC connector or GitHub connector for different providers, can also act the same way.

Because Gitlab 15.0 is already out and Cloud Gitlab already does not support infinity access tokens, we can merge this PR now and fix refreshing for all connectors in generic later.

@dhaus67, thank you for your work and your patience.

[nabokihms]

@sagikazarmark, if you are with me on this, could you please take it to v2.32?

[dhaus67]

Thanks for the update @nabokihms !
I'd be happy to help with the generic fix for RefreshConnectors.

[phoerious]

Bump. Can we merge this, pretty please?

[wacuuu]

Hi,
sorry to bother, but when can we expect to see this in a release?

[sagikazarmark]

The current target milestone is scheduled for July 18, 2022. If it doesn't get delayed, this PR will be part of the next release.

[phoerious]

July 18 was yesterday. Any news?

[nabokihms]

@phoerious hello! Yeah, sorry about that. We are a little bit off schedule because of the PTO season. I moved the release date one week forward.

[nabokihms]

This PR is approved and I tested it on 50 Dex installations. There are some issues, yet it looks like solving them is not required to merge this PR.

Thank your for the contribution, @dhaus67! LGTM.