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: fire authorization sucesss and failure events for HTTP upgrade security checks #45800

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

Conversation

michalvavrik
Copy link
Member

@sberyozkin
Copy link
Member

sberyozkin commented Jan 22, 2025

Thanks Michal, chatbot links can be offered as part of the HTTP application, I wonder, how does the event listener know where exactly it failed, at the HTTP Upgrade time, or at some non-WS HTTP request time ?

@sberyozkin
Copy link
Member

The other question, so we recommend using authentication at the HTTP upgrade time, but also allow it on methods like onMessage, after the upgrade, do you plan to have it covered too ?

@michalvavrik michalvavrik force-pushed the feature/ws-next-http-upgrade-authz-events branch from c00b4c0 to 7521bf2 Compare January 22, 2025 18:54
@michalvavrik
Copy link
Member Author

Thanks Michal, chatbot links can be offered as part of the HTTP application, I wonder, how does the event listener know where exactly it failed, at the HTTP Upgrade time, or at some non-WS HTTP request time ?

This really depends on where is is thrown, sadly I don't have a short answer, so please endure:

It depends on where is authorization failure / success performed:

  • The reason why I didn't mention about security events were not fired for HTTP upgrade security checks was that this is the only security thing that WS Next does on it's own. For a good reason, it must be done eagerly and using CDI interceptors is not a good option there.
  • the events I am firing in this PR add: endpoint id, HTTP request (so you can get path, headers), security check name; IMHO that is all you need, there is no actual method, so we can't be more specific
  • CDI interceptors knows method, if you inspect the test that I added, you will see that authorization events for @PermissionChecker payload tests has class#method because they actually secure some java method; but generally you can't access there routing context or path because it is not available because it is performed in Quarkus Security extension that doesn't know about that (but for the checker we always add RoutingContext so that is exception as well)
  • authorization using configuration (HTTP security policies) are adding routingcontext which allows you to inspect path and that again fits configuration, because you configure these checks for some path

Long answer short: you need to inspect event properties. I think we could unify it somehow, but this will always differ based on what is authorized. For example eagerly performed security checks on Quarkus REST endpoints has again different set of properties because there you know: method (it is annotated right?), routingcontext, security check name

The other question, so we recommend using authentication at the HTTP upgrade time, but also allow it on methods like onMessage, after the upgrade, do you plan to have it covered too ?

Not sure what you mean, I think I have tests for everything. if I missed something please let me know and I'll fix it. If you wonder what happens there in regards of security events? every authorization has it's own event, so HTTP Security policies has their own authorization event instance fired and security annotations has their own event instance fired. That's not new, because you can't know if next authorization is coming in a different level, so you can't fire one event per all different authorizations.

@sberyozkin
Copy link
Member

@michalvavrik It is just that the PR says HTTP upgrade security checks while in your previous PR, there was a dedicated doc section on security bean methods (after insecure upgrade)

Copy link

quarkus-bot bot commented Jan 22, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 7521bf2.

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

@michalvavrik
Copy link
Member Author

@michalvavrik It is just that the PR says HTTP upgrade security checks while in your previous PR, there was a dedicated doc section on security bean methods (after insecure upgrade)

This test method https://github.com/quarkusio/quarkus/pull/45800/files#diff-d45d0fb9a839e6fdf80b00f4e49eea69992399f252c82e5ad3206c231b25ef9dR224 checks security events on bean methods after insecure upgrade.

Sorry if I am slow, it's just that it is something that had worked before, this PR is only fixing events for security annotations placed next to @WebSocket (class level). I am adding tests for other scenarios just to be sure it works. Just let me know if you see something missing and I'll add it.

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.

2 participants