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

Offer a way to block inside custom SecurityContext.isUserInRole with resteasy-reactive #38940

Closed
JChrist opened this issue Feb 21, 2024 · 28 comments · Fixed by #39920
Closed

Comments

@JChrist
Copy link

JChrist commented Feb 21, 2024

Description

When using resteasy-reactive and creating a custom SecurityContext, attempting to block inside isUserInRole is forbidden, e.g. for looking up a specific role requested with @RolesAllowed in the database.

Unfortunately, there is currently no (easy) way to overcome this situation, since, even if the resolved endpoint containing the RolesAllowed annotation is blocking, the check will happen on an IO thread.

The only workaround I have found so far, is to check inside isUserInRole if blocking is allowed, with BlockingOperationControl.isBlockingAllowed(). If not, then return true, so that the request is not rejected immediately and add a @ServerRequestFilter (allowed to block) that injects ResourceInfo, checks if there are RolesAllowed annotations present in the matched endpoint and actually perform the role check.

I have created an example project with this: https://github.com/JChrist/quarkus-blocking-roles
While this example is trivial to resolve, by not needing to block to check the role, let's assume that in a real-world scenario that wouldn't be the case, e.g. in the case where the number of roles would be too many to pre-load.

Ideally, I would like for a way to instruct quarkus that this check is performed after the request is offloaded to a worker thread. I understand that this means that all requests would need to become blocking, but as things stand, even if all requests are blocking, this check still happens on IO thread.

Implementation ideas

No response

@JChrist JChrist added the kind/enhancement New feature or request label Feb 21, 2024
Copy link

quarkus-bot bot commented Feb 21, 2024

/cc @FroMage (resteasy-reactive), @geoand (resteasy-reactive), @sberyozkin (security), @stuartwdouglas (resteasy-reactive)

@sberyozkin
Copy link
Member

@JChrist Hi, can you clarify one point please,

let's assume that in a real-world scenario that wouldn't be the case, e.g. in the case where the number of roles would be too many to pre-load.

The current security context represents an individual authenticated identity, a user, with one or more roles, but a single user would not have a big number of roles. Can you clarify why you can't load roles specific to the current user only at the security context creation time ?

@JChrist
Copy link
Author

JChrist commented Feb 22, 2024

For my specific use-case, the number of available roles to each user can be in the thousands and behind the scenes, isUserInRole is re-using the user-role cache (that may go to db and block).
One solution would be to extract the static roles that appear on RolesAllowed and load those in SecurityContext. However, with this approach, the number of roles could still be relatively high (a few hundreds), so the query that loads the principal and creates the SecurityContext would need to load much more data than really needed and I feel that it would become error-prone: adding RolesAllowed is not enough to allow access, one would still need to change the loaded roles to work.

@sberyozkin
Copy link
Member

sberyozkin commented Feb 22, 2024

@JChrist Can you issue a query to the DB where the query only checks if the roles listed in RolesAllowed exist in the DB set of roles corresponding to the current user ?

@JChrist
Copy link
Author

JChrist commented Feb 22, 2024

@sberyozkin as I tried to explain, this could be a solution, but the total number of roles listed in @RolesAllowed is a few hundred ones (what I referred to as static roles) and each user could have any number of them, quite possibly all.
Usually each @RolesAllowed contains a single role, but each endpoint contains a different one.
This means that (for my specific use-case), this is still impractical.

@sberyozkin
Copy link
Member

@JChrist Sure, but in context of the specific JAX-RS method invocation, there is only one or very few roles configured in @RolesAllowed, so what I've been thinking about is that the request filter which creates that method specific security context can check, in a blocking mode, if the user has that role, and then isUserInRole will return the result of that check.

It looks like that this is more or less what you'd like to do anyway but from isUserInRole, so the proposal is to do it at the security context creation time - except that to get the user role you'd need check ResourceInfo of the matched class.

Do you think what I'm suggesting is totally different to what you are proposing ?

@JChrist
Copy link
Author

JChrist commented Feb 22, 2024

Oh, sorry, now I understood your suggestion!
That would pretty much be exactly what I need.
My issue is that the SecurityContext is created at pre-matching time, where ResourceInfo is not yet available (or that's my understanding of the inner workings).

@sberyozkin
Copy link
Member

@JChrist Np, thanks for clarifying. Is there any particular reason you prefer to do it at the pre-matching time ?

@JChrist
Copy link
Author

JChrist commented Feb 22, 2024

@sberyozkin no, not really. Just that I couldn't make it work any other way. My tests showed that my normal (i.e. not pre-matching) filter would not even be reached otherwise.

@sberyozkin
Copy link
Member

sberyozkin commented Feb 22, 2024

@JChrist Right, please consider creating a small reproducer related to the post-match filter not reached, sounds like it should work so if it does not for any obvious reasons there might be a bug lurking somewhere.
I know this is not exactly per your original suggestion, but managing isUserInRole would be more challenging if possible at all, since it can be called directly from the user code as well...

@JChrist
Copy link
Author

JChrist commented Feb 23, 2024

Hi @sberyozkin,
I've updated my test project (here https://github.com/JChrist/quarkus-blocking-roles) to keep only the post-matching filter and I've added a test that should pass if the post-matching filter was reached.
The test for the endpoint with the RolesAllowed fails with a 401 response, which is not generated from the application, it's produced by quarkus.

I also see in the documentation a notice about using pre-matching for custom security context here: https://quarkus.io/guides/security-customization#jaxrs-security-context

@sberyozkin
Copy link
Member

Hi @JChrist

I also see in the documentation a notice about using pre-matching for custom security context here

Sorry I forgot about it.

Hmm... What I'm surprised though is that the filter is not reached at all, if it were reached then maybe we could update the CurrentIdentityAssociation directly.

Can you please give me a favor and check a couple of things: 1) can you just try @Provider and implements ContainerRequestFilter abd run it at the earliest Priority possible for the post match filters - as far as I recall @ServerRequestFilter optimizes things so I wonder if these filters are run after all other post match filters are done
2) Please check in the debug mode it was indeed not reached - sorry if you already did it

Perhaps another option is to use a pre-match filter but instead of RolesAllowed use HTTP policy configuration and access it in the per-match JAX-RS filter, but lets try to explore the post-match filter option a bit more.

@michalvavrik, if you have any other ideas, please comment

@michalvavrik
Copy link
Member

This works as I expected. I can't react all that is written (there is too much of it), but I run your tests:

  1. The gr.jchrist.MyFilter#filterAfterMatch is not invoked because we run security checks for the RolesAllowed annotation that secures the gr.jchrist.GreetingResource#hello` as the first thing after match. You need to run it before match in order to augment the identity with roles required by security checks.
  2. The gr.jchrist.MyFilter#filter works (DB is accessed) but you try to get resource class / method there, which is expected to be null in pre-match phase.

Your tests are passing if you do not require resource match (I'm sure you know that, but just that we have same starting point), that is with this patch:

diff --git a/src/main/java/gr/jchrist/MyFilter.java b/src/main/java/gr/jchrist/MyFilter.java
index 16c4153..cf30362 100644
--- a/src/main/java/gr/jchrist/MyFilter.java
+++ b/src/main/java/gr/jchrist/MyFilter.java
@@ -33,8 +33,8 @@ public class MyFilter {
     // @Blocking // this fails with IllegalStateException: Wrong usage(s) of @Blocking found
     // returning Optional<Response> will not help either
     // if this is not enabled, then the `filterAfterMatch` is never reached, the request is rejected before it is called.
-    // @ServerRequestFilter(preMatching = true)
-    /* public Uni<Void> filter(ContainerRequestContext requestContext) {
+    @ServerRequestFilter(preMatching = true)
+    public Uni<Void> filter(ContainerRequestContext requestContext) {
         return vertx.executeBlocking(() -> {
             if (requestContext.getHeaders().containsKey(HttpHeaders.AUTHORIZATION)) {
                 var myEntity = fetchMyEntity(requestContext.getHeaderString(HttpHeaders.AUTHORIZATION));
@@ -49,9 +49,9 @@ public class MyFilter {
             }
             return null;
         });
-    } */
+    }
 
-    @ServerRequestFilter()
+    @ServerRequestFilter
     public Optional<Response> filterAfterMatch(ContainerRequestContext requestContext) {
         MySecurityContext securityContext = null;
         MyEntity myEntity = null;
@@ -63,7 +63,7 @@ public class MyFilter {
         }
         if (resourceInfo == null || resourceInfo.getResourceClass() == null || resourceInfo.getResourceMethod() == null) {
             logger.error("resource info injection failed!");
-            return Optional.of(Response.serverError().build());
+            return Optional.of(Response.ok().build());
         }
         Set<String> roles = new HashSet<>();
         if (resourceInfo.getResourceClass().isAnnotationPresent(RolesAllowed.class)) {
@@ -115,7 +115,7 @@ public class MyFilter {
 
     public record MySecurityContext(MyEntity entity, Set<String> roles) implements SecurityContext {
         public MySecurityContext(MyEntity myEntity) {
-            this(myEntity, new HashSet<>());
+            this(myEntity, Set.of(myEntity.field));
         }
 
         @Override

Which means your only problem is that you want to enhance identity based on the resource match. That can be done in Quarkus 3.9 (and 999-SNAPSHOT as it requires #38772), please see that resource method is available and identity is augmented:

diff --git a/pom.xml b/pom.xml
index a5422eb..f1ea153 100644
--- a/pom.xml
+++ b/pom.xml
@@ -13,8 +13,8 @@
     <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
     <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
     <quarkus.platform.artifact-id>quarkus-bom</quarkus.platform.artifact-id>
-    <quarkus.platform.group-id>io.quarkus.platform</quarkus.platform.group-id>
-    <quarkus.platform.version>3.7.4</quarkus.platform.version>
+    <quarkus.platform.group-id>io.quarkus</quarkus.platform.group-id>
+    <quarkus.platform.version>999-SNAPSHOT</quarkus.platform.version>
     <skipITs>true</skipITs>
     <surefire-plugin.version>3.2.5</surefire-plugin.version>
   </properties>
diff --git a/src/main/java/gr/jchrist/AugmentorPolicy.java b/src/main/java/gr/jchrist/AugmentorPolicy.java
index 4045fed..b030457 100644
--- a/src/main/java/gr/jchrist/AugmentorPolicy.java
+++ b/src/main/java/gr/jchrist/AugmentorPolicy.java
@@ -1,4 +1,40 @@
 package gr.jchrist;
 
-public class AugmentorPolicy {
+import io.quarkus.security.identity.SecurityIdentity;
+import io.quarkus.security.runtime.QuarkusPrincipal;
+import io.quarkus.security.runtime.QuarkusSecurityIdentity;
+import io.quarkus.security.spi.runtime.BlockingSecurityExecutor;
+import io.quarkus.vertx.http.runtime.security.HttpSecurityPolicy;
+import io.smallrye.mutiny.Uni;
+import io.vertx.ext.web.RoutingContext;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.inject.Inject;
+import jakarta.ws.rs.container.ResourceInfo;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@ApplicationScoped
+public class AugmentorPolicy implements HttpSecurityPolicy {
+
+    private static final Logger logger = LoggerFactory.getLogger(AugmentorPolicy.class);
+
+    @Inject
+    ResourceInfo resourceInfo;
+
+    @Inject
+    BlockingSecurityExecutor blockingExecutor;
+
+    @Override
+    public Uni<CheckResult> checkPermission(RoutingContext request, Uni<SecurityIdentity> identity, AuthorizationRequestContext requestContext) {
+        return blockingExecutor.executeBlocking(() -> {
+            // TODO: DO YOUR BLOCKING STUFF
+            logger.info("Matched resource method is " + resourceInfo.getResourceMethod());
+            return new CheckResult(true, QuarkusSecurityIdentity.builder().addRole("field-1").setPrincipal(new QuarkusPrincipal("Sergey")).build());
+        });
+    }
+
+    @Override
+    public String name() {
+        return "augmentor";
+    }
 }
diff --git a/src/main/java/gr/jchrist/MyFilter.java b/src/main/java/gr/jchrist/MyFilter.java
index 16c4153..36460db 100644
--- a/src/main/java/gr/jchrist/MyFilter.java
+++ b/src/main/java/gr/jchrist/MyFilter.java
@@ -51,7 +51,7 @@ public class MyFilter {
         });
     } */
 
-    @ServerRequestFilter()
+//    @ServerRequestFilter()
     public Optional<Response> filterAfterMatch(ContainerRequestContext requestContext) {
         MySecurityContext securityContext = null;
         MyEntity myEntity = null;
diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties
index d7ab9c9..2115846 100644
--- a/src/main/resources/application.properties
+++ b/src/main/resources/application.properties
@@ -39,4 +39,8 @@ quarkus.flyway.validate-on-migrate=false
 
 # jakarta-rs api path
 #quarkus.resteasy.path=/
-quarkus.resteasy-reactive.path=/
\ No newline at end of file
+quarkus.resteasy-reactive.path=/
+
+quarkus.http.auth.permission.identity-augmentation.paths=/*
+quarkus.http.auth.permission.identity-augmentation.policy=augmentor
+quarkus.http.auth.permission.identity-augmentation.applies-to=JAXRS
\ No newline at end of file

Which reminds me I'll need to improve JAX-RS HTTP perms docs as I'm worried people using it like this without more info. It will only work for endpoints secured with RBAC or @Tenant (as it is meant for @Tenant and the other is side effect).

Anyway, I hope this solves your problem and let me know if I missed something.

@michalvavrik
Copy link
Member

michalvavrik commented Feb 26, 2024

@sberyozkin if being able to access matched resource method during security augmentation is of interest, I could tweak JAX-RS HTTP permissions little so that they can be used for this augmentation in all cases, because now you can only use them in one of following cases:

  • endpoint is annotated with @Tenant
  • endpoint is annotated with standard security annotation
  • default JAX-RS security is in place

We could add some config property that would always enable it. However I have certain doubts this is common requirement, isn't this rather edge scenario? Maybe what we have is already enough (+docs improvements).

@sberyozkin
Copy link
Member

@michalvavrik Oh, cool, that is a great thing that you can now inject ResourceInfo in the custom policy, with applies to JAX-RS, great stuff.
I think the suggested patch above does not work as @JChrist does not want to preload a massive number of roles they have in this case, which is why the idea was to issue a query with the expected roles.
But I think something can be worked out.
Let me comment more in the next couple of days re your suggestions.

By the way, the reason I was surprised the post match filter was not reached was because I recall at some point an anonymous identity was created when no security extensions were available, but then I checked and saw smallrye-jwt in the pom...

Thanks for the super fast feedback

@JChrist
Copy link
Author

JChrist commented Mar 29, 2024

Hi @sberyozkin,
pinging about this.
Perhaps with Quarkus 3.9.x out, is there any better approach?

@FroMage
Copy link
Member

FroMage commented Mar 29, 2024

This code, from Quarkus-WebAuthn supports blocking calls as part of security on the IO thread, here's how it's done:

    @SuppressWarnings({ "rawtypes", "unchecked" })
    private <T> Uni<T> runPotentiallyBlocking(Supplier<Uni<? extends T>> supplier) {
        if (BlockingOperationControl.isBlockingAllowed()
                || isNonBlocking(userProvider.getClass())) {
            return (Uni<T>) supplier.get();
        }
        if (isRunOnVirtualThread(userProvider.getClass())) {
            return Uni.createFrom().deferred(supplier).runSubscriptionOn(VirtualThreadsRecorder.getCurrent());
        }
        // run it in a worker thread
        return vertx.executeBlocking(Uni.createFrom().deferred((Supplier) supplier));
    }

    private boolean isNonBlocking(Class<?> klass) {
        do {
            if (klass.isAnnotationPresent(NonBlocking.class))
                return true;
            if (klass.isAnnotationPresent(Blocking.class))
                return false;
            if (klass.isAnnotationPresent(RunOnVirtualThread.class))
                return false;
            klass = klass.getSuperclass();
        } while (klass != null);
        // no information, assumed non-blocking
        return true;
    }

That should help you :)

@FroMage
Copy link
Member

FroMage commented Mar 29, 2024

You can use this to fetch your user roles from a DB within the security layer, when creating the identity.

@JChrist
Copy link
Author

JChrist commented Mar 29, 2024

Hi @FroMage and thanks for the input!
I'm a bit confused on how to use this within my example, as I'm using a custom security context and the problematic method is isUserInRole which does not return a Uni.

@michalvavrik
Copy link
Member

michalvavrik commented Mar 29, 2024

I'm using a custom security context and the problematic method is isUserInRole which does not return a Uni.

I'm sorry, but I don't understand this part. I thought I explained both why your resource is null and how you should augment the identity in a blocking manner based on matched resource.

What I mean is: there can't be better way for reasons explained in previous comment in detail. And BTW my suggestion returns Uni.

Only thing is that I waited for @sberyozkin for advertised feedback before making this general solution (meaning -> allow using JAX-RS HTTP perms without @Tenant or RBAC or default JAX-RS security).

That should help you :)

@FroMage should maybe https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/VertxBlockingSecurityExecutor.java use VirtualThreadsRecorder.getCurrent() when on virtual thread? I expect it is used on worker thread or IO thread, but I am yet to study virtual threads in Quarkus. I should probably check or do you know, please?

@JChrist
Copy link
Author

JChrist commented Apr 1, 2024

Hi @michalvavrik!
I finally found how to successfully apply your suggested code (sorry it took me so long, somehow I always missed the application.properties part 🤦).
Now, I have everything in place, apart from how/when to set the custom SecurityContext, which is injected in my jakarta-rs endpoints.
If I remove the filter and keep only the augmentor, then the user principal in the injected security context is null.
If I set the custom security context at the filter in the pre-matching phase (@ServerRequestFilter(preMatching = true)), then the injected ResourceInfo in the augmentor contains null method/class.
If I set the custom security context at the filter in the post-match phase (@ServerRequestFilter), then I have to perform the checks again, as the injected SecurityIdentity is anonymous (i.e. not the one from augmentor). While this seems that it could work, it feels a bit clunky to have to duplicate the checks. Is there anything I missed again or is this the way?

@michalvavrik
Copy link
Member

michalvavrik commented Apr 1, 2024

Hi @michalvavrik! I finally found how to successfully apply your suggested code (sorry it took me so long, somehow I always missed the application.properties part 🤦).

np, I'll use this issue eventually (might take few weeks before I get to it) to document this and improve it.

Now, I have everything in place, apart from how/when to set the custom SecurityContext, which is injected in my jakarta-rs endpoints. If I remove the filter and keep only the augmentor, then the user principal in the injected security context is null. If I set the custom security context at the filter in the pre-matching phase, then the injected ResourceInfo in the augmentor contains null method/class. If I set the custom security context at the filter in the post-patch phase, then I have to perform the checks again, as the injected SecurityIdentity is anonymous (i.e. not the one from augmentor). While this seems that it could work, it feels a bit clunky to have to duplicate the checks. Is there anything I missed again or is this the way?

You need to remember the order of things - your SecurityContext filter runs before match and the JAX-RS HTTP Security Policy as the first thing after the match, but before any authorization using configuration or before the security checks applied for the standard security annotations likes the @RolesAllowed.

It would be easier to look at the code, e.g. if you provided updated reproducer, however I will try to answer step by step:

If I remove the filter and keep only the augmentor, then the user principal in the injected security context is null.

I would expect that if you remove your filter and leave the default one io.quarkus.resteasy.reactive.server.runtime.ResteasyReactiveSecurityContext then the principal is taken from user and everything is ready before the authorization begins.

If I set the custom security context at the filter in the pre-matching phase, then the injected ResourceInfo in the augmentor contains null method/class.

I can't see any connection between these 2, how one could affect the other. It would be bug provided you are sure about this. If so, please provide a reproducer.

My first thought would be whether you activate CDI request inside that pre-match filter and in what fashion.

If I set the custom security context at the filter in the post-patch phase, then I have to perform the checks again, as the injected SecurityIdentity is anonymous (i.e. not the one from augmentor). While this seems that it could work, it feels a bit clunky to have to duplicate the checks. Is there anything I missed again or is this the way?

Absolutely, don't do that. You can either use custom JAX-RS SecurityContext in pre-match phase or JAX-RS HTTP Security Policy that will have the resource available. You can use both, but remember the order.

@JChrist
Copy link
Author

JChrist commented Apr 2, 2024

I have now updated the sample code to include the HttpSecurityPolicy.
If you run the test as is, then it showcases that the injected ResourceInfo inside the AugmentorPolicy contains null class / method.
If you comment out @ServerRequestFilter(preMatching = true) in line 33 of MyFilter, then the injected ResourceInfo is OK, but then, inside GreetingResource the injected SecurityContext contains null user principal.
@michalvavrik could you please take a look?

@michalvavrik
Copy link
Member

I have now updated the sample code to include the HttpSecurityPolicy. If you run the test as is, then it showcases that the injected ResourceInfo inside the AugmentorPolicy contains null class / method. If you comment out @ServerRequestFilter(preMatching = true) in line 33 of MyFilter, then the injected ResourceInfo is OK

Right, it's null because the resource info is request scoped bean and you create it during pre-match when it's still null (it's injected in that filter as well...). By no means I'm authority on this, but FWIW not a bug. Just don't create the bean during pre-match.

, but then, inside GreetingResource the injected SecurityContext contains null user principal. @michalvavrik could you please take a look?

Because identity augmented by HTTP Permissions is not reflected in the JAX-RS SecurityContext. In other words, we do synchronize it, but before it happens. That's a bug, I'll fix that.

TL;DR; @JChrist you scenario is legit, but we need little tweaks for it. I'd rely on the SecurityIdentity and JAX-RS HTTP SecurityPolicies for now. Or you can wait for next release, I'll address it this week (+ review). Thanks for the patience.

@michalvavrik
Copy link
Member

To stay safe, I suggest to wait for the fix.

@FroMage
Copy link
Member

FroMage commented Apr 2, 2024

should maybe https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/VertxBlockingSecurityExecutor.java use VirtualThreadsRecorder.getCurrent() when on virtual thread? I expect it is used on worker thread or IO thread, but I am yet to study virtual threads in Quarkus. I should probably check or do you know, please?

Ask @cescoffier

@cescoffier
Copy link
Member

No, it should not use the virtual thread scheduler. The virtual thread scheduler makes a best-effort approach to return to the worker thread pool; however, there are subtle differences, and it was only made for methods annotated with @RunOnVirtualThread (which is not your case here).

@michalvavrik
Copy link
Member

No, it should not use the virtual thread scheduler. The virtual thread scheduler makes a best-effort approach to return to the worker thread pool; however, there are subtle differences, and it was only made for methods annotated with @RunOnVirtualThread (which is not your case here).

Understand, thank you for your time @cescoffier .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment