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

GenericOAuth: Set sub as auth id #65902

Merged
merged 2 commits into from Apr 5, 2023
Merged

Conversation

kalleep
Copy link
Contributor

@kalleep kalleep commented Apr 4, 2023

What is this feature?
When connecting with generic oauth we have never set authID in user_auth table.

This leads to some strange edge cases we need to handle to not create a new insert in user_auth table on every login,
see https://github.com/grafana/grafana/blob/main/pkg/services/authn/authnimpl/sync/user_sync.go#L323-L331

This prevents generic oauth to sync a new email because the email is always the primary identifier when using generic oauth. So if a user change their email in IDP a new user is created inside grafana.

If we use id token or userinfo endpoint the sub should always be present and can be used as a unique identifier. This will make it possible to be able to change the email for a user and still resolve it to the same one inside grafana. But in order for this to work properly the user first has to login with this changes in place to the auth id is correctly set.

I opted not to force the present of a sub initially because that can maybe break non oidc compatible idps, but we should enforce this in the future.

If we wan't to enforce it we should start with adding a deprecation log when a login is performed without the present of a sub claim

Fixes #64688

Special notes for your reviewer:

@kalleep kalleep added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Apr 4, 2023
@kalleep kalleep added this to the 9.6.0 milestone Apr 4, 2023
@kalleep kalleep requested a review from Jguer April 4, 2023 11:18
@kalleep kalleep requested a review from a team as a code owner April 4, 2023 11:18
@kalleep kalleep self-assigned this Apr 4, 2023
@kalleep kalleep requested review from danielkenlee and removed request for a team April 4, 2023 11:18
@kalleep kalleep requested review from gamab and mgyongyosi April 4, 2023 11:36
@zerok zerok modified the milestones: 9.6.0, 10.0.0 Apr 4, 2023
@mgyongyosi mgyongyosi changed the title GenericOAuth: Set sud as auth id GenericOAuth: Set sub as auth id Apr 5, 2023
@kalleep kalleep merged commit 7cd6018 into main Apr 5, 2023
7 checks passed
@kalleep kalleep deleted the kalleep/generic-oauth/auth-id branch April 5, 2023 15:24
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
@leona-ya
Copy link

Actually I think this needs to be backported to all 9.x versions. With the security fix (CVE-2023-3128) it's necessary for all users to have an valid id. As far as I understand the change and the 9.x code, there is no way when using the generic_oauth login provider to set the required ID.

@IevaVasiljeva IevaVasiljeva added the backport v9.5.x Bot will automatically open backport PR label Aug 14, 2023
@grafana-delivery-bot
Copy link
Contributor

Hello @IevaVasiljeva!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@IevaVasiljeva IevaVasiljeva added the product-approved Pull requests that are approved by product/managers and are allowed to be backported label Aug 14, 2023
@IevaVasiljeva IevaVasiljeva removed the backport v9.5.x Bot will automatically open backport PR label Aug 14, 2023
@IevaVasiljeva IevaVasiljeva added the backport v9.5.x Bot will automatically open backport PR label Aug 14, 2023
grafana-delivery-bot bot pushed a commit that referenced this pull request Aug 14, 2023
* GenericOAuth: Set sud as auth id

* GenericOAuth: Extract function to reduce complexity

(cherry picked from commit 7cd6018)
IevaVasiljeva pushed a commit that referenced this pull request Aug 14, 2023
GenericOAuth: Set sub as auth id (#65902)

* GenericOAuth: Set sud as auth id

* GenericOAuth: Extract function to reduce complexity

(cherry picked from commit 7cd6018)

Co-authored-by: Karl Persson <kalle.persson@grafana.com>
grafana-delivery-bot bot added a commit that referenced this pull request Aug 17, 2023
GenericOAuth: Set sub as auth id (#65902)

* GenericOAuth: Set sud as auth id

* GenericOAuth: Extract function to reduce complexity

(cherry picked from commit 7cd6018)

Co-authored-by: Karl Persson <kalle.persson@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend backport v9.5.x Bot will automatically open backport PR no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes product-approved Pull requests that are approved by product/managers and are allowed to be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic OAuth: Use sub claim
7 participants