-
Notifications
You must be signed in to change notification settings - Fork 674
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
SOLR-16896 Add support of OAuth 2.0/OIDC 'code with PKCE' flow (front-end) #1791
SOLR-16896 Add support of OAuth 2.0/OIDC 'code with PKCE' flow (front-end) #1791
Conversation
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 think there are a bunch of console.log
that could be console.error
if its an error or console.debug
if its just debug logging.
use some console.debug/error instead of console.log
Thanks @risdenk |
I’m on vacation so please proceed without my review. I’m thrilled to see this be completed. Ok to support impl flow for 9.x but it should be removed for 10.0 |
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.
Read through with some comments.
A pity we don't have unit tests for frontend, cause there are some of this which is hard to test, especially error cases.
I have not spun this up yet. May have a chance to test it with a KeyCloak protected cluster next week, where we currently use Solr with imiplicit flow.
Using a standard js lib?
I see you implemented PKCE flow with custom JS, which was not as bad/large as I feared it would be. Still, I have a feeling we should delegate all of this to some standard JS lib at some point, which would buy us token background refresh capabilities as well as a more tested and tried impl. When I had a look at this a few years ago I found some OpenID js libs, but most of them tendend to be coupled with React or abandoned. I jumped back and found that oidc-client-ts seems to be well maintained.
Couldn't find an easy way to integrate third party libraries without some heavy changes in the front-end code. Given we don't have unit tests and the non maintainability of AngularJS, I thought it'd better, cleaner an easier to maintain/debug if we code it ourselves, especially this is straightforward logic (catch the returned code from the redirection and use it to make a second call to get the tokens.) |
Use server side default value of 'authorization_flow'
I tested in a real KeyCloak idp environment today, with my custom image based on this branch, which is a merge of backend and frontend PRs plus the patch from the comment above. This all worked nicely, pulling the token endpoint from |
Backend PR merged, meaning we are unblocked to finalize this too. I'm going to mark the JIRA as blocker for 9.4 since the ref guide already promises the functionality. |
@laminelam Not much left on this now, do you need help with anything? |
Hi @janhoy |
I think it's good for a merge now. |
…t-end) (#1791) Co-authored-by: Lamine Idjeraoui <[email protected]> Co-authored-by: Jan Høydahl <[email protected]> (cherry picked from commit 086dcbe)
https://issues.apache.org/jira/browse/SOLR-16896
Description
Solr’s JWT authentication plugin uses implicit flow to request Access & ID tokens via OIDC/OAuth 2.0.
Due to its inherent security weaknesses, such as potential exposure of access tokens in the browser's history or the risk of leakage at the redirect stage, the Implicit flow has fallen out of favor. Its usage has been deprecated in OAuth 2.1, and many OIDC/OAuth 2.0 providers no longer support it. A decade ago, Implicit flow was the only practical way to retrieve tokens in a single call through browser redirection when cross-origin requests were blocked to shield applications from cross-site scripting attacks.
However, the development of Single Page Applications (SPAs) and advancements in modern browsers' handling of CORS requests have made the Implicit flow obsolete. Current applications and Content Security Policies (CSPs) can be configured to permit CORS requests.
OAuth 2.0 offers various flows (protocols) suited to different use cases. The Authorization Code Flow, recommended for SPAs and native apps, is available in two variants:
Authorization Code Flow with a secret: This involves server-side communication with the authorization server, necessitating secure secret storage. The retrieved tokens are then shared with the client.
Authorization Code Flow with PKCE: Given that the source code of native apps and SPAs is accessible to client devices, storing the secret client-side is impractical. The PKCE solution facilitates an exchange of a verifier code between the client and the authorization server, granting access to the tokens. Hence the acronym: Proof Key for Code Exchange (PKCE).
Given that the Solr admin Webapp is an SPA, this contribution employs the PKCE method. However, we can incorporate support for the Authorization Code Flow with a secret, while still reusing the front-end code, to allow these two options to coexist in a configurable manner.
This implementation is expected to enhance security by mitigating the risk of token interception, ensuring tokens are delivered to the intended client, and providing a safer overall user authentication process.
More details are available in the below resources:
OAuth 2.0 RFC (implicit flow)
PKCE RFC
The State of the Implicit Flow in OAuth2
Why the implicit flow is no longer recommended
This PR represents the "front-end" part of the contribution. See the "back-end" PR
The PKCE code flow process retrieves tokens in two stages:
For the sake of backward compatibility, the 'implicit flow' is maintained and established as the default mode.
One outstanding question is: how can we incorporate the /token URL into the list of allowed URLs in the CSP connect-src directive? For now, this is manually added in server/etc/jetty.xml.’
Tests
Test cases added to the back-end PR.
Front-end app does not support unit testing.
Extensive tests were done on the browser.
Ref guide to be updated.
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.