WebSockets Next: Support for secure upgrade with security annotations only

[michalvavrik]

closes: #40622

[github-actions]

🙈 The PR is closed and the preview is expired.

[cescoffier]

The idea looks good.

Might be unrelated to this PR, but how can a user implement sub-protocol negotiation?

[mkouba]

I cannot review the security parts but the rest looks good.

[mkouba]

Might be unrelated to this PR, but how can a user implement sub-protocol negotiation?

@cescoffier First of all, we should describe the current subprotol support. The server can configure a list of supported subprotocols (globally, not per-endpoint). Now if a client sends the Sec-WebSocket-Protocol HTTP header in the upgrade request and none of the requested subprotocol is available then the connection is rejected. If the requested subprotocol is available the connection is established and the selected subprotocol is accessible via WebSocketConnection#subprotocol(). I'm not so sure what happens if multiple subprotocols match. Probably the first one is selected.

With a custom HttpUpgradeCheck the application can inspect/modify the Sec-WebSocket-Protocol header and/or reject the connection immediately, e.g. before the upgrade is perfomed. However, it is not possbile to set the selected subprotocol on the connection, i.e. WebSocketConnection#subprotocol() may return null if quarkus.websockets-next.server.supported-subprotocols is empty.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 5332010.

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

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

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

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

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/reactive-messaging-kafka

✖ io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

• Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

---

⚙️ JVM Tests - JDK 21

📦 integration-tests/reactive-messaging-kafka

✖ io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

• Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

[sberyozkin]

Thanks @michalvavrik, it looks fine but a bit complex to review from PTO 🙂, so will do next week

[mkouba]

@sberyozkin ping. Did you get a chance to review the security parts?

[sberyozkin]

@michalvavrik So, in the example above, open() is secured and echo is not ? It would be unexpected IMHO.

[michalvavrik]

In the example above "open" and "echo" are not secured, but HTTP upgrade is secured. The HTTP upgrade happens before "open" and "echo", therefore they are only not secured if you inject "Endpoint" class as a bean into another CDI bean and call these methods directly.

That is:

@Inject
Endpoint endpoint

public void do() {
   endpoint.open();
}

if we were to secure echo, it would mean for every single message is performed authorization despite known result (you know it will pass because the HTTP upgrade is secured).

I can point you to tests that asserts security is not relaxed where not documented.

[sberyozkin]

@michalvavrik Michal, these are low level technical details that HTTP upgrade happens before onOpen, to me, on onOpen actually represents a successful HTTP upgrade completion, even if it is not quite a correct picture.

Let me clarify how it aligns with #40534.

So there we have an HTTP security policy securing an upgrade:

quarkus.http.auth.permission.secured.paths=/end
quarkus.http.auth.permission.secured.policy=authenticated

and then we have

@WebSocket(path = "/end")
public class Endpoint {
    @Inject
    SecurityIdentity currentIdentity;
    
    @RolesAllowed("admin")
    @OnTextMessage
    String echo(String message) {
        return message;
    }
}

so @RolesAllowed must be placed on the echo method to ensure the current security identity is present and matches the sec constraint before echo is called.

Let's say, with your PR, I only want that echo is accessed by the non-anonymous security identity.

Are you saying we must do:

@WebSocket(path = "/end")
@Authenticated
public class Endpoint {
    @Inject
    SecurityIdentity currentIdentity;
    
    @Authenticated
    @OnTextMessage
    String echo(String message) {
        return message;
    }
}

to have echo secured ?

Also, does

@WebSocket(path = "/end")
@RolesAllowed("admin")
public class Endpoint {
 
}

works for a secure HTTP upgrade ? And if yes, should users also duplicate @RolesAllowed("admin") on the echo method for the echo be secured ?

I guess, all I'm asking, why can't we have a single @Authenticated at the class level, and not require users duplicate across various methods like onOpen, onEcho, etc, if all users want is, for ex, an authenticated access ?

[mkouba]

Are you saying we must do:

@WebSocket(path = "/end")
@Authenticated
public class Endpoint {
    @Inject
    SecurityIdentity currentIdentity;
    
    @Authenticated
    @OnTextMessage
    String echo(String message) {
        return message;
    }
}

to have echo secured ?

Nope, @Authenticated declared on an endpoint means that no callback can be called for non-anonymous security identity. In other words, we secure the HTTP upgrade and if the check fails then no method declared on the endpoint is invoked.

[mkouba]

Also, does

@WebSocket(path = "/end")
@RolesAllowed("admin")
public class Endpoint {
 
}

works for a secure HTTP upgrade ? And if yes, should users also duplicate @RolesAllowed("admin") on the echo method for the echo be secured ?

It's the same principle, i.e. no need to duplicate the annotation.

[michalvavrik]

Can you suggest a message @sberyozkin ? It's optional, but I'd appreciate it. Otherwise I'll try to rewrite it, but I don't know if it will fit your suggestion.

[sberyozkin]

@michalvavrik

IMHO noone will inject endpoint and use it as CDI bean

May be this is the source of the confusion on my behalf and simply dropping this message will help ? See, I'm not thinking as a user in terms of CDI beans, when I see an example like

@WebSocket(path = "/end")
@Authenticated
public class Endpoint {
    @Inject
    SecurityIdentity currentIdentity;
    
    @OnTextMessage
    String echo(String message) {
        return message;
    }
}

I expect that the callback echo and onOpen methods are secured, I don't want to inject this endpoint anywhere, it is like a JAX-RS resource, we don't inject it somewhere else...

[sberyozkin]

Which is why reading Placing a security annotation on an endpoint bean will not secure bean methods, only the HTTP upgrade. confuses me a lot

[sberyozkin]

Anyway, I don't consider it as a blocker and minor updates to docs can be backported later if agreed to, so @mkouba @gsmet please merge if all looks good to you otherwise

[michalvavrik]

I think small follow-up docs PR where we can iterate would be better than do it on this PR as CI takes longer and I think we need more than attempt. +1 to merge this

[sberyozkin]

LGTM, please merge for this PR to make it to 3.12.

I suggest to rework the message related to the securing the upgrade vs the onOpen/onTextMesage callbacks if possible, but is not a requirement for this PR

[sberyozkin]

OK, let me merge it myself then