RESTEasy Reactive - prevent repeating of standard security checks

[michalvavrik]

fix: #26536

EagerSecurityHandler performs standard security checks for endpoints annotated with an RBAC annotation and the same checks are later done by SecurityHandler. We still need interceptors to run for CDI beans and gRPC. This PR prevents repeating of security checks by propagating the information that check is done to SecurityHandler via invocation context.

[michalvavrik]

cc @sberyozkin @geoand

[geoand]

Thanks for this!

CI seems unhappy however :)

[sberyozkin]

@michalvavrik Thanks, it looks technically totally fine, thanks for working out how to handle it.
But it feels like we are doing a workaround, which in the RestEasy Reactive security case will involve running an extra interceptor with a few extra checks involved.
Would it be possible instead to remove RolesAllowedInterceptor/etc at the build level in the case where they are known to duplicate the checks ?

[michalvavrik]

@sberyozkin yes, it would be possible and I actually already had it in progress... I stopped as the binding is between annotation (and its values) and the interceptor, not between method and the interceptor. Please remember we still need other annotated methods not checked by EagerSecurityHandler to be intercepted.

I had in progress InterceptedMethodBindingFilter that would be registered in the build time and break the binding between the method and interceptor during the build time somewhere around io.quarkus.arc.processor.BeanInfo#initInterceptedMethods. I just didn't think you would be happy if I messed with Arc Processor :-).

Is that how you meant it?

[michalvavrik]

My point is that the check can only be removed on "method invocation" scope, not request scope as you need nested CDI beans to be able to perform its own (different checks) within same request.

[sberyozkin]

Hey @michalvavrik the last thing that should be of concern is if some code in PR can make me less excited :-), you keep creating good quality PRs I may not be even getting from the first round of review :-).
But IMHO, if tweaking around io.quarkus.arc.processor.BeanInfo#initInterceptedMethods can disable those CDI interceptors then it would be preferred; have a look please when you get a chance, I guess we should not rush with merging it asap, so please take your time
thanks

[michalvavrik]

Sure, I'll find a build time solution as discussed. I changed PR to draft then.

[michalvavrik]

@sberyozkin ready for review

[sberyozkin]

@michalvavrik Great, sorry for a delay, can you please resolve the conflict ?

[michalvavrik]

Sure, conflict resolved.

[sberyozkin]

@michalvavrik I think this is a very nicely researched and done PR.

However, I think we need to get at least one more review as there might be some details that I've missed.

Thanks

[sberyozkin]

@mkouba Hi Martin, can you please have a look at the Arc related code in this PR when you are get a chance ?

[Ladicek]

I personally don't like the concept of "interceptor binding filtering", as it seems too easy to misuse, but I didn't see the code, so my hesitation is really quite a bit uninformed.

That said, let me draw a parallel with a very similar problem I'm facing with SmallRye Fault Tolerance (#21431). If a JAX-RS resource method is annotated @Blocking and e.g. @CircuitBreaker, then both RESTEasy Reactive and SmallRye Fault Tolerance will notice the @Blocking annotation and offload the method call to another thread. Essentially the same situation -- 1 annotation used by 2 frameworks that don't know about each other and can't easily communicate out of band. There's one more thing these two situations have in common -- the double behavior (be it double security check, or double thread offload) isn't really problematic in essence (as in, it doesn't cause bugs), it's just useless work (which should obviously be avoided).

It makes me wonder: is there an easy way to figure out if an intercepted method is a JAX-RS resource method? An interceptor (be it the security interceptor, or the fault tolerance interceptor) could adjust its behavior based on that. I think resource methods always have to have the HTTP verb annotation (@GET etc.), no? There's a fixed set of those annotations, and all other HTTP verb annotations must have one meta-annotation (whose name I can't remember). Right? (Not sure how subresource locators fit into this...) Would that make sense?

[geoand]

I like the idea @Ladicek, but I am wondering if we can make it a little more general.
My thinking is that each framework (in this case JAX-RS) knows about itself, so maybe it could push information into the CDI execution context in order to identify itself?

[Ladicek]

That would be great, I also thought about that a little -- problem is that the invocation context starts to exist only when the method in question is actually invoked. So at the point when RESTEasy Reactive does the security check, there's no shared storage for that information. A request-scoped bean might work, but it would bring a hard dependency on context propagation.

What I can imagine is ArC exposing an API for invoking a method and adding some contextual information to the invocation context.

I guess RESTEasy Reactive currently has to have some code that invokes the resource method. I don't know how it looks, but if the invocation is reflective, there must be something like this: method.invoke(instance, params). It could probably look like this instead:

Map<String, Object> data = Map.of("resteasy.reactive.resource.method", Boolean.TRUE);
Arc.container().runWithInterceptionContext(() -> method.invoke(instance, params), data);

(It would probably be a bit more complex, but you get the idea...)

[geoand]

The invocation in RR is not reflective, it's generated code, so we can do whatever we like in there :)

[Ladicek]

Right, gotcha.

The API I suggest above still requires some form of out-of-band communication, but that would now have a very short lifespan, so a thread local variable should be fine. It would still be quite a bit unsafe. Maybe @mkouba can think of something better/safer, but I think the approach should work.

[geoand]

👍🏼

[Ladicek]

I just realized that the concept of "executable methods", which we unfortunately failed to explore in the CDI 4.0 timeframe (see jakartaee/cdi#460), would be a perfect fit for this. If we had that, it would likely be straightforward to extend to support extending the context data map. Oh well.

[mkouba]

My thinking is that each framework (in this case JAX-RS) knows about itself, so maybe it could push information into the CDI execution context in order to identify itself?

Well, there's no such thing like "CDI execution context". Yes, we have javax.interceptor.InvocationContext for intercepted methods but it's not stored in a threadlocal variable so you can't easily get a reference outside of the interceptors chain.

...but that would now have a very short lifespan, so a thread local variable should be fine. It would still be quite a bit unsafe.

It's safe until the execution is offloaded to a different thread.

I think that an idiomatic solution would be to introduce a new interceptor with priority 0 for any resource method, and this interceptor would set a flag in the context data map, i.e. something like InvocationContext.getContextData().put("resteasy.reactive.resource.method", Boolean.TRUE). Other interceptors in the chain could then read the context data map and adapt the behavior accordingly.

[michalvavrik]

@mkouba that was my solution number one (it's already implemented), as long as other agrees, we could use interceptor solution for now.

[geoand]

Fine with me

[michalvavrik]

Done.

[geoand]

I added a minor comment, but otherwise this is a good with me!

@sberyozkin, @mkouba mind taking a look as well?

[mkouba]

I added a minor comment, but otherwise this is a good with me!

@sberyozkin, @mkouba mind taking a look as well?

I've added a minor comment too and it's already addressed ;-). So let's wait for Sergey?

[gsmet]

General comment but I see a lot of you here so it's in good hands: we should be extremely careful given we are changing a behavior related to security. Better check things three times to make sure we are all good.

[mkouba]

General comment but I see a lot of you here so it's in good hands: we should be extremely careful given we are changing a behavior related to security. Better check things three times to make sure we are all good.

That's the reason why we wait for Sergey's opinion as well.

[quarkus-bot]

Failing Jobs - Building e2e94f1

Status	Name	Step	Failures	Logs	Raw logs
✖	Gradle Tests - JDK 11	Build	Failures	Logs	Raw logs
✔️	Gradle Tests - JDK 11 Windows

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.CompositeBuildWithDependenciesDevModeTest.main line 24 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

[michalvavrik]

ping @sberyozkin

[sberyozkin]

Hi All, apologies for keeping you waiting, was back on Mon but missed this one. So it is back to the original solution, which is fine, it is the easiest to understand.
@michalvavrik sorry you've had to do so many iterations :-), but as I said IMHO it was worth a try, as some very interesting comments were added, and Ladislav has identified what may prove to be the perfect solution once CDI executable methods are available, so there is a chance we can revisit this solution in the future :-)