Security roles checked after constraints validation with quarkus-resteasy-reactive

[gwenneg]

Describe the bug

I noticed something weird while migrating a project from quarkus-resteasy to quarkus-resteasy-reactive:

• with quarkus-resteasy, security roles are checked before constraints validation when the request content type is JSON
• with quarkus-resteasy-reactive, security roles are checked after constraints validation when the request content type is JSON

There's no difference between the two extensions when the content type is text.

Expected behavior

I would expect the same behavior with both versions of quarkus-resteasy*.

Actual behavior

Different behavior.

To Reproduce

https://github.com/gwenneg/quarkus-resteasy-reactive-security

It contains two folders with the exact same code with one exception: the quarkus-resteasy* dependency.

Steps to reproduce the behavior:

1. cd quarkus-resteasy
2. ./mvnw clean test > BUILD SUCCESS
3. cd quarkus-resteasy-reactive
4. ./mvnw clean test > BUILD FAILURE

Environment (please complete the following information):

Output of java -version

openjdk version "11.0.9.1" 2020-11-04
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.9.1+1)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.9.1+1, mixed mode)

Quarkus version or git rev

1.13.3.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)

[quarkus-bot]

/cc @FroMage, @geoand, @sberyozkin, @stuartwdouglas

[sberyozkin]

@gwenneg Hi, what side-effects do you observe ? In both cases the end resultt should be the method is only being invoked if both security roles and constrains validation checks pass ?

thanks

[gwenneg]

I'm still in the middle of the migration and didn't have the occasion to use the app with quarkus-resteasy-reactive. So the only side-effect I observed so far is one of our unit tests which checks security that started failing.

I created this issue because I'm a bit concerned that security comes second. The problem could also be larger than the simple @NotNull constraint I used in the reproducer.

[sberyozkin]

@gwenneg I see - as far as the test is concerned - it is a test problem since it is failing now because of the constraints check failing. The constraints check in itself has no side-effects (I hope, CC @gsmet ) so as long the method is protected all is safe - but I agree it would be better to run the sec check first - @geoand - hi, looks like an authorization filter priority is not taken into consideration ?

[gsmet]

Security checks should ALWAYS be executed first. Otherwise you expose your endpoint somehow. Obviously, it's not a big problem if your constraint is @NotNull but if you have some business related constraints, you could expose some logic that you don't want to expose.

[sberyozkin]

So the constraints check can have side-effects, they are not just checking the input values...

[sberyozkin]

@gsmet By the way - if the constraint validation exception can return information about the endpoint - it would be a CVE material.

[gsmet]

They are checking the input values but they can be business related anyway i.e. "@ValidOrderNumber" or whatever.

People can implement their own constraints and you have absolutely no idea of what they will do with them. So you cannot assume they are entirely safe to execute before the security checks.

[gwenneg]

Poorly written validation can also lead to DoS attacks. I wouldn't want that to be exposed before the security check is done.

[geoand]

@geoand - hi, looks like an authorization filter priority is not taken into consideration ?

I haven't looked into it (and probably won't for the next few days as we have some public holidays here), but I would be surprised if that is the case as the TCK tests this extensively.
What does the filter look like?

[sberyozkin]

@gsmet @gwenneg thanks for the explanation - you see I've been naively thinking it is just NotNull or This Integer must be > 1, etc :-)

[sberyozkin]

Hey @geoand Enjoy the long weekend, I thought it was a JAX-RS filter but it is a CDI interceptor

[geoand]

Hey @geoand Enjoy the long weekend, I thought it was a JAX-RS filter but it is a CDI interceptor

Aha, OK :)

[geoand]

I had a quick look and what you are seeing is actually not caused by the interceptors at all - neither the validation interceptor nor the security interceptor get invoked at all.

The code that results in HTTP 400 is:
https://github.com/quarkusio/quarkus/blob/1.13.3.Final/extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson/runtime/src/main/java/io/quarkus/resteasy/reactive/jackson/runtime/serialisers/ServerJacksonMessageBodyReader.java#L55

I think we have had this conversation somewhere before... The question is whether we should not return HTTP 400, but let the processing continue with a null value.
In this case it may make sense, but in general it seems pretty risky...

[stuartwdouglas]

At the moment this is implemented as a CDI interceptor (this is not just a Resteasy Reactive thing, @RolesAllowed works on any CDI bean).

We probably need to turn this into a handler, and push this up the chain and run it first thing after the pre-match interceptors.

[geoand]

We could certainly do that, as it would run security before input is ever read

[sberyozkin]

@RolesAllowed works on any CDI bean

Cool, this must be documented - will take care of it, thanks Stuart @stuartwdouglas for reminding about it

[geoand]

I wonder if it makes sense to this sort of thing with other kinds of interceptors as well - like Caching for example