-
Notifications
You must be signed in to change notification settings - Fork 2.9k
WebSocket Next: enable users to update SecurityIdentity before previous bearer access token expires #47675
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
base: main
Are you sure you want to change the base?
Conversation
michalvavrik
commented
May 3, 2025
- closes WebSocket-Next - Refresh OIDC AccessToken without reconnection #43434
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview 12c75e0 has been successfully built and deployed to https://quarkus-pr-main-47675-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
If you can see a way to do the work to support Quarkus LangChain4j fix without having to support the token injection, then please consider a dedicated PR - simply because, if that is considered a bug fix, it can be backported to 3.20 LTS, as this PR is unlikely to be backported |
My plan is to use the fact that this PR stores security support on the connection. It should fix it inherently, as either LangChain4j uses the same duplicate context where we have the connection, or a new duplicated context created from the original one (there is a hierarchy and this will be parent one). Anyway, I don't think it is important, the user that reported it didn't bother to add reproducer so I can't prove it is actual fix and fixing it after this get in is ok as this could be very edge case. Also, the way I plan to propose it (it may never get it), it could possibly provide further improvements, but not sort of code changes you want to backport. It is just idea, maybe never gets in. |
@michalvavrik Once the sample SPA sending a bearer token to HTTP upgrade is available, you can use it as a reproducer for the Quarkus LangChain4j issue too, so overall, it is worth it, not only to check the PR idea works, but also as a base for StepUp authentication and other experiments |
cbcc107
to
bd4f6a3
Compare
Here is working example quarkusio/quarkus-quickstarts#1534. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AuthenticationRequestContext authenticationRequestContext) { | ||
return authenticate(request.getCredential().getToken(), getRoutingContextAttribute(request)) | ||
.onItem().transformToUni(newIdentity -> { | ||
TokenIntrospection introspection = OidcUtils.getAttribute(newIdentity, OidcUtils.INTROSPECTION_ATTRIBUTE); |
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.
@michalvavrik I think we should start with enforcing that the sub
is available, either via TokenIntrospection
or JWT. You can check if it is JWT checking if newIdentity.getPrincipal()
is an instance of JsonWebToken
- if it is, the sub
claim must be available, if not, it must be in the introspection response.
That may not work with OAuth2 providers, but I believe we should start in a strict mode.
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.
The sub
claim check is now enforced.
...in/java/io/quarkus/websockets/next/runtime/spi/telemetry/WebSocketIdentityUpdateRequest.java
Outdated
Show resolved
Hide resolved
...in/java/io/quarkus/websockets/next/runtime/spi/telemetry/WebSocketIdentityUpdateRequest.java
Outdated
Show resolved
Hide resolved
This is very useful to have, it is easier to understand this PR with it for sure. |
I'd also appreciate if @cescoffier could have a look. Clement, I think this PR is good, the idea is that the expired token associated with the WS connection is optionally updated with the refreshed token on the same connection, without closing it. Otherwise, when the token expires, the connection is closed. My only remaining concern is that the bearer access token is updated on the WS connection via the front-channel, with the SPA forwarding it to Quarkus over the current WS connection. With I can't imagine right now any risks. The token refreshed on the connection undergoes the same security checks, and is also compared against the previous identity using a unique Please think about it when you get some time, also CC Martin |
We can't access |
bd4f6a3
to
0410ce0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0410ce0
to
f5e5b28
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@michalvavrik Right, I did not mean to suggest it must be implemented similarly to the Quarkus OIDC authorization flow, I was implying the security characteristics of refreshing tokens were different. |
Isn’t this close to the legacy oauth2 flow, where the token is sent to the client/spa? There are some security risks there. Also, what is required from the WebSocket client (especially in the browser)? |
if (result && connectionOpened) { | ||
console.log('Token updated, sending new token to the server') | ||
socket.send(JSON.stringify({ | ||
metadata: { |
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.
This is creating a specific protocol. It should be a WS subprotocol, I guess.
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 proposed subprotocol, but it does enforce concrete data structure and we cannot do this to users. We don't know how binary or even text messages look like and it is up to customers how they send metadata between FE and BE. I think this is much more flexible. This example your are commenting on is an example. You can use a different DTO.
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.
Sure, but if there is a well-established sub-protocol that does what we need, we should use that instead. I’m checking right now, but I can not find anything obvious.
...s/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/TenantSpecificOidcIdentityProvider.java
Outdated
Show resolved
Hide resolved
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 understand the idea, but we need to check if there is no existing WS subprotocol handling this.
If there is more than one backend microservice, than I don't believe users can use Quarkus OIDC web-app (authorization code flow on the Quarkus side) because they would have to authenticate with the every back-end service. So sending bearer access token must be pretty much standard? Now sending it with the WS is definitely less secure even over wss, I agree.
I'd like to learn more about these security risks, can you say more, please? Apart of having it unintentionally in access logs etc. I honestly don't see a difference to the current situation, refresh token is stored in the memory, that variable is only accessible in the ESM module. I am sure that sending token via headers using JS client has security risks, I am just trying to understand the increased risk by sending refreshed token as a message.
Here is working example with JS WS client quarkusio/quarkus-quickstarts#1534. I can be more specific in my answer, but I am not sure if I know what specifically you are asking about. JS WS client sends a new access token as a message.
I didn't find any "de-facto" standard (or any FWIW). But I don't think it means there is none, I just googled it, not an expert 🤷♂️ . |
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 just ran into this. Looking forward to this
f5e5b28
to
8c13f0b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8c13f0b
to
dac2304
Compare
I have just rebased on the current main as it has been a while, no changes. @sberyozkin @mkouba I think it is your turn :-) |
Status for workflow
|
Status for workflow
|