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

Auth: Check id token expiry date #69829

Conversation

akselleirv
Copy link
Contributor

What is this feature?

Verify that the ID token has not expired.

Why do we need this feature?

When using the feature toggle accessTokenExpirationCheck it only checks for the expiry date of the access token and not the ID token. When using AzureAD to authenticate it issues an ID token which expires before the access token and it results in the user experiencing 401.

Who is this feature for?

For users which uses accessTokenExpirationCheck.

Which issue(s) does this PR fix?:

Fixes #65380

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@akselleirv akselleirv requested a review from a team as a code owner June 9, 2023 07:19
@akselleirv akselleirv requested review from sakjur, papagian and zserge and removed request for a team June 9, 2023 07:19
@grafanabot grafanabot added area/backend pr/external This PR is from external contributor labels Jun 9, 2023
@CLAassistant
Copy link

CLAassistant commented Jun 9, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@akselleirv akselleirv force-pushed the fix/access-token-expiration-check-for-id-token branch from df64af3 to e7cf1c9 Compare June 13, 2023 09:08
@akselleirv akselleirv requested a review from a team as a code owner June 13, 2023 09:08
@akselleirv akselleirv requested review from academo and removed request for a team June 13, 2023 09:08
@akselleirv akselleirv force-pushed the fix/access-token-expiration-check-for-id-token branch 3 times, most recently from dc8ec83 to a0ff662 Compare June 13, 2023 09:13
@Jguer Jguer requested a review from mgyongyosi June 27, 2023 16:53
@Jguer
Copy link
Contributor

Jguer commented Jun 27, 2023

@mgyongyosi can you take a look?

@akselleirv I think your implementation is missing in the second path pkg/services/authn/authnimpl/sync/oauth_token_sync.go

The context handler path will be removed soon.

[feature_toggles]
authnService = true

can be used to enable the new path (although it should now be the default in main)

@akselleirv akselleirv requested a review from a team as a code owner June 29, 2023 06:04
@akselleirv akselleirv requested review from vtorosyan and removed request for a team June 29, 2023 06:04
@akselleirv
Copy link
Contributor Author

Hello @Jguer and @mgyongyosi,

I've added the expiry check to the handler you mentioned.
Let me know if I should it remove it from the original handler path.

Copy link
Contributor

@mgyongyosi mgyongyosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @akselleirv,

Thank you for your contribution to Grafana! Currently I'm working on this part, so expect some changes from upstream, but those should be minor. I left some feedback to you, could you please change your implementation based on those suggestions?

pkg/services/authn/authnimpl/sync/oauth_token_sync.go Outdated Show resolved Hide resolved
pkg/services/contexthandler/contexthandler.go Outdated Show resolved Hide resolved
pkg/services/authn/authnimpl/sync/oauth_token_sync.go Outdated Show resolved Hide resolved
pkg/services/authn/authnimpl/sync/oauth_token_sync.go Outdated Show resolved Hide resolved
@mgyongyosi mgyongyosi added this to the 10.1.x milestone Jul 12, 2023
@mgyongyosi mgyongyosi requested a review from Jguer July 14, 2023 14:01
@Jguer Jguer changed the title fix: check id token expiry date Auth: Check id token expiry date Jul 17, 2023
@Jguer Jguer assigned Jguer and linoman and unassigned Jguer Jul 17, 2023
@akselleirv akselleirv force-pushed the fix/access-token-expiration-check-for-id-token branch from 6f3a611 to c211271 Compare July 20, 2023 05:44
Copy link
Contributor

@linoman linoman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Hello @akselleirv and thank you for your contribution! As @Jguer and @mgyongyosi have suggested, you don't need to change anything at pkg/services/contexthandler/contexthandler.go since it will be removed soon. This is already blocked from being used by the feature flag that @Jguer mentioned.

Please revert it in order to approve and merge this PR.

Please review @mgyongyosi comments.

PS I left a small non-blocking code suggestion.

pkg/services/authn/authnimpl/sync/oauth_token_sync.go Outdated Show resolved Hide resolved
Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>
@akselleirv
Copy link
Contributor Author

Thank you for the the detailed feedback @mgyongyosi! I've updated the PR with your suggested changes :)

@linoman linoman modified the milestones: 10.1.x, 10.2.x Jul 24, 2023
* Remove unnecessary contexthandler changes
@mgyongyosi mgyongyosi force-pushed the fix/access-token-expiration-check-for-id-token branch from 9eb2389 to 059a7e9 Compare July 31, 2023 14:48
@mgyongyosi mgyongyosi merged commit 6d98d06 into grafana:main Aug 1, 2023
9 checks passed
@mgyongyosi
Copy link
Contributor

Stellar work on this @akselleirv! ⭐ Thank you again, your PR has been merged!

@akselleirv akselleirv deleted the fix/access-token-expiration-check-for-id-token branch August 1, 2023 09:41
sarahzinger pushed a commit that referenced this pull request Aug 1, 2023
* fixed: added id token expiry check to oauth token sync

* use go-jose and id token in cache

* Update pkg/services/authn/authnimpl/sync/oauth_token_sync.go

* refactored getOAuthTokenCacheTTL and added unit tests

* Small changes to oauth_token_sync

* Remove unnecessary contexthandler changes

---------

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>
Co-authored-by: Mihaly Gyongyosi <mgyongyosi@users.noreply.github.com>
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
* fixed: added id token expiry check to oauth token sync

* use go-jose and id token in cache

* Update pkg/services/authn/authnimpl/sync/oauth_token_sync.go

* refactored getOAuthTokenCacheTTL and added unit tests

* Small changes to oauth_token_sync

* Remove unnecessary contexthandler changes

---------

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>
Co-authored-by: Mihaly Gyongyosi <mgyongyosi@users.noreply.github.com>
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/auth area/backend no-backport Skip backport of PR pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grafana sends expired id token in X-Id-Token header to datasource when using "Forward OAuth Identity"
7 participants