-
Notifications
You must be signed in to change notification settings - Fork 374
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
feat: Support for OAuth callback proxy. #725
feat: Support for OAuth callback proxy. #725
Conversation
Corresponds with: supabase/auth#725
e850e24
to
2581929
Compare
Just force pushed the removal of a debugging change I'd introduced that reported to consumers the underlying OAuth error. Not a bad change per se, but unnecessary/unrelated as far as this PR goes. |
Hope this gets approved, or at least the core functionality needed to finally put this issue to rest. |
Would this solve the issue with OAuth Authentication with Server Side GoTrue Libraries? |
2581929
to
605bc4a
Compare
@kangmingtay I've addressed the issue with FB auth not using the proxy - identified by static analysis. If you're able to authorize the GHA workflow again that'd be most appreciated. |
@tschuehly I'm not in that Discord. However, no, this doesn't change anything to-do with the JWT token. What's delivered to the proxy is the authorization code, which isn't exchanged by the proxy, rather it's forwarded on to GoTrue to exchange as per usual. If you want to access the JWT auth and refresh tokens server side you can submit them there yourself and store them in a cookie. Or you can use Supabase's auth-helper library which does precisely that https://github.com/supabase/auth-helpers/blob/c81d74910800573069b31a9576e69e2c7dd20554/packages/react/src/components/UserProvider.tsx + https://github.com/supabase/auth-helpers/blob/c81d74910800573069b31a9576e69e2c7dd20554/packages/nextjs/src/handlers/callback.ts EDIT: Oh, unless you mean the Google tokens, not Supabase. Although if that's the case I'm not too sure what the benefit of using GoTrue/Supabase is. |
Hey @Benjamin-Dobell thank you for this PR! The team is a bit busy with some priorities regarding MFA so it may take us a bit longer to do a proper review of this. There's also some internal effort (not part of this team) on supporting custom domains. I'll check on this and let you know. |
proxy := query.Get("proxy") | ||
|
||
if proxy != "" { | ||
ctx = withExternalProxy(ctx, proxy) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of thoughts on this:
- I'm not sure the name
proxy
sufficiently explains what the purpose of the param is. A better alternative may be something closer toredirect_via
orredirect_through
. - There's a class of security vulnerabilities for Unvalidated Redirects and Forwards that need to be guarded for here. This value needs to be parsed as a URL and then matched to the
SiteURL
and the other glob patterns before being accepted for processing. - Depending on the output of the above, either an error should be returned, or the default mechanism be used. (I personally feel the latter is the right choice.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a class of security vulnerabilities for Unvalidated Redirects and Forwards that need to be guarded for here. This value needs to be parsed as a URL and then matched to the SiteURL and the other glob patterns before being accepted for processing.
I mentioned this in a response below. But should probably add it here where there's more context.
It's the OAuth provider that redirects to the proxy URL. Supabase hands it to the OAuth provider instead of https://<project-ref>.supabase.co/auth/v1/callback
. So the proxy URL needs to be added as an authorized URL with the OAuth provider, I'm not sure there's any additional value in Supabase itself performing validations (from a security perspective) - although, I'm more than happy to be told I'm wrong about this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the name proxy sufficiently explains what the purpose of the param is. A better alternative may be something closer to
redirect_via
orredirect_through
.
Sorry, forgot to address this. I'm not in any way attached to the name proxy
. Happy to make this change 👍 The only thing is technically the URL doesn't need to redirect. It could be a reverse proxy to Supabase to save on the redirect - but yeah, your average user really isn't going to want to set that up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the OAuth provider that redirects to the proxy URL. Supabase hands it to the OAuth provider instead of
https://<project-ref>.supabase.co/auth/v1/callback
. So the proxy URL needs to be added as an authorized URL with the OAuth provider, I'm not sure there's any additional value in Supabase itself performing validations (from a security perspective) - although, I'm more than happy to be told I'm wrong about this!
The assumption is that the system is secure, which often opens up the door for security vulnerabilities. It's often the case when building auth software that you need to take extra precautions everywhere. The practice is called Defense in Depth where you layer multiple independent checks for any assumption.
Imagine a misconfigured or vulnerable OAuth provider: a user has forgotten a redirect domain or has a typo in it. Now it's easy to misconfigure the OAuth provider, but it's not easy to change the hosting location of your Supabase project. So the existing solution has 2 checks for the misconfigured OAuth case.
By introducing the proxy you need to add one additional compensating check, as a guard to a misconfigured OAuth provider. Because with the proxy you can now change (and even from a frontend call) the "hosting URL" very easily. You can compensate for this by validating the URL that GoTrue receives. If it is a trusted URL then you can forward the request to the OAuth provider, otherwise you do nothing. Both the OAuth misconfiguration and the GoTrue misconfiguration must align for an attacker to abuse the system.
What I mean is that I can extend the library to get a JWT with that Google Authentication Token. My problem is that currently the OAuth Login is entirely client side. When logging in I receive the JWT as an Anchor Tag. I would like to use supabase this way as it simplifies my authentication with Spring Security immensely compared to the js client. |
@Benjamin-Dobell I did some preliminary review and it's looking great. We usually don't accept PRs that change functionality in a significant way without any tests. It would be great if you could add those, as it would take a while before the team can help you out directly on this. Here are some suggestions for test scenarios:
I'm anticipating a few other complications just voicing them now tho need further thought.
|
Hey @tschuehly there's a still WIP (but not far off) (PR) official guide on SSR. Do have a read through it and leave some comments if you want to. |
@hf Thanks for the feedback. Busy atm, but I'll try get on those tests sooner rather than later.
Would you mind elaborating a little on this one? The proxy should be redirecting back to GoTrue with all params intact, namely the The proxy itself on the other hand has no mechanism to evaluate the authenticity of the data it's receiving. However, it shouldn't be doing anything with the data, just naively redirecting back to GoTrue. The proxy URL needs to be registered as an approved OAuth callback URL with OAuth providers themselves, so that ought to be secure too i.e. you drop in the proxy URL instead of If Supabase staff want to have a play (sorry, not an invitation to the public at large 😉) then feel free to try the Google OAuth flow at https://app.staging.beta.camp/join. This is using a self-hosted GoTrue hosted on fly.io, talking to a regular managed Supabase instance. As to how I got that all working, don't ask... It's fragile, like, real fragile. |
Think of it this way: GoTrue today has a trust relationship with the OAuth providers. It is incredibly unlikely that any of the providers on the list will, out-of-the-blue, call into GoTrue with "valid" credentials in an attempt to impersonate a user. However, by introducing this proxy you are breaking the trust relationship. Now, the proxy acts as a person in the middle that is able to store, forward, replay or spoof credentials. Even though we advise users to keep the proxy secure, or only have a redirect mechanism, it is difficult to base trust on this assumption. Thus, GoTrue should be able to verify the trust it has placed in the proxy. Usually this is done by correlating a login request (
Hope this makes sense? |
I guess I don't understand the threat vector here. But also, I'm not a security engineer, so like, there's that 😆 Not trying to be difficult. It's entirely possible I'm just missing something! If you're willing, I'd love to learn. The GoTrue
Isn't this covered by OAuth providers themselves i.e. when the authorization code is exchanged? If you hang onto an authorization code for too long it'll be rejected. You also can't replay the authorization codes, they're single use.
Shouldn't the |
You're not being difficult. I could also be wrong, so having this discussion is worth it!
Yes those things are exchanged, but they are exchanged with a really trustworthy party. Virtually all of the OAuth providers supported today are very secure and can be trusted to do the right thing. So GoTrue does not enforce too many additional checks because of this. From that point of view, say Twitter had an OAuth breach, they could impersonate virtually any person on any app on earth. However, by adding the proxy you're essentially taking that trust and adding yet another component that is very difficult to secure and maintain properly. (Not that it can't be, it's just that it won't have the resources Twitter has to do that.) It is thus my thinking that GoTrue should try to do the right thing and enforce some additional checks if this approach is being used. Like, add an additional layer of security to make attacks that abuse the proxy even more difficult. We want to have GoTrue be "secure by default" as much as possible, to fail safe instead of open.
It is but that does not take into account a proxy in the middle that could be potentially actively abusing the flow. Do note that we're trying to prevent user accounts stored in GoTrue from being abused and not accounts on the social login provider. Assume that the proxy is malicious, then it can take an authorization code received from the OAuth provider, send it to a third party and then return to the legitimate user some error. The malicious party invokes then the callback as if they were the legitimate user. Without some additional checks, the malicious party would be able to impersonate the user account in GoTrue, while the legitimate user is unable to access the account.
Exactly, the proxy can store, forward or do whatever it wants to the authorization code (so long as it is valid) while leaving the legitimate user out to dry.
It is not doing much today because the trust in the OAuth providers as-is is assumed; but the security model changes drastically if you place a proxy that does not have the balance sheet of Google to dispose of. 😅 All of the above being said, I'm not proposing a solution, just raising something that needs to be checked and considered when opening up the existing solution to yet another party that was not accounted for in the previous implementation. My gut feeling is that some slight modification of the state parameter and adding maybe some simple form of browser binding may just be sufficient to make such attacks very difficult to pull off (at scale). I know some of these chains of steps to get to an exploit sometimes sound ludicrous and really far-fetched, but that's precisely how many of them occur in the wild. I can easily come up with very plausible situations about the proxy getting hacked in basically any language or framework and become a conduit for account takeovers with phisihng, social engineering or just plain theft of OAuth authorization codes. While yes, this is ultimately the responsibility of the owner of the proxy or app, we need to at least provide some basic sanity checking that it's being used as intended. |
Wanted to bump and say that I am interested in this and thanks for everyone working on it :) |
bump |
Hoping that this change can be merged ASAP. Really appreciate the effort on this issue! |
hey everyone, with custom domains available on Supabase, I don't think this is needed anymore. You can check out our custom domains guide here for more information. |
@kangmingtay As long as custom domains support works for multiple domains, then that's fine with me. Does it? |
Custom domain support supports only one domain at a time for now. |
This MR is still needed. supabase/supabase#12429 (comment) |
Just dropping in to provide a quick update. I absolutely still want this feature. Support for multiple domains is very important for us since we're already using Supabase this way. So I do plan on rebasing this. However, I'm super swamped and will be for at least the next 2 weeks. That is to say, if someone else wants to champion this - whether that's another community contributor or a Supabase employee, then I'm cool with someone else taking over. If not, I will get around to this, but not as swiftly as I probably ought to. |
All good @Benjamin-Dobell - no pressure! Thanks so much for your efforts, much appreciated. |
## What kind of change does this PR introduce? * Improves on #725, albeit with a slightly different approach * Gotrue will accept an allow list of domains via a comma-separate string (`DOMAIN_ALLOW_LIST`) , which includes the `API_EXTERNAL_URL` by default. On each request, gotrue will check that the domain being used is also included in the allow list. * When gotrue starts up, it will take the `DOMAIN_ALLOW_LIST` and convert it into a map where the key is the hostname and the value is the url * When a request is made to gotrue, gotrue will check the `DomainAllowListMap` to check if there is a matching hostname before allowing the request through. If there isn't a matching hostname used, gotrue will default to use the `API_EXTERNAL_URL` instead. * This helps to make gotrue usable with multiple custom domains, and also allows the email links to contain the custom domain. * Since the `EXTERNAL_XXX_REDIRECT_URI` is derived during runtime, we can remove that config once this PR is merged in as long as the `REDIRECT_URI` is also included in the `DOMAIN_ALLOW_LIST` --------- Co-authored-by: Joel Lee <lee.yi.jie.joel@gmail.com>
Has been stale for about a year now. |
What kind of change does this PR introduce?
This addresses a major pain point for hosted Supabase consumers whereby Google et al. OAuth consent screens indicate you're logging into
<gibberish>.supabase.co
, rather than your app.GoTrue clients may now optionally supply a "proxy" param that is provided to external providers as the callback, rather than the
GOTRUE_EXTERNAL_<PROVIDER>_REDIRECT_URI
(which is not configurable unless you're self hosting).The proxy end-point is something Supabase consumers implement and lives at their app's domain, and simply redirects back to GoTrue (or rather Kong). It'd make sense to implement this end-point in https://github.com/supabase/auth-helpers so any consumers of those libraries obtain this functionality for free.
What is the current behavior?
Addresses the auth component of supabase/supabase#12429
What is the new behavior?
For this to work we do two (related) things.
proxy
query param at/authorize
. We give this to providers as the callback URL, instead of<id>.supabase.co/auth/v1/callback
.proxy
in ourstate
JWT. This is done so that when GoTrue's/callback
is called (post proxy redirect) we're able to retrieve this URL and include it as theredirect_uri
when exchanging the auth code.Additional context
JS PR: supabase/auth-js#466
Closes supabase/supabase#142