WebSocket Next: enable users to update SecurityIdentity before previous bearer access token expires

[michalvavrik]

• closes WebSocket-Next - Refresh OIDC AccessToken without reconnection #43434

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

@michalvavrik

I have more plans for WS Next and while I didn't mention it (no reason to mention it), what I have added to this PR will allow me to easily fix quarkiverse/quarkus-langchain4j#1418 in the follow up.

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

[michalvavrik]

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.

[sberyozkin]

@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

[michalvavrik]

@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

Here is working example quarkusio/quarkus-quickstarts#1534.

[sberyozkin]

@michalvavrik

Here is working example quarkusio/quarkus-quickstarts#1534.

This is very useful to have, it is easier to understand this PR with it for sure.

[sberyozkin]

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 quarkus-oidc authorization code flow, if the refresh token happens, it is driven by Quarkus itself, avoiding the browser.

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 sub key.

Please think about it when you get some time, also CC Martin

[michalvavrik]

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 quarkus-oidc authorization code flow, if the refresh token happens, it is driven by Quarkus itself, avoiding the browser.

We can't access HttpOnly session cookie, therefore using Quarkus OIDC authorization code flow would require different mechanism, like once you are authenticated, return some unique one-time short-lived token that can be passed when the connection is opened and this way, we can link the OIDC session with newly opened WS connection. I think it requires a new Quarkus enhancement issue because it is not trivial.

[michalvavrik]

Isn’t this close to the legacy oauth2 flow, where the token is sent to the client/spa?

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.

There are some security risks there.

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.

Also, what is required from the WebSocket client (especially in the browser)?

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 understand the idea, but we need to check if there is no existing WS subprotocol handling this.

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 🤷‍♂️ .

[melloware]

I just ran into this. Looking forward to this

[michalvavrik]

I have just rebased on the current main as it has been a while, no changes. @sberyozkin @mkouba I think it is your turn :-)

[sberyozkin]

Thanks @michalvavrik, a few doc suggestions, please rebase and it shoukd be ready to go

[sberyozkin]

@michalvavrik I also propose to mark this feature experimental in case we need a few follow up updates

[michalvavrik]

Sorry, I just resolved merge conflicts and accidentally pushed it before addressing @sberyozkin points, looking at Sergey comments right now.

[michalvavrik]

@michalvavrik I also propose to mark this feature experimental in case we need a few follow up updates

done

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 93dd15d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 93dd15d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Integration Tests - JDK 17

📦 integration-tests/kafka-devservices

✖ io.quarkus.it.kafka.continuoustesting.DevServicesDevModeTest.testDevModeServiceDoesNotRestartContainersOnCodeChange - History

• expected: <false> but was: <true> - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertFalse.failNotFalse(AssertFalse.java:63)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:36)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:31)
	at org.junit.jupiter.api.Assertions.assertFalse(Assertions.java:231)
	at io.quarkus.it.kafka.continuoustesting.DevServicesDevModeTest.testDevModeServiceDoesNotRestartContainersOnCodeChange(DevServicesDevModeTest.java:81)

📦 integration-tests/opentelemetry

✖ io.quarkus.it.opentelemetry.MetricsTest.testAllJvmMetrics - History

• Metric should be greater than 0: jvm.memory.used value: 0.0 - java.lang.AssertionError

java.lang.AssertionError: Metric should be greater than 0: jvm.memory.used value: 0.0
	at io.quarkus.it.opentelemetry.MetricsTest.lambda$testAllJvmMetrics$6(MetricsTest.java:135)
	at java.base/java.lang.Iterable.forEach(Iterable.java:75)
	at io.quarkus.it.opentelemetry.MetricsTest.testAllJvmMetrics(MetricsTest.java:108)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.junit.QuarkusTestExtension.runExtensionMethod(QuarkusTestExtension.java:1000)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:848)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

---

⚙️ JVM Integration Tests - JDK 21

📦 integration-tests/kafka-devservices

✖ io.quarkus.it.kafka.continuoustesting.DevServicesDevModeTest.testDevModeServiceDoesNotRestartContainersOnCodeChange - History

• expected: <false> but was: <true> - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertFalse.failNotFalse(AssertFalse.java:63)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:36)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:31)
	at org.junit.jupiter.api.Assertions.assertFalse(Assertions.java:231)
	at io.quarkus.it.kafka.continuoustesting.DevServicesDevModeTest.testDevModeServiceDoesNotRestartContainersOnCodeChange(DevServicesDevModeTest.java:81)

[sberyozkin]

Thanks @michalvavrik