quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.79k stars 2.68k forks source link

WebSockets Next: security integration #40312

Closed mkouba closed 5 months ago

mkouba commented 6 months ago

Description

Follow-up of https://github.com/quarkusio/quarkus/pull/39142.

Implementation ideas

No response

sberyozkin commented 6 months ago

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 commented 6 months ago

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 https://github.com/quarkusio/quarkus/issues/40307 must be done first before planning this enhancement work ?

nope, not related

mkouba commented 6 months ago

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 https://github.com/quarkusio/quarkus/issues/40307 must be done first before planning this enhancement work ?

nope, not related

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

michalvavrik commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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