-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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: initial version of security integration #40534
Conversation
Thanks @mkouba It looks perfect, though Michal may have some more suggestions. Let me check how the identity expiry time can be provided |
@sberyozkin Pls create a separate issue for this topic. |
...s-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/WebSocketServerRecorder.java
Outdated
Show resolved
Hide resolved
@mkouba @michalvavrik I can support this PR with the Docs update once the code is ready to go |
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.
@mkouba , looking forward for some more tests and docs, but IMO code is alright. maybe you want to gather opinions from others like @stuartwdouglas (if he has time).
As far as I am concerned, good job!
Quick question, why disabling the proactive authentication is required for the identity be stored at the HTTP to WS upgrade time ? |
It's not required. However, if |
Hi @mkouba Oh I see, I wrongly assumed from the PR description that disabling it was required, but you are only explaining there what must be done in case of the lazy security check. Thanks |
@sberyozkin the code is IMO ready to go. At least for the first version ;-). |
This comment has been minimized.
This comment has been minimized.
client.sendAndAwait("hello"); // forbidden | ||
client.sendAndAwait("hi"); // user service | ||
client.waitForMessages(2); | ||
assertEquals(Set.of("42", "forbidden"), Set.copyOf(client.getMessages().stream().map(Object::toString).toList())); |
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.
@mkouba if connection would be opened for endUri
and the bean would look like this
@Authenticated
@WebSocket(path = "/end")
public static class Endpoint {
(which is how it looks in other tests, but not here)
If you use quarkus.http.auth.permission.secured.paths=/end
as in other tests, it fails immediately and @OnError
is not called, it happens before the request even gets to the WS Next extension.
However when the @Authenticated
annotation is used, anonymous user can keep sending messages that are bound to fail. Instead, user will receive responses from @OnError
.
Situation is similar in case of authorization, if you know that user will be unauthorized because in the initial handshake for that URI he didn't have these roles, how many times should he be allowed to send a message like you do
client.sendAndAwait("hello");
client.sendAndAwait("hi");
in theory, can it be 100 000 even though we know what the response will be?
I am asking because there are things I don't know yet, for example if the client.connect(basicAuth("user", "user"), endUri);
is opened for certain URI, can we be sure it is only open for that URI? If so, either now or in the future, we need to decide whether we just keep this connection open.
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.
However when the @authenticated annotation is used, anonymous user can keep sending messages that are bound to fail. Instead, user will receive responses from @onerror.
Yes, and it's up to the endpoint author to decide what to do. For example, you can do something like:
@OnError
void error(ForbiddenException e, WebSocketConnection conn) {
conn.closeAndAwait(); // and possibly specify the close reason
}
Situation is similar in case of authorization, if you know that user will be unauthorized because in the initial handshake for that URI he didn't have these roles, how many times should he be allowed to send a message like you do
Until the connection is closed.
I am asking because there are things I don't know yet, for example if the client.connect(basicAuth("user", "user"), endUri); is opened for certain URI, can we be sure it is only open for that URI?
I'm not sure I'm following. What do you mean?
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'm not sure I'm following. What do you mean?
My thinking was that only sensible reason to not close the connection is that same connection can be used for different URIs, but I think it's clear now that it cannot. Thanks
Yes, and it's up to the endpoint author to decide what to do.
Cool, let's stick with that. But I think the documentation (when it is written later) will have to make clear that on error you should close the connection unless you have a good reason. Just so that people won't forget, it doesn't make too much of a sense to me to keep this open if you cannot change the identity.
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.
@mkouba speaking of what, according to the https://quarkus.io/guides/websockets-next-reference#websocket-endpoint-methods the @OnError
is not required, am I correct? If so, what will happen when there is no @OnError
, can we just close the connection in the future (not in this PR)?
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.
Just so that people won't forget, it doesn't make too much of a sense to me to keep this open if you cannot change the identity.
Well, it depends on how the endpoint is used. You cannot change the identity but the message that resulted in a ForbiddenException
could have been a mistake. It's very similar to a webapp where a user intentionally or unintentionally attemps to access a resource that is forbidden for this particular user 🤷. Do we log off the user automatically?
@mkouba speaking of what, according to the https://quarkus.io/guides/websockets-next-reference#websocket-endpoint-methods the
@OnError
is not required, am I correct?
You're right.
If so, what will happen when there is no
@OnError
, can we just close the connection in the future (not in this PR)?
Currently, we only log an error message. But we can certainly close the connection by default.
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.
Hm, the problem is that JAX-RS endpoints work diferently - each method handles an HTTP request, so it makes sense to map the security checks to methods. In our case, an endpoint method does not represent an HTTP request, in fact the endpoint instance is created and callbacks are invoked after the initial HTTP request upgrade finished successfully.
Maybe we should merge this PR and improve the integration in follow-up PRs. The discussion is getting harder to follow.
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.
Hm, the problem is that JAX-RS endpoints work diferently - each method handles an HTTP request, so it makes sense to map the security checks to methods. In our case, an endpoint method does not represent an HTTP request, in fact the endpoint instance is created and callbacks are invoked after the initial HTTP request upgrade finished successfully.
It makes sense. There should be consent and more people need to comment on how this should work. Whatever there is settlement on, it will need to be highlighted in the docs in the future.
Personally I'm still worried about expectations. For example in the ideal world, I'd expect that if you want to close connection then you put the annotation like this:
@Authenticated
@OnOpen
String open(@PathParam String name) {
return "Hello " + name + "!";
}
than it works and connection is closed immediately for anonymous user.
Maybe we should merge this PR and improve the integration in follow-up PRs. The discussion is getting harder to follow.
Absolutely, this could go on for a long time. Before you merge, please let's hear from @stuartwdouglas first. I won't comment anymore to limit discussions in this PR down. Thanks!
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.
Thanks Michal. Re closed connection - yes, I think that we could make it configurable and enable it by default, i.e. a connection is closed automatically if a security failure occurs.
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.
Are we sure logging is the best option when an unhandled failure of any kind occurs? Unlike HTTP there is no standard way to report back to the client that there was a problem, so closing the connection seems like a reasonable repose.
That said I think we should make sure that if the endpoint has security annotations on the class itself it should affect the upgrade process, and make sure it will send back a challenge if the user is not authenticated. Opening the connection and then immediately closing it or just essentially ignoring all messages are both the wrong things to do.
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.
Are we sure logging is the best option when an unhandled failure of any kind occurs? Unlike HTTP there is no standard way to report back to the client that there was a problem, so closing the connection seems like a reasonable repose.
@stuartwdouglas I don't know, for me closing a connection on any error seems a bit too "aggressive" strategy. But it might make sense to make this configurable and provide several strategies out of the box: (1) close connection (default), (2) log an error and (3) noop. I will open a new issue as it's not a security-only thing. #40648
That said I think we should make sure that if the endpoint has security annotations on the class itself it should affect the upgrade process, and make sure it will send back a challenge if the user is not authenticated. Opening the connection and then immediately closing it or just essentially ignoring all messages are both the wrong things to do.
Thanks @mkouba, and @michalvavrik It is a perfect start IMHO. |
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.
LGTM, can't help waiting for trying it with quarkus-langchain4j demos :-)
Lets see if more comments will come early next week from Stuart, Clement, before merging |
That's what I tried to find out, but so far my understanding is that WS client cannot use the same connection to try another method. |
@mkouba I added a doc commit, please let me know if you are OK with it, also if you'd like, please squash. Note, I'm listing Planning to merge tomorrow once the docs are good, unless Stuart adds some more comments. IMHO, it is good to go with some improvements to follow up. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🙈 The PR is closed and the preview is expired. |
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.
@sberyozkin I think that it's a good start! I've added a few minor comments...
Status for workflow
|
Status for workflow
|
3.11 has just been branched so I'm adding a backport label, would be great to have it 3.11.0 |
} | ||
---- | ||
<1> The echo callback method can only be invoked if the current security identity has an `admin` role. | ||
<2> The error handler is invoked in case of the authorization failure. |
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.
It can also be UnauthorizedException
for @RolesAllowed
.
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 in this case it probably won't happen since the upgrade is secured with the HTTP authentication policy, while @RolesAllowed is run on the callbacks with SecurityIdentity being already around.
I guess this doc can be updated to list another possible exception when @mkouba has a chance to look at the next security enhancement, like supporting the secure upgrade with the annotations, to test the @RolesAllowed @OnOpen
combination.
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 in this case it probably won't happen since the upgrade is secured with the HTTP authentication policy, while @RolesAllowed is run on the callbacks with SecurityIdentity being already around.
with anonymous identity around, it's going to happen; WS Next authenticates eagerly, but does not enforce authorization eagerly
I guess this doc can be updated to list another possible exception when @mkouba has a chance to look at the next security enhancement, like supporting the secure upgrade with the annotations, to test the @RolesAllowed @onopen combination.
yeah, it was just a thought, no worry
Awesome! |
This is the initial POC for security integration. When
quarkus-security
is present andquarkus.http.auth.proactive=false
, then we force the authentication before the HTTP upgrade so that it's possible to capture theSecurityIdentity
and set it afterwards for all endpoint callbacks.CC @sberyozkin @michalvavrik