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: security integration #40312

Closed
mkouba opened this issue Apr 26, 2024 · 15 comments · Fixed by #40534
Closed

WebSockets Next: security integration #40312

mkouba opened this issue Apr 26, 2024 · 15 comments · Fixed by #40534
Assignees
Milestone

Comments

@mkouba
Copy link
Contributor

mkouba commented Apr 26, 2024

Description

Follow-up of #39142.

Implementation ideas

No response

@sberyozkin
Copy link
Member

Hi @mkouba, can you explain a little bit how one can retain the property (such as a token) which is available at the on-open step in the WS-Next, such that this property is returned with subsequent on-message steps ?
If there was some code/test showing how it can be done for some custom header, let's say X-WebSocket HTTP header is available at the upgrade/onOpen time, and then it is bound to the WS channel and this header is accessible in the onMessage methods, then it would be ideal.

I guess we'll have to follow a step similar to the one shown in https://github.com/quarkusio/quarkus/files/15130826/quarkus-sec-issue.zip (a reproducer for #40307)

Another question, is resolving #40307 must be done first before planning this enhancement work ?

Thanks Sergey

@michalvavrik
Copy link
Member

michalvavrik commented May 2, 2024

I was thinking the authentication must have HTTP request scope rather than to be session-scoped bean. Which would allow us to rely on what we have already. But that's how far I got in studying WS-next reference. Let's see what Martin thinks.

Another question, is resolving #40307 must be done first before planning this enhancement work ?

nope, not related

@mkouba
Copy link
Contributor Author

mkouba commented May 2, 2024

I was thinking the authentication must have HTTP request scope rather than to be session-scoped bean. Which would allow us to rely on what we have already. But that's how far I got in studying WS-next reference. Let's see what Martin thinks.

The problem is that for WebSockets there is only the initial handshake HTTP request and afterwards, after upgrade, when processing incoming messages there's no HTTP request available at all; e.g. we do activate the request context for an @OnTextMessage callback but it's not backed by an HTTP request.

Another question, is resolving #40307 must be done first before planning this enhancement work ?

nope, not related

It's not related to WebSockets at all actually ;-)

@michalvavrik
Copy link
Member

michalvavrik commented May 2, 2024

The problem is that for WebSockets there is only the initial handshake HTTP request and afterwards, after upgrade, when processing incoming messages there's no HTTP request available at all; e.g. we do activate the request context for an @OnTextMessage callback but it's not backed by an HTTP request.

Thanks. I guess I need to dig more before I make any conclusions. But AFAICT CDI request context activation, we don't really care as what I wanted to reuse is HTTP Authenticator / Authorizer and they do work with RoutingContext rather than CDI req. ctx. I mean one connection == one authentication. However if WS will send authN headers later, we need something smarter. Okay.

@mkouba
Copy link
Contributor Author

mkouba commented May 2, 2024

The problem is that for WebSockets there is only the initial handshake HTTP request and afterwards, after upgrade, when processing incoming messages there's no HTTP request available at all; e.g. we do activate the request context for an @OnTextMessage callback but it's not backed by an HTTP request.

Thanks. I guess I need to dig more. But AFAICT CDI request context activation, we don't really care as what I wanted to reuse is HTTP Authenticator / Authorizer and they do work with RoutingContext rather than CDI req. ctx. I mean one connection == one authentication.

I guess I need to dig more as well :-)

However if WS will send authN headers later, we need something smarter. Okay.

It's not that it sends the headers later. In fact, WS itself does not send metadata along with messages. That's up to the procotol which is something we don't control.

Maybe we could authenticate/authorize the initial handshake request?

FYI the opening handshake is described here: https://datatracker.ietf.org/doc/html/rfc6455#section-1.3

BTW this the security part of the Jakarta WS spec: https://jakarta.ee/specifications/websocket/2.1/jakarta-websocket-spec-2.1#security

@michalvavrik
Copy link
Member

However if WS will send authN headers later, we need something smarter. Okay.

It's not that it sends the headers later. In fact, WS itself does not send metadata along with messages. That's up to the procotol which is something we don't control.

Maybe we could authenticate/authorize the initial handshake request?

Yes, that sounds good to me ATM (see below)

FYI the opening handshake is described here: https://datatracker.ietf.org/doc/html/rfc6455#section-1.3

BTW this the security part of the Jakarta WS spec: https://jakarta.ee/specifications/websocket/2.1/jakarta-websocket-spec-2.1#security

Thank you, I'll study it.

Sorry @mkouba I didn't mean to steal @sberyozkin show, could you please also answer his question when the time is right for you. This was helpful though.

@mkouba
Copy link
Contributor Author

mkouba commented May 2, 2024

Hi @mkouba, can you explain a little bit how one can retain the property (such as a token) which is available at the on-open step in the WS-Next, such that this property is returned with subsequent on-message steps ? If there was some code/test showing how it can be done for some custom header, let's say X-WebSocket HTTP header is available at the upgrade/onOpen time, and then it is bound to the WS channel and this header is accessible in the onMessage methods, then it would be ideal.

I have to admit that I have no idea yet ;-). In any case, we can extract the information from the initial handshake request and store it either in a @SessionScoped bean or directly in the connection object.

I guess we'll have to follow a step similar to the one shown in https://github.com/quarkusio/quarkus/files/15130826/quarkus-sec-issue.zip (a reproducer for #40307)

But this reproducer does not provide a working solution, or?

Another question, is resolving #40307 must be done first before planning this enhancement work ?

Thanks Sergey

@sberyozkin
Copy link
Member

Thanks @michalvavrik @mkouba, and yeah, was a good joke, Michal, that it was my show :-)

The use case which I have in mind, and is particularly relevant to the Quarkus LangChain4j Chat Bot demos, is when I authenticate to Quarkus with OIDC, and now I request a Chat Bot resource which upgrades the request to the WS Next session.
At this point, at the time of the initial handshake, Quarkus Security must've already created a Security Identity, and this is what I guess we need to retain.

So what Martin said,

In any case, we can extract the information from the initial handshake request and store it either in a @SessionScoped bean or directly in the connection object.

Sounds good and hopefully simple enough, IMHO this is the path forward. I'm not sure what is best though, using the session scoped bean or the connection object as a storage. The only requirement is to have @Inject SecurityIdentity available from the @Authenticated onMessage handlers.

@mkouba @michalvavrik, what do you think ?

@sberyozkin
Copy link
Member

sberyozkin commented May 3, 2024

using the session scoped bean or the connection object as a storage

The latter is probably better as it will let users avoid having to use @SessionScoped or is @SessionScoped a prerequisite for WS Next ?

I've read https://quarkus.io/guides/websockets-next-reference#cdi-scopes-for-websocket-endpoints, looks like @SessionScoped is not required, it is implicitly managed.

@mkouba
Copy link
Contributor Author

mkouba commented May 3, 2024

using the session scoped bean or the connection object as a storage

The latter is probably better as it will let users avoid having to use @SessionScoped or is @SessionScoped a prerequisite for WS Next ?

I've read https://quarkus.io/guides/websockets-next-reference#cdi-scopes-for-websocket-endpoints, looks like @SessionScoped is not required, it is implicitly managed.

Each connection has its own session context. When a server/client endpoint callback is executed this session context is activated automatically. The advantage of the latter approach is that you could access the connection outside an endpoint callback invocation. Which might be helpful but not necessary.

@sberyozkin
Copy link
Member

sberyozkin commented May 3, 2024

Thanks @mkouba,

The advantage of the latter approach is that you could access the connection outside an endpoint callback invocation. Which might be helpful but not necessary.

Interesting, I believe with Quarkus LangChain4j, users get responses in scope of the callback. I'm not certain if retaining it in the connection object might increase the likelihood of the identity going stale. I suppose we can start with storing it in the session scope bean, and move it to the connection if necessary or make it configurable.

How would we get the security info stored in the WS Next session scope bean though, can we add a quarkus-security dependency only to it for WS Next to check the availability of SecurityIdentity ? (Not sure what else but SecurityIdentity we can retain, seems simplest)

@sberyozkin
Copy link
Member

In general, this is how I'd suggest for a non-security extension to pick up whatever one of the security extensions converted into SecurityIdentity. https://github.com/quarkusio/quarkus-security alone which just SPI should give it, it gives SecurityIdentity interface but also an interface like TokenCredential which may come from OIDC, smallrye-jwt, etc as well. A light weight dependency.

@michalvavrik
Copy link
Member

In general, this is how I'd suggest for a non-security extension to pick up whatever one of the security extensions converted into SecurityIdentity. https://github.com/quarkusio/quarkus-security alone which just SPI should give it, it gives SecurityIdentity interface but also an interface like TokenCredential which may come from OIDC, smallrye-jwt, etc as well. A light weight dependency.

+1

@mkouba
Copy link
Contributor Author

mkouba commented May 7, 2024

@sberyozkin @michalvavrik So if I understand it correctly, the only thing we want to do is to capture the SecurityIdentity from the RoutingContext of the initial handshake request and then set this identity to the CurrentIdentityAssociation after the request context is activated during a WebSocket callback invocation. I.e. something similar to Reactive Routes: https://github.com/quarkusio/quarkus/blob/main/extensions/reactive-routes/runtime/src/main/java/io/quarkus/vertx/web/runtime/RouteHandler.java#L51-L56. Then interceptors for @RolesAllowed and other annotations should work out of the box. I will try to prepare something by the end of this week.

@sberyozkin
Copy link
Member

@mkouba Yes, something like that would be perfect to have I think.

I will try to prepare something by the end of this week.

It would be awesome, if you get a chance to look at it

Thanks

mkouba added a commit to mkouba/quarkus that referenced this issue May 9, 2024
- fixes quarkusio#40312
- also create a new Vertx duplicated context for error handler
invocation
mkouba added a commit to mkouba/quarkus that referenced this issue May 9, 2024
- fixes quarkusio#40312
- also create a new Vertx duplicated context for error handler
invocation
mkouba added a commit to mkouba/quarkus that referenced this issue May 9, 2024
- when quarkus-security is present and quarkus.http.auth.proactive=false,
then we force the authentication before the HTTP upgrade so that it's possible
to capture the SecurityIdentity and set it afterwards for all endpoint callbacks
- fixes quarkusio#40312
- also create a new Vertx duplicated context for error handler
invocation
mkouba added a commit to mkouba/quarkus that referenced this issue May 10, 2024
- when quarkus-security is present and quarkus.http.auth.proactive=false,
then we force the authentication before the HTTP upgrade so that it's possible
to capture the SecurityIdentity and set it afterwards for all endpoint callbacks
- fixes quarkusio#40312
- also create a new Vertx duplicated context for error handler
invocation
mkouba added a commit to mkouba/quarkus that referenced this issue May 14, 2024
- when quarkus-security is present and quarkus.http.auth.proactive=false,
then we force the authentication before the HTTP upgrade so that it's possible
to capture the SecurityIdentity and set it afterwards for all endpoint callbacks
- fixes quarkusio#40312
- also create a new Vertx duplicated context for error handler
invocation
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 15, 2024
@gsmet gsmet modified the milestones: 3.12 - main, 3.11.0 May 21, 2024
gsmet pushed a commit to gsmet/quarkus that referenced this issue May 21, 2024
- when quarkus-security is present and quarkus.http.auth.proactive=false,
then we force the authentication before the HTTP upgrade so that it's possible
to capture the SecurityIdentity and set it afterwards for all endpoint callbacks
- fixes quarkusio#40312
- also create a new Vertx duplicated context for error handler
invocation

(cherry picked from commit f1f7a20)
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
- when quarkus-security is present and quarkus.http.auth.proactive=false,
then we force the authentication before the HTTP upgrade so that it's possible
to capture the SecurityIdentity and set it afterwards for all endpoint callbacks
- fixes quarkusio#40312
- also create a new Vertx duplicated context for error handler
invocation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants