Upgrading to 3.9.2 from 3.8.3 RolesAllowed on implementing class of an interface using jaxrs are not used. about quarkus

[sberyozkin]

Hey Michal, well, we don't want to resurface the old vulnerabilities, for sure. I'm only a little bit confused why, with @RolesAllowed being on the interface, we correctly handle things in 3.8.x, without any vulnerabilities, the PR, #38622, is about consolidating/making things more robust, as opposed to fixing anything in particular .
In any case, we don't have to rush things, lets discuss things in the next few days, it is not fair to expect you keeping every possible subtlety in mind, the work you've done around these issues, have improved things significantly, we'll figure something re this issue too, good night :-), I'm certainly signing off :-)

[quarkus-bot]

/cc @pedroigor (bearer-token)

[sberyozkin]

Thanks for debugging @psini,

AFAIK, this is done to avoid various side-effects related to the implementation classes missing out on some of the interface related security settings.

@michalvavrik Hi Michal, it is a migration from the recent version, it is this commit, 46b39c8

This scenario looks legit, what do you think ?

[psini]

If i add the @RolesAllowed on the Interface (that violates the spec) it restart to work

[michalvavrik]

After the upgrade it goes in deny (my default policy is deny) because it doen't read the RolesAllowed Annotation.
46b39c8

we perform security checks sooner to limit processing; that alone is a good thing

If i add the @RolesAllowed on the Interface (that violates the spec) it restart to work

we have tests that checks default JAX-RS deny all configuration option on the interface that is overriden without PATH / GET

• https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/DenyAllJaxRsTest.java#L68
• https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResourceInterface.java
• https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResource.java#L23

then I put RolesAllowed on the implementation and the security check is in place

diff --git a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResource.java b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResource.java
index 7339c9c1eda..1104238c3fe 100644
--- a/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResource.java
+++ b/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/security/UnsecuredResource.java
@@ -2,6 +2,7 @@
 
 import jakarta.annotation.security.DenyAll;
 import jakarta.annotation.security.PermitAll;
+import jakarta.annotation.security.RolesAllowed;
 import jakarta.ws.rs.GET;
 import jakarta.ws.rs.Path;
 import jakarta.ws.rs.PathParam;
@@ -19,6 +20,7 @@ public String defaultSecurity() {
         return "defaultSecurity";
     }
 
+    @RolesAllowed("barik")
     @Override
     public String defaultSecurityInterface() {
         return UnsecuredResourceInterface.super.defaultSecurityInterface();

@psini simple reproducer would help, or at least examples of the classes and configuration properties please

that violates the spec

according to the spec you can use Path, GET on the interface but not RolesAllowed? please can you link the specs and the section

[psini]

I don't want to use @RolesAllowed on Interfaces.
I have this properties set
quarkus.security.jaxrs.deny-unannotated-endpoints=true

example of my code is :

package a;

import jakarta.annotation.security.RolesAllowed;
import jakarta.ws.rs.Consumes;
import jakarta.ws.rs.POST;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.PathParam;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.QueryParam;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.Response;
import org.eclipse.microprofile.openapi.annotations.Operation;
import org.eclipse.microprofile.openapi.annotations.parameters.RequestBody;
import org.eclipse.microprofile.openapi.annotations.responses.APIResponse;
import org.eclipse.microprofile.openapi.annotations.responses.APIResponses;
import org.eclipse.microprofile.openapi.annotations.tags.Tag;

import java.io.InputStream;
import java.util.List;

@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
@Path("/api/v1/executors/gw")

public interface A  {
    
    @APIResponses(value = {@APIResponse(responseCode = "400", description = "AAA"), @APIResponse(responseCode = "200", description = "BBB.")})
    @Operation(summary = "VVV", description = "XXXXXX")
    @Tag(name = "XXXX")
    @POST
    @ReturnStatus(Response.Status.OK)
    @Path("/resources")
    Response handleA (
            @RequestBody(description = "XXXX", required = true) String req
    ) throws ServiceException;
}


@RequestScoped
@RolesAllowed({"A", "B"})
public class CloudTaskLauncherGWEndpointV1Rest extends AbstractGWEndpoint implements CloudTaskLauncherGWEndpointV1 {

   
    @Override
    public Response handleA(String request) throws ServiceException {

        return Response.ok().build();
    }
}

[michalvavrik]

Thank you. I have created a reproducer https://github.com/michalvavrik/roles-allowed-on-interface-resteasy-classic-reproducer (use mvn clean test). This should help us to have a reference point for our discussion.

The RESTEasy Classic produces jakarta.ws.rs.container.ResourceInfo that says that resource class is org.acme.GreetingInterface and method is org.acme.GreetingInterface#sayHi. Not overriden method without any Path and GET.

You set quarkus.security.jaxrs.deny-unannotated-endpoints=true which denies all unannotated endpoints as its name says. And your overriden method is not an endpoint according to what ResourceInfo returns.

@psini I understand that you don't want to use the RolesAllowed annotation on the interface and you are right it would be wrong to use it for the REST client. If there is something that is violating specs as you said, please comment. Let's gather opinions on this situation, I provided my view.

therefore to answer @sberyozkin questions:

This scenario looks legit, what do you think ?

my view is that it is not legit for the reasons I explained above

@geoand please comment if you have what to say or if my endpoint-identification comments are wrong. you know this field much better. I have tried "my" reproducer on Quarkus REST and it behaves same.

[gsmet]

My understanding is that the same code being properly protected in 3.8 is not protected in 3.9?

If I understood things correctly, then we have a problem, regardless of is it per spec or not.

[michalvavrik]

My understanding is that the same code being properly protected in 3.8 is not protected in 3.9?

Access is denied by default JAX-RS policy, therefore it is protected. Roles allowed CDI interceptor would still be applied if the default JAX-RS policy didn't run eagerly. It is more safety, not less.

If I understood things correctly, then we have a problem, regardless of is it per spec or not.

I believe we need to gather other opinions, because I provided the best arguments I could.

[michalvavrik]

To be clear what changed @gsmet : previously in Quarkus RESTEasy Classic, the default JAX-RS security didn't detect the interface endpoint, so no JAX-RS security was applied and therefore, it could go down to the RolesAllowed interceptor (which is still there, is's a bean after all, just drop deny-all and you will get roles allowed check applied).

This is the same behavior as the Quarkus REST have and I introduced it because the JAX-RS security speaks about endpoints and previously we didn't detect some of them (like in this case).

[psini]

Thank you. I have created a reproducer https://github.com/michalvavrik/roles-allowed-on-interface-resteasy-classic-reproducer (use mvn clean test). This should help us to have a reference point for our discussion.

The RESTEasy Classic produces jakarta.ws.rs.container.ResourceInfo that says that resource class is org.acme.GreetingInterface and method is org.acme.GreetingInterface#sayHi. Not overriden method without any Path and GET.

You set quarkus.security.jaxrs.deny-unannotated-endpoints=true which denies all unannotated endpoints as its name says. And your overriden method is not an endpoint according to what ResourceInfo returns.

@psini I understand that you don't want to use the RolesAllowed annotation on the interface and you are right it would be wrong to use it for the REST client. If there is something that is violating specs as you said, please comment. Let's gather opinions on this situation, I provided my view.

Yes It is what I have done before and it was working. I don't want to change my code unless it is wrong

therefore to answer @sberyozkin questions:

This scenario looks legit, what do you think ?

I think yes but what is your opinion @sberyozkin?

my view is that it is not legit for the reasons I explained above

@geoand please comment if you have what to say or if my endpoint-identification comments are wrong. you know this field much better. I have tried "mine" reproducer on Quarkus REST and it behaves same.

[michalvavrik]

Yes It is what I have done before and it was working. I don't want to change my code unless it is wrong

I think https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-3.1.pdf section 3.6 annotation inheritance refers to the jakarta.ws.rs annotations like GET etc. while RolesAllowed is in jakarta.annotation.security and I don't think that section applies on that. The issue you opened is about when quarkus.security.jaxrs.deny-unannotated-endpoints=true should be applied. And that depends on the definition of what is the endpoint. The section 3.3 Resource methods says Resource methods are methods of a resource class annotated with a request method designator. and that is what we use for the "endpoint" identification right now.

@path("/api/v1/executors/gw")
public interface A {

Again, I am not JAX-RS expert at all, but this seems like the core of the issue. The spec says Note that inheritance of class or interface annotations is not supported. If you moved the @path("/api/v1/executors/gw") to the class and leave just methods on the interface, all the discussion is over as it will work.

I hope it won't be too confusing, but I'll try to provide some context:

Ad endpoint definition: I didn't find in the specs what the definition is. in the RESTEasy Classic we took the information from the ResourceInfo which returns the interface class and method. But Quarkus REST returns in the ResourceInfo actual class and not the interface, which suggest that returned information is implementation specific. Some JAX-RS expert must tell.

That said, the implementation of this check is different in the Quarkus REST, which allows to have same behavior in regards of the quarkus.security.jaxrs.deny-unannotated-endpoints=true when you switch the dependencies.

What can be done:

• in Quarkus REST (though this issue is opened for RESTEasy Classic) we know both declaring class and the invoked, so we can decide if we do one, both ... That must be done with greater consensus because it would mean that we make endpoint definition vague. I'd like experts to decide.

• in RESTEasy Classic I don't know what could be source of the information of what the implemented class is. We rely on jakarta.ws.rs.container.ResourceInfo. Is there another source that provides different information?

Hope this helps, thanks.

[psini]

Ok so if I'm correct you are saying that this is an error in my code and I have to add a jax-rs annotation? I'm fine with it but i think it's better to document it because if you read at https://docs.jboss.org/resteasy/docs/6.2.8.Final/userguide/html/ch50.html#Sharing_interfaces It states that you could share the interface between client and server and up to my understanding and the most natural way is to have @path on interface and not it duplicated on both interface and implementation.

[michalvavrik]

Ok so if I'm correct you are saying that this is an error in my code and I have to add a jax-rs annotation?

I mostly tried to explain why it works this way now :-) It is true that moving the path from the interface level (but keeping it on the interface methods) fixed my reproducer and I think it might corresponds to that specs note.

I'm fine with it but i think it's better to document it because if you read at https://docs.jboss.org/resteasy/docs/6.2.8.Final/userguide/html/ch50.html#Sharing_interfaces It states that you could share the interface between client and server and up to my understanding and the most natural way is to have @path on interface and not it duplicated on both interface and implementation.

It's Quarkus specific feature, so as long as there is agreement, we can support this on the implementation. It will take at least couple of days to gather opinions.

And then, ATM I only know how to fix this for Quarkus REST as that one I know better. Maybe there is some hidden info we can use in the RESTEasy Classic during the request processing. If not or it would mean to go to the build time again and works with endpoint detection. It is doable.

[michalvavrik]

One else thing needs to be considered: your scenario is very simple, but advantage of the current approach is that it is simple, you can use the rule of thumb (predictable) what annotation is precedence.

Just consider if your interface had PermitAll, what would have precedence if we accept that both are endpoints? RolesAllowed or PermitAll? implementation or the interface? It will be mess to determine how it should behave.

That is why if we determine the endpoint according to the declaring class, we have a rule to follow. My personal preference would be to keep it. What will be done depends on what others has to say.

[psini]

i would like that implementation overrides the interfaces because it's more specific , but it's only my opinion ;)

[sberyozkin]

@gsmet There is no security relaxation regression here, but an unexpected denial of access.
@michalvavrik Thanks for all the checks, I'll try to provide some feedback tomorrow.

@psini The JAX-RS inheritance is confusing, I've just checked the spec, apparently, the class/interface annotations (ex, top level @Path("/api/v1/executors/gw") can not be inherited. Oh yes, this is what Michal said above.

That said, Michal, if putting RolesAllowed on the interface makes it work then I'm not sure why the following is only the case if we have RolesAllowed on the implementation class.

The RESTEasy Classic produces jakarta.ws.rs.container.ResourceInfo that says that resource class is org.acme.GreetingInterface and method is org.acme.GreetingInterface#sayHi. Not overriden method without any Path and GET.180484 You set quarkus.security.jaxrs.deny-unannotated-endpoints=true which denies all unannotated endpoints as its name says. And your overriden method is not an endpoint according to what ResourceInfo returns.

Should we consider reverting that PR if it was only meant to make things better in general and revisit it say for 3.11 ?

[michalvavrik]

That said, Michal, if putting RolesAllowed on the interface makes it work then I'm not sure why the following is only the case if we have RolesAllowed on the implementation class.

1. The roles allowed check is still in place on the implementation.
2. In order to treat serialization DoS CVEs we introduced eager checks for the endpoints. it runs before the roles allowed.
3. If you say that the implementation is the endpoint, it will effectively means you must prefer it's standard security annotations on the implementation over those on the interface. But that doesn't match what is my understanding of the specs. If we say whatever is more specific wins and users expect RolesAllowed to be applied on the place where Jakarta REST Resource method is actually declared (interface), that that is what is a bug as well.

I don't have anything else to say, just a warning that situation will get even more complex when we consider subresources. I'll wait for others to provide opinions.

Side note to annotation inheritance rules mentioned above: RolesAllowed is not JAX-RS annotation, it is https://jakarta.ee/specifications/annotations/3.0/annotations-spec-3.0.pdf annotation. You can't just apply same inheritance rules. You can see from examples in the 3.1. General Guidelines for Inheritance of Annotations that you can't inherit it from interfaces to the impl. If that was clear, sorry, I just wanted to have a baseline.

Should we consider reverting that PR if it was only meant to make things better in general and revisit it say for 3.11 ?

I will leave decision on you. As long as you understand that previous case left interface endpoints unsecured when RolesAllowed were not used but JAX-RS security deny all was in place. Therefore by reverting, you will introduce a vulnerability.

cc @starksm64 @stuartwdouglas in case you are interested in the case

[michalvavrik]

I will leave decision on you. As long as you understand that previous case left interface endpoints unsecured when RolesAllowed were not used but JAX-RS security deny all was in place. Therefore by reverting, you will introduce a vulnerability.

Sorry, little correction. That can be true for some cases because basically I said that the detection is complex and we can't be sure as we had a mistake in that algorithm in past. I can't tell without investing time I don't have. Your call. And the Quarkus REST behaves same, so you will introduce migration inconsistency which we should document for the users switching to the Quarkus REST.

[michalvavrik]

Hey Michal, well, we don't want to resurface the old vulnerabilities, for sure.
In any case, we don't have to rush things, lets discuss things in the next few days, it is not fair to expect you keeping every possible subtlety in mind, the work you've done around these issues, have improved things significantly, we'll figure something re this issue too, good night :-), I'm certainly signing off :-)

Sure. I do not try to discourage you from the revert. It's completely valid decision, especially as there is a user experiencing issues. I just don't want to participate on this move, because I am not sure whether the previous state was safe. It would take some effort to verify the safetiness, but that can be done as well. Thanks

I'm only a little bit confused why, with @RolesAllowed being on the interface, we correctly handle things in 3.8.x

If you believe that previous state was correct, than this is a bug and indeed it needs to be reverted. I do not share this opinion. If you believe this, I suggest to also fix the Quarkus REST that behaves same. Which could be a bug, I need feedback from others.

the PR, #38622, is about consolidating/making things more robust, as opposed to fixing anything in particular .

#38622 assured that endpoints are secured, whether build time algorithm that differs to the runtime algorithm is fine or not. needless to say it wasn't correct in the past as it caused a CVE (don't remember if it was one or more for this particular one).

I am really not able to decide this situation, it must be based on conclusion what is expected behavior.

[michalvavrik]

Reproducer:

git clone [email protected]:michalvavrik/roles-allowed-on-interface-resteasy-classic-reproducer.git
git checkout feature/reproducer2
PASS (main branch): mvn clean test
FAIL (3.8.2): mvn clean test -Dquarkus.platform.version=3.8.2 -Dquarkus.platfom.group-id=io.quarkus.platform

[gsmet]

@sberyozkin

There is no security relaxation regression here, but an unexpected denial of access.

Is it?
In the OP's case, the default policy is deny (that's what they wrote). What if the default policy is to allow things to pass?

[gsmet]

Also let's take a step back. I will ping you both.

[geoand]

Sorry, I have yet to look at this one