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

Apply additional JAX-RS security only for endpoints and not all resource methods #28820

Merged

Conversation

michalvavrik
Copy link
Contributor

fixes: #28791

When using JAX-RS security configuration properties quarkus.security.jaxrs.deny-unannotated-endpoints and quarkus.security.jaxrs.default-roles-allowed all resource methods that can be intercepted (endpoints or not) are secured by security interceptors (e.g. DenyAllInterceptor and RolesAllowedInterceptor). This leads to unexpected behavior when non-endpoints are invoked in a fashion that can be intercepted, one example - add @ServerExceptionMapper to a resource (methods annotated with this annotation also expects to be in resource, see its Javadoc), this mapper method will be secured (and if it handles Unauthorized, it should lead to circular reference).

This PR secures endpoints rather than classes. To do that in RR, I had to extract logic behind endpoint selection (to keep the logic in one place).

@geoand
Copy link
Contributor

geoand commented Oct 25, 2022

Thanks for this.

I'll have a look tomorrow

@sberyozkin
Copy link
Member

Hi @michalvavrik Thanks for another quality PR, but I'm concerned that such changes are needed to address this rather edge case. Let me also comment inline...

@michalvavrik
Copy link
Contributor Author

@sberyozkin I don't mind to drop the PR (you can close it yourselves as far as I'm concerned), however the issue is quite serious when JAX-RS config are used - all resource methods (public non static etc.) are secured with interceptor, it can lead to quite unexpected behavior. I can only provide my user opinion - that's not what I expect when I use deny-unannotated-endpoints that speaks about endpoints only.

If you have better suggestion, bring it on, please.

@sberyozkin
Copy link
Member

sberyozkin commented Oct 25, 2022

Hey @michalvavrik

I don't mind to drop the PR

Just because I'm not immediately getting your changes does not mean I'd like the current PR be dropped :-)
It just feels strange right now that invoking a mapper declared on a resource method requires these changes only for deny-unannotated-endpoints and default-roles-allowed properties, we did not need them for other cases involving the same custom exception mapper, right ?
What happens if we say, instead of using deny-unannotated-endpoints, do

quarkus.http.auth.permission.deny1.paths=/hello/*
quarkus.http.auth.permission.deny1.policy=deny

Can you please check what happens in such a case with and without your PR ?

And what happens if we add @DenyAll to HelloResource (instead of using the config properties), what would happen with/without your PR ?

I think it would be useful to see a bigger picture of how having @ServerExceptionMapper on a resource method affects the processing...

Cheers

@michalvavrik
Copy link
Contributor Author

michalvavrik commented Oct 25, 2022

It just feels strange right now that invoking a mapper declared on a resource method requires these changes only for deny-unannotated-endpoints and default-roles-allowed properties, we did not need them for other cases involving the same custom exception mapper, right ?

@sberyozkin thank you for taking your time, but I think you got this wrong - it's not connected to exception mappers or quarkus.http.auth.permission.deny1.paths, the issue only affect cases where we add annotation on the class (where we produce io.quarkus.security.spi.AdditionalSecuredClassesBuildItem) and that's only for 2 properties -
quarkus.security.jaxrs.deny-unannotated-endpoints and quarkus.security.jaxrs.default-roles-allowed. I've checked and when you replace deny-unannotated-endpoints with quarkus.http.auth.permission.deny1.paths, there is no issue.

The reason for this is that for each path we apply path matching check, there are no interceptors in action. Here, the issue comes from applying interceptors where they shouldn't be applied.

And what happens if we add @Denyall to HelloResource (instead of using the config properties), what would happen with/without your PR ?

If you add yourself @DenyAll to HelloResource, you get secured all methods (with/without my PR), endpoint or not. But there, you do it yourself, it's same as applying @DenyAll to a bean (so it's irrelevant whether it's resource or not).

I think it would be useful to see a bigger picture of how having @ServerExceptionMapper on a resource method affects the processing...

As I tried to point out, the very same happen when without @ServerExceptionMapper, let see the test I added to RESTEasy Classic. it's just another public method.

    @Path("/hello")
    public static class HelloResource {

        @PermitAll
        @GET
        public String hello() {
            return getHello();
        }

        public String getHello() {
            return "hello";
        }

    }

would you expect getHello() to be secured when using deny-unannotated-endpoints? Because it is.

@sberyozkin
Copy link
Member

sberyozkin commented Oct 25, 2022

@michalvavrik, I was thinking about it on the way home, and I believe I came to the conclusion that opening up non-endpoint methods which are invoked as part of the failed authentication request processing is a risky idea, can cause a surprise and introduces inconsistency. Please take more feedback/don't close the PR, but IMHO we need a different solution.

Let me comment more on it below,

If you add yourself @Denyall to HelloResource, you get secured all methods (with/without my PR), endpoint or not. But there, you do it yourself, it's same as applying @Denyall to a bean (so it's irrelevant whether it's resource or not).

I think I don't agree with this justification. Don't I do it myself as well with quarkus.security.jaxrs.deny-unannotated-endpoints ? We are not talking I think about the mechanics, but about the fact I either use the configuration or the annotation to deny the request. The result should be the same, the security exception, mapped if required by the user.

Re your updated example

would you expect getHello() to be secured when using deny-unannotated-endpoints? Because it is.

Yes, because it is also secured with @DenyAll or HTTP security policy.

I'd like to propose to take a step back and ask why BlockingOperationNotAllowedException is reported and resolve it by a technical approach as opposed to disabling the security checks for such a flow.

Thanks

@quarkus-bot

This comment has been minimized.

@michalvavrik
Copy link
Contributor Author

I'd like to propose to take a step back and ask why BlockingOperationNotAllowedException is reported and resolve it by a technical approach as opposed to disabling the security checks for such a flow.

As you wish; BlockingOperationNotAllowedException is thrown as @ServerExceptionMapper is running on IO thread and return stuff in a synchronous manner (so blocking get identity is used). I don't know if it's expected to run the mapper on IO thread, but I'll try to find out.

@geoand
Copy link
Contributor

geoand commented Oct 26, 2022

I think the issue being reported here is real and makes sense to try to be addressed.

However I need to take a good look at the changes themselves as they seem to be pretty far reaching.

existingClassNameBindings, considerApplication, httpAnnotationToMethod, index, applicationScanningResult,
getAnnotationStore());
Map<String, BasicResourceClassInfo> basicResourceClassInfoMap = new HashMap<>();
for (FoundEndpoint endpoint : endpoints) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't really understand the changes here. Can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

createEndpoints does 2 things:

  • determine if resource method is an endpoint
  • create endpoint

I had a PR back in the days where I needed to collect all endpoints and you pointed out I should keep the logic in one place (one version of #26567). The changes here just try to divide the two things without adding extra action. So for performance reasons, with each endpoint method it remembers its class, classNameBindings and httpMethod. If you look couple lines below, the logic is same -> lines are just moved to collectEndpoints.


public class AdditionalRolesAllowedTransformer implements AnnotationsTransformer {

private final Set<String> classNames;
private final Set<MethodDescriptor> methods;
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I would much rather prefer us to use something other than a Gizmo type here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's replaced by MethodDescription, that should be ok as it belongs to security and is very much related (used for security checks).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll have look on Monday

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, well appreciated.

@sberyozkin
Copy link
Member

sberyozkin commented Oct 26, 2022

Hey @michalvavrik

As you wish; BlockingOperationNotAllowedException is thrown as @ServerExceptionMapper is running on IO thread and return stuff in a synchronous manner (so blocking get identity is used). I don't know if it's expected to run the mapper on IO thread, but I'll try to find out.

It is not a wish :-), I just I feel we should address it differently. What if we have some custom non-sec exception mapper declared on a method expecting a SecurityIdentiy injection, etc...

Hey @geoand

I think the issue being reported here is real and makes sense to try to be addressed.

+1 to it, my concern though is that it should not be addressed by disabling the implicit security checks for the methods declaring the mapper since, among other things, it would be inconsistent with how it works when the same restriction is enabled via the direct annotation or HTTP security policy.

@geoand
Copy link
Contributor

geoand commented Oct 26, 2022

@sberyozkin you are probably right. I admit I have not looked into the originally reported issue.

I'll do that and post back

@geoand
Copy link
Contributor

geoand commented Oct 26, 2022

My conclusion is that what @michalvavrik is correct. The originally reported problem is caused because the security interceptor is getting called when it clearly should not (for reasons explained in this PR).

@sberyozkin
Copy link
Member

@geoand But I don't agree we can have a case where setting these properties opens up such mapper methods while having @DenyAll or HttpSecurityPolicy deny all settings does not.

@geoand
Copy link
Contributor

geoand commented Oct 26, 2022

Sorry, I don't understand what you mean. Can you explain with an example perhaps?

Thanks

@sberyozkin
Copy link
Member

sberyozkin commented Oct 26, 2022

@geoand sure, I commented about it in #28820 (comment), and then in #28820 (comment) as well, we have 3 mechanisms to request that the method access is denied. This PR is dealing with one of such mechanisms. The mechanics are different for each of them but the intention is the same, and it has to remain consistent IMHO how non-endpoint methods are treated - for the 2 cases the security is enforced.

The other point I made to Michal is that fixing the Blocking exception by making non-endpoint methods open to the anonymous access seems risky. I have no specific attack scenario in mind right now, but I'm finding it somehow wrong that a secure method invocation switches to the anonymous access method to complete the call, it feels the whole call should remain under the same constraints, it is the same thread which tries both the method invocation and then attempts to map the exception.

@sberyozkin
Copy link
Member

The other scenario I mentioned, we have a method-level mapper for some custom exception which happens as part of the successfully authenticated request - the mapper would like to have the security identity injected. If it works with exception mappers in separate classes but not with the method level mappers then it is a problem IMHO

@michalvavrik
Copy link
Contributor Author

@geoand sure, I commented about it in #28820 (comment), and then in #28820 (comment) as well, we have 3 mechanisms to request that the method access is denied. This PR is dealing with one of such mechanisms. The mechanics are different for each of them but the intention is the same, and it has to remain consistent IMHO how non-endpoint methods are treated - for the 2 cases the security is enforced.

I think HTTP security policy has no way of securing the exception mappers as it uses path matching so mappers are not secured. If you gave concrete example (or point me to docs) it would be great. Thank you

The other point I made to Michal is that fixing the Blocking exception by making non-endpoint methods open to the anonymous access seems risky. I have no specific attack scenario in mind right now, but I'm finding it somehow wrong that a secure method invocation switches to the anonymous access method to complete the call, it feels the whole call should remain under the same constraints, it is the same thread which tries both the method invocation and then attempts to map the exception.

One test I added shows add if you have endpoint secured with @PermitAll and using deny-unannotated-endpoints than mapper is secured harder than endpoint (it goes other way around to the example you mention).

@sberyozkin
Copy link
Member

@michalvavrik

I think HTTP security policy has no way of securing the exception mappers as it uses path matching so mappers are not secured. If you gave concrete example (or point me to docs) it would be great. Thank you

I've asked you to check earlier

quarkus.http.auth.permission.deny1.paths=/hello/*
quarkus.http.auth.permission.deny1.policy=deny

I thought you did, sorry if you did not, please check.

One test I added shows add if you have endpoint secured with @permitAll and using deny-unannotated-endpoints than mapper is secured harder than endpoint (it goes other way around to the example you mention).

Nonetheless it how it also works with @Denyall. There is nothing conflicting there if having @Denyall behavior is accepted.

Also, I've expressed two other concerns:

  • It is correct that during the same invocation 2 different levels of security are provided, from stronger to lower.
  • What happens if a custom method level mapper invoked due to the exception which happened inside a successfully authenticated method requires an injected SecurityIdentity, example, check a user name

What is your take on the last two points ?

@michalvavrik
Copy link
Contributor Author

@michalvavrik

I think HTTP security policy has no way of securing the exception mappers as it uses path matching so mappers are not secured. If you gave concrete example (or point me to docs) it would be great. Thank you

I've asked you to check earlier

quarkus.http.auth.permission.deny1.paths=/hello/*
quarkus.http.auth.permission.deny1.policy=deny

I thought you did, sorry if you did not, please check.

I added them to the issue reproducer and its tests succeeded. Now I did it again. They succeeded as the mapper is not secured by the policy. I only tried to point out that it's not possible to secure non-endpoints with ^^^ as the check (that's how I read the code, I can be simply wrong) is applied directly to the route in Vert.X handler.

One test I added shows add if you have endpoint secured with @permitAll and using deny-unannotated-endpoints than mapper is secured harder than endpoint (it goes other way around to the example you mention).

Nonetheless it how it also works with @Denyall. There is nothing conflicting there if having @Denyall behavior is accepted.

Sure, agree.

Also, I've expressed two other concerns:

  • It is correct that during the same invocation 2 different levels of security are provided, from stronger to lower.

Agree.

  • What happens if a custom method level mapper invoked due to the exception which happened inside a successfully authenticated method requires an injected SecurityIdentity, example, check a user name

io.quarkus.security.identity.CurrentIdentityAssociation#getIdentity fails as the mapper is running on the IO thread (now I re-check, it's easy -> all that is needed is to annotate the mapper with @PermitAll). It's not unlike #23547 for beans, however here I think is fix possible. Georgios probably knows better why they are running on the IO thread, but I suppose I'll find out with little digging.

From my point of view, I think I understand Sergeys arguments and I don't have anything else to contribute to the discussion. Thanks for the time you spend on the review.

@sberyozkin
Copy link
Member

@michalvavrik Thanks, one thing I believe is important is that we have a consistent approach, so we have

  1. deny-unannotated-endpoints - the method level mapper is secured
  2. HTTP deny policy - the method level mapper is not secured
  3. @DenyAll the mapper level mapper is secured.

It feels like we'll need another issue to get it all in sync :-), I'd suggest tightening HTTP security policy in this case but may be it is only me...
Anyway, thanks for your time so far, sorry it is proving more time consuming than was expected :-)

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

Here are my thoughts:

  • I completely agree we should be consistent in our approach.
  • I also think that what @michalvavrik has done here in terms of excluding non JAR-RS methods from applying security is correct

@sberyozkin
Copy link
Member

@geoand Hi, yes, PR is a quality one which I agreed to, but I can't shake the feeling we are on the wrong path here, we are essentially working around the technical decision made at the RestReasy Reactive level, which is to treat the exception mapping provided at the JAX-RS method level as a class method invocation which picks up all the security interceptors, and to make it work we have to let the same thread which failed the security check re-enter the application endpoint without any restriction. It just feels wrong - I can't offer anything but theoretical risk analysis here right now - but nonetheless it feels wrong.

In all other cases this is simply not a problem because the same thread is not re-entering the same invocation chain, so the problem of non-endpoint methods being secured is non-existent in practice in all but this case where a mapper method is called like a regular resource method.

Please have a look at #28820 (comment), what is your take on the 2 concerns there (at the end of that comment) ?

IMHO it is worth examining other alternatives:

  • simply get the Blocking exception fixed, I recall Michal did some work for other sec interceptors on the Resteasy Reactive chain and it will just work as well and won't have any even theoretical sec concerns
  • consider updating RestEasy Reactive to call the resource mapper method such that it skips the CDI interceptor layer

I'm not going to block this PR if Stuart approves

Cheers

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

Hi, yes, PR is a quality one which I agreed to, but I can't shake the feeling we are on the wrong path here, we are essentially working around the technical decision made at the RestReasy Reactive level, which is to treat the exception mapping provided at the JAX-RS method level as a class method invocation which picks up all the security interceptors, and to make it work we have to let the same thread which failed the security check re-enter the application endpoint without any restriction. It just feels wrong - I can't offer anything but theoretical risk analysis here right now - but nonetheless it feels wrong.

Class level exceptions mappers is a feature plenty of people wanted and have known from Spring as well. On the implementation side, having it invoked via the same JAX-RS Resource class and thread that handles the actual Resource Method invocation is the only thing that makes sense IMHO.

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

What happens if a custom method level mapper invoked due to the exception which happened inside a successfully authenticated method requires an injected SecurityIdentity, example, check a user name

When you say injected, you mean with @Inject SecurityIdentity on a field of the class? If so, what's the concern? The field can be accessed if it has been populated by Arc.

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

consider updating RestEasy Reactive to call the resource mapper method such that it skips the CDI interceptor layer

Yeah, this could be interesting to check out. @mkouba is there a way to get a bean without its interceptors?

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

simply get the Blocking exception fixed, I recall Michal did some work for other sec interceptors on the Resteasy Reactive chain and it will just work as well and won't have any even theoretical sec concerns

How do you propose this would be done? If it means, switching threads, then no, that should not be done

@sberyozkin
Copy link
Member

@geoand

Class level exceptions mappers is a feature plenty of people wanted and have known from Spring as well. On the implementation side, having it invoked via the same JAX-RS Resource class and thread that handles the actual Resource Method invocation is the only thing that makes sense IMHO.

The feature is cool, no doubt, it is the fact the the thread which is associated with the failed security check re-enters the application code which is of concern. If we have a usual ExceptionMapper it is not a problem...

@sberyozkin
Copy link
Member

When you say injected, you mean with @Inject SecurityIdentity on a field of the class? If so, what's the concern? The field can be accessed if it has been populated by Arc.

I'm not sure what it will mean it the thread which entered the mapper has not gone through a sec check ? That said, yeah, looks like this particular concern is unfounded, as the same situation would take place with a regular ExceptionMapper

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

The feature is cool, no doubt, it is the fact the the thread which is associated with the failed security check re-enters the application code which is of concern. If we have a usual ExceptionMapper it is not a problem...

In that case, which thread is used?

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

I'm not sure what it will mean it the thread which entered the mapper has not gone through a sec check ? That said, yeah, looks like this particular concern is unfounded, as the same situation would take place with a regular ExceptionMapper

Yeah, that's my point

@sberyozkin
Copy link
Member

How do you propose this would be done? If it means, switching threads, then no, that should not be done

I was thinking more about the actual cause, what is really blocking in this deny interceptor

Lets see also if Martin has some suggestion re skipping CDI interceptors in this case, Resteasy Reactive does not apply them to the regular ExceptionMapper so I guess if it were possible to skip them in this case it would be all right...

I'll sign off in 40 mins or so for a long weekend break, thanks Michal, Georgios for the discussion :-)

@sberyozkin
Copy link
Member

If it is the regular mapper then it is also an IO thread I believe, but the resource class is not invoked, so no CDI interceptors, no Blocking problems, this is my understanding...

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

I'll sign off in 40 mins or so for a long weekend break, thanks Michal, Georgios for the discussion :-)

Thank you!

@mkouba
Copy link
Contributor

mkouba commented Oct 27, 2022

consider updating RestEasy Reactive to call the resource mapper method such that it skips the CDI interceptor layer

Yeah, this could be interesting to check out. @mkouba is there a way to get a bean without its interceptors?

Nope, there's no way to do this. For an intercepted bean Foo we generate an intercepted subclass Foo_Subclass extends Foo that is stored in the context, i.e. an instance of Foo_Subclass is the contextual instance of Foo.

@geoand
Copy link
Contributor

geoand commented Oct 27, 2022

Nope, there's no way to do this. For an intercepted bean Foo we generate an intercepted subclass Foo_Subclass extends Foo that is stored in the context, i.e. an instance of Foo_Subclass is the contextual instance of Foo.

I was afraid that would be the answer :)

Thanks for the insights

@geoand
Copy link
Contributor

geoand commented Oct 31, 2022

Unfortunately I am being pulled in too many directions to be able to properly understand and assess the implicattions of changes being introduced here.
I am really sorry, but this will have to wait... The reason is that if I don't wrap my head around what is being done, I can't properly support this piece of code (for which I am currently responsible)

@michalvavrik
Copy link
Contributor Author

@geoand please take your time / request changes / whatever you like. This is not urgent, just get back to it one day, please.

@geoand
Copy link
Contributor

geoand commented Oct 31, 2022

I definitely will!

Thanks again for all your great work and for your patience :)

@sberyozkin
Copy link
Member

@michalvavrik Can you clarify please what is causing IO thread to block in this case ? This is the last question I have before totally realizing this PR fix is the only possible fix, thanks

@michalvavrik
Copy link
Contributor Author

michalvavrik commented Nov 1, 2022

@michalvavrik Can you clarify please what is causing IO thread to block in this case ? This is the last question I have before totally realizing this PR fix is the only possible fix, thanks

@sberyozkin sure, in this case RolesAllowedInterceptor/DenyAllInterceptor (depending on config option) needs a security identity and because method around which it is invoked returns thing in a synchronous manner (it's exception mapper that returns Response), identity is accessed by blocking call while on IO thread.

@geoand
Copy link
Contributor

geoand commented Nov 2, 2022

I'm fine with RESTEasy Reactive changes.

@sberyozkin
Copy link
Member

Thanks @michalvavrik Now all the options have been explored (IMHO it was worth it), your original implementation idea does is indeed best :-), let me merge as Georgios also agrees, thanks

@sberyozkin sberyozkin self-requested a review November 2, 2022 10:41
@sberyozkin sberyozkin merged commit 0506e81 into quarkusio:main Nov 2, 2022
@quarkus-bot quarkus-bot bot added this to the 2.15 - main milestone Nov 2, 2022
@michalvavrik michalvavrik deleted the feature/only-secure-endpoints branch November 2, 2022 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants