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

RestEasy Reactive performs authorization checks twice #26536

Closed
knutwannheden opened this issue Jul 4, 2022 · 24 comments · Fixed by #26567
Closed

RestEasy Reactive performs authorization checks twice #26536

knutwannheden opened this issue Jul 4, 2022 · 24 comments · Fixed by #26567

Comments

@knutwannheden
Copy link
Contributor

Description

(I am filing this issue as an enhancement, because it isn't really a bug, but I suspect that performing the authorization twice isn't the desired behavior either.)

When using RestEasy Reactive the authorization checks will be performed twice: Once using RestEasy Reactive's own EagerSecurityHandler (registered as a ServerRestHandler) and once by the curresponding CDI interceptor (e.g. RolesAllowedInterceptor) provided by quarkus-security. It would seem like it would make sense to only perform the check once, as with RestEasy Classic.

I stumbled across this while migrating to RestEasy Reactive, since we also use the CDI interceptor approach for some custom security logging. With RestEasy Reactive this mechanism no longer works in the "negative" case (when the authorization check fails), since the check performed by RestEasy Reactive will short-circuit the handling. As an alternative for the negative case I can however use an ExceptionMapper instead.

Implementation ideas

No response

@knutwannheden knutwannheden added the kind/enhancement New feature or request label Jul 4, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 4, 2022

/cc @FroMage, @geoand, @stuartwdouglas

@quarkus-bot quarkus-bot bot added the area/rest label Jul 4, 2022
@knutwannheden
Copy link
Contributor Author

/cc @sberyozkin

@sberyozkin
Copy link
Member

sberyozkin commented Jul 4, 2022

@geoand Can EagerSecurityHandler be dropped if a CDI RolesAllowedInterceptor is there anyway doing the checks ?

@sberyozkin
Copy link
Member

sberyozkin commented Jul 4, 2022

Ive looked at the code and I guess just dropping this handler may not be a good idea, but it looks like it might be possible to avoid doing the authorization checks specifically.
@michalvavrik If it can be of interest/if you can get some time, can you please investigate, CC Georgios, myself if it can get to PR ? Asking because you've done 2 good PRs related to the RestEasy Reactive security recently.

@michalvavrik
Copy link
Member

I'll have a look later this week, busy with my work now.

@geoand
Copy link
Contributor

geoand commented Jul 4, 2022

I am pretty sure we want this to be the other way around - the code in RolesAllowedInterceptor should not be executed if a relevant check has already been done in RESTEasy Reactive.

@michalvavrik
Copy link
Member

Agreed, it does not make sense to do check twice.

@sberyozkin
Copy link
Member

The only question is where to do these checks, I thought RolesAllowedInterceptor was supposed to work across all the stacks;

@geoand
Copy link
Contributor

geoand commented Jul 4, 2022

It does, no question about that.

My point is that we should have some kind of switch that make RolesAllowedInterceptor be a no-op for the current request (perhaps a flag on a new request scoped bean).

@michalvavrik
Copy link
Member

Not just on this one, on AuthenticatedInterceptor, DenyAllInterceptor, PermitAllInterceptor too.

@geoand
Copy link
Contributor

geoand commented Jul 4, 2022

Yes, correct

@sberyozkin
Copy link
Member

But what is the problem with not doing the authorization check at the EagerSecurityHandler ? There are 2 steps in the security flow, authentication (which EagerSecurityHandler can continue participating in) and authorization - and my question is, why to add a flag and skip RolesAllowedInterceptor etc and keep the authorization related code in EagerSecurityHandler as opposed to letting RolesAllowedInterceptor/etc do their job ?
@knutwannheden has said that with EagerSecurityHandler interposing on the authorization checks, his custom CDI interceptors do not work in the negative case (while they do in a similar case when RolesAllowedInterceptor).
IMHO, unless there is a technical reason why it should not be done, or I am simply missing something obvious, RolesAllowedInterceptor should be used ?

@geoand
Copy link
Contributor

geoand commented Jul 4, 2022

We had multiple issues in RR with the security flow, which is why things were moved to EagerSecurityHandler.
IMHO, we should set whatever request-scoped related flags we need there and take them into account in the interceptors

@sberyozkin
Copy link
Member

@geoand Sure, have they been related to running RolesAllowedInterceptor/etc independently ?

@geoand
Copy link
Contributor

geoand commented Jul 4, 2022

I can't remember honestly, sorry

@sberyozkin
Copy link
Member

@geoand Sorry, I don't mean to be annoying though I accept I might still be :-). What I think would be good though, once Michal finds time to check it out, is to figure out, if it is principally important to collocate both the authentication and authorization related code in this EagerSecurityHandler and thus having to add a flag to make the follow up security CDI interceptors no-op, or is it can be reviewed and the authorization checks dropped. IMHO it is important to have a better idea what is going on.

If the flag based solution is implemented, then I'm not sure what would it mean for the the custom security CDI interceptors in the RestEasy reactive flow, I guess we'd need to update the docs with some guidance as per @knutwannheden's comment (use ExceptionMapper for the exceptions, CDI interceptor for the accepted requests, etc).

Either way, thanks for the patience

Thanks

@geoand
Copy link
Contributor

geoand commented Jul 4, 2022

Not a problem at all :).

I can certainly try and dig up the old issues we had.
I might be missing something, but I don't see why having some flags that act as controls and are read inside the existing CDI beans would cause an issue.

@sberyozkin
Copy link
Member

Hi @geoand Yeah, it will work fine for sure and will most likely be the simplest fix, I'm not trying to be pedantic. It is likely that the authorization checks have been done from this handler because there was some mis-sync with the CDI chain when it comes to running those existing sec interceptors. It is just a sense of uncertainty about running those existing (custom) sec interceptors in Resteasy reactive flows that would be good to clarify IMHO, at least indirectly by adding some docs...

Thanks

@geoand
Copy link
Contributor

geoand commented Jul 4, 2022

Completely understood. I'll try to dig up the history tomorrow

@sberyozkin
Copy link
Member

Thanks @geoand :-), lets keep discussing when you get some time

@michalvavrik
Copy link
Member

for record: security checks are performed twice when an RBAC annotation is on the endpoint, once when CDI bean method is annotated (by quarkus-security interceptor).

@knutwannheden
Copy link
Contributor Author

I had another look at this yesterday and looked into using exception mappers to log the failed authorization checks and a CDI interceptor for successful authorizations. This asymmetry and the fact that it doesn't cover the case when authorization rules are defined using configuration properties (where I still haven't found a solution) lead me to create a feature request for an SPI: #26549

Using exception mappers to log the negative authorization check outcome also poses another problem for us: Since we aren't directly building an application, but a set of Quarkus extensions to be used by many applications in the inhouse development of an enterprise, the application developers would have to be careful to not inadvertently disable the security logging when registering their own exception mappers. Here the approach with the additive CDI interceptors is the better alternative for us.

@geoand
Copy link
Contributor

geoand commented Jul 5, 2022

#19598 was the original change introducing the eager security feature.

From the git history of EagerSecurityHandler one can also see various bugs that were fixed around it.

@knutwannheden
Copy link
Contributor Author

FWIW, I am currently looking into the possibility of using a Vert.x handler (registered via @RouteFilter) to handle my use case.

michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Jul 6, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Jul 6, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Jul 11, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Jul 11, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Jul 12, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Jul 12, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Jul 26, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Aug 10, 2022
@quarkus-bot quarkus-bot bot added this to the 2.13 - main milestone Aug 19, 2022
sheilamjones pushed a commit to sheilamjones/quarkus that referenced this issue Aug 22, 2022
fercomunello pushed a commit to fercomunello/quarkus that referenced this issue Aug 31, 2022
miador pushed a commit to miador/quarkus that referenced this issue Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants