WebSockets Next: Support for secure upgrade with security annotations only about quarkus

[mkouba]

@cescoffier @mkouba I am not sure what additional SPI is required from your comments, but here #40957 is PR based on HTTPUpgradeCheck bean. Ordering is possible by CDI bean priority. If you have additional SPI requirements, I'll need more details, ideally in from on enhancement issue.

No problem. I think that HttpUpgradeCheck should work for most use cases. We will create a new issue once it's clear what those other requirements are.

I put it Integer.MAX_VALUE - 100 as security should happen (almost) first. Why to process something for unauthorized requests. I can change if you deem it as important as I only mentioned this comment now.

Integer.MAX_VALUE - 100 should be fine. But it's important to define a public constant that holds the priority value so that other impls can reference this value, e.g. SecurityHttpUpgradeCheck.PRIORITY + 1.

[mkouba]

Another thing we discussed is the ability for the user (application developer) to contribute before the upgrade (sub-protocol negotiation, custom security...). Note that these 'contributions' can change the upgrade response (like the selected sub-protocol), or reject the connection.

Well, the HttpUpgradeCheck implementations can "contribute" in some ways. It can reject the connection, modify request headers, add response headers, etc. And they are sorted by bean priority (higher priority is called first). So the security implementation should use a public constant as the priority (e.g. 2500).

Because we can have multiple 'contributions', the plan was to use CDI bean order to build the chain. Security would be a part of this chain ;around the middle so the user can contribute before and after).

This contribution SPI is not yet define.

Yes, we should probably start with a list of "contributions" ;-)

[michalvavrik]

I think your example should be preferred way to do authorization, considering the SecurityIdentity won't change, it doesn't make sense to run security checks on every message. It's wasting. We should strongly recommend to secure OnOpen when this is implemented. Please let me have a look.

[michalvavrik]

for this kind of stuff we would need an SPI that would allow us to perform a security check for a class/set of annotation instances - the current SPI seems to be very method-oriented. Actually, we could use the security checks like io.quarkus.security.runtime.interceptor.check.RolesAllowedCheck directly, or even mimic the logic used there - it's pretty straightforward. But ideally, the SecurityCheckStorage would declare something like getSecurityCheck(Class<?>)

Securing classes doesn't make sense for CDI beans (conflict between annotated methods vs inherited class level annotations + you do not invoke bean, you invoke method), it would be WebSocket NEXT specific. I think we should simply secure @OnOpen and when user doesn't provide this method, we register implicit (which is void Uni) against SecurityCheckStorage.

I think it makes sense that when class or @OnOpen is annotated, that authorization is happening before the connection is opened, therefore before upgrade and no @OnError is invoked.

If that is acceptable, I'll implement it.

[michalvavrik]

@mkouba ^^

[michalvavrik]

Generated endpoint inherits io.quarkus.websockets.next.runtime.WebSocketEndpointBase#onOpen which means there always is that method when the endpoint class is annotated.

[mkouba]

for this kind of stuff we would need an SPI that would allow us to perform a security check for a class/set of annotation instances - the current SPI seems to be very method-oriented. Actually, we could use the security checks like io.quarkus.security.runtime.interceptor.check.RolesAllowedCheck directly, or even mimic the logic used there - it's pretty straightforward. But ideally, the SecurityCheckStorage would declare something like getSecurityCheck(Class<?>)

Securing classes doesn't make sense for CDI beans (conflict between annotated methods vs inherited class level annotations + you do not invoke bean, you invoke method), it would be WebSocket NEXT specific. I think we should simply secure @OnOpen and when user doesn't provide this method, we register implicit (which is void Uni) against SecurityCheckStorage.

I think it makes sense that when class or @OnOpen is annotated, that authorization is happening before the connection is opened, therefore before upgrade and no @OnError is invoked.

My concern with @OnOpen is that it indicates that the method is invoked when the WS connection is opened. Which is not the case here - we need to perform security checks before the HTTP upgrade is done. In any case, we will need something WS next specific (inconsistent with CDI interceptros etc.).

Generated endpoint inherits io.quarkus.websockets.next.runtime.WebSocketEndpointBase#onOpen which means there always is that method when the endpoint class is annotated.

Well, that does not really matter IMO. The generated endpoint class merely delegates to the endpoint bean instance.

[michalvavrik]

makes sense @mkouba , I'll make it WS Next specific. Thanks for the feedback.

[michalvavrik]

also my understanding is that the description of this issue is not exactly what should be implemented from @mkouba feedback. if I get it right, the upgrade should be secured only when endpoint is annotated on the class level. and @OnOpen will still rely on CDI interceptors as it does now. Correct?

[mkouba]

also my understanding is that the description of this issue is not exactly what should be implemented from @mkouba feedback. if I get it right, the upgrade should be secured only when endpoint is annotated on the class level. and @OnOpen will still rely on CDI interceptors as it does now. Correct?

Yes, I believe that this would be the most understandable for users. In any case, it has to be properly documented because it's not exactly obvious. In fact, we should probably remove the security annotations declared on the endpoint class through a transformer so that the interceptors are not applied to the methods later on...

CC @cescoffier

[michalvavrik]

In fact, we should probably remove the security annotations declared on the endpoint class through a transformer so that the interceptors are not applied to the methods later on...

+1; normally we keep the interceptors as a fallback and it saved us in past when the detection of annotations wasn't perfect, but here it must work differently and there are no conflicts between methods / classes annotations.

[cescoffier]

@mkouba and I discussed about this last week. @mkouba can you write what we discussed (@authenticated and @ROLE Allowed on the class, the "upgrade" SPI...)

[mkouba]

Yes, sure.

1. We will use an io.quarkus.websockets.next.HttpUpgradeCheck (introduced in #40873) to perform the security checks declared on the endpoint class.
2. We will remove those annotations using an annotation transformer so that they're NOT taken into account by CDI.
3. And we will need to document this properly because the behavior differs from the standard way of processing security annotations.

[michalvavrik]

Yes, sure.

1. We will use an io.quarkus.websockets.next.HttpUpgradeCheck (introduced in WebSockets NEXT: add ability to inspect/reject HTTP upgrade #40873) to perform the security checks declared on the endpoint class.
2. We will remove those annotations using an annotation transformer so that they're NOT taken into account by CDI.
3. And we will need to document this properly because the behavior differs from the standard way of processing security annotations.

yeah, I already have it implemented (tests including). I just need to write docs (maybe tomorrow), but there is no hurry as Sergey is on PTO this week and we will need him to review. It required some changes in the Security extension.

[cescoffier]

Another thing we discussed is the ability for the user (application developer) to contribute before the upgrade (sub-protocol negotiation, custom security...). Note that these 'contributions' can change the upgrade response (like the selected sub-protocol), or reject the connection.

Because we can have multiple 'contributions', the plan was to use CDI bean order to build the chain. Security would be a part of this chain ;around the middle so the user can contribute before and after).

This contribution SPI is not yet define.

[michalvavrik]

@cescoffier @mkouba I am not sure what additional SPI is required from your comments, but here #40957 is PR based on HTTPUpgradeCheck bean. Ordering is possible by CDI bean priority. If you have additional SPI requirements, I'll need more details, ideally in from on enhancement issue.

[michalvavrik]

Because we can have multiple 'contributions', the plan was to use CDI bean order to build the chain. Security would be a part of this chain ;around the middle so the user can contribute before and after).

I put it Integer.MAX_VALUE - 100 as security should happen (almost) first. Why to process something for unauthorized requests. I can change if you deem it as important as I only mentioned this comment now.