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

WebSockets Next: Show how to map request headers to Authorization header that OIDC Bearer token authentication require #45809

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Jan 23, 2025

  • related to: quarkus-websockets-next connecting from browser with JWT #42824
  • what I try with this PR: we know there are users that insist on using WS JS API https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API and don't know how to configure Quarkus to use it, so I want to show them, but I also want to make it absolutely clear that it is not a recommendation, we must not say: this is a way to go
  • for more context, please see discussions in linked issue, I waited for a while till it was clearer there are no more ideas or suggestion
  • I have tested it in Quickstarts WebSockets Next, I added OIDC, enabled OIDC Dev Svc, got token in DEV UI and passed it as query param to WS JS client and
const urlSearchParams = new URLSearchParams(window.location.search);
const token = urlSearchParams.get("bearer")
socket = new WebSocket("ws://" + location.host + "/chat/" + username, ["bearer", token]);

that is not in the content of this PR because I don't suggest it, users have proper way to get tokens, I just give you steps to test this PR

Copy link

quarkus-bot bot commented Jan 23, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 53bde96.

✅ 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.

You can consult the Develocity build scans.

Copy link

quarkus-bot bot commented Jan 23, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 53bde96.

Failing Jobs

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Failures Logs Raw logs Build scan
Documentation Build Build ⚠️ Check → Logs Raw logs 🚧

@michalvavrik
Copy link
Member Author

michalvavrik commented Jan 23, 2025

I'll not waste CI resources and fix docs build failure till I hear from reviewers because I have feeling this could go many ways, maybe even closing this PR completely.

==== Bearer token authentication

The xref:security-oidc-bearer-token-authentication.adoc[OIDC Bearer token authentication] expects that bearer tokens are passed in the `Authorization` header during the initial HTTP handshake.
While we recommend to use WebSocket clients that support setting custom headers in the initial HTTP handshake, there are users that rely on https://websockets.spec.whatwg.org/#the-websocket-interface[WebSockets API].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding that anyone writing JavaScript-based WS applications has no choice but to use that API, right ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it makes sense to have 2 sub-sections, one for WS Client, another for WS API, right now it reads like using WS API is quite a bad idea

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding that anyone writing JavaScript-based WS applications has no choice but to use that API, right ?

My understanding is there are some JS clients that also implements WS and supports that. One that seem mostly trustworthy is websockets/ws#1925 (pointing to the issue because it is clear they are setting headers on the client side) https://github.com/websockets/ws/blob/master/lib/websocket.js#L743. The issue with this is that some users think like I would: do my employer trust this in production and why?

IMHO it makes sense to have 2 sub-sections, one for WS Client, another for WS API, right now it reads like using WS API is quite a bad idea

I thought we need to make very clear line between what is possible and what we suggest (nothing, you are on your own). We can use to use WebSockets API, but I think it requires additional concepts like CORS and custom protocol that has message types and we would authenticate on one of these message types. If you have idea, please suggest specific test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalvavrik IMHO it makes sense to make it clear how users who have no other option but to use WS API can approach it, as opposed to us discouraging them, with some warnings added.
Look at Quarkus LangChain4j demos, chatbots are everywhere, opened from scripts, they can't use Quarkus Java WS Client.
We should not be recommending specific JS client libraries, as under the hood the probably do these workarounds anyway, and I agree, we don't want to take risk and guide users to use specific libraries and then be asked why did we do it...

As far as using WS API is concerned, we should give simple suggestions:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense @sberyozkin , I'll rewrite this later in the afternoon. Thank you

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @michalvavrik, take your time please

private static final String TOKEN_PREFIX = BEARER_PROTOCOL + ",";
void observe(@Observes Router router) {
router.get().order(-250).handler(routingContext -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, would it make sense for WS-Next provide such a handler for users not having to write this code ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #42824 (comment), it can be discussed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO optionally enabling a handler which does a header conversion according the documented scheme is all right, it is not a workaround for users who have to use WS API

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it would be easier to understand than ask users to add this handler. We must be clear in docs about security aspects, that's all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we just explain what users who really need to retain tokens can do, may be we can offer some basic support like having this handler enabled if requests, and ask users take WS API security concerns seriously

@michalvavrik
Copy link
Member Author

I thought about it more and I'll drop closing: # issue because maybe we should follow up with different work, not sure if it will ever happen. Spring seems to have integration with SocketJS https://docs.spring.io/spring-security/reference/servlet/integrations/websocket.html#websocket-authorization-notes-messagetypes that does authentication when connection has been established, but that won't integrate with what we have now.

@sberyozkin
Copy link
Member

sberyozkin commented Jan 23, 2025

@michalvavrik Hi Michal,

I'll not waste CI resources and fix docs build failure till I hear from reviewers because I have feeling this could go many ways, maybe even closing this PR completely.

It should definitely not be closed, it is a step in the right direction, this is the most often asked question, how to get the token passed from Java Script WS API, thanks for initiating this work, it is a big deal, IMHO, it might need a bit more work, but I'm actually very positive about your PR.

By the way, setting it to Draft is the right way if you expect a few iterations with the PR

@michalvavrik
Copy link
Member Author

@michalvavrik Hi Michal,

I'll not waste CI resources and fix docs build failure till I hear from reviewers because I have feeling this could go many ways, maybe even closing this PR completely.

It should definitely not be closed, it is a step in the right direction, this is the most often asked question, how to get the token passed from Java Script WS API, thanks for initiating this work, it is a big deal, IMHO, it might need a bit more work, but I'm actually very positive about your PR.

By the way, setting it to Draft is the right way if you expect a few iterations with the PR

Thanks Sergey, I can make it draft, but yesterday I took it as far as I can without feedback. So I feel like I need @mkouba and @sberyozkin to comment when you have time.

@michalvavrik michalvavrik marked this pull request as draft January 23, 2025 11:31
@mkouba
Copy link
Contributor

mkouba commented Jan 23, 2025

@michalvavrik Hi Michal,

I'll not waste CI resources and fix docs build failure till I hear from reviewers because I have feeling this could go many ways, maybe even closing this PR completely.

It should definitely not be closed, it is a step in the right direction, this is the most often asked question, how to get the token passed from Java Script WS API, thanks for initiating this work, it is a big deal, IMHO, it might need a bit more work, but I'm actually very positive about your PR.
By the way, setting it to Draft is the right way if you expect a few iterations with the PR

Thanks Sergey, I can make it draft, but yesterday I took it as far as I can without feedback. So I feel like I need @mkouba and @sberyozkin to comment when you have time.

I'll take a look tomorrow...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants