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

Known issues with the id token grant type endpoint #412

Closed
kangmingtay opened this issue Mar 8, 2022 · 25 comments
Closed

Known issues with the id token grant type endpoint #412

kangmingtay opened this issue Mar 8, 2022 · 25 comments
Labels
bug Something isn't working

Comments

@kangmingtay
Copy link
Member

kangmingtay commented Mar 8, 2022

Bug report

The following thread aims to describe the issues with the /token?grant_type=id_token endpoint:

  1. [Google] IdToken returned by google does not contain a hashed nonce but currently, gotrue hashes the nonce in the request and checks if it matches the nonce in the id token payload.
  2. [Google] Does not require a nonce for the id_token flow based on the firebase-flutter integration guide.
  • nonce might not be required for mobile because we can check the app package name (appId) and the signature
  1. [Apple] Does not work if existing users were using "sign-in with apple" on the web initially (/authorize?provider=apple). Sign-in with apple using an id_token should sign users into the same account regardless of which flow is used.
@DanMossa
Copy link
Contributor

DanMossa commented Mar 19, 2022

Things we should double check.

  1. Is there a nonce in the returned idtoken? If so, can we use that nonce?
  2. Is it acceptable to remove a nonce requirement from the tokens endpoint?
  3. Is it worth trying to make a PR for the native Google sign in to return a nonce? (Probably not?)

@anggoran
Copy link

anggoran commented Mar 25, 2022

After some days trying some ways like decode and encode jwt to add nonce in payload (which is impossible since the signature already signed by Google...), I found out that nonce is not returned by default even in the native android google sign in code:
https://github.com/flutter/plugins/blob/main/packages/google_sign_in/google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

The idToken wasn't built by map but it was built by getIdToken() method. I think what we can do (if we want to edit the native code to return nonce), we can change that code on line 401 by set the nonce inside the getSignInIntent(), which is documented in this link:
GetSignInIntentRequestBuilder
SignInClient

That is from me, I've never done a PR, but I'm so interested with this topic xD. So maybe I can take some times to learn and help if there is any new suggestion or plan about this issue.

(The nonce issue on Android quite similar to iOS afaik)

@DanMossa
Copy link
Contributor

@anggoran Great research!

I found this
flutter/flutter#85439
And the fact that it's been marked as a new feature and p5 makes me feel like this is something that Google wants added eventually

Lol we might have to actually update the Google plugin

@anggoran
Copy link

anggoran commented Mar 27, 2022

Sorry I think I mentioned a different api. The google_sign_in package uses gms.auth.api.signin instead of gms.auth.api.identity. It means that if we want to insert nonce, we need to change the whole code and sacrifice some methods, since both api have very different approaches (in terms of methods availability). Maybe we have to choose one of these options:

• Change the whole google_sign_in Java code to utilize identity api
• Fork and create a new google_sign_in package that implement the identity api either from googleapis or google_sign_in Dart package or native Java package like what this unofficial package did :
Google One Tap Sign In Unofficial Package
• Use the above package which is unofficial
• Change nonce as nullable

The identity api provides two options, which are SignInWithGoogle for mobile and OneTapSignIn for web:
Google Identity Preview

(note: either flutter or react native package use signin api instead of identity api for Android and iOS)

For Web, there is a news regarding api migration:
Web Migration
flutter/flutter#88084

@DanMossa
Copy link
Contributor

DanMossa commented Mar 27, 2022

  1. Migrating the official sign in with Google plugin is definitely the best way, but I'm not sure if it's something I can/really want to try and do. The end of life is March 31st 2023 so it needs to be updated by then but I'd like to think someone else will have worked on it before then.

  2. It doesn't look like nonce is required for open id. You can see that in the spec. https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest but it's recommended from a security standpoint.

My thoughts on this are that we change nonce to optional since it's not a required field and it's incorrect for it to be required.
If you want to use a nonce, you can either use the third party package or wait for the official sign in with Google package to be updated.

I can start working on PRs to make nonce optional

@DanMossa
Copy link
Contributor

DanMossa commented Mar 27, 2022

Fix for this here!
#430

@k0shk0sh
Copy link

k0shk0sh commented May 1, 2022

going deeper into this, google SignIn on iOS returns a nonce unfortunately but we can't access this nonce to verify it on backend.

I receive this: Passed nonce and nonce in id_token should either both exist or not.

I tried passing the nonce from the jwt before sending the request but I got this msg:

invalid nonce

@k0shk0sh
Copy link

k0shk0sh commented May 1, 2022

on the other hand: apple signin: gives me this error msg: oidc: invalid configuration, clientID must be provided or SkipClientIDCheck must be set<…>

@megacherry
Copy link

@k0shk0sh: I have exactly the same issue and I wrote @kangmingtay about that via support. His answer was

"Hi,

Could you describe the errors you're currently facing? We have opened up a thread on the gotrue repo github issues here to document the known errors currently with this endpoint (#412).

Regarding your question on the OIDC providers supported, you can find them here: https://github.com/supabase/gotrue/blob/master/api/token.go#L83-L103

Hope this helps!"

I understand the error with Google Signin, because google hashes the nonce itself, so it's optional but i don't understand the Apple Auth error either.

@k0shk0sh
Copy link

k0shk0sh commented May 1, 2022

@k0shk0sh: I have exactly the same issue and I wrote @kangmingtay about that via support. His answer was

"Hi,

Could you describe the errors you're currently facing? We have opened up a thread on the gotrue repo github issues here to document the known errors currently with this endpoint (#412).

Regarding your question on the OIDC providers supported, you can find them here: https://github.com/supabase/gotrue/blob/master/api/token.go#L83-L103

Hope this helps!"

I understand the error with Google Signin, because google hashes the nonce itself, so it's optional but i don't understand the Apple Auth error either.

I gave up for now tbh. Its been 9 months of wait and after getting some hopes up, they are demolished now 😅.

@megacherry
Copy link

@kangmingtay anything new? :/

@J0
Copy link
Contributor

J0 commented May 30, 2022

Hey @megacherry,

Thanks for your patience with the issue so far. Appreciate it! Am aware that it can be frustrating to wait but we're doing our best to find an effective solution to the issue. We'll raise the issue in a team meeting.

In the meantime, it'd be really helpful if you could let us know what the ideal behavior of the id_token grant type endpoint would be as well as any alternatives(in addition to the ones by @DanMossa and @anggoran) that might work for you.

This would help us lay a path forward and best convey the issue to the other stakeholders so that we can resolve existing issues.

Thanks!

@DanMossa
Copy link
Contributor

@J0

There are currently 2 problems that exist. One is an issue with Google and the other is an issue with Supabase

The Google issue:

The issue is that the official Sign in With Google package for Flutter is doing things incorrectly for iOS. The package is using Google's deprecated APIs.

A hashed nonce is being returned even though a raw nonce is not being provided. This does not follow the openID specification.

There are discussions going on about migrating the package to the newer APIs that don't have this issue.

The Supabase issue:

When using native sign in, you need to create different client IDs for each device on Google Cloud Platform.

You have the option of selecting web, android, iOS, and a few more.

If you're using Google sign in on a website you have to select web.
If you're using Google sign in on an iOS device, you have to select iOS.

Luckily for Android, you can select web or android.

This means that I can select web, put that client id inside the Supabase dashboard, and I can now use Google sign in with for Android and Web.

But if I want to also use iOS then I'm out of luck, because Supabase only supports 1 clientID.

Supabase needs to be able to support multiple clientIDs.

@megacherry
Copy link

@J0

Unfortunately, I can't give any technical input on this, but do you have anything new about this issue?

@megacherry
Copy link

@J0 @kangmingtay

I have to ask again here. When do you think we can expect a solution for native Apple and Google Sign-in?

@DanMossa
Copy link
Contributor

Ok i'm officially at a point where I need to use Native sign in with Apple and I'm getting
oidc: invalid configuration, clientID must be provided or SkipClientIDCheck must be set Even thought I'm passing in a clientId. I'll look into it and see what's happening

@DanMossa
Copy link
Contributor

DanMossa commented Sep 28, 2022

@DanMossa
Copy link
Contributor

DanMossa commented Sep 28, 2022

#689
@k0shk0sh @kangmingtay @dshukertjr
This will fix broken native login for iOS.

@Xanewok
Copy link
Contributor

Xanewok commented Sep 30, 2022

I know you guys are hard at work with the MFA and whatnot so this might not be exactly in the scope, but the change in #689 is effectively a single line and unlocks the native Apple login; the PR has been up for 2 weeks now.

Do you think we could squeeze that in a subsequent release? 🙏

@dshukertjr
Copy link
Member

Just wanted to cross post this for visibility, but it seems like the current OAuth signin flow using web version of Oauth that we provide got rejected by Apple during the app's review process.
supabase/supabase-flutter#5 (comment)

@DanMossa
Copy link
Contributor

Just wanted to cross post this for visibility, but it seems like the current OAuth signin flow using web version of Oauth that we provide got rejected by Apple during the app's review process. supabase-community/supabase-flutter#5 (comment)

Yeah, that's because it's not using native sign in. But #689 fixes that.

@Xanewok
Copy link
Contributor

Xanewok commented Dec 15, 2022

For what it's worth, I don't think the third point from the OP applies anymore or I couldn't reproduce it:

  1. [Apple] Does not work if existing users were using "sign-in with apple" on the web initially (/authorize?provider=apple).

I had previous users signed in using the web version and they can properly log in using the now-unlocked native variant. Spitballing here, but maybe it's becase of the new account linking algorithm? c709ed5

In case it's useful, here's an example React Native native iOS sign in code that I've been using successfully: #689 (comment)

hf added a commit to supabase/auth-js that referenced this issue Feb 15, 2023
Adds `signInWithIdToken` as experimental. This brings back the ID token
login flow for [Sign in with
Apple](https://developer.apple.com/documentation/sign_in_with_apple/sign_in_with_apple_rest_api/authenticating_users_with_sign_in_with_apple)
and [Sign in with
Google](https://developers.google.com/identity/sign-in/android/backend-auth)
for iOS and Android apps.

It's marked as experimental since there are some
[known](supabase/auth#434)
[issues](supabase/auth#412) with this
endpoint, but we do realize the importance of having this method
available as Sign in with Apple is mandatory on iOS devices.
@sonipranjal
Copy link

going deeper into this, google SignIn on iOS returns a nonce unfortunately but we can't access this nonce to verify it on backend.

I receive this: Passed nonce and nonce in id_token should either both exist or not.

I tried passing the nonce from the jwt before sending the request but I got this msg:

invalid nonce

Hi @k0shk0sh, I am facing the similar issue, would be great if you could point me in some direction how should I resolve this!

I am facing the issue with google sign in, apple sign in works as I am able to pass in the nonce but I can't pass a nonce while making sign in request to google.

Related issue: react-native-google-signin/google-signin#1176

@egor-romanov
Copy link
Contributor

Closing the issue, as it seems like the original issue has been partially resolved or there is a working solution. I'd prefer someone to create a new issue with a fresh description if it's still an issue.
Please do not hesitate to ping me if it is still relevant, and I will happily reopen it.
Cheers!

@lukasnevosad
Copy link

For anyone still having issues with Google and nonce, this is the solution and I think it needs to be linked here: https://github.com/orgs/supabase/discussions/17801#discussioncomment-8003255

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests