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.

Sign up for GitHub

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: Support for secure upgrade with security annotations only #40957

Merged
merged 1 commit into from
Jun 11, 2024
Merged

WebSockets Next: Support for secure upgrade with security annotations only #40957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 47 additions & 7 deletions docs/src/main/asciidoc/websockets-next-reference.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,20 +655,60 @@

`SecurityIdentity` is initially created during a secure HTTP upgrade and associated with the websocket connection.

Currently, for an HTTP upgrade be secured, users must configure an HTTP policy protecting the HTTP upgrade path.
For example, to secure the `open()` method in the above websocket endpoint, one can add the following authentication policy:
NOTE: When OpenID Connect extension is used and token expires, Quarkus automatically closes connection.

[source,properties]
== Secure HTTP upgrade

An HTTP upgrade is secured when standard security annotation is placed on an endpoint class or an HTTP Security policy is defined.
The advantage of securing HTTP upgrade is less processing, the authorization is performed early and only once.
You should always prefer HTTP upgrade security unless, like in th example above, you need to perform action on error.

Check warning on line 664 in docs/src/main/asciidoc/websockets-next-reference.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale 

[vale] reported by reviewdog 🐶
[Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'need to'.

Raw Output:
{"message": "[Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'need to'.", "location": {"path": "docs/src/main/asciidoc/websockets-next-reference.adoc", "range": {"start": {"line": 664, "column": 75}}}, "severity": "INFO"}

.Use standard security annotation to secure an HTTP upgrade
[source, java]
----
quarkus.http.auth.permission.secured.paths=/end
quarkus.http.auth.permission.secured.policy=authenticated
package io.quarkus.websockets.next.test.security;

import io.quarkus.security.Authenticated;
import jakarta.inject.Inject;

import io.quarkus.security.identity.SecurityIdentity;
import io.quarkus.websockets.next.OnOpen;
import io.quarkus.websockets.next.OnTextMessage;
import io.quarkus.websockets.next.WebSocket;

@Authenticated <1>
@WebSocket(path = "/end")
public class Endpoint {

@Inject
SecurityIdentity currentIdentity;

@OnOpen
String open() {
return "ready";
}

@OnTextMessage
String echo(String message) {
return message;
}
}
----
<1> Initial HTTP handshake ends with the 401 status for anonymous users.
You can also redirect the handshake request on authorization failure with the `quarkus.websockets-next.server.security.auth-failure-redirect-url` configuration property.

Other options for securing HTTP upgrade requests, such as using the security annotations, will be explored in the future.
IMPORTANT: HTTP upgrade is only secured when a security annotation is declared on an endpoint class next to the `@WebSocket` annotation.
Placing a security annotation on an endpoint bean will not secure bean methods, only the HTTP upgrade.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@michalvavrik michalvavrik Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

You must always verify that your endpoint is secured as intended.

NOTE: When OpenID Connect extension is used and token expires, Quarkus automatically closes connection.
.Use HTTP Security policy to secure an HTTP upgrade
[source,properties]
----
quarkus.http.auth.permission.http-upgrade.paths=/end
quarkus.http.auth.permission.http-upgrade.policy=authenticated
----

== Inspect and/or reject HTTP upgrade

Check warning on line 711 in docs/src/main/asciidoc/websockets-next-reference.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale 

[vale] reported by reviewdog 🐶
[Quarkus.TermsWarnings] Consider using 'a or b' or 'a, b, or both' rather than 'and/or' unless updating existing content that uses the term.

Raw Output:
{"message": "[Quarkus.TermsWarnings] Consider using 'a or b' or 'a, b, or both' rather than 'and/or' unless updating existing content that uses the term.", "location": {"path": "docs/src/main/asciidoc/websockets-next-reference.adoc", "range": {"start": {"line": 711, "column": 1}}}, "severity": "WARNING"}

To inspect an HTTP upgrade, you must provide a CDI bean implementing the `io.quarkus.websockets.next.HttpUpgradeCheck` interface.
Quarkus calls the `HttpUpgradeCheck#perform` method on every HTTP request that should be upgraded to a WebSocket connection.
Expand Down
114 changes: 72 additions & 42 deletions ...ity/deployment/src/main/java/io/quarkus/security/deployment/PermissionSecurityChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.quarkus.security.deployment;

import static io.quarkus.arc.processor.DotNames.CLASS;
import static io.quarkus.arc.processor.DotNames.STRING;
import static io.quarkus.security.PermissionsAllowed.AUTODETECTED;
import static io.quarkus.security.PermissionsAllowed.PERMISSION_TO_ACTION_SEPARATOR;
Expand Down Expand Up @@ -34,7 +35,9 @@

interface PermissionSecurityChecks {

Map<MethodInfo, SecurityCheck> get();
Map<MethodInfo, SecurityCheck> getMethodSecurityChecks();

Map<DotName, SecurityCheck> getClassNameSecurityChecks();

Set<String> permissionClasses();

Expand All @@ -43,8 +46,8 @@ final class PermissionSecurityChecksBuilder {
private static final DotName STRING_PERMISSION = DotName.createSimple(StringPermission.class);
private static final DotName PERMISSIONS_ALLOWED_INTERCEPTOR = DotName
.createSimple(PermissionsAllowedInterceptor.class);
private final Map<MethodInfo, List<List<PermissionKey>>> methodToPermissionKeys = new HashMap<>();
private final Map<MethodInfo, LogicalAndPermissionPredicate> methodToPredicate = new HashMap<>();
private final Map<AnnotationTarget, List<List<PermissionKey>>> targetToPermissionKeys = new HashMap<>();
private final Map<AnnotationTarget, LogicalAndPermissionPredicate> targetToPredicate = new HashMap<>();
private final Map<String, MethodInfo> classSignatureToConstructor = new HashMap<>();
private final SecurityCheckRecorder recorder;

Expand All @@ -53,22 +56,37 @@ public PermissionSecurityChecksBuilder(SecurityCheckRecorder recorder) {
}

PermissionSecurityChecks build() {
final Map<LogicalAndPermissionPredicate, SecurityCheck> cache = new HashMap<>();
final Map<MethodInfo, SecurityCheck> methodToCheck = new HashMap<>();
final Map<DotName, SecurityCheck> classNameToCheck = new HashMap<>();
for (var targetToPredicate : targetToPredicate.entrySet()) {
SecurityCheck check = cache.computeIfAbsent(targetToPredicate.getValue(),
new Function<LogicalAndPermissionPredicate, SecurityCheck>() {
@Override
public SecurityCheck apply(LogicalAndPermissionPredicate predicate) {
return createSecurityCheck(predicate);
}
});

var annotationTarget = targetToPredicate.getKey();
if (annotationTarget.kind() == AnnotationTarget.Kind.CLASS) {
DotName className = annotationTarget.asClass().name();
classNameToCheck.put(className, check);
} else {
MethodInfo securedMethod = annotationTarget.asMethod();
methodToCheck.put(securedMethod, check);
}
}

return new PermissionSecurityChecks() {
@Override
public Map<MethodInfo, SecurityCheck> get() {
final Map<LogicalAndPermissionPredicate, SecurityCheck> cache = new HashMap<>();
final Map<MethodInfo, SecurityCheck> methodToCheck = new HashMap<>();
for (var methodToPredicate : methodToPredicate.entrySet()) {
SecurityCheck check = cache.computeIfAbsent(methodToPredicate.getValue(),
new Function<LogicalAndPermissionPredicate, SecurityCheck>() {
@Override
public SecurityCheck apply(LogicalAndPermissionPredicate predicate) {
return createSecurityCheck(predicate);
}
});
methodToCheck.put(methodToPredicate.getKey(), check);
}
return methodToCheck;
public Map<MethodInfo, SecurityCheck> getMethodSecurityChecks() {
return Map.copyOf(methodToCheck);
}

@Override
public Map<DotName, SecurityCheck> getClassNameSecurityChecks() {
return Map.copyOf(classNameToCheck);
}

@Override
Expand Down Expand Up @@ -99,8 +117,8 @@ public Set<String> permissionClasses() {
*/
PermissionSecurityChecksBuilder createPermissionPredicates() {
Map<PermissionCacheKey, PermissionWrapper> permissionCache = new HashMap<>();
for (Map.Entry<MethodInfo, List<List<PermissionKey>>> entry : methodToPermissionKeys.entrySet()) {
final MethodInfo securedMethod = entry.getKey();
for (var entry : targetToPermissionKeys.entrySet()) {
final AnnotationTarget securedTarget = entry.getKey();
final LogicalAndPermissionPredicate predicate = new LogicalAndPermissionPredicate();

// 'AND' operands
Expand All @@ -113,7 +131,7 @@ PermissionSecurityChecksBuilder createPermissionPredicates() {
// 'AND' operands

for (PermissionKey permissionKey : permissionKeys) {
var permission = createPermission(permissionKey, securedMethod, permissionCache);
var permission = createPermission(permissionKey, securedTarget, permissionCache);
if (permission.isComputed()) {
predicate.markAsComputed();
}
Expand All @@ -128,15 +146,15 @@ PermissionSecurityChecksBuilder createPermissionPredicates() {
predicate.and(orPredicate);

for (PermissionKey permissionKey : permissionKeys) {
var permission = createPermission(permissionKey, securedMethod, permissionCache);
var permission = createPermission(permissionKey, securedTarget, permissionCache);
if (permission.isComputed()) {
predicate.markAsComputed();
}
orPredicate.or(permission);
}
}
}
methodToPredicate.put(securedMethod, predicate);
targetToPredicate.put(securedTarget, predicate);
}
return this;
}
Expand All @@ -153,7 +171,7 @@ private boolean isInclusive(List<PermissionKey> permissionKeys) {
}

PermissionSecurityChecksBuilder validatePermissionClasses(IndexView index) {
for (List<List<PermissionKey>> keyLists : methodToPermissionKeys.values()) {
for (List<List<PermissionKey>> keyLists : targetToPermissionKeys.values()) {
for (List<PermissionKey> keyList : keyLists) {
for (PermissionKey key : keyList) {
if (!classSignatureToConstructor.containsKey(key.classSignature())) {
Expand Down Expand Up @@ -187,7 +205,8 @@ PermissionSecurityChecksBuilder validatePermissionClasses(IndexView index) {

PermissionSecurityChecksBuilder gatherPermissionsAllowedAnnotations(List<AnnotationInstance> instances,
Map<MethodInfo, AnnotationInstance> alreadyCheckedMethods,
Map<ClassInfo, AnnotationInstance> alreadyCheckedClasses) {
Map<ClassInfo, AnnotationInstance> alreadyCheckedClasses,
List<AnnotationInstance> additionalClassInstances) {

// make sure we process annotations on methods first
instances.sort(new Comparator<AnnotationInstance>() {
Expand Down Expand Up @@ -217,7 +236,7 @@ public int compare(AnnotationInstance o1, AnnotationInstance o2) {
methodInfo.name(), methodInfo.declaringClass()));
}

gatherPermissionKeys(instance, methodInfo, cache, methodToPermissionKeys);
gatherPermissionKeys(instance, methodInfo, cache, targetToPermissionKeys);
} else {
// class annotation

Expand Down Expand Up @@ -245,7 +264,7 @@ public int compare(AnnotationInstance o1, AnnotationInstance o2) {
// ignore method annotated with other security annotation
boolean noMethodLevelSecurityAnnotation = !alreadyCheckedMethods.containsKey(methodInfo);
// ignore method annotated with method-level @PermissionsAllowed
boolean noMethodLevelPermissionsAllowed = !methodToPermissionKeys.containsKey(methodInfo);
boolean noMethodLevelPermissionsAllowed = !targetToPermissionKeys.containsKey(methodInfo);
if (noMethodLevelSecurityAnnotation && noMethodLevelPermissionsAllowed) {

gatherPermissionKeys(instance, methodInfo, cache, classMethodToPermissionKeys);
Expand All @@ -261,12 +280,16 @@ public int compare(AnnotationInstance o1, AnnotationInstance o2) {
}
}
}
methodToPermissionKeys.putAll(classMethodToPermissionKeys);
targetToPermissionKeys.putAll(classMethodToPermissionKeys);
for (var instance : additionalClassInstances) {
gatherPermissionKeys(instance, instance.target(), cache, targetToPermissionKeys);
}
return this;
}

private static void gatherPermissionKeys(AnnotationInstance instance, MethodInfo methodInfo, List<PermissionKey> cache,
Map<MethodInfo, List<List<PermissionKey>>> methodToPermissionKeys) {
private static <T extends AnnotationTarget> void gatherPermissionKeys(AnnotationInstance instance, T annotationTarget,
List<PermissionKey> cache,
Map<T, List<List<PermissionKey>>> targetToPermissionKeys) {
// @PermissionsAllowed value is in format permission:action, permission2:action, permission:action2, permission3
// here we transform it to permission -> actions
final var permissionToActions = new HashMap<String, Set<String>>();
Expand Down Expand Up @@ -299,9 +322,15 @@ private static void gatherPermissionKeys(AnnotationInstance instance, MethodInfo
}

if (permissionToActions.isEmpty()) {
throw new RuntimeException(String.format(
"Method '%s' was annotated with '@PermissionsAllowed', but no valid permission was provided",
methodInfo.name()));
if (annotationTarget.kind() == AnnotationTarget.Kind.METHOD) {
throw new RuntimeException(String.format(
"Method '%s' was annotated with '@PermissionsAllowed', but no valid permission was provided",
annotationTarget.asMethod().name()));
} else {
throw new RuntimeException(String.format(
"Class '%s' was annotated with '@PermissionsAllowed', but no valid permission was provided",
annotationTarget.asClass().name()));
}
}

// permissions specified via @PermissionsAllowed has 'one of' relation, therefore we put them in one list
Expand All @@ -324,13 +353,8 @@ private static void gatherPermissionKeys(AnnotationInstance instance, MethodInfo
}

// store annotation value as permission keys
methodToPermissionKeys
.computeIfAbsent(methodInfo, new Function<MethodInfo, List<List<PermissionKey>>>() {
@Override
public List<List<PermissionKey>> apply(MethodInfo methodInfo) {
return new ArrayList<>();
}
})
targetToPermissionKeys
.computeIfAbsent(annotationTarget, at -> new ArrayList<>())
.add(List.copyOf(orPermissions));
}

Expand Down Expand Up @@ -378,10 +402,10 @@ private SecurityCheck createSecurityCheck(LogicalAndPermissionPredicate andPredi
return securityCheck;
}

private PermissionWrapper createPermission(PermissionKey permissionKey, MethodInfo securedMethod,
private PermissionWrapper createPermission(PermissionKey permissionKey, AnnotationTarget securedTarget,
Map<PermissionCacheKey, PermissionWrapper> cache) {
var constructor = classSignatureToConstructor.get(permissionKey.classSignature());
return cache.computeIfAbsent(new PermissionCacheKey(permissionKey, securedMethod, constructor),
return cache.computeIfAbsent(new PermissionCacheKey(permissionKey, securedTarget, constructor),
new Function<PermissionCacheKey, PermissionWrapper>() {
@Override
public PermissionWrapper apply(PermissionCacheKey permissionCacheKey) {
Expand Down Expand Up @@ -568,8 +592,14 @@ private static final class PermissionCacheKey {
private final boolean computed;
private final boolean passActionsToConstructor;

private PermissionCacheKey(PermissionKey permissionKey, MethodInfo securedMethod, MethodInfo constructor) {
private PermissionCacheKey(PermissionKey permissionKey, AnnotationTarget securedTarget, MethodInfo constructor) {
if (isComputed(permissionKey, constructor)) {
if (securedTarget.kind() != AnnotationTarget.Kind.METHOD) {
throw new IllegalArgumentException(
"@PermissionAllowed instance that accepts method arguments must be placed on a method");
}
MethodInfo securedMethod = securedTarget.asMethod();

// computed permission
this.permissionKey = permissionKey;
this.computed = true;
Expand Down
21 changes: 21 additions & 0 deletions ...ent/src/main/java/io/quarkus/security/deployment/RegisterClassSecurityCheckBuildItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package io.quarkus.security.deployment;

import org.jboss.jandex.AnnotationInstance;
import org.jboss.jandex.DotName;

import io.quarkus.builder.item.MultiBuildItem;

/**
* Registers security check against {@link io.quarkus.security.spi.ClassSecurityCheckStorageBuildItem}
* for security annotation instances passed in this build item.
*/
final class RegisterClassSecurityCheckBuildItem extends MultiBuildItem {

final DotName className;
final AnnotationInstance securityAnnotationInstance;

RegisterClassSecurityCheckBuildItem(DotName className, AnnotationInstance securityAnnotationInstance) {
this.className = className;
this.securityAnnotationInstance = securityAnnotationInstance;
}
}
Loading
Loading