-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Upgrading to 3.9.2 from 3.8.3 RolesAllowed on implementing class of an interface using jaxrs are not used. #39964
Comments
/cc @pedroigor (bearer-token) |
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 ? |
If i add the @RolesAllowed on the Interface (that violates the spec) it restart to work |
we perform security checks sooner to limit processing; that alone is a good thing
we have tests that checks default JAX-RS deny all configuration option on the interface that is overriden without PATH / GET
then I put
@psini simple reproducer would help, or at least examples of the classes and configuration properties please
according to the spec you can use Path, GET on the interface but not RolesAllowed? please can you link the specs and the section |
I don't want to use @RolesAllowed on Interfaces. 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();
}
} |
Thank you. I have created a reproducer https://github.com/michalvavrik/roles-allowed-on-interface-resteasy-classic-reproducer (use The RESTEasy Classic produces You set @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:
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. |
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. |
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.
I believe we need to gather other opinions, because I provided the best arguments I could. |
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). |
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 yes but what is your opinion @sberyozkin?
|
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
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 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 That said, the implementation of this check is different in the Quarkus REST, which allows to have same behavior in regards of the What can be done:
Hope this helps, thanks. |
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. |
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.
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. |
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. |
i would like that implementation overrides the interfaces because it's more specific , but it's only my opinion ;) |
@gsmet There is no security relaxation regression here, but an unexpected denial of access. @psini The JAX-RS inheritance is confusing, I've just checked the spec, apparently, the class/interface annotations (ex, top level That said, Michal, if putting
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 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.
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 |
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. |
Hey Michal, well, we don't want to resurface the old vulnerabilities, for sure. I'm only a little bit confused why, with |
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
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.
#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. |
Reproducer:
|
Is it? |
Also let's take a step back. I will ping you both. |
Sorry, I have yet to look at this one |
In our case we are using jax-rs interfaces and classes as well. Mostly to build a server. To protect the application from any unauthorized access we set the property Usually we generate the interfaces from an OpenAPI spec. Typically these generated interfaces don't have any Before 3.9.x the access to an API endpoint is denied only if it's missing the proper Now starting with Quarkus 3.9.x any request is blocked due to the EagerSecurityFilter / DenyAllCheck interceptor which is not checking the implementation class for Our current workaround is not using the Maybe the described usage scenario helps for discussing an upcoming solution. |
Describe the bug
Upgrading to 3.9.2 from 3.8.3 RolesAllowed on implementing class of an interface using jaxrs are not used.
I have an interface where I define my endpoints (
@Path
@Consumes
@Get
) so I can use it also as a client, and RolesAllowed in the implementing class. After the upgrade it goes in deny (my default policy is deny) because it doen't read the RolesAllowed Annotation.Expected behavior
The call is succesfull
Actual behavior
I get a forbidden
How to Reproduce?
No response
Output of
uname -a
orver
No response
Output of
java -version
No response
Quarkus version or git rev
No response
Build tool (ie. output of
mvnw --version
orgradlew --version
)No response
Additional information
In debug i saw that in EagerSecurityFilter
quarkus/extensions/resteasy-classic/resteasy/runtime/src/main/java/io/quarkus/resteasy/runtime/EagerSecurityFilter.java
Lines 71 to 80 in 510e89e
at line 73 the var description contains the interface and not the implementation class.
The text was updated successfully, but these errors were encountered: